Skip to content

fix(net): reject inconsistent terminal sync replies #7079

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
317787106 wants to merge 11 commits into
base: release_v4.8.3
Choose a base branch
from
+491 −66
Open

fix(net): reject inconsistent terminal sync replies #7079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a8d9cfd
fix(net): reject inconsistent sync replies and retry another peer
317787106 Nov 2, 2026
6c90efc
revert(net): remove proactive sync peer retries
317787106 Nov 3, 2026
daa5bd4
refactor(net): clarify advertised inventory block number
317787106 Nov 3, 2026
3916a58
format the code
317787106 Nov 3, 2026
722061c
adjust executuion timeout of VerifyTransferProof from 80ms to 200ms
317787106 Nov 3, 2026
9b52e54
fix(net): shorten isolation recovery delay
317787106 Nov 4, 2026
408a3e5
Merge branch 'release_v4.8.3' into fix/sync-terminal-inventory
317787106 Nov 11, 2026
0c3d791
fix(net): allow terminal sync replies with missing blocks
317787106 Nov 11, 2026
f74542d
fix(net): simplify terminal sync block check
317787106 Nov 11, 2026
dfa9b98
fix(net): separate isolation recovery from long bans
317787106 Nov 12, 2026
e722247
fix(net): tolerate one block of lag in terminal sync replies
317787106 Nov 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions framework/src/main/java/org/tron/core/net/messagehandler/ChainInventoryMsgHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@
import org.tron.core.exception.P2pException.TypeEnum;
import org.tron.core.net.TronNetDelegate;
import org.tron.core.net.message.TronMessage;
import org.tron.core.net.message.handshake.HelloMessage;
import org.tron.core.net.message.sync.ChainInventoryMessage;
import org.tron.core.net.peer.PeerConnection;
import org.tron.core.net.peer.TronState;
import org.tron.core.net.service.sync.SyncService;
import org.tron.protos.Protocol.Inventory.InventoryType;

@Slf4j(topic = "net")
@Component
Expand Down Expand Up @@ -95,6 +97,7 @@ public void processMessage(PeerConnection peer, TronMessage msg) throws P2pExcep
}

private void check(PeerConnection peer, ChainInventoryMessage msg) throws P2pException {
HelloMessage hello = peer.getHelloMessageReceive();
Pair<Deque<BlockId>, Long> requested = peer.getSyncChainRequested();
if (requested == null) {
throw new P2pException(TypeEnum.BAD_MESSAGE, "not send syncBlockChainMsg");
Expand Down Expand Up @@ -147,6 +150,41 @@ private void check(PeerConnection peer, ChainInventoryMessage msg) throws P2pExc
+ ", futureMaxNum: " + maxFutureNum);
}
}

