Update syncing logic to fix duplicate block requests #3410
Conversation
kpandl
force-pushed
the
fix-sync-duplicate-block-request
branch
from
9f546d8 to
88d5574
Compare
node/sync/src/block_sync.rs
Outdated
| } | ||
| }, | ||
| Err(err) => { | ||
| warn!("{err}"); |
There was a problem hiding this comment.
note: this is a change that might surprise users who relied on the format of the old log
There was a problem hiding this comment.
Thanks, good find! I've added more expressive warnings similar to the previous code here: d6e4f73
Co-authored-by: ljedrz <3750347+ljedrz@users.noreply.github.com>
| } | ||
|
|
||
| /// Removes the block request for the given peer IP, if it exists. | ||
| #[allow(dead_code)] | ||
| fn remove_block_request_to_peer(&self, peer_ip: &SocketAddr, height: u32) { | ||
| let mut can_revoke = self.responses.read().get(&height).is_none(); | ||
| let mut can_revoke = self.process_next_block(height).is_none(); |
There was a problem hiding this comment.
Why is remove_block_request_to_peer now calling process_next_block instead of just removing it from the responses?
There was a problem hiding this comment.
Nevermind, I answered my own question. You are now nesting the response removal inside process_next_block and adding some additional checks.
There was a problem hiding this comment.
while I also believe it to be fine, an adjustment to the function name or extending the doc comment could be beneficial here
Motivation
As issue #3404 described, validators send out duplicate block requests when syncing. This PR updates the syncing logic to make sure we only remove a block response after it was added to the ledger or if we encountered an error.
For background, currently, syncing works as follows:
BlockSyncrequests the blocks, and stores it in the internalresponsesmap.Synccallsblock_sync.process_next_block(current_height)to get the next block to process. This removes it fromBlockSync’sresponsesand returns it toSync.Syncperforms the checks, and if successful, adds the block to theledger. This also updates BlockSync’scanon.The issue is that
BlockSync‘s view of thecanonis only updated afterSyncis done processing blocks (and adding to the ledger). There’s a window whereBlockSyncis unaware of blocks that are pending validation inSync. Thus, it re-requests them.This PR adjusts the block processing interface to the
Syncmodule as follows:process_next_blockREADS a block from a particular heightremove_block_responseREMOVES a block from a particular height, which MUST be called after advancing completed or failedTest Plan
Ran it locally and verified it removes the duplicate block request.
Also ran it with light load in a cloud devnet, and verified syncing works for validators and clients.
The following figure shows a syncing process before the fix, with a longer pause until a duplicate request is generated:
The following figure shows the syncing process with the fix (note there is no double request and no delay, as such, the syncing is much faster):
Closes #3404