Conversation
…quested.getKey().peekLast()
| if (blockIdWeGet.size() == 1 && tronNetDelegate.containBlock(blockIdWeGet.peek())) { | ||
| peer.setRemainNum(0); | ||
| peer.setTronState(TronState.SYNC_COMPLETED); | ||
| // This trusts the peer's claim that no blocks remain. A peer can echo an earlier | ||
| // known summary block while withholding newer blocks, so ending this download | ||
| // does not prove that we have caught up with the peer's actual chain. todo fix | ||
| peer.setNeedSyncFromPeer(false); |
There was a problem hiding this comment.
[SHOULD] A single known block can still prematurely terminate synchronization despite being inconsistent with the peer’s advertised height.
If the peer advertises head=200 in HELLO and the local node sends summary [0,50,100], a response containing only [50] with remainNum=0 still passes check(). It then clears syncChainRequested, sets SYNC_COMPLETED, and sets needSyncFromPeer=false. When needSyncFromUs=false, isSyncFinish() also becomes true, so the connection immediately exits both the response-timeout check and the 30-second sync-no-progress check.
Please handle terminal responses that are inconsistent with the height previously advertised by the peer, while preserving appropriate no-progress handling and compatibility with normal responses from honestly lagging peers. Simply rejecting responses that end before the last block in the summary is insufficient: [100], remainNum=0 produces the same result.
There was a problem hiding this comment.
Addressed in 4e46755.
check() now rejects single-block responses below the peer’s advertised HELLO head before clearing the pending request or changing synchronization state. Both [50], remainNum=0 and [100], remainNum=0 therefore trigger a SYNC_FAIL disconnect when HELLO advertised height 200.
Normal responses from lagging peers remain accepted—for example, HELLO=50 followed by [50], 0 when our local head has advanced to 100. A response observed during a temporary rollback can also trigger disconnection; using SYNC_FAIL allows recovery through a fresh handshake without the one-hour BAD_PROTOCOL ban.
Regression tests cover rejection, unchanged synchronization state, the actual disconnect reason, and compatibility with lagging peers. All 58 related tests and both Checkstyle checks passed locally.
| } | ||
| } | ||
|
|
||
| if (blockIds.size() == 1) { |
There was a problem hiding this comment.
[SHOULD] Apply the HELLO head check to multi-block terminal responses as well.
The head check is currently limited to single-block responses, which still allows a peer to return remainNum=0 with 2–3 consecutive, locally known blocks whose final height is below the HELLO head. As long as the first block comes from the current summary, the response passes validation.
The known blocks are then popped and passed to setBlockBothHave(), refreshing the timestamp used by the no-progress timeout. Once the queue is empty, syncNext() sends another request and refreshes the inventory request time. Repeating this can keep the sync connection alive without any height progress while continuously triggering summary construction.
Keep the existing single-block rule, but for all responses with remainNum == 0, require the last block height to be at least the HELLO head height, and reject invalid responses before mutating peer state.
Add regression cases for two- and three-block responses, asserting SYNC_FAILED and that syncNext() is not called. Normal paginated responses with remainNum > 0 should still be allowed to end below the HELLO head.
What does this PR do?
Add a response timeout for outstanding chain-summary requests and harden chain-inventory validation, while preserving upload-direction state when the local download finishes.
syncChainRequestedagainst the existing five-secondSYNC_TIME_OUT, using the original request timestamp. The existing periodic status check disconnects the responsible peer withTIME_OUTafter the threshold is exceeded, regardless of sync direction flags. PING/PONG, valid inbound requests, and other block progress do not extend this deadline.remainNumand finish the local download. PreserveneedSyncFromUsfor both the summary tail and earlier blocks: an existing upload requirement remains active, and a new one is not inferred from the response.TronState.SYNC_COMPLETEDso a later download can restart. Unknown queued blocks still follow the fetch path, and normal multi-block validation and paged downloads remain in place.Why are these changes required?
The existing block-progress timeout does not provide a deadline tied to an individual chain-summary request. A peer must answer that request even when other traffic or block activity continues.
A single-block response also does not initiate a download on the remote peer. For example, if our summary ends at block 100 and an existing peer replies with the known block 50, setting
needSyncFromUs=truelocally can suppress normal inventory exchange while waiting for a remote download that never starts. Preserving the upload flag keeps BLOCK/TRX inventory exchange compatible with existing peers and retains any upload synchronization already in progress.This PR has been tested by:
Follow up
Ending the local download still trusts the peer's claim that no blocks remain. A peer can echo an earlier known summary block while withholding newer blocks; this PR does not verify the peer's actual head. Chain-inventory responses do not receive block contribution credit.
When integrating with #6993, retain both Hello and chain-summary timeout checks in the common status-check flow.