Skip to content

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

Open
317787106 wants to merge 30 commits into
tronprotocol:release_v4.8.3from
317787106:fix/invalid_inv
Open

317787106 wants to merge 30 commits into
tronprotocol:release_v4.8.3from
317787106:fix/invalid_inv

Conversation

@317787106

@317787106 317787106 commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Correct BLOCK INV activity accounting, block contribution timestamps, block-fetch scheduling, retry tracking, and disconnect recovery on top of release_v4.8.3. Reject malformed block headers before clearing request state. Preserve the advertised-block validation behavior introduced by #7050: a signature or Merkle failure ends that block ID's current fetch, disconnects the provider, and does not immediately request the same ID from another advertiser.

Related net work:

Scope PR
Application-layer Hello admission, Hello timeout, and locally verifiable head checks #6993
Normal sync completion and chain-inventory validation, included in the release branch #7000
Advertised-block validation and inactive-witness tolerance, included in the release branch #7050
BLOCK INV activity, contribution, fetch scheduling, retry, and disconnect recovery This PR

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 tronNetDelegate.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. Reject malformed BLOCK headers before fetch completion

Bug and impact

An invalid block number or parent reference must be rejected before the handler clears a requested block's state. Otherwise, disconnect recovery cannot identify the outstanding fetch.

Solution

In BlockMsgHandler.check(), verify the raw parent hash length and parent/child block numbers before sanitize() and before either sync or advertised request state is cleared. Reject a nonpositive block number, a parent hash whose size is not Sha256Hash.LENGTH, or a parent number other than blockNum - 1. This check also applies to fast-forward and relay paths. A rejected structural response leaves its outstanding request available for normal disconnect recovery.

For an advertised response that passes these structural checks, call blockFetchSuccess(blockId, peer) before validAdvBlock(peer, block), as #7050 does. This removes the provider's request and clears matching fetch tracking even if signature or Merkle validation later throws. The provider is disconnected with BAD_BLOCK; its cleared request does not cause an immediate new request to another advertiser. A backup request already in flight remains outstanding.

validAdvBlock() checks active-witness membership and then block timestamp, Merkle root, and signature. The first inactive-witness occurrence within a new observation window can return false after validation; a repeat within half a maintenance interval throws BAD_BLOCK. A false result stops broadcast processing without synchronization, activity credit, or block contribution.

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, an advertised block can return for an inactive witness or catch a processing exception internally. Sync blocks can also return from the message handler before asynchronous validation and processing complete.

A validated block with a missing parent can start synchronization, whereas old or already-known blocks and shutdown returns should not receive useful-delivery credit.

Solution

Remove BLOCK from the dispatcher's generic updateLastInteractiveTime() branch. In the advertised path, update the sender's lastInteractiveTime only after validAdvBlock() returns true. Return a BlockResult and update blockRcvTime only for ACCEPTED or SYNC_REQUIRED.

Advertised processing result BlockResult Sender activity Sender block contribution
Processing succeeds and isHitDown() is false ACCEPTED Yes Yes
Validated block is neither old nor 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 after isHitDown() IGNORED Yes No
validAdvBlock() returns false for the first inactive-witness occurrence STATE_FAILED No No
Validation succeeds but processBlock(block, false) throws STATE_FAILED Yes No
Structural, signature, or Merkle validation throws Exception No No

Fetch completion for advertised responses is independent of these contribution results, as described in section 2. Keep broadcast() after validation and the head/parent checks, before processBlock(block, false). Execution failures are logged and return STATE_FAILED; they do not automatically start synchronization or disconnect the provider.

In SyncService.processSyncBlock(), update lastInteractiveTime after signature validation and block processing succeed and isHitDown() is false. Update blockRcvTime only when the block was useful before processing: its number was at least the local head and the exact block ID was not already stored. Only the providing peer receives block contribution. Sync processing failures release the global request and disconnect the provider without disconnecting other peers solely because they queued the same block.

4. Pending TRX requests can exclude otherwise eligible block providers

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. When no fully idle announcer was available, this could defer BLOCK scheduling until transaction requests completed.

