fix(net): correct block validation, fetching, and peer contribution

[317787106]

What does this PR do?

Fix block-handling paths where unverified announcements can refresh peer activity, invalid responses can complete fetches too early, and a short fetch timeout can leave later announcements unable to trigger a retry. The changes also separate transaction requests from block-fetch eligibility and make blockRcvTime reflect the result of processing a block supplied by that peer.

This is the "block fetching, validation, and contribution timestamps" work described as PR 2 in peer-quality-evaluation-design.md (sections 4.3–4.5, 5.2–5.3, and 6.3). Related parts of that design have separate PRs:

Scope	PR
Application-layer Hello admission, Hello timeout, and locally verifiable head checks	tronprotocol/java-tron#6993
Normal sync completion, chain-inventory validation, and syncChainRequested timeout	tronprotocol/java-tron#7000
BLOCK INV activity, block validation/results, block-fetch scheduling, retry, and disconnect recovery	This PR

These are separate fixes; #6993 and #7000 are not code dependencies of this PR. The details below describe the implementation against release_v4.8.3.

1. Unverified BLOCK INV can refresh lastInteractiveTime

Bug and impact

InventoryMsgHandler previously updated lastInteractiveTime when a BLOCK INV advertised a height above the local head and advInvSpread did not contain the item. At that point, the peer had supplied only a block hash. The node had not received or validated the block.

A peer could therefore advertise invented block IDs to appear active without delivering valid block data. At the same time, a peer that announces a valid block may not be selected to provide its body, so its announcement needs to be distinguished from actual block delivery.

Solution

Call AdvService.recordInventory(peer, item, receivedAt) before addInv(). Continue recording inventory in advInvReceive, and record an eligible BLOCK announcement's receipt time separately in PeerConnection.advBlockInvReceive.

Eligibility is captured at receipt: the block height must exceed the local head, and advInvSpread must not contain the item. Capturing it before fetching matters because an immediate response can advance the head or broadcast the block before the inventory handler finishes.

After the matching block is successfully processed through the broadcast or sync path, and after the isHitDown() check, call confirmBlockInventory(blockId). It removes the matching pending entry and calls updateLastInteractiveTime(receivedAt). A different hash at the same height does not confirm the announcement.

Confirmation uses the recorded receipt time, and updateLastInteractiveTime() only advances the timestamp. Announcements do not update blockRcvTime. The pending cache holds at most 100 entries, expires entries one minute after write, and is cleared on disconnect; its eviction does not remove advInvReceive information needed for fetching.

2. Invalid BLOCK responses can complete a fetch before validation

Bug and impact

The broadcast-block path removed the responding peer's advInvRequest entry and called blockFetchSuccess() before tronNetDelegate.validBlock(). A requested block with a bad signature or Merkle root could therefore clear both the request record and retry tracking before being rejected.

When the peer was then disconnected, AdvService.onDisconnect() could no longer use that request entry to retry the block from another peer.

Solution

In BlockMsgHandler.processMessage(), check the raw parent hash length and the parent/child block numbers before sanitize(). Reject a nonpositive block number, a parent hash whose size is not Sha256Hash.LENGTH, or a parent number other than blockNum - 1.

In the broadcast processBlock() path, call tronNetDelegate.validBlock() before blockFetchSuccess() and removal of the peer's advInvRequest entry. This preserves the request when structural, Merkle, or signature validation throws, so disconnect recovery can still find it.

Fetch completion remains separate from full block execution. Once validBlock() returns without throwing, the response completes the fetch; its activeWitness result then determines whether processing continues or synchronization starts. An inactive witness does not receive either timestamp update.

3. A normally returning handler does not necessarily mean useful block delivery

Bug and impact

P2pEventHandlerImpl refreshed lastInteractiveTime after a BLOCK handler returned normally. However, BlockMsgHandler.processBlock() could return for an inactive witness or catch a processing exception internally. The dispatcher could not distinguish these outcomes from successful processing. Sync blocks could also return from the message handler before asynchronous validation and processing completed.

Contribution updates had different gaps: a validated block with a missing parent did not update blockRcvTime, while already-known blocks or shutdown returns could reach an update without useful processing. These cases should not be treated as the same result.

Solution

Remove BLOCK from the dispatcher's generic updateLastInteractiveTime() branch. In the broadcast path, update the sending peer's lastInteractiveTime only after validBlock() returns true. Return an explicit BlockResult and let the caller update blockRcvTime only for ACCEPTED or SYNC_REQUIRED.

Broadcast processing result	BlockResult	Update sender's lastInteractiveTime	Update sender's blockRcvTime
Block processing succeeds and isHitDown() is false	ACCEPTED	Yes	Yes
Validated block is not old/known, but its parent is missing; start sync	SYNC_REQUIRED	Yes	Yes
Validated block is below the head or already known	IGNORED	Yes	No
Processing returns with isHitDown()	IGNORED	Yes, after preliminary validation	No
validBlock() returns false for an inactive witness; start sync	STATE_FAILED	No	No
Preliminary validation succeeds but processBlock(block, false) throws; start sync	STATE_FAILED	Yes, after preliminary validation	No

