feat(net): optimize fetch inventory message processing logic#5895
feat(net): optimize fetch inventory message processing logic#5895kuny0707 merged 5 commits intotronprotocol:release_v4.8.1from
Conversation
| return true; | ||
| } | ||
| for (Sha256Hash hash : msg.getHashList()) { | ||
| if (peer.getAdvInvSpread().getIfPresent(new Item(hash, InventoryType.BLOCK)) == null) { |
There was a problem hiding this comment.
The second request will be treated as synchronous and the check will fail. Refer to the code in the check function.
if (!isAdv) {
if (!peer.isNeedSyncFromUs()) {
throw new P2pException(TypeEnum.BAD_MESSAGE, "no need sync");
}
for (Sha256Hash hash : fetchInvDataMsg.getHashList()) {
long blockNum = new BlockId(hash).getNum();
long minBlockNum =
peer.getLastSyncBlockId().getNum() - 2 * NetConstants.SYNC_FETCH_BATCH_NUM;
if (blockNum < minBlockNum) {
throw new P2pException(TypeEnum.BAD_MESSAGE,
"minBlockNum: " + minBlockNum + ", blockNum: " + blockNum);
}
if (peer.getSyncBlockIdCache().getIfPresent(hash) != null) {
throw new P2pException(TypeEnum.BAD_MESSAGE, new BlockId(hash).getString() + " is exist");
}
peer.getSyncBlockIdCache().put(hash, System.currentTimeMillis());
}
}
|
Hi! Is there a related issue that explains the background of this PR? Or is this PR still a work in progress? |
|
The |
This PR was raised early, and there is no corresponding issue yet. |
| for (Sha256Hash hash : fetchInvDataMsg.getHashList()) { | ||
| Item item = new Item(hash, type); | ||
| if (isAdv) { | ||
| peer.getAdvInvSpread().invalidate(item); |
There was a problem hiding this comment.
Could you explain the reason for adding this operation here?
There was a problem hiding this comment.
A broadcast list can only be requested once. After a peer has requested a list, it can be removed from the cache. If it is requested again the next time, the check will fail, thus preventing the same list from being requested multiple times.
There was a problem hiding this comment.
If requests are allowed multiple times, is it possible that it could lead to a potential attack?
There was a problem hiding this comment.
If multiple requests are allowed, there is a DDoS attack.
There was a problem hiding this comment.
The logic is too jumpy, and there are no comments here, which makes it confusing. You could add comment like this: cache the Inventory sent to the peer; once a FetchInvData message is received from the peer, remove this Inventory from the cache. If the same FetchInvData request is received from the peer again and it is no longer in the cache, then reject the request.
| } | ||
|
|
||
| private void check(PeerConnection peer, FetchInvDataMessage fetchInvDataMsg) throws P2pException { | ||
| public boolean isAdvInv(PeerConnection peer, FetchInvDataMessage msg) { |
There was a problem hiding this comment.
I have a question, isAdvInv function takes into account the scenario of block broadcasting. Why doesn't the check function consider the scenario of block broadcasting?
There was a problem hiding this comment.
The broadcast scenario is also considered. For example, transactions belong to broadcast. The block will determine whether it is broadcast or synchronized. However, the broadcast block does not require other checking logic.
There was a problem hiding this comment.
Actually, the real question for me lies in your last sentence., why the broadcast block does not require other checking logic ? Is there a risk of duplicate requests of broadcast block as broadcast transaction?
…tocol#5895) * feat(net): optimize fetch inventory message broadcast processing logic * feat(net): solve checkstyle problem * add the missing code in the code conflict
…tocol#5895) * feat(net): optimize fetch inventory message broadcast processing logic * feat(net): solve checkstyle problem * add the missing code in the code conflict
What does this PR do?
Optimize fetch inventory message processing logic.
Why are these changes required?
This PR has been tested by:
Follow up
Extra details