Pending requests alone do not establish either spare capacity or saturation. Allowing another request can also cause block and transaction responses to queue on the same connection.

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() first prefers fully idle eligible announcers. Only when none are available does it select an eligible source with pending TRX requests. Within each group, prefer smaller InvSender.getSize(peer) values, which count assignments in the current pass rather than all outstanding requests.
  • FetchBlockService continues to rank eligible backups by lower getFetchLatency() values. This existing estimator uses an EWMA for measured fetch latency and falls back to channel average latency before the first sample. Backup selection does not add the initial-fetch idle preference.

The initial-fetch fallback removes the requirement to wait for transaction requests to complete before scheduling a block request. It does not assume spare connection capacity or guarantee a lower block-arrival latency. No comparative block-propagation latency measurements have been performed for this policy; tests cover source preference, fallback eligibility, and request handling.

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 release baseline also permits an early switch when the current provider's latency estimate reaches or exceeds fetchTimeOut and a candidate has a lower estimate. With retained retry tracking, this saturation branch could authorize another dispatch without satisfying the stricter latency-benefit conditions below.

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() permits a backup when either:

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

A high latency estimate no longer bypasses these early-switch conditions. 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 arrived, or whether fetchBlockInfo still referred to the disconnected peer.

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

Solution

Call FetchBlockService.onDisconnect(peer) before examining the disconnected peer's outstanding requests. For each such 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 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 recovery applies to unanswered requests and structurally rejected block responses whose request remains outstanding. For signature and Merkle failures, section 2's fetch completion has already removed the failed provider's request, so disconnect does not schedule a new fetch for that block ID. An existing backup request is left intact.

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. A worker using an older snapshot could also clear or replace newer tracking.

Solution

  • Register fetchBlockInfo before sending 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; clear successful tracking only when the block hash matches.
  • Coordinate inventory admission, queue reservation, and disconnect recovery through the AdvService lock. Serialize eligible INV registration and confirmation through the TronNetDelegate lock.

Regression tests cover immediate first and backup responses, stale worker snapshots, both orders of completion versus recovery request access, and concurrent INV registration versus confirmation. These tests establish the covered service-level orderings; broader atomicity across block handling and disconnect processing remains follow-up work.

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:

On ARM64 / JDK 17, the following checks passed after merging release_v4.8.3:

  • ./gradlew clean build -x test
  • ./gradlew test (full repository suite)
  • ./gradlew :framework:checkstyleMain :framework:checkstyleTest :plugins:checkstyleMain
  • bash .github/scripts/check_math_usage.sh

Focused regressions also passed for these areas:

Test class Main coverage
BlockContributionTest Fetch completion before advertised validation; signature/Merkle failures end the fetch without contribution; malformed parent height retains the request; inactive witness, missing parent, old/known blocks, execution failure, and shutdown
BlockFetchFailureTest Signature/Merkle provider disconnect without a new request to another advertiser; inactive-witness timing; provider-only sync failure handling
BlockInventoryActivityTest Receipt-time eligibility; registration before an immediate response; exact-ID confirmation; cache eviction/expiration; concurrent registration and confirmation
FetchBlockRetryTest Late INV, provider ranking, two-request limit, timeout and disconnect recovery, invalid Merkle completion without a new retry, stale workers, and immediate first/backup responses
SyncBlockContributionTest Actual sender contribution; signature and processing failures disconnect only the provider; INV confirmation without advertiser contribution
TransactionsMsgHandlerTest Transaction-signature disconnect recovery through another announcer, busy-announcer handling, and deduplication release

The timing and retry cases use controlled local tests. x86/JDK 8 execution and manual network testing have not been run.

Follow up and scope boundaries

  • Already-sent future-height requests are not automatically promoted into fetchBlockInfo when the head advances.
  • Cross-service atomicity between block handling and disconnect recovery, and blockRcvTime attribution for concurrent duplicate responses, remain follow-up work.
  • The Hello changes remain in #6993; #7000 is part of the merged release baseline. 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, including #7000 and #7050. This PR adds no wire-format, database-format, configuration, or dependency changes. 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(). Initial fetching prioritizes fully idle eligible sources, allows pending-TRX sources as a fallback, and uses current-pass batch size within each group. Backup fetching retains its getFetchLatency() ranking.

