Skip to content

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

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

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

Conversation

@317787106

@317787106 317787106 commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fix chain-summary request timeouts and local sync completion, and reject terminal chain-inventory responses that contradict the peer's HELLO head.

  • Check outstanding syncChainRequested requests against the existing five-second SYNC_TIME_OUT, using the original request timestamp. The periodic status check disconnects the peer with TIME_OUT after the threshold is exceeded, regardless of sync direction flags. Other traffic and block progress do not extend this deadline.
  • Reject negative remaining-block counts and prevent overflow from bypassing the future-height limit. Validate responses against the captured request before clearing it or changing peer state.
  • For every response with remainNum == 0, require the last block height to be at least the HELLO head. Reject lower terminal responses with SYNC_FAILED, mapped to a SYNC_FAIL disconnect. Intermediate pages with remainNum > 0 may still end below HELLO.
  • For a valid, known single-block response from the requested summary, reset remainNum and finish the local download while preserving needSyncFromUs. Keep TronState.SYNC_COMPLETED so a later download can restart; unknown queued blocks continue through the fetch path.

Why are these changes required?

The block-progress timeout does not provide a deadline for an individual chain-summary request. Repeated terminal responses containing known blocks below the HELLO head can also refresh progress timestamps and trigger more summary requests without advancing synchronization. The terminal-height check closes this gap.

A single-block response does not initiate a remote download. For example, when HELLO advertised height 50 and our summary ends at 100, a known-block response [50], remainNum=0 can legitimately finish our download. Setting needSyncFromUs=true would instead suppress normal inventory exchange while waiting for a remote download that never starts. Preserving the upload flag retains any existing upload synchronization without inventing a new one.

This PR has been tested by:

  • 59 tests passed across eight related classes on ARM64 / JDK 17, with no failures, errors, or skipped tests.
  • Regression coverage includes single-block, two-block, three-block, and maximum-size terminal responses; rejection before peer-state mutation; no follow-up syncNext() on rejection; the actual SYNC_FAIL disconnect reason; valid terminal heights; and pagination below HELLO.
  • checkstyleMain, checkstyleTest, and git diff --check passed.

Compatibility and integration notes

In rare cases, a fork rollback can temporarily put an honest peer below its HELLO head and cause a SYNC_FAIL disconnect. This transient disconnect is an accepted tradeoff: the peer may reconnect with a fresh HELLO after the default one-minute cooldown. Reconnection is not guaranteed exactly one minute later.

The height check enforces consistency with the peer's HELLO advertisement; it does not prove the peer's actual current 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
}
}

if (msg.getRemainNum() == 0) {

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] Complete synchronization for terminal inventories containing only known blocks.

This guard still accepts a multi-block terminal response once its last block reaches the HELLO height. If every returned block is already known, processMessage() drains syncBlockToFetch but falls through to syncNext() at lines 95–101, leaving needSyncFromPeer=true.

A peer can repeatedly return a summary-linked suffix of known blocks with remainNum=0. Each round refreshes both the shared-block timestamp and the inventory-request timestamp, and rebuilds the summary under forkLock. For prolonged operation, the peer can periodically advance the suffix to blocks already obtained from honest peers before the summary’s lower bound overtakes it.

Please complete synchronization when remainNum == 0 && syncBlockToFetch.isEmpty() after removing known blocks, preserving needSyncFromUs. The existing testKnownMultiBlockResponseRequestsNextSummary currently asserts the redundant continuation and should instead assert completion and no further syncNext() call.

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