Skip to content

fix(net): correct sync completion and chain summary request timeouts - #7000

Open
317787106 wants to merge 7 commits into
tronprotocol:release_v4.8.3from
317787106:fix/sync_complete
Open

317787106 wants to merge 7 commits into
tronprotocol:release_v4.8.3from
317787106:fix/sync_complete

Conversation

@317787106

Copy link
Copy Markdown
Collaborator

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.

  • Check syncChainRequested against the existing five-second SYNC_TIME_OUT, using the original request timestamp. The existing periodic status check disconnects the responsible peer with TIME_OUT after the threshold is exceeded, regardless of sync direction flags. PING/PONG, valid inbound requests, and other block progress do not extend this deadline.
  • Reject negative remaining-block counts and prevent arithmetic overflow from bypassing the existing future-height limit. Validate responses against the captured request before clearing it or changing peer state.
  • For a known single-block response from the requested summary with zero remaining blocks, reset remainNum and finish the local download. Preserve needSyncFromUs for both the summary tail and earlier blocks: an existing upload requirement remains active, and a new one is not inferred from the response.
  • Retain TronState.SYNC_COMPLETED so 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=true locally 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:

  • 55 tests passed across nine related classes, including 30 tests added by this PR, with zero failures, errors, or skipped tests.

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.

@github-actions
github-actions Bot requested a review from xxo1shine September 24, 2026 09:15
Comment on lines 55 to 61
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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants