Skip to content

Commit 19681e4

Browse files
wbwb
authored andcommitted
fix(net): harden shared state concurrency
Network workers, synchronization tasks, metrics collectors, and backup services access several mutable states across threads without consistent visibility or synchronization guarantees. Use concurrent collections and synchronized state transitions where compound access must remain atomic, publish cross-thread fields safely, serialize peer cleanup with existing manager operations, and prevent duplicate-witness metrics from exposing incomplete companion state.
1 parent 4a21592 commit 19681e4

16 files changed

Lines changed: 349 additions & 32 deletions

File tree

‎framework/src/main/java/org/tron/common/backup/BackupManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public class BackupManager implements EventHandler {
5858
private MessageHandler messageHandler;
5959

6060
@Getter
61-
private BackupStatusEnum status = MASTER;
61+
private volatile BackupStatusEnum status = MASTER;
6262

6363
private volatile long lastKeepAliveTime;
6464

‎framework/src/main/java/org/tron/core/metrics/blockchain/BlockChainMetricManager.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,10 @@ public void applyBlock(BlockCapsule block) {
138138
BlockCapsule oldBlock = witnessInfo.get(witnessAddress);
139139
if ((!oldBlock.getBlockId().equals(block.getBlockId()))
140140
&& oldBlock.getTimeStamp() == block.getTimeStamp()) {
141+
dupWitnessBlockNum.put(witnessAddress, block.getNum());
141142
MetricsUtil.counterInc(MetricsKey.BLOCKCHAIN_DUP_WITNESS + witnessAddress);
142143
Metrics.counterInc(MetricKeys.Counter.MINER, 1,
143144
StringUtil.encode58Check(address), MetricLabels.Counter.MINE_DUP);
144-
dupWitnessBlockNum.put(witnessAddress, block.getNum());
145145
}
146146
}
147147
witnessInfo.put(witnessAddress, block);
@@ -204,7 +204,7 @@ private List<DupWitnessInfo> getDupWitness() {
204204
for (Map.Entry<String, Counter> entry : dupWitnessMap.entrySet()) {
205205
DupWitnessInfo dupWitness = new DupWitnessInfo();
206206
String witness = entry.getKey().substring(MetricsKey.BLOCKCHAIN_DUP_WITNESS.length());
207-
long blockNum = dupWitnessBlockNum.get(witness);
207+
long blockNum = dupWitnessBlockNum.getOrDefault(witness, 0L);
208208
dupWitness.setAddress(witness);
209209
dupWitness.setBlockNum(blockNum);
210210
dupWitness.setCount((int) entry.getValue().getCount());

‎framework/src/main/java/org/tron/core/net/messagehandler/ChainInventoryMsgHandler.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,15 @@ public void processMessage(PeerConnection peer, TronMessage msg) throws P2pExcep
8686
peer.setFetchAble(true);
8787
return;
8888
}
89-
}
9089

91-
peer.setFetchAble(true);
92-
if ((chainInventoryMessage.getRemainNum() == 0 && !peer.getSyncBlockToFetch().isEmpty())
93-
|| (chainInventoryMessage.getRemainNum() != 0
94-
&& peer.getSyncBlockToFetch().size() > syncFetchBatchNum)) {
95-
syncService.setFetchFlag(true);
96-
} else {
97-
syncService.syncNext(peer);
90+
peer.setFetchAble(true);
91+
if ((chainInventoryMessage.getRemainNum() == 0 && !peer.getSyncBlockToFetch().isEmpty())
92+
|| (chainInventoryMessage.getRemainNum() != 0
93+
&& peer.getSyncBlockToFetch().size() > syncFetchBatchNum)) {
94+
syncService.setFetchFlag(true);
95+
} else {
96+
syncService.syncNext(peer);
97+
}
9898
}
9999
}
100100

‎framework/src/main/java/org/tron/core/net/peer/PeerConnection.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import java.net.InetAddress;
1111
import java.net.InetSocketAddress;
1212
import java.util.Deque;
13-
import java.util.HashSet;
1413
import java.util.List;
1514
import java.util.Locale;
1615
import java.util.Map;
@@ -158,7 +157,7 @@ public class PeerConnection {
158157
private volatile Pair<Deque<BlockId>, Long> syncChainRequested = null;
159158
@Setter
160159
@Getter
161-
private Set<BlockId> syncBlockInProcess = new HashSet<>();
160+
private Set<BlockId> syncBlockInProcess = ConcurrentHashMap.newKeySet();
162161
@Setter
163162
@Getter
164163
private volatile boolean needSyncFromPeer = true;

‎framework/src/main/java/org/tron/core/net/peer/PeerManager.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -84,13 +84,16 @@ public static synchronized PeerConnection remove(Channel channel) {
8484
return peerConnection;
8585
}
8686

87-
private static void remove(PeerConnection peerConnection) {
88-
peers.remove(peerConnection);
87+
private static synchronized boolean remove(PeerConnection peerConnection) {
88+
if (!peers.remove(peerConnection)) {
89+
return false;
90+
}
8991
if (peerConnection.getChannel().isActive()) {
9092
activePeersCount.decrementAndGet();
9193
} else {
9294
passivePeersCount.decrementAndGet();
9395
}
96+
return true;
9497
}
9598

9699
public static synchronized void sortPeers() {
@@ -126,13 +129,9 @@ private static void check() {
126129
long disconnectTime = peer.getChannel().getDisconnectTime();
127130
if (disconnectTime != 0 && now - disconnectTime > DISCONNECTION_TIME_OUT) {
128131
logger.warn("Notify disconnect peer {}.", peer.getInetSocketAddress());
129-
peers.remove(peer);
130-
if (peer.getChannel().isActive()) {
131-
activePeersCount.decrementAndGet();
132-
} else {
133-
passivePeersCount.decrementAndGet();
132+
if (remove(peer)) {
133+
peer.onDisconnect();
134134
}
135-
peer.onDisconnect();
136135
}
137136
}
138137
}

‎framework/src/main/java/org/tron/core/net/service/fetchblock/FetchBlockService.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public class FetchBlockService {
3434
@Autowired
3535
private ChainBaseManager chainBaseManager;
3636

37-
private FetchBlockInfo fetchBlockInfo = null;
37+
private volatile FetchBlockInfo fetchBlockInfo = null;
3838

3939
private final long fetchTimeOut = CommonParameter.getInstance().fetchBlockTimeout;
4040

@@ -159,4 +159,4 @@ public FetchBlockInfo(Sha256Hash hash, PeerConnection peer, long time) {
159159

160160
}
161161

162-
}
162+
}

‎framework/src/main/java/org/tron/core/net/service/statistics/MessageCount.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ public class MessageCount {
1515

1616
private long totalCount = 0;
1717

18-
private void update() {
18+
private synchronized void update() {
1919
long time = System.currentTimeMillis() / 1000;
2020
long gap = time - indexTime;
2121
int k = gap > SIZE ? SIZE : (int) gap;
@@ -28,19 +28,19 @@ private void update() {
2828
}
2929
}
3030

31-
public void add() {
31+
public synchronized void add() {
3232
update();
3333
szCount[index]++;
3434
totalCount++;
3535
}
3636

37-
public void add(int count) {
37+
public synchronized void add(int count) {
3838
update();
3939
szCount[index] += count;
4040
totalCount += count;
4141
}
4242

43-
public int getCount(int interval) {
43+
public synchronized int getCount(int interval) {
4444
if (interval > SIZE) {
4545
logger.warn("Param interval({}) is gt SIZE({})", interval, SIZE);
4646
return 0;
@@ -53,16 +53,16 @@ public int getCount(int interval) {
5353
return count;
5454
}
5555

56-
public long getTotalCount() {
56+
public synchronized long getTotalCount() {
5757
return totalCount;
5858
}
5959

60-
public void reset() {
60+
public synchronized void reset() {
6161
totalCount = 0;
6262
}
6363

6464
@Override
65-
public String toString() {
65+
public synchronized String toString() {
6666
return String.valueOf(totalCount);
6767
}
6868

‎framework/src/main/java/org/tron/core/services/WitnessProductBlockService.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@
33
import com.google.common.cache.Cache;
44
import com.google.common.cache.CacheBuilder;
55
import java.util.Arrays;
6-
import java.util.HashMap;
76
import java.util.HashSet;
87
import java.util.Map;
98
import java.util.Set;
9+
import java.util.concurrent.ConcurrentHashMap;
1010
import java.util.concurrent.atomic.AtomicInteger;
1111
import lombok.extern.slf4j.Slf4j;
1212
import org.springframework.stereotype.Service;
@@ -20,7 +20,7 @@ public class WitnessProductBlockService {
2020
private Cache<Long, BlockCapsule> historyBlockCapsuleCache = CacheBuilder.newBuilder()
2121
.initialCapacity(200).maximumSize(200).build();
2222

23-
private Map<String, CheatWitnessInfo> cheatWitnessInfoMap = new HashMap<>();
23+
private Map<String, CheatWitnessInfo> cheatWitnessInfoMap = new ConcurrentHashMap<>();
2424

2525
public void validWitnessProductTwoBlock(BlockCapsule block) {
2626
try {

‎framework/src/test/java/org/tron/common/backup/BackupManagerTest.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.lang.reflect.Field;
44
import java.lang.reflect.Method;
5+
import java.lang.reflect.Modifier;
56
import java.net.InetAddress;
67
import java.net.InetSocketAddress;
78
import java.util.ArrayList;
@@ -52,6 +53,13 @@ public void tearDown() {
5253
Args.clearParam();
5354
}
5455

56+
@Test
57+
public void statusIsVolatileForCrossThreadVisibility() throws Exception {
58+
Field status = BackupManager.class.getDeclaredField("status");
59+
60+
Assert.assertTrue(Modifier.isVolatile(status.getModifiers()));
61+
}
62+
5563
@Test
5664
public void test() throws Exception {
5765
CommonParameter.getInstance().setBackupPriority(8);
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package org.tron.core.metrics.blockchain;
2+
3+
import java.lang.reflect.Method;
4+
import java.util.List;
5+
import org.junit.Assert;
6+
import org.junit.Test;
7+
import org.tron.common.parameter.CommonParameter;
8+
import org.tron.core.metrics.MetricsKey;
9+
import org.tron.core.metrics.MetricsUtil;
10+
11+
public class BlockChainMetricManagerTest {
12+
13+
@Test
14+
@SuppressWarnings("unchecked")
15+
public void missingDuplicateWitnessBlockNumberDefaultsToZero() throws Exception {
16+
CommonParameter parameter = CommonParameter.getInstance();
17+
boolean nodeMetricsEnabled = parameter.isNodeMetricsEnable();
18+
String witness = "missing-block-number-" + System.nanoTime();
19+
parameter.setNodeMetricsEnable(true);
20+
try {
21+
MetricsUtil.counterInc(MetricsKey.BLOCKCHAIN_DUP_WITNESS + witness);
22+
23+
Method getDupWitness = BlockChainMetricManager.class.getDeclaredMethod("getDupWitness");
24+
getDupWitness.setAccessible(true);
25+
List<DupWitnessInfo> dupWitnesses = (List<DupWitnessInfo>) getDupWitness.invoke(
26+
new BlockChainMetricManager());
27+
28+
DupWitnessInfo dupWitness = dupWitnesses.stream()
29+
.filter(info -> witness.equals(info.getAddress()))
30+
.findFirst()
31+
.orElse(null);
32+
Assert.assertNotNull(dupWitness);
33+
Assert.assertEquals(0L, dupWitness.getBlockNum());
34+
} finally {
35+
parameter.setNodeMetricsEnable(nodeMetricsEnabled);
36+
}
37+
}
38+
}

0 commit comments

Comments
 (0)