if (msg.getRemainNum() == 0) {
BlockId lastBlockId = blockIds.get(blockIds.size() - 1);
long lastNum = lastBlockId.getNum();
long helloHeadNum = hello.getHeadBlockId().getNum();
if (lastNum < helloHeadNum) {

@xxo1shine xxo1shine Nov 3, 2026

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.

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 317787106 Nov 3, 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.

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.

// A fork rollback can temporarily lower the head; allow a fresh HELLO on reconnect.
throw new P2pException(TypeEnum.SYNC_FAILED,
"lastNum " + lastNum + " (remainNum=0) is below hello head " + helloHeadNum);
}

// A peer that has the terminal block in our summary should reply with that block alone.
if (blockIds.size() > 1 && requested.getKey().contains(lastBlockId)) {
// A concurrent fork switch can also produce this response; use a sync failure.
throw new P2pException(TypeEnum.SYNC_FAILED,
"terminal block already in summary: " + lastBlockId.getString());
}

long requestedAt = requested.getValue();
// Only use recent announcements received before this request, not a newer head
// announced while the peer was preparing its response. A concurrent fork switch also cause it
long maxAdvInvNum = peer.getAdvInvReceive().asMap().entrySet().stream()

@xxo1shine xxo1shine Nov 3, 2026

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.

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 317787106 Nov 3, 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.

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.

.filter(entry -> entry.getKey().getType() == InventoryType.BLOCK)
.filter(entry -> entry.getValue() <= requestedAt
&& entry.getValue() >= requestedAt - NetConstants.ADV_TIME_OUT)
.mapToLong(entry -> new BlockId(entry.getKey().getHash()).getNum())
.filter(height -> height >= 0)
.max()
.orElse(lastNum);
// Tolerate one block awaiting application, and allow fetching unknown terminal blocks.
if (lastNum < maxAdvInvNum - 1 && tronNetDelegate.containBlock(lastBlockId)) {
throw new P2pException(TypeEnum.SYNC_FAILED,
"lastNum " + lastNum + " (remainNum=0) is below advertised block " + maxAdvInvNum);
}
}
}

}
32 changes: 26 additions & 6 deletions framework/src/main/java/org/tron/core/net/service/effective/ResilienceService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ public class ResilienceService {

private static final long inactiveThreshold =
CommonParameter.getInstance().getInactiveThreshold() * 1000L;
public static final long blockNotChangeThreshold = 60 * 1000L;
// Leave room for local GC pauses and disk stalls; a shorter threshold can evict healthy peers.
public static final long blockNotChangeThreshold = 30 * 1000L;

@bladehan1 bladehan1 Nov 10, 2026

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.

[NIT] Add regression coverage for the earlier isolation trigger

The existing isolation test sets the last-save time relative to blockNotChangeThreshold, then invokes the check directly. That scenario still passes if the threshold returns to 60 seconds, and it does not exercise the polling schedule registered by init. It therefore does not protect the earlier-trigger behavior introduced here.

Suggestion: Add a fixed elapsed-time scenario that distinguishes the old and new thresholds, plus a controlled-scheduler test for the post-startup polling behavior. These should verify when recovery is attempted, without asserting that peer discovery or catch-up completes within 40 seconds.

// Free slots early, but keep long bans at the previous threshold and cadence.
private static final long blockNotChangeBanThreshold = 60 * 1000L;
private static final long banRoundInterval = 30 * 1000L;

//when node is isolated, retention percent peers will not be disconnected
public static final double retentionPercent = 0.8;
Expand All @@ -40,6 +44,7 @@ public class ResilienceService {
private static final String esName = "resilience-service";
private final ScheduledExecutorService executor = ExecutorServiceManager
.newSingleThreadScheduledExecutor(esName);
private long lastBanRoundTime;

@Autowired
private TronNetDelegate tronNetDelegate;
Expand Down Expand Up @@ -68,13 +73,14 @@ public void init() {
}
}, initialDelay, 10, TimeUnit.SECONDS);

// Limit polling delay after the no-progress threshold is reached.
executor.scheduleWithFixedDelay(() -> {
try {
disconnectIsolated2();
} catch (Exception e) {
logger.error("DisconnectIsolated node failed", e);
}
}, initialDelay, 30, TimeUnit.SECONDS);
}, initialDelay, 10, TimeUnit.SECONDS);

@bladehan1 bladehan1 Nov 10, 2026

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.

[DISCUSS] Evaluate the shorter recovery interval together with the existing ban policy

disconnectIsolated2() still disconnects selected peers with BAD_PROTOCOL. PeerConnection.processDisconnect() maps that to Channel.close(3_600_000), recording a one-hour IP ban expiry. This differs from the terminal-inventory check's SYNC_FAIL path, which uses the ordinary disconnect cooldown. Keeping the eviction policy unchanged therefore does not keep its exposure unchanged when the threshold and polling interval are shortened.

A source-derived scenario worth comparing:

  • The node is past startup, with four non-trusted outbound peers in broadcast mode and minActiveConnections=3.
  • Local block application stalls for 45 seconds, while networking and the recovery scheduler keep running. This is not a stop-the-world JVM pause.
  • With checks at 30s and 40s, timely disconnect callbacks, no replacement connections, and no other eviction policy firing, the new configuration can evict two healthy peers (4 -> 3 -> 2), each through the one-hour-ban path. The old 60s threshold would not fire if block progress resumed at 45s.