Structural, Merkle, and signature validation failures throw before these successful-validation timestamp updates.

Keep broadcast() after validation and the head/parent checks, before processBlock(block, false). Moving broadcast after full execution would add execution time to every propagation hop. Broadcast-path execution failures start synchronization without converting every exception into a bad-block penalty.

In SyncService.processSyncBlock(), update lastInteractiveTime after validSignature() and processBlock(block, true) succeed and isHitDown() is false. Update blockRcvTime only if useful was true before processing: the block number was at least the local head and containBlock(blockId) was false. Both paths update blockRcvTime only for the peer that supplied the block.

The existing sync-path BAD_BLOCK handling remains, including its provider-only signature/Merkle failure branch. This PR does not complete the design's broader penalty-classification work.

4. Pending TRX requests and a hard latency cutoff can exclude usable block peers

Bug and impact

Initial and backup fetching used PeerConnection.isIdle(), which requires the entire advInvRequest map to be empty. A peer with pending TRX requests was therefore excluded even if it had announced the next missing block and had no block or sync work outstanding.

Backup selection also discarded peers whose getPeerTop75() exceeded fetchBlockTimeout. This could exclude the only available alternative after the original request had already waited long enough.

Solution

Add isBlockFetchIdle(): no BLOCK entries in advInvRequest, no outstanding sync block or chain-summary requests, and no syncBlockInProcess entries. Pending TRX requests are allowed.

Both initial and backup selection use FetchBlockService.canFetchBlock(), which additionally checks that the peer is connected, both sync-direction flags are false, and the item's advInvReceive timestamp is within ADV_TIME_OUT.

Keep separate ranking policies:

• AdvService.consumerInvToFetch() prefers smaller InvSender.getSize(peer) values. These count assignments in the current pass, not all outstanding requests.
• FetchBlockService prefers lower getPeerTop75() values among eligible backups, without the hard P75 cutoff.

Because reserving a BLOCK request makes that peer busy for subsequent block selection, sort queued BLOCK items by height before checkAndPutAdvInvRequest(). Sorting only the outgoing message would be too late to control which block gets the peer. Skip cached, stored, or already requested blocks; leave items without an eligible peer queued. An unavailable earlier block does not prevent fetching an available later block.

5. A short fetch timeout can leave retry tracking empty while INV deduplication remains

Bug and impact

AdvService.addInv() puts the item in invToFetchCache before fetching. Previously, FetchBlockService cleared fetchBlockInfo when the short fetchTimeOut elapsed without an alternative, and also cleared it after sending a backup.

For example:

1. Peer A advertises block H, and the node requests it.
2. A does not respond before fetchTimeOut; no backup is available, so tracking is cleared.
3. Peer B later advertises H. Its advInvReceive entry is recorded, but addInv() returns because H is still in invToFetchCache.
4. There is no tracked fetch for the worker to retry from B.

The cache uses blockCacheTimeout in minutes, with a default of 60. This is not a guaranteed hour-long stall—disconnect recovery can intervene—but the short timeout no longer provides continued retry handling.

Solution

Keep fetchBlockInfo when no alternative is currently eligible and after a backup is sent. The worker can discover later announcements through per-peer advInvReceive, even when global addInv() deduplication rejects the same item.

fetchBlock() registers the current headNum + 1 request using the timestamp already in that peer's advInvRequest. Switching tracking does not remove the original peer's pending request or restart its deadline.

At ADV_TIME_OUT, leave final timeout enforcement to PeerStatusCheck, which examines each peer's own request timestamps and disconnects the responsible peer with TIME_OUT. Clear obsolete tracking when its block height is at or below the head. Queued BLOCK items use ADV_TIME_OUT; expired queue entries release their deduplication entries so a future announcement can enqueue them again.

6. Continued retry handling needs a limit on outstanding backup requests

Problem introduced by retaining retry state

Once fetchBlockInfo is retained after a backup, repeatedly applying the old selection logic could send the same block request to more advertisers on subsequent worker passes.

The old shouldFetchBlock() also returned true whenever oldPeerTop75 > fetchTimeOut, even if the current request had only just been sent. Retaining retry state without changing this condition would make that repeated dispatch especially easy.

Solution

Count active peers with the same BLOCK item in advInvRequest. When the count reaches MAX_IN_FLIGHT_REQUESTS_PER_BLOCK = 2, keep retry tracking and send no further backup. If either pending peer disconnects, the released capacity can allow a replacement.

shouldFetchBlock() now requires either:

• oldPeerSpendTime >= fetchTimeOut; or
• both faster-peer conditions: newPeerTop75 < oldPeerLeftTime * BLOCK_FETCH_LEFT_TIME_PERCENT and oldPeerSpendTime + newPeerTop75 < fetchTimeOut.

A high historical P75 alone no longer triggers dispatch. The existing early switch to a sufficiently faster peer remains available.

