Skip to content

feat(net): optimize disconnectRandom by tracking block receive time #6704

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

Merged
lvs0075 merged 1 commit into from
May 9, 2026
Merged

feat(net): optimize disconnectRandom by tracking block receive time #6704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ private void processBlock(PeerConnection peer, BlockCapsule block) throws P2pExc

try {
tronNetDelegate.processBlock(block, false);
peer.setBlockRcvTime(System.currentTimeMillis());
witnessProductBlockService.validWitnessProductTwoBlock(block);

Item item = new Item(blockId, InventoryType.BLOCK);
Expand Down
6 changes: 5 additions & 1 deletion framework/src/main/java/org/tron/core/net/messagehandler/InventoryMsgHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import org.tron.common.utils.Sha256Hash;
import org.tron.core.capsule.BlockCapsule.BlockId;
import org.tron.core.config.args.Args;
import org.tron.core.exception.P2pException;
import org.tron.core.exception.P2pException.TypeEnum;
Expand Down Expand Up @@ -44,7 +45,10 @@ public void processMessage(PeerConnection peer, TronMessage msg) throws P2pExcep
peer.getAdvInvReceive().put(item, System.currentTimeMillis());
advService.addInv(item);
if (type.equals(InventoryType.BLOCK) && peer.getAdvInvSpread().getIfPresent(item) == null) {
peer.setLastInteractiveTime(System.currentTimeMillis());
long headNum = tronNetDelegate.getHeadBlockId().getNum();
if (new BlockId(id).getNum() > headNum) {
Copy link
Copy Markdown
Collaborator

@halibobo1205 halibobo1205 Apr 28, 2026

Choose a reason for hiding this comment

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

[QUESTION] Small wording suggestion. The PR description says this prevents "attackers from forging activity via stale block hashes" — the stale-replay part is correct and a real improvement, but I don't think arbitrary forgery is prevented:

BlockId(Sha256Hash) reinterprets the first 8 bytes of the hash as a big-endian long (BlockCapsule.java:347-352) without verifying the hash matches any real block. An adversary can craft a 32-byte hash whose high bytes encode headNum + 1 and still pass the check.

The check still has clear value (this is the only path that refreshes lastInteractiveTime on INV — P2pEventHandlerImpl.updateLastInteractiveTime doesn't include INVENTORY in its switch, so without this, tightening any historical block hash refreshes the inactivity clock). Just suggest narrowing the description to "prevent stale block hashes from refreshing lastInteractiveTime" rather than the broader "forging activity" framing — keeps expectations accurate.

Copy link
Copy Markdown
Collaborator Author

@xxo1shine xxo1shine May 5, 2026

Choose a reason for hiding this comment

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

This modification doesn't completely solve the update problem, but rather increases the cost of malicious activity, preventing attackers from refreshing the time using expired block hashes—an attack with very low cost.

The attack you mentioned does exist, but attackers would need to construct malicious blocks, significantly increasing the cost of such an attack.

peer.setLastInteractiveTime(System.currentTimeMillis());
}
Copy link
Copy Markdown
Collaborator

@317787106 317787106 May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

[SHOULD] Good comparing. If new BlockId(id).getNum() < solidity Num, we should set the LastInteractiveTime as very old time.

Copy link
Copy Markdown
Collaborator Author

@xxo1shine xxo1shine May 5, 2026

Choose a reason for hiding this comment

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

If the time isn't refreshed, the time will fall behind, so the added logic isn't very significant. It's also possible that the other party, due to network latency, is slightly behind and might be mistakenly affected.

Copy link
Copy Markdown
Collaborator

@317787106 317787106 May 5, 2026

Choose a reason for hiding this comment

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

Let's improve it, suppose if new BlockId(id).getNum() < solidityNum - THRESHOLD, it takes into account the impact of network latency under extreme conditions. Since you’ve modified this part, it’s recommended to imporove the logic here.

Copy link
Copy Markdown
Collaborator Author

@xxo1shine xxo1shine May 6, 2026

Choose a reason for hiding this comment

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

I think this is redundant logic. Besides increasing complexity, it doesn't provide any defensive capability. Previously, malicious nodes would do this because broadcasting blocks below the solidified block level could refresh the time. Now that refreshing is no longer possible, and there's no longer a list of blocks below the solidified block level being broadcast.

}
}
}
Expand Down
4 changes: 4 additions & 0 deletions framework/src/main/java/org/tron/core/net/peer/PeerConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ public class PeerConnection {
@Setter
private volatile long lastInteractiveTime;

@Setter
@Getter
private volatile long blockRcvTime;

@Getter
@Setter
private volatile TronState tronState = TronState.INIT;
Expand Down
13 changes: 12 additions & 1 deletion 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 @@ -3,8 +3,10 @@
import static org.tron.common.math.Maths.ceil;
import static org.tron.common.math.Maths.max;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashMap;
import java.util.IdentityHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -44,7 +46,7 @@ public class ResilienceService {

@Autowired
private ChainBaseManager chainBaseManager;

public void init() {
if (Args.getInstance().isOpenFullTcpDisconnect) {
executor.scheduleWithFixedDelay(() -> {
Expand Down Expand Up @@ -86,6 +88,7 @@ private void disconnectRandom() {
.collect(Collectors.toList());

if (peers.size() >= minBroadcastPeerSize) {
peers = getRandomDisconnectionPeers(peers);
long now = System.currentTimeMillis();
Map<Object, Integer> weights = new HashMap<>();
peers.forEach(peer -> {
Expand Down Expand Up @@ -121,6 +124,14 @@ private void disconnectRandom() {
}


private List<PeerConnection> getRandomDisconnectionPeers(List<PeerConnection> peers) {
Map<PeerConnection, Long> snapshot = new IdentityHashMap<>(peers.size());
peers.forEach(p -> snapshot.put(p, p.getBlockRcvTime()));
List<PeerConnection> sorted = new ArrayList<>(peers);
sorted.sort(Comparator.comparingLong(snapshot::get));
return sorted.subList(0, sorted.size() / 2);
Copy link
Copy Markdown
Collaborator

@317787106 317787106 May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

[SHOULD] There are two problems:

  1. Exclude latest peer is enough, not half of peers.
  2. Is there any necessaary to create new Map instead of sorting by peer's blockRcvTime ?

Copy link
Copy Markdown
Collaborator Author

@xxo1shine xxo1shine May 5, 2026

Choose a reason for hiding this comment

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

The main purpose is to increase randomness; things that are too certain are more vulnerable to attack. If not sorted by blockRcvTime, then what should be used for sorting?

}

private void disconnectLan() {
if (!isLanNode()) {
return;
Expand Down
1 change: 1 addition & 0 deletions framework/src/main/java/org/tron/core/net/service/sync/SyncService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,7 @@ private void processSyncBlock(BlockCapsule block, PeerConnection peerConnection)
try {
tronNetDelegate.validSignature(block);
tronNetDelegate.processBlock(block, true);
peerConnection.setBlockRcvTime(System.currentTimeMillis());
pbftDataSyncHandler.processPBFTCommitData(block);
} catch (P2pException p2pException) {
logger.error("Process sync block {} failed, type: {}",
Expand Down
52 changes: 52 additions & 0 deletions framework/src/test/java/org/tron/core/net/services/ResilienceServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.io.IOException;
import java.net.InetSocketAddress;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import javax.annotation.Resource;
import org.junit.After;
Expand Down Expand Up @@ -97,6 +98,57 @@ public void testDisconnectRandom() {
Assert.assertEquals(maxConnection - 1, PeerManager.getPeers().size());
}

@Test
public void testDisconnectRandomPreservesRecentBlockRcvTimePeer() {
int maxConnection = 30;
Assert.assertEquals(0, PeerManager.getPeers().size());

ApplicationContext ctx = (ApplicationContext) ReflectUtils.getFieldObject(p2pEventHandler,
"ctx");

// Create maxConnection + 1 peers (triggers disconnectRandom)
for (int i = 0; i < maxConnection + 1; i++) {
InetSocketAddress inetSocketAddress = new InetSocketAddress("202.0.0." + i, 10001);
Channel c1 = spy(Channel.class);
ReflectUtils.setFieldValue(c1, "inetSocketAddress", inetSocketAddress);
ReflectUtils.setFieldValue(c1, "inetAddress", inetSocketAddress.getAddress());
ReflectUtils.setFieldValue(c1, "ctx", spy(ChannelHandlerContext.class));
Mockito.doNothing().when(c1).send((byte[]) any());
PeerManager.add(ctx, c1);
}

// Set first minBroadcastPeerSize peers as broadcast-state
List<PeerConnection> peers = PeerManager.getPeers();
for (PeerConnection peer : peers.subList(0, ResilienceService.minBroadcastPeerSize)) {
peer.setNeedSyncFromPeer(false);
peer.setNeedSyncFromUs(false);
peer.setLastInteractiveTime(System.currentTimeMillis() - 1000);
}
for (PeerConnection peer : peers.subList(ResilienceService.minBroadcastPeerSize,
maxConnection + 1)) {
peer.setNeedSyncFromPeer(false);
peer.setNeedSyncFromUs(true);
}

// Give the LAST broadcast peer a very recent blockRcvTime — it must NOT be disconnected
PeerConnection bestPeer = peers.stream()
.filter(p -> !p.isNeedSyncFromUs() && !p.isNeedSyncFromPeer())
.reduce((a, b) -> b) // last broadcast peer
.orElseThrow(() -> new AssertionError("no broadcast peer"));
bestPeer.setBlockRcvTime(System.currentTimeMillis());

InetSocketAddress bestPeerAddress = bestPeer.getChannel().getInetSocketAddress();

// With minBroadcastPeerSize=3 broadcast peers, getRandomDisconnectionPeers returns
// the 1 peer with oldest blockRcvTime (0). bestPeer has most recent time → exempt.
ReflectUtils.invokeMethod(service, "disconnectRandom");

boolean bestPeerStillConnected = PeerManager.getPeers().stream()
.anyMatch(p -> p.getChannel().getInetSocketAddress().equals(bestPeerAddress));
Assert.assertTrue("Peer with most recent blockRcvTime should not be disconnected",
bestPeerStillConnected);
}

@Test
public void testDisconnectLan() {
int minConnection = 8;
Expand Down
Loading