If those peers are the only useful block sources and discovery cannot replace them promptly, a short local stall could become a longer source-availability problem. The ban filters ordinary discovered outbound candidates and non-trusted inbound admission; this does not imply that the whole node must remain disconnected for an hour. The severe outcome and its frequency have not been reproduced or measured.

Could we compare old/new behavior under this bounded local-stall scenario, measuring peer loss, ban expiry and time to the next successfully applied block? Depending on the result, we could retain the current combination or consider a normal cooldown, bounded repeated eviction, or separating the timing change from penalty policy. This discussion is about the net recovery tradeoff, beyond protecting the new timing constants with regression tests.

}

private void disconnectRandom() {
Expand Down Expand Up @@ -158,6 +164,12 @@ private void disconnectIsolated2() {
}
logger.warn("Node is isolated, try to disconnect from peers");

long now = System.currentTimeMillis();
long stalled = now - chainBaseManager.getLatestSaveBlockTime();
boolean ban = stalled >= blockNotChangeBanThreshold
&& now - lastBanRoundTime >= banRoundInterval;
ReasonCode reason = ban ? ReasonCode.BAD_PROTOCOL : ReasonCode.SYNC_FAIL;

//disconnect from the node whose lastActiveTime is smallest
int activePeerSize = (int) tronNetDelegate.getActivePeer().stream()
.filter(peer -> peer.getChannel().isOutbound())
Expand All @@ -169,8 +181,12 @@ private void disconnectIsolated2() {
.collect(Collectors.toList());

Optional<PeerConnection> one = getEarliestPeer(peers);
one.ifPresent(peer -> disconnectFromPeer(peer, ReasonCode.BAD_PROTOCOL,
DisconnectCause.ISOLATE2_ACTIVE));
one.ifPresent(peer -> {
disconnectFromPeer(peer, reason, DisconnectCause.ISOLATE2_ACTIVE);
if (ban) {
lastBanRoundTime = System.currentTimeMillis();
}
});
}

//disconnect from some passive nodes, make sure retention nodes' num <= 0.8 * maxConnection,
Expand All @@ -195,8 +211,12 @@ private void disconnectIsolated2() {
}
logger.info("All peer Size:{}, plan size:{}, candidate size:{}, real size:{}", peerSize,
disconnectSize, candidateSize, peers.size());
peers.forEach(peer -> disconnectFromPeer(peer, ReasonCode.BAD_PROTOCOL,
DisconnectCause.ISOLATE2_PASSIVE));
peers.forEach(peer -> {
disconnectFromPeer(peer, reason, DisconnectCause.ISOLATE2_PASSIVE);
if (ban) {
lastBanRoundTime = System.currentTimeMillis();
}
});
}
}

Expand Down
2 changes: 1 addition & 1 deletion framework/src/test/java/org/tron/common/runtime/vm/PrecompiledContractsVerifyProofTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3699,7 +3699,7 @@ public void verifyBurnWrongHash() throws ZksnarkException {

private Pair<Boolean, byte[]> verifyTransfer(byte[] input) {
transferContract.getEnergyForData(input);
transferContract.setVmShouldEndInUs(System.nanoTime() / 1000 + 80 * 1000);
transferContract.setVmShouldEndInUs(System.nanoTime() / 1000 + 200 * 1000);
return transferContract.execute(input);
}

Expand Down
2 changes: 1 addition & 1 deletion framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2376,7 +2376,7 @@ private byte[] abiEncodeForBurn(ShieldedTRC20Parameters params, long value) {

private Pair<Boolean, byte[]> verifyTransfer(byte[] input) {
transferContract.getEnergyForData(input);
transferContract.setVmShouldEndInUs(System.nanoTime() / 1000 + 80 * 1000);
transferContract.setVmShouldEndInUs(System.nanoTime() / 1000 + 200 * 1000);
Pair<Boolean, byte[]> ret = transferContract.execute(input);
return ret;
}
Expand Down
Loading
Loading