@github-actions
github-actions Bot requested a review from xxo1shine September 29, 2026 06:47
Comment thread framework/src/main/java/org/tron/core/net/P2pEventHandlerImpl.java
Comment thread framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java Outdated
Comment thread framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java Outdated

@3for 3for left a comment

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.

Since the two remaining issues will be addressed separately and don't block the fixes in this PR, LGTM now.

*
* <p>Callers must check connection state and sync direction flags separately before fetching.
*/
public boolean isBlockFetchIdle() {

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] Stale sync-processing state prevents block fetching after synchronization completes.

The new isBlockFetchIdle() check can prevent a peer from fetching further blocks even after synchronization completes.

Reproduction scenario:

  1. Received sync blocks enter syncBlockInProcess, awaiting asynchronous processing.
  2. Other peers persist these blocks first. A subsequent chain inventory response removes the now-known blocks from the download queue and completes synchronization.
  3. The sync worker cannot find the corresponding queue head, leaving stale entries in syncBlockInProcess.
  4. Since isBlockFetchIdle() requires this collection to be empty, the peer remains ineligible for further block fetching.

The previous isIdle() did not check this collection, so the new eligibility condition introduces a regression that may persist until another sync operation or disconnection.

Confirmed with real handlers: synchronization completes with empty request and download queues; the old isIdle() returns true, but isBlockFetchIdle() still returns false.

Please clean up stale syncBlockInProcess entries when already-known blocks leave the sync pipeline, while preserving necessary in-flight checks. Add regression coverage for this scenario.

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.

Good catch. Fixed in 28d03e3. Chain inventory updates now trigger another handleSyncBlock() pass. The worker clears a pending response and its syncBlockInProcess marker only when the block is already stored locally and absent from all active sync queues. Blocks still queued or not yet stored retain their in-flight state, so the isBlockFetchIdle() guard remains unchanged. I added regression coverage for cleanup, queue preservation, and the inventory-triggered recheck.

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] Upstream merge reintroduces premature backup request dispatch.

Reintroduced by merge commit 62b4d0ab.

The current condition dispatches a backup request as soon as the original peer's estimated latency reaches the timeout threshold and the backup is slightly faster, bypassing the documented checks for actual elapsed time and expected benefit.

For example, with a 500 ms timeout, only 100 ms elapsed, an original peer estimate of 500 ms, and a backup estimate of 490 ms, the backup is still dispatched. This causes unnecessary duplicate downloads and occupies both request slots, preventing a faster source discovered later from participating.

Both targeted tests reproduce the issue, and both pass when this condition alone is temporarily removed. The upstream switch to EWMA-based latency estimates is not the issue; the premature dispatch condition is the regression.

Please restore the elapsed-time and expected-benefit checks before dispatching backup requests, and retain regression coverage for both scenarios.

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.

Thanks for catching this. While merging release_v4.8.3, I accidentally restored the latency-saturation fast-switch condition, overwriting the earlier change in this PR. I’ve now reverted that accidental override in adab431. A backup fetch starts only after the request actually times out or when the candidate offers a sufficient expected time benefit. I also added regression tests for saturated latency estimates.

@3for

3for commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

[SHOULD] Sync recovery test passes despite taking the exception path.

Location: SyncServiceTest.java (line 233).

testProcessingBlockResumesThrottledFetch is missing the newly required getHeadBlockId() mock setup and AdvService injection. As a result, the test throws a NullPointerException before normal block processing, enters the failure path, yet still passes its existing assertions.

Please complete the test setup and explicitly verify that processBlock(block, true) is executed successfully and the peer remains connected. Otherwise, the test does not establish that block fetching resumes after successful processing.

# Conflicts:
#	framework/src/main/java/org/tron/core/net/service/fetchblock/FetchBlockService.java
Locally stored blocks pruned from sync inventories could retain pending data and in-process markers, leaving peers ineligible for block fetching.

Recheck pending blocks after inventory updates and release their state only when no active sync queue still needs the block.
Treat a saturated latency estimate as a prediction rather than an elapsed timeout. Require sufficient expected benefit before starting an early backup, while retaining the actual timeout fallback.

