[317787106]

What does this PR do?

1. Reject terminal (remain_num=0) chain inventory replies whose last block height is below the peer's HELLO head or a recent BLOCK announcement received before the sync request. Also reject multi-block terminal replies whose final block already appears in the requested summary. These validation failures disconnect the peer with SYNC_FAIL.
2. Reduce blockNotChangeThreshold from 60 seconds to 30 seconds, and run disconnectIsolated2 every 10 seconds instead of 30 seconds.

Why are these changes required?

A peer can announce and deliver newer blocks while answering a sync request with only a block already known locally. Accepting that reply marks the connection synchronized without identifying the missing blocks. The validation breaks this no-progress pattern.

The isolation-recovery timing must also leave time to find another data source before the next block-production opportunity. Under the normal 27-witness schedule, an SR's production cycle is 27 × 3 = 81 seconds. Saving a locally produced block, or a fresh block received near that slot, resets latestSaveBlockTime. If the connected peers then stop supplying usable blocks, the node may remain stalled until isolation recovery frees connection slots and another source becomes available.

For an already-running node that meets the isolation conditions, the no-progress threshold plus the polling interval should stay comfortably below 81 seconds, leaving time for peer discovery, handshake, fetching, and catch-up:

Configuration	No-progress threshold	Check interval	Nominal recovery trigger	Time left in an 81-second cycle
Previous	60s	30s	About 60–90s	At most about 21s; recovery can start after the next slot
Updated	30s	10s	About 30–40s	About 41–51s

The updated budget is 30 + 10 = 40 seconds, leaving roughly 40 seconds to try alternative data sources before the next production slot when the existing sources are ineffective. These are scheduling estimates, not a guarantee of finding a usable peer or completing catch-up.

The threshold must not be made arbitrarily small: local GC pauses and disk stalls can temporarily stop block progress even when peers are healthy. Keeping a 30-second allowance balances earlier recovery with tolerance for these local interruptions.

This PR has been tested by:

• 43 targeted unit tests in ChainInventoryMsgHandlerTest, InventoryMsgHandlerTest, and SyncServiceTest.
• All 4 tests in ResilienceServiceTest passed for the isolation-recovery change.
• ./gradlew clean build -x test.
• ./gradlew :framework:checkstyleMain :framework:checkstyleTest :plugins:checkstyleMain.
• bash .github/scripts/check_math_usage.sh and git diff --check.
• Compared the validation conditions with FullNode synchronization-stall logs. No live-node test was performed; the full test suite was not run.

Follow up

Evaluate proactive synchronization recovery and the isolation-recovery peer-selection, eviction-frequency, and penalty policies separately.

Extra details

Fork rollbacks and broadcast-before-apply timing can temporarily contradict an earlier handshake, summary, or announcement. The terminal-reply checks are a defensive synchronization-source policy and accept occasional recoverable disconnects; they do not establish malicious intent.

The isolation change adjusts the threshold and polling interval while retaining the existing startup delay, eligibility conditions, peer selection, and disconnect reasons.

The diff also raises the VerifyTransferProof execution budget in two test helpers from 80ms to 200ms.

[xxo1shine]

This also treats a temporary inconsistency caused by a legitimate fork switch as a sync failure. For example, a peer can advertise height 200 in HELLO, switch forks and temporarily roll back to 198, then be disconnected for returning lastNum=198, remainNum=0. A fork switch interleaved with summary generation can produce a similar result.

Although the PR description and tests explicitly accept this behavior, recovery still requires the connection cooldown, reconnection and a new handshake. Please consider bounded renegotiation before disconnecting a peer for a persistent inconsistency, and add coverage for an honest peer recovering synchronization after a fork switch.

[317787106]

Thanks for pointing this out. We agree that a legitimate fork switch can temporarily make the current head inconsistent with the earlier HELLO snapshot, and that interleaving a fork switch with summary generation can produce a similar response. This PR intentionally accepts a recoverable disconnect in these cases; the check is not intended to establish that the peer is malicious.

For the HELLO-triggered initial synchronization case, the peer advertised a higher head, so we attempted to synchronize from it. If a concurrent fork switch makes that attempt fail the consistency checks, we consider it acceptable to abandon this connection’s initial synchronization attempt. More precisely, this is a post-handshake initialization failure, since the HELLO exchange itself has already completed.