7. Disconnect recovery can resend work that another peer already has pending

Bug and impact

AdvService.onDisconnect() previously requeued an item whenever another peer had advertised it. It did not first check whether another peer already had that BLOCK request pending, whether the block had already arrived, or whether fetchBlockInfo still referred to the disconnected peer.

With an original request and a backup in flight, recovery could schedule another request unnecessarily or leave tracking attached to a disconnected peer.

Solution

Call FetchBlockService.onDisconnect(peer) before examining the disconnected peer's outstanding requests. For each item:

Remaining state	Recovery
BLOCK is in blockCache or local storage	Skip another fetch
Another connected peer has the same BLOCK in advInvRequest	Reuse that request without resending; fetchBlock() reads its original advInvRequest timestamp
Another connected peer has an advInvReceive entry	Requeue in invToFetch and let normal scheduling check eligibility
No source remains	Remove the queued item and invalidate invToFetchCache so a later announcement can enqueue it

This also makes the retained request from a rejected block response usable for recovery, as described in section 2.

8. Sending before registration and processing stale snapshots can restore obsolete state

Bug and impact

InvSender.sendFetch() previously sent the message before registering fetchBlockInfo. An immediate response could complete the fetch before registration, after which the sender would register an already-completed request.

The fetch worker also used a snapshot without synchronized state transitions. A worker operating on an older record could clear or replace tracking established by another thread. The original HashSet used for syncBlockInProcess was also unsafe for concurrent updates and idle checks.

Solution

• Register fetchBlockInfo before sending both initial and backup requests.
• Make the reference volatile, make FetchBlockInfo.peer/hash/time final, and synchronize registration, completion, disconnect, and worker transitions.
• Reject stale worker snapshots with fetchBlockInfo != fetchBlock; only clear successful tracking when the block hash matches.
• Use ConcurrentHashMap.newKeySet() for syncBlockInProcess.
• Coordinate inventory admission, queue reservation, and disconnect recovery through the AdvService lock. Register eligible INV receipt times and confirm them under that same lock so confirmation cannot miss registration.

These changes address the covered races within each service. Atomic completion across block processing and disconnect recovery remains a follow-up item.

Why are these changes required?

lastInteractiveTime is an activity signal used by existing peer-quality logic; blockRcvTime records useful block delivery by the actual sender. Correctness requires keeping these meanings separate and tying request timeout decisions to the peer that still owes a response.

The fixes apply to ordinary slow responses, busy peers, and concurrent processing as well as peers that advertise data they do not deliver. They provide the block-handling foundation for later connection-quality changes.

This PR has been tested by:

Local ARM64 / JDK 17 tests passed for the following areas, together with related existing message-handler, service, peer-status, and resilience regressions:

Test class	Main coverage
BlockContributionTest	Validation → fetch completion → broadcast → execution ordering; actual sender contribution; bad signature/Merkle and invalid parent height retaining requests; inactive witness, missing parent, old/known blocks, execution failure, and shutdown
BlockInventoryActivityTest	Receipt-time eligibility; registration before an immediate fetch response; confirmation by matching block ID; monotonic updates; failure paths; cache eviction/expiration; concurrent registration and confirmation
PeerBlockIdleTest, PeerConnectionTest	Pending TRX requests remain eligible for block fetching; sync requests/processing exclude peers; monotonic activity updates
FetchBlockRetryTest	Late INV despite deduplication; eligibility and ranking; height-ordered reservation; two-request limit; early faster-peer selection and timeout fallback; disconnect replacement; original request deadlines; stale workers and immediate responses
SyncBlockContributionTest	Successful sync processing; old/known blocks; shutdown; signature and processing failures; INV confirmation without advertiser contribution

./gradlew checkstyleMain checkstyleTest passed with zero violations in the framework and plugins production/test reports.

Full-repository tests, x86/JDK 8 execution, and manual network testing have not been run for this PR. Retry and timing scenarios use controlled local tests; this description does not claim a live replay of the TronSpark incident.

Follow up and scope boundaries

• Already-sent future-height requests are not automatically promoted into fetchBlockInfo when the head advances.
• Atomic handling of blockFetchSuccess() versus disconnect recovery, and blockRcvTime updates for concurrent duplicate responses, remain unresolved.
• The Hello and chain-summary fixes are proposed separately in #6993 and #7000. Transaction error classification, minimum active-connection protection, and connection rotation/recovery remain separate work.
• This PR does not add active head probing, transaction-starvation recovery, trxRcvTime, peer scores, or persistent reputation.
• The design's libp2p handshake and MessageCount concurrency fixes are external integration prerequisites. This PR does not implement them or update dependency versions.

Extra details

Targets release_v4.8.3. Existing public method signatures, wire formats, database formats, and user configuration remain unchanged. Added methods and collections are Java 8 compatible.

The design's proposed isBlockIdle() is implemented as isBlockFetchIdle(). Initial and backup fetching share eligibility through canFetchBlock(), while retaining their separate batch-size and P75 ranking policies.