Cover saturated latency cases and keep the final-timeout test focused on block requests.
Complete the head-block and AdvService mock setup so the recovery test reaches the successful block-processing path. Assert that processBlock runs and the peer stays connected before checking the next fetch.
@317787106

Copy link
Copy Markdown
Collaborator Author

[SHOULD] Sync recovery test passes despite taking the exception path.

Location: SyncServiceTest.java (line 233).

testProcessingBlockResumesThrottledFetch is missing the newly required getHeadBlockId() mock setup and AdvService injection. As a result, the test throws a NullPointerException before normal block processing, enters the failure path, yet still passes its existing assertions.

Please complete the test setup and explicitly verify that processBlock(block, true) is executed successfully and the peer remains connected. Otherwise, the test does not establish that block fetching resumes after successful processing.

@3for Thanks for catching this false positive. I’ve updated testProcessingBlockResumesThrottledFetch in ffa1bf0 to mock getHeadBlockId() and inject AdvService. The test now verifies that processBlock(block, true) runs and the peer is not disconnected before checking that fetching resumes. The targeted test passes

|| blockCapsule.getParentHashStr().size() != Sha256Hash.LENGTH
|| new BlockId(blockCapsule.getParentHash()).getNum() != blockCapsule.getNum() - 1) {
throw new P2pException(TypeEnum.BAD_BLOCK, "block number does not follow parent");
}