With other available peers, the node does not need to wait for this particular peer’s cooldown, reconnection, and fresh handshake before making synchronization progress. SYNC_FAIL follows the ordinary disconnect/cooldown path rather than requesting the longer penalty associated with BAD_PROTOCOL or BAD_BLOCK. We therefore accept the cost of temporarily losing this candidate synchronization source. The same policy applies when an existing connection encounters this inconsistency during a later synchronization attempt.

Bounded renegotiation could preserve some honest connections, but it would introduce retry accounting, timeout handling, and additional state transitions. Given the intended tradeoff—accepting occasional disconnects while avoiding repeated unproductive synchronization attempts—we do not consider same-connection renegotiation necessary for this PR.

The existing fork-switch test deliberately expects SYNC_FAIL to document this policy. Additional coverage showing that a fresh connection can synchronize after the fork has settled would be useful, but it can be added independently of bounded renegotiation.

[xxo1shine]

The .max() operation traverses the entire advInvReceive cache, including transaction announcements that are subsequently filtered out. The cache has a capacity of 20,000 entries, so a terminal reply can require scanning approximately twenty thousand entries.

Please consider maintaining recent BLOCK announcements separately, avoiding a scan of the entire mixed transaction/block cache just to check a small number of block announcements.

[317787106]

The observation that .max() traverses the mixed cache is correct. However, I do not think this alone justifies introducing a separate BLOCK-announcement cache in this PR.

First, this operation is a single-pass, in-memory reduction with O(N) complexity, rather than a sorting operation. Transaction entries are rejected by the first type filter. Timestamp filtering and BlockId construction are performed only for the relevant BLOCK entries, with block-number extraction limited to announcements eligible for this request. The scan itself performs no database reads or network operations.

Second, 20,000 is the cache’s maximum capacity, not its actual entry count on every invocation. We should still account for a full cache, but the worst-case work remains a bounded linear traversal. Capacity establishes an upper bound; it does not demonstrate an unacceptable processing delay.

Third, the invocation frequency matters. This check executes only for terminal chain inventory replies (remain_num=0) associated with an outstanding synchronization request. It is not executed for every transaction announcement, every block announcement, or every normally received block. In normal steady-state operation, this is a low-frequency synchronization path rather than a per-message hot path.

The assessment also includes visiting entries outside the 15-second eligibility window. That window determines which announcements can constrain the response; it does not remove those entries from the cache or eliminate their traversal cost. The justification is the combination of bounded work, linear complexity, and low invocation frequency.

Maintaining BLOCK announcements separately would add per-peer state and require coordinated updates, expiration, cleanup, and concurrency handling. It would also need to preserve the current semantics: considering recent announcements received before the particular synchronization request, while excluding later announcements. That is additional complexity for a performance benefit that has not yet been quantified.

This scan has a nonzero cost, but there are currently no measurements showing that it causes meaningful handler latency or allocation pressure. I therefore suggest retaining the existing implementation in this focused correctness fix. If profiling demonstrates a material bottleneck, a separate BLOCK-announcement structure can be evaluated as a follow-up optimization.

[xxo1shine]

The existing BlockMsgHandler broadcasts a block before calling tronNetDelegate.processBlock. A peer can therefore announce block 100, subsequently fail to apply it locally, and remain at canonical height 99.

If we initiate a sync request within 15 seconds of receiving that announcement, the peer can truthfully return lastNum=99, remainNum=0 and still trigger SYNC_FAILED here because it previously announced 100. An announcement establishes that the peer forwarded the block, not that its canonical chain successfully applied it.

Please avoid treating a terminal reply as a sync failure solely on the basis of the announced height, and add a regression test for broadcasting a block followed by a local application failure.

[317787106]

We agree that BlockMsgHandler can broadcast a block before local application succeeds. Therefore, a previous announcement does not prove that the block was successfully applied to the peer’s canonical chain. An honest peer can encounter the scenario described here.

However, this check is intended as a defensive synchronization-source policy. It addresses a repeated no-progress cycle observed in a production FullNode synchronization-stall incident.

A malicious peer can exploit the following sequence:

1. Advertise blocks and become a source from which the receiver fetches block data.
2. Exploit an existing synchronization delay, or contribute to it by delaying a required parent block. Subsequently delivered higher blocks cannot be linked locally, causing the receiver to request synchronization.
3. Answer SYNC_BLOCK_CHAIN with only the already-known block at the end of the receiver’s summary, together with remain_num=0. This supplies no missing block IDs, despite the peer having recently announced and delivered higher blocks.
4. The existing receiver treats that singleton response as synchronization completion and resumes accepting announcements from the connection.
5. Repeat the cycle with newer blocks. The connection remains eligible as a broadcast source while repeatedly failing to help recover the missing chain. This can prolong the stall until another peer provides useful synchronization data.

The following is a sanitized, condensed extract from the observed logs. Peer identities and hashes are removed, heights are expressed relative to the stalled local head H, and timestamps are relative to when H was processed. The original comparisons used complete hashes and the same connection lifetime.

T+00.000  LOCAL: head=H

T+04.099  Peer A -> LOCAL: INVENTORY, BLOCK=H+3
T+05.218  Peer A -> LOCAL: BLOCK, height=H+3
T+05.220  LOCAL -> Peer A: SYNC_BLOCK_CHAIN, summary tail=H
T+05.399  Peer A -> LOCAL: BLOCK_CHAIN_INVENTORY,
                         size=1, last=H, remain=0

T+07.077  Peer A -> LOCAL: INVENTORY, BLOCK=H+4
T+07.730  Peer A -> LOCAL: BLOCK, height=H+4
T+07.732  LOCAL -> Peer A: SYNC_BLOCK_CHAIN, summary tail=H
T+07.912  Peer A -> LOCAL: BLOCK_CHAIN_INVENTORY,
                         size=1, last=H, remain=0

... further repetitions ...

T+61.097  Peer A -> LOCAL: INVENTORY, BLOCK=H+22
T+61.465  Peer A -> LOCAL: BLOCK, height=H+22
T+61.467  LOCAL -> Peer A: SYNC_BLOCK_CHAIN, summary tail=H
T+61.647  Peer A -> LOCAL: BLOCK_CHAIN_INVENTORY,
                         size=1, last=H, remain=0

T+61.747  Peer B -> LOCAL: BLOCK_CHAIN_INVENTORY,
                         blocks=[H..H+22], remain=0
T+62.567  LOCAL -> Peer B: FETCH_INV_DATA, blocks=[H+1..H+22]
T+63.350  LOCAL: successfully processes H+1

This was a 63.350-second interval without advancement beyond H, despite continuing to receive newer block announcements and block bodies. Progress resumed after another peer supplied the missing range.

Across the inspected log file, we found 36 such replies from 10 peer endpoints, out of 80 received ChainInventory replies. All 36 were singleton replies echoing the exact requested summary-tail hash with remain_num=0. In every case, the same connection had announced a higher block 71–1505ms before the synchronization request and had already delivered its matching block body. The other 44 replies had no preceding same-connection BLOCK INV in the available log and were not comparable under this metric. These are reply counts, not 36 independent stalls.

These observations establish the repeated message pattern, not malicious intent. Even receiving the higher block body does not prove that the peer successfully applied it. The defensive decision does not require that proof: we temporarily stop relying on a source whose recent announcements and terminal synchronization response indicate inconsistent availability.

Consequently, an honest peer affected by a fork or application failure may also be disconnected. We intentionally accept that possibility, using SYNC_FAIL and the ordinary disconnect/cooldown path. With alternative peers available, temporarily losing one candidate source is an acceptable cost compared with continuing a synchronization cycle that repeatedly makes no progress.

The core tradeoff is to accept occasional false-positive disconnects to reduce the risk of sustained synchronization stalls. This check breaks continued reliance on that connection; it does not claim to identify malicious peers conclusively or guarantee immediate recovery.

[0xbigapple]

Thanks for the log extract, it makes the target clear. All 36 bad replies have the same shape: size=1, last=<summary tail>, remain=0, i.e. they hit the blockIdWeGet.size() == 1 && containBlock(...) shortcut in processMessage that marks the peer SYNC_COMPLETED. That shortcut is the only path where the harm in the PR description can happen; a multi-block terminal reply queues the missing ids into syncBlockToFetch and recovers via fetch / syncNext.

