feat(net): add P2P message deduplication and length validation#6712
feat(net): add P2P message deduplication and length validation#6712xxo1shine wants to merge 1 commit intotronprotocol:developfrom
Conversation
- FetchInvDataMsgHandler: reject messages with duplicate hashes - TransactionsMsgHandler: reject messages with duplicate transactions - SyncBlockChainMsgHandler: reject blockIds list exceeding 30 entries - Add MAX_SYNC_CHAIN_IDS = 30 constant to NetConstants - Add unit tests covering duplicate rejection and boundary values All violations throw P2pException(BAD_MESSAGE), triggering peer disconnect via existing P2pEventHandlerImpl error path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| public static final int MSG_CACHE_DURATION_IN_BLOCKS = 5; | ||
| public static final int MAX_BLOCK_FETCH_PER_PEER = 100; | ||
| public static final int MAX_TRX_FETCH_PER_PEER = 1000; | ||
| public static final int MAX_SYNC_CHAIN_IDS = 30; |
There was a problem hiding this comment.
Nice to have a named constant here instead of a magic number! 👍
Quick question — how was 30 chosen? Observed from real traffic, or estimated from getBlockChainSummary()'s log-step algorithm?
There was a problem hiding this comment.
Based on the chain digest algorithm, using a binary search from the solidified block to the head block, 30 blocks can generate 2^30 block digests, which is theoretically large enough, and 30 blocks will not put pressure on the system.
| if (hashList.size() != new HashSet<>(hashList).size()) { | ||
| throw new P2pException(TypeEnum.BAD_MESSAGE, | ||
| "FetchInvData contains duplicate hashes, size: " + hashList.size()); | ||
| } |
There was a problem hiding this comment.
Good check. Validation has already been added for FetchInvDataMessage, SyncBlockChainMessage, and TransactionsMessage. Why not also add deduplication checks for InventoryMessage and ChainInventoryMessage?
There was a problem hiding this comment.
Inventory Messages have rate limits and perform deduplication during processing, and they don't consume CPU, so there's no need to perform a second check before processing. ChainInventoryMessages have stricter checks than deduplication, while also ensuring that there are no duplicate lists.
7 participants
What does this PR do?
Add P2P message deduplication and length validation:
All violations throw
P2pException(BAD_MESSAGE), triggering peer disconnect via existingP2pEventHandlerImplerror path.Fixes #6667
Why are these changes required?
This PR has been tested by:
Follow up
Extra details