feat(net): add P2P message deduplication and length validation#6712
Conversation
| 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.
There was a problem hiding this comment.
ChainInventoryMessages's block number's order must be increasing, we can ignore it. But although inventory messages don't need consume CPU, deduplication checks are still needed to keep consistency with other message handling logic and to defend against malicious peers.
There was a problem hiding this comment.
InventoryMessages does not constitute an attack, and the lack of duplicate checks has no impact on the system.
xxo1shine
force-pushed
the
feature/p2p-msg-validation-1
branch
2 times, most recently
from
5713d02 to
e023c2e
Compare
- 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.
xxo1shine
force-pushed
the
feature/p2p-msg-validation-1
branch
from
e023c2e to
805f5b5
Compare
…validation-1 # Conflicts: # framework/src/test/java/org/tron/core/net/messagehandler/TransactionsMsgHandlerTest.java
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