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

[317787106]

What does this PR do?

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

• Increase the shared SYNC_TIME_OUT from five to ten seconds. It applies to both outstanding syncChainRequested chain-summary requests and requested sync blocks. A sync fetch can request up to 100 blocks from one peer in a single batch. The periodic status check disconnects the peer with TIME_OUT after the threshold is exceeded. Chain-summary requests use their original request timestamp; other traffic and block progress do not extend that 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 five-second timeout could expire before all blocks in a batch of up to 100 arrive from a responsive peer, causing a premature disconnect and interrupting synchronization. Ten seconds gives the batch more time while retaining a deadline for unresponsive peers. The block-progress timeout also does not provide a deadline for an individual chain-summary request. Repeated terminal responses containing known blocks below the HELLO head can 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:

• Before the timeout adjustment, 59 tests passed across eight related classes on ARM64 / JDK 17, with no failures, errors, or skipped tests. After the adjustment, 12 focused tests in PeerStatusCheckMockTest and PeerStatusCheckTest passed. These cover six-second chain-summary and sync-block requests remaining active, as well as requests past the ten-second threshold timing out.
• 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 and checkstyleTest passed during development. After the timeout adjustment, :framework: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.

[3for]

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

[317787106]

Thanks for the review. Fixed in 562e004. After removing locally known blocks, a terminal inventory with an empty fetch queue now completes the download without calling syncNext(), while preserving needSyncFromUs. I updated the regression test to cover two- and three-block responses with needSyncFromUs both true and false.

[SeriousCoding789]

The fixes here look correct to me: the request snapshot is validated before any state change, the future-height check no longer overflows, negative remainNum is rejected, and the new chain-inventory timeout uses the request's own timestamp.

One gap seems to remain beyond the terminal-response case raised in the review above. Intermediate pages (remainNum > 0) are not covered by the HELLO-head check, and a full page made up only of blocks we already have can pass every check without moving blockBothHave forward; the handler then drains the queue and calls syncNext() again. The underlying reason is that the 30-second progress check in PeerStatusCheck looks at blockBothHaveUpdateTime, which setBlockBothHave() refreshes whether or not the height increased, so activity is counted as progress.

Two possible directions:

• For remainNum > 0, require the last block of the page to be higher than peer.getBlockBothHave().getNum(). An honest page starts from the highest summary block on the peer's main chain and spans SYNC_FETCH_BATCH_NUM blocks, and fork depth is bounded by the solidified height, so an honest page should always end above blockBothHave.
• More generally, refresh blockBothHaveUpdateTime only when the shared height increases, so any response shape that does not advance synchronization is caught by the existing 30-second check. The other setBlockBothHave() callers (broadcast handling, serving fetches) would need to be checked against that change.

[3for]

[SHOULD] Premature completion of a multi-block terminal response leaves the remote peer in upload state.

Introduced by commit 562e004e31.

The new early return can complete local synchronization without clearing the remote peer's upload state.

Reproduction scenario:

1. Peer A is at height 100 and requests a chain summary from peer B at height 102.
2. B responds with [100, 101, 102] and remainNum=0, setting needSyncFromUs=true on its connection.
3. Before the response arrives, A obtains blocks 101 and 102 from other peers.
4. The new handler finds all returned blocks already known, completes synchronization, and returns without sending the final summary confirmation.

A now considers synchronization complete, but B still believes A is downloading. Consequently, B drops subsequent block and transaction announcements from A and stops broadcasting announcements to A until another synchronization or reconnection resets the state.

The remote upload flag is cleared only when a subsequent summary request produces a single-block response in SyncBlockChainMsgHandler.java (line 48). The affected announcement filters are in InventoryMsgHandler.java (line 72) and AdvService.java (line 306).

Confirmed by a two-peer handler regression test:

Behavior	Current code	Pre-change handler
A completes synchronization	Yes	Yes
B clears upload state	No	Yes
B accepts subsequent block/transaction announcements	Both dropped	Both accepted

Please preserve a bounded final confirmation exchange so the remote upload state is cleared, while retaining protection against repeated summary loops. Add a two-peer regression test covering this multi-block terminal response.

The existing multi-block tests only verify local state and therefore miss this cross-peer regression.

[317787106]

Thanks for pointing out the asymmetric peer state. Fixed in 82feab3: when all blocks in a multi-block terminal response are already available locally, we send an updated chain summary instead of completing sync immediately. The remote peer can then return a single-block response and clear needSyncFromUs. I added a two-peer regression test covering sync completion and subsequent inventory processing.

[xxo1shine]

Please also consider reducing ADV_TIME_OUT. The current 20-second timeout for block and transaction fetch requests following broadcast announcements is rather long, delaying the handling of unresponsive peers and the rescheduling of outstanding requests.

[xxo1shine]

The HELLO null check here duplicates the centralized entry-point check in #6993. Once #6993 is merged, CHAIN_INVENTORY messages received before handshake completion will be rejected before dispatch, so this handler does not need to check again.

[xxo1shine]

Synchronization should not be marked complete here solely because remainNum is zero and the fetch queue is empty. When the remote peer returns multiple blocks, it sets needSyncFromUs=true. We still need to send an updated chain summary through syncNext so that the remote peer can return a single common block and clear that state. Local synchronization can then complete through the existing single-block branch.

Please remove this early-completion branch. Repeated responses containing known blocks from a malicious peer should be handled by separately limiting repeated exchanges that make no progress, without skipping the normal final confirmation exchange. Tests should also cover synchronization state on both peers and verify that subsequent announcement exchange resumes.

[317787106]

Agreed. I removed the early completion branch in 82feab3. To prevent the previous no-progress loop, the handler now rejects a multi-block terminal response if its terminal block was already included in the requested summary; at that point, a normal peer should return a single block. A regression test covers the repeated-response case.