The three throws in check() are wider than that. A multi-block reply ending below the HELLO head or below a recent announcement (the exact [H, H+1, H+2] an honest peer returns while it is still applying H+3, since BlockMsgHandler only relays blocks whose parent it already holds) carries useful ids and now costs the connection, and the size() > 1 summary-membership check cannot match any of the 36 samples at all.

Suggest scoping the policy to the observed signature: throw SYNC_FAILED only when the reply is a singleton echo of the summary tail with remain_num=0 and the peer announced a higher block in the window before the request. Multi-block replies keep the existing fetch path. That covers 100% of the incident pattern without the honest-peer disconnects in this thread and in the L158 thread, and does not need bounded renegotiation.

[317787106]

The broadcast-before-apply race is possible, but its practical likelihood needs to account for the complete message exchange. Receiving a BLOCK INV does not directly trigger synchronization. The receiver first fetches the block and discovers that its parent is missing:

Peer P                                      Local node W

Broadcast INV(B) -------------------------->
                 <-------------------------- FETCH_INV_DATA(B)
Send BLOCK(B) ----------------------------->
                                             Detect missing parent
                                             Build chain summary
                 <-------------------------- SYNC_BLOCK_CHAIN
Read current head
Generate ChainInventory ------------------->

From P broadcasting the inventory to P reading its head for the synchronization response, this involves approximately two network round trips, plus block transfer, message processing, summary generation, and any queueing:

T ≈ 2 × RTT + transfer / processing / queueing time

Using 300ms as an illustrative block execution time, an RTT of 150–180ms already accounts for approximately 300–360ms before those additional costs. Under normal, successful block processing, P will therefore often have applied the advertised block by the time it handles SYNC_BLOCK_CHAIN.

The incident logs also support this timing argument. Among the 36 previously documented contradictory replies:

• In 25 cases (69.44%), the interval from the local node receiving the preceding BLOCK INV to sending SYNC_BLOCK_CHAIN already exceeded 300ms.
• The median interval was 368.5ms.
• These intervals exclude the initial INV delivery time and the subsequent delivery of SYNC_BLOCK_CHAIN to the responder.

Assuming execution starts promptly after broadcasting and successfully completes within 300ms, those 25 cases cannot be explained solely by the responder still executing that advertised block.

This does not eliminate the race for low-latency connections, unusually slow execution, application failures, or fork switches. However, the existence of the race alone does not establish that honest-peer disconnects would be frequent. In the observed deployment, the complete exchange often provided more time than the assumed block execution duration. We accept occasional defensive disconnects to reduce the risk of sustained synchronization stalls.

[317787106]

I also measured the actual local block-processing times from the production FullNode logs, covering approximately 9.5 hours.

The dataset contains 10,968 successfully processed broadcast blocks, excluding duplicate-block fast returns and other cases without actual block processing.

All durations below are in milliseconds:

Measurement	Samples	Mean	Median	P95	P99	Maximum	≤300ms
Manager.pushBlock processing	10,968	110.43	106	151	173	783	99.87%
“Ready to broadcast” → “Success process block”	10,968	112.69	108	154	176	786	99.87%
Same broadcast-to-success interval within the 10-minute incident window	168	151.83	147.5	181	531	568	98.81%

The broadcast-to-success measurement is particularly relevant here: it includes broadcast overhead and subsequent lock waiting, rather than measuring only transaction execution. Records were matched by thread and complete block ID.

These measurements make 300ms a conservative reference for ordinary broadcast-block processing on this FullNode, although longer processing times do occur.

For comparison, in 33 of the 36 contradictory-reply cases (91.67%), the local interval from receiving the preceding BLOCK INV to sending SYNC_BLOCK_CHAIN already exceeded the measured 176ms P99 broadcast-to-success interval. This excludes the initial INV delivery time and the subsequent delivery of the synchronization request to the responder.

These are local measurements, not direct measurements of the remote peers. Nevertheless, they support the timing argument: the complete INV → FETCH → BLOCK → SYNC exchange will often outlast ordinary block processing. The broadcast-before-apply race remains possible, but its existence alone does not establish that this check would frequently disconnect honest peers.