@SeriousCoding789 SeriousCoding789 Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[Discuss] Suggest put this new logic inside the above check, and also move the if (!fastForward && !peer.isRelayPeer()) { inside check. Thus, all the necessary general check will be uniformly inside the check.

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.

Updated as suggested. processMessage() now calls check() unconditionally, with the fast-forward/relay condition applied only to request matching inside check(). Parent-hash and parent/child height validation always run before sanitize().

Added regression coverage to ensure invalid parent heights are still rejected on both fast-forward and relay paths.

Move request matching and parent linkage validation into check while preserving their order and the fast-forward and relay request exemptions. Add regression coverage for invalid parent heights on both exempt paths.
Comment thread framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java Outdated
Remove the unconditional sync restart after broadcast block execution fails. Retain failure logging and the failed result, and verify that the provider is neither resynchronized nor disconnected on this path.
Extract per-item recovery into a synchronized helper so onDisconnect retains orchestration. Preserve pending request reuse, requeueing, cache invalidation, and lock boundaries.
Ignore blocks from inactive witnesses without starting a synchronization
attempt, preventing peers from repeatedly triggering sync without verified
chain progress. Update the regression expectation and fetch comment.

@SeriousCoding789 SeriousCoding789 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In isBlockFetchIdle() (PeerConnection.java:277) a peer counts as idle for block fetching as long as it has no BLOCK entry in advInvRequest; outstanding TRX requests are ignored. The old isIdle() (:267) required advInvRequest to be empty, so a peer that was slow or unresponsive on the transaction side was implicitly kept out of block fetching too. With canFetchBlock() (FetchBlockService.java:106) now gating on isBlockFetchIdle(), that implicit backpressure is gone: a peer sitting on a large backlog of unanswered TRX requests still looks fully eligible and can be picked for block fetches again and again.

Decoupling block fetching from transaction-side load looks like the intent here, and block requests are still bounded by ADV_TIME_OUT and PeerStatusCheck, so this isn't a stall. But a peer that is clearly unhealthy on the TRX side being repeatedly chosen for blocks is a behavior change from the current release. A cap on outstanding TRX requests inside canFetchBlock() would restore some of that backpressure if the unbounded case is a concern.

// latency ranking is reserved for backup fetches in FetchBlockService.
peers.stream()
.filter(peer -> fetchBlockService.canFetchBlock(peer, item, now))
.min(Comparator.comparingInt(invSender::getSize))

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.

When multiple peers have advertised the same block, could we prioritize fully idle peers when choosing where to request it?

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.

This is the same idle-source selection issue discussed in the related review comment, addressed in 32827e7.

Initial block fetching now prioritizes fully idle eligible announcers. Sources with pending TRX requests are selected only when no fully idle eligible source is available. Within each group, the existing batch-size ordering is retained.

Added regression coverage and updated the PR description accordingly

BlockCapsule blockCapsule = msg.getBlockCapsule();
if (blockCapsule.getNum() <= 0
|| blockCapsule.getParentHashStr().size() != Sha256Hash.LENGTH
|| new BlockId(blockCapsule.getParentHash()).getNum() != blockCapsule.getNum() - 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.

These checks depend only on the block message itself, not on peer state or local chain state, so they should be placed in BlockMessage(byte[] data) and performed immediately after parsing. This would reject structurally invalid block messages earlier, while keeping context-dependent checks, such as matching outstanding requests, in the handler.

@317787106 317787106 Oct 22, 2026 •

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.

These checks enforce application-level block invariants, so we prefer to keep them in the handler alongside the existing block size and timestamp checks. Message constructors generally focus on protobuf parsing and format validation.

The current handler applies these checks to sync, broadcast, fast-forward, and relay paths before clearing request state or processing the block. Moving them into the constructor would reject the same invalid blocks slightly earlier, with limited practical benefit. We would therefore keep the current placement in this PR.

// Recheck eligibility after earlier reservations. Initial fetches prefer smaller batches;
// latency ranking is reserved for backup fetches in FetchBlockService.
peers.stream()
.filter(peer -> fetchBlockService.canFetchBlock(peer, item, now))

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.

Is this change supported by measurements or comparative tests? Outstanding transaction requests only tell us that a peer still has requests in flight; they do not establish whether the connection has enough spare capacity. Determining whether requesting the block from that peer immediately reduces block arrival latency compared with waiting until the next scheduling pass to select an idle peer requires considering the transaction response backlog, scheduling interval, and connection throughput.

The current tests primarily verify that such peers are eligible for selection, rather than demonstrating an improvement in block propagation latency. I suggest prioritizing fully idle sources and explaining the rationale for relaxing eligibility when none are available, including whether this could increase queueing between block and transaction responses.

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.

Updated in 32827e7.

Initial block fetching now prioritizes fully idle eligible announcers. Sources with pending TRX requests are used only when no fully idle eligible source is available. Within each group, the existing batch-size ordering is retained.

The fallback avoids requiring transaction requests to complete before scheduling a block request. Responses may still queue on the same connection, and we do not have comparative latency measurements demonstrating a propagation improvement.

Added a regression test for idle-source preference; existing tests cover fallback eligibility. The PR description has also been updated to document this policy, its rationale, and the limits of the available test evidence.

peer.getInetSocketAddress(), blockId.getString(),
Hex.toHexString(block.getWitnessAddress().toByteArray()));
return;
return BlockResult.STATE_FAILED;

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.

For a block with a valid signature whose witness is not in the active set, this path clears the request and returns without imposing any restriction on the sending peer. If the peer keeps announcing self-signed blocks with different IDs, it can repeatedly trigger downloads and validation. Deduplication by block ID does not prevent this, and INV rate limiting only limits the frequency. Should we introduce a cumulative failure counter, a cooldown, or disconnection for peers that repeatedly supply such blocks, to prevent sustained resource consumption?

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.

The concern is valid. Deduplication by block ID does not prevent a peer from supplying different self-signed blocks, and INV rate limiting bounds their frequency rather than eliminating the cumulative download and validation cost.

Clearing the request is still appropriate here: the response has arrived and passed signature and Merkle validation. Fetching the same block ID again would not resolve a mismatch with the local witness set. The release baseline also completed the fetch and returned for an inactive witness without imposing a specific penalty.

This PR ensures that such responses do not update the sender’s lastInteractiveTime or blockRcvTime. However, that does not provide an explicit resource budget or guarantee timely eviction of a peer that repeatedly supplies these blocks.

We should also distinguish this result from an objectively invalid signature or Merkle root. Active-witness membership is checked against the local current schedule. Maintenance transitions, differences in chain progress, or forks can temporarily produce a mismatch, and the sending peer may simply be relaying the block. A single occurrence therefore does not provide sufficient evidence for a bad-block ban.

A cumulative counter or temporary block-fetch cooldown could help address repeated cases, but it needs defined rules for:

  • The observation window, threshold, and counter reset or decay.
  • Avoiding false positives during witness-set transitions or local chain lag.
  • Whether to suspend block fetching from the peer or disconnect it.
  • Recovery after the restriction expires.

To keep this PR focused, we will retain the current request cleanup and timestamp handling. Repeated nonproductive responses should be addressed separately, with tests covering both sustained unique-ID responses and recovery from legitimate witness-set mismatches.

// Snapshot connected peers; block fetching can still use peers with pending TRX requests.
Collection<PeerConnection> peers = tronNetDelegate.getActivePeer().stream()
.filter(peer -> peer.isIdle())
.filter(peer -> !peer.isDisconnect())

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.

getActivePeer() ultimately calls PeerManager.getPeers(), which already filters out disconnected peers and returns a new list. Filtering by !peer.isDisconnect() and collecting another list here is redundant; we can use tronNetDelegate.getActivePeer() directly.

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.

Removed in 152f83f.

consumerInvToFetch() now uses tronNetDelegate.getActivePeer() directly. Since PeerManager.getPeers() already returns a new list excluding disconnected peers, the additional filtering and list copy were redundant.

Preserve sync wakeup regression coverage alongside upstream queue and remaining-count checks. Adapt the witness ingress fixture to exercise processing of a new block.
Prefer fully idle eligible providers before comparing current batch sizes. Keep providers with pending transaction requests available when no idle provider is eligible.
@github-actions
github-actions Bot requested a review from xxo1shine October 22, 2026 12:49
Use the connected-peer snapshot returned by getActivePeer() directly to avoid duplicate filtering and list allocation.
Release sync processing markers and request cache entries when parsing
fails or no sync queue needs a response, allowing eligible peers and
pending blocks to be fetched again.

Move inventory registration and confirmation to TronNetDelegate under a
shared monitor, preserving receipt timestamps and confirmation timing.
Remove the provider request and clear retry tracking under the same fetch
service monitor so disconnect recovery cannot restore completed tracking.
Cover both completion/recovery orderings and adapt handler tests to the
combined completion operation.

Remove the redundant Optional presence check before ifPresent.
# Conflicts:
#	framework/src/test/java/org/tron/core/net/services/RelayServiceTest.java
Keep advertised block fetch completion before signature and Merkle validation, so a rejected provider disconnect does not immediately retry the same block ID. Preserve provider-only blame and update regression tests for the merged validation API.

@xxo1shine xxo1shine left a comment

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.

I suggest narrowing the scope of this PR. For now, keep the existing block-processing flow and FetchBlockService retry state machine, and focus primarily on allowing peers with pending TRX requests to provide blocks and avoiding duplicate requests during disconnect recovery.

For BLOCK INV activity accounting, I do not think a full confirmation mechanism is necessary at this point. Once a fabricated INV enters the request flow, the provider is penalized if it returns an invalid block, or disconnected on timeout if it does not respond. Updating the activity timestamp does not bypass those checks. In the normal request path, the provider gains only temporary activity credit before the penalty. Deduplication and provider-selection edge cases can leave some announcements without a corresponding request, but we have not yet established that this discrepancy significantly affects connection quality. Activity is only one auxiliary dimension of peer-quality assessment. The benefit does not currently justify adding a pending-confirmation cache, confirmation across block-processing paths, and concurrency coordination.

I suggest applying similar scope control to the other changes:

  • Request cleanup after invalid blocks: retain the signature/Merkle failure policy established by #7050, penalize the provider, and end the current fetch. For now, avoid expanding retry and recovery handling around invalid responses.
  • Activity and contribution timestamps after block processing: correcting these timestamps has limited benefit for peers that are ultimately penalized and disconnected. Accounting accuracy for other paths that do not result in a penalty can be discussed separately, without introducing new block-processing result classifications in this PR.
  • Continued retries after the short timeout and a cap on concurrent downloads: these form an additional fast-recovery strategy. For now, retain the existing backup-request mechanism, recovery after the final request timeout and disconnection, and synchronization triggered by a subsequent valid block or a new connection, without also introducing continued retry tracking and a new request limit.

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