Skip to content

Commit 2a687f2

Browse files
Federico2014Federico2014
committed
revert(net): roll back PBFT dedupe-by-SR-address
1 parent 815ac06 commit 2a687f2

6 files changed

Lines changed: 25 additions & 112 deletions

File tree

‎actuator/src/main/java/org/tron/core/utils/TransactionUtil.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -223,9 +223,8 @@ public TransactionSignWeight getTransactionSignWeight(Transaction trx) {
223223
tswBuilder.setPermission(permission);
224224
if (trx.getSignatureCount() > 0) {
225225
List<ByteString> approveList = new ArrayList<>();
226-
// Read-only introspection: pass osakaAllowed=false so a historical tx
227-
// committed pre-Osaka with a sig above MAX_SIGNATURE_SIZE is still
228-
// resolvable; the upper bound only gates new submissions post-Osaka.
226+
// Read-only introspection: pass checkMaxSignatureSize=false so a historical tx
227+
// committed with a sig above MAX_SIGNATURE_SIZE is still resolvable
229228
long currentWeight = TransactionCapsule.checkWeight(permission, trx.getSignatureList(),
230229
Sha256Hash.hash(CommonParameter.getInstance()
231230
.isECKeyCryptoEngine(), trx.getRawData().toByteArray()), approveList,

‎consensus/src/main/java/org/tron/consensus/pbft/PbftMessageHandle.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ public void onPrePrepare(PbftMessage message) {
129129
PbftMessage paMessage = message.buildPrePareMessage(miner);
130130
forwardMessage(paMessage);
131131
try {
132-
paMessage.analyzeSignature(chainBaseManager.getDynamicPropertiesStore());
132+
paMessage.analyzeSignature(
133+
chainBaseManager.getDynamicPropertiesStore().signatureMaxSizeChecked());
133134
} catch (SignatureException e) {
134135
logger.error("", e);
135136
}
@@ -175,7 +176,8 @@ public synchronized void onPrepare(PbftMessage message) {
175176
doneMsg.put(message.getNo(), cmMessage);
176177
forwardMessage(cmMessage);
177178
try {
178-
cmMessage.analyzeSignature(chainBaseManager.getDynamicPropertiesStore());
179+
cmMessage.analyzeSignature(
180+
chainBaseManager.getDynamicPropertiesStore().signatureMaxSizeChecked());
179181
} catch (SignatureException e) {
180182
logger.error("", e);
181183
}

‎consensus/src/main/java/org/tron/consensus/pbft/message/PbftBaseMessage.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,10 @@ public DataType getDataType() {
9797

9898
public abstract String getNo();
9999

100-
public void analyzeSignature(DynamicPropertiesStore dynamicPropertiesStore)
100+
public void analyzeSignature(boolean checkMaxSignatureSize)
101101
throws SignatureException {
102102
ByteString signature = getPbftMessage().getSignature();
103-
if (!SignUtils.isValidLength(signature.size(),
104-
dynamicPropertiesStore.signatureMaxSizeChecked())) {
103+
if (!SignUtils.isValidLength(signature.size(), checkMaxSignatureSize)) {
105104
throw new SignatureException("PBFT signature size is " + signature.size());
106105
}
107106
byte[] hash = Sha256Hash.hash(true, getPbftMessage().getRawData().toByteArray());

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

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -123,28 +123,31 @@ private void processPBFTCommitMessage(PbftCommitMessage pbftCommitMessage) {
123123
private boolean validPbftSign(Raw raw, List<ByteString> srSignList,
124124
List<ByteString> currentSrList) {
125125
//valid sr list
126-
if (srSignList.size() < Param.getInstance().getAgreeNodeCount()) {
127-
return false;
128-
}
129126
if (srSignList.size() != 0) {
127+
Set<ByteString> srSignSet = new ConcurrentSet();
128+
srSignSet.addAll(srSignList);
129+
if (srSignSet.size() < Param.getInstance().getAgreeNodeCount()) {
130+
logger.error("sr sign count {} < sr count * 2/3 + 1 == {}", srSignSet.size(),
131+
Param.getInstance().getAgreeNodeCount());
132+
return false;
133+
}
130134
byte[] dataHash = Sha256Hash.hash(true, raw.toByteArray());
131135
Set<ByteString> srSet = Sets.newHashSet(currentSrList);
132-
Set<ByteString> validSrAddressSet = new ConcurrentSet();
133136
List<Future<Boolean>> futureList = new ArrayList<>();
134137
for (ByteString sign : srSignList) {
135138
futureList.add(executorService.submit(
136-
new ValidPbftSignTask(raw.getViewN(), validSrAddressSet, dataHash, srSet, sign)));
139+
new ValidPbftSignTask(raw.getViewN(), srSignSet, dataHash, srSet, sign)));
137140
}
138141
for (Future<Boolean> future : futureList) {
139142
try {
140-
future.get();
143+
if (!future.get()) {
144+
return false;
145+
}
141146
} catch (Exception e) {
142147
logger.error("", e);
143148
}
144149
}
145-
if (validSrAddressSet.size() < Param.getInstance().getAgreeNodeCount()) {
146-
logger.error("sr sign count {} < sr count * 2/3 + 1 == {}", validSrAddressSet.size(),
147-
Param.getInstance().getAgreeNodeCount());
150+
if (srSignSet.size() != 0) {
148151
return false;
149152
}
150153
}
@@ -154,15 +157,15 @@ private boolean validPbftSign(Raw raw, List<ByteString> srSignList,
154157
private class ValidPbftSignTask implements Callable<Boolean> {
155158

156159
private long viewN;
157-
private Set<ByteString> validSrAddressSet;
160+
private Set<ByteString> srSignSet;
158161
private byte[] dataHash;
159162
private Set<ByteString> srSet;
160163
private ByteString sign;
161164

162-
ValidPbftSignTask(long viewN, Set<ByteString> validSrAddressSet,
165+
ValidPbftSignTask(long viewN, Set<ByteString> srSignSet,
163166
byte[] dataHash, Set<ByteString> srSet, ByteString sign) {
164167
this.viewN = viewN;
165-
this.validSrAddressSet = validSrAddressSet;
168+
this.srSignSet = srSignSet;
166169
this.dataHash = dataHash;
167170
this.srSet = srSet;
168171
this.sign = sign;
@@ -183,7 +186,7 @@ public Boolean call() throws Exception {
183186
ByteArray.toHexString(srAddress));
184187
return false;
185188
}
186-
validSrAddressSet.add(ByteString.copyFrom(srAddress));
189+
srSignSet.remove(sign);
187190
} catch (SignatureException e) {
188191
logger.error("viewN {} valid sr list sign fail!", viewN, e);
189192
return false;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public void processMessage(PeerConnection peer, PbftMessage msg) throws Exceptio
5454
&& currentEpoch - msg.getEpoch() > expireEpoch) {
5555
return;
5656
}
57-
msg.analyzeSignature(chainBaseManager.getDynamicPropertiesStore());
57+
msg.analyzeSignature(chainBaseManager.getDynamicPropertiesStore().signatureMaxSizeChecked());
5858
String key = buildKey(msg);
5959
Lock lock = striped.get(key);
6060
try {

‎framework/src/test/java/org/tron/core/net/messagehandler/PbftDataSyncHandlerTest.java‎

Lines changed: 0 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,11 @@
55
import java.lang.reflect.Field;
66
import java.lang.reflect.Method;
77
import java.util.ArrayList;
8-
import java.util.Arrays;
98
import java.util.Collections;
10-
import java.util.List;
119
import java.util.Map;
12-
import org.bouncycastle.util.encoders.Hex;
1310
import org.junit.Assert;
1411
import org.junit.Test;
1512
import org.mockito.Mockito;
16-
import org.tron.common.crypto.SignInterface;
17-
import org.tron.common.crypto.SignUtils;
18-
import org.tron.common.utils.PublicMethod;
1913
import org.tron.common.utils.Sha256Hash;
2014
import org.tron.consensus.base.Param;
2115
import org.tron.core.ChainBaseManager;
@@ -95,88 +89,4 @@ public void testValidPbftSignPaddedSigOsakaRejected() throws Exception {
9589
Collections.singletonList(ByteString.copyFrom(new byte[21])));
9690
Assert.assertFalse(valid);
9791
}
98-
99-
@Test
100-
public void testValidPbftSignDedupesByAddress() throws Exception {
101-
PbftDataSyncHandler pbftDataSyncHandler = new PbftDataSyncHandler();
102-
DynamicPropertiesStore dynamicPropertiesStore = Mockito.mock(DynamicPropertiesStore.class);
103-
ChainBaseManager chainBaseManager = Mockito.mock(ChainBaseManager.class);
104-
Mockito.when(chainBaseManager.getDynamicPropertiesStore()).thenReturn(dynamicPropertiesStore);
105-
Mockito.when(dynamicPropertiesStore.signatureMaxSizeChecked()).thenReturn(true);
106-
107-
Field field = PbftDataSyncHandler.class.getDeclaredField("chainBaseManager");
108-
field.setAccessible(true);
109-
field.set(pbftDataSyncHandler, chainBaseManager);
110-
111-
Param.getInstance().setAgreeNodeCount(2);
112-
Method method = PbftDataSyncHandler.class.getDeclaredMethod("validPbftSign",
113-
Protocol.PBFTMessage.Raw.class, java.util.List.class, java.util.List.class);
114-
method.setAccessible(true);
115-
116-
Protocol.PBFTMessage.Raw raw = Protocol.PBFTMessage.Raw.newBuilder()
117-
.setViewN(1)
118-
.setEpoch(0)
119-
.setDataType(Protocol.PBFTMessage.DataType.BLOCK)
120-
.setMsgType(Protocol.PBFTMessage.MsgType.COMMIT)
121-
.setData(ByteString.copyFromUtf8("block"))
122-
.build();
123-
124-
SignInterface signer = SignUtils.fromPrivate(
125-
Hex.decode(PublicMethod.getRandomPrivateKey()), true);
126-
byte[] sig65 = signer.Base64toBytes(
127-
signer.signHash(Sha256Hash.hash(true, raw.toByteArray())));
128-
List<ByteString> paddedSigs = Arrays.asList(
129-
ByteString.copyFrom(sig65),
130-
ByteString.copyFrom(Arrays.copyOf(sig65, 66)),
131-
ByteString.copyFrom(Arrays.copyOf(sig65, 67)),
132-
ByteString.copyFrom(Arrays.copyOf(sig65, 68)));
133-
List<ByteString> currentSrList = Collections.singletonList(
134-
ByteString.copyFrom(signer.getAddress()));
135-
136-
boolean valid = (boolean) method.invoke(pbftDataSyncHandler, raw,
137-
paddedSigs, currentSrList);
138-
Assert.assertFalse("trailing-byte padded duplicates must not inflate quorum", valid);
139-
}
140-
141-
@Test
142-
public void testValidPbftSignAcceptsQuorumDespiteOneBadSig() throws Exception {
143-
PbftDataSyncHandler pbftDataSyncHandler = new PbftDataSyncHandler();
144-
DynamicPropertiesStore dynamicPropertiesStore = Mockito.mock(DynamicPropertiesStore.class);
145-
ChainBaseManager chainBaseManager = Mockito.mock(ChainBaseManager.class);
146-
Mockito.when(chainBaseManager.getDynamicPropertiesStore()).thenReturn(dynamicPropertiesStore);
147-
Mockito.when(dynamicPropertiesStore.signatureMaxSizeChecked()).thenReturn(true);
148-
149-
Field field = PbftDataSyncHandler.class.getDeclaredField("chainBaseManager");
150-
field.setAccessible(true);
151-
field.set(pbftDataSyncHandler, chainBaseManager);
152-
153-
Param.getInstance().setAgreeNodeCount(3);
154-
Method method = PbftDataSyncHandler.class.getDeclaredMethod("validPbftSign",
155-
Protocol.PBFTMessage.Raw.class, java.util.List.class, java.util.List.class);
156-
method.setAccessible(true);
157-
158-
Protocol.PBFTMessage.Raw raw = Protocol.PBFTMessage.Raw.newBuilder()
159-
.setViewN(1)
160-
.setEpoch(0)
161-
.setDataType(Protocol.PBFTMessage.DataType.BLOCK)
162-
.setMsgType(Protocol.PBFTMessage.MsgType.COMMIT)
163-
.setData(ByteString.copyFromUtf8("block"))
164-
.build();
165-
byte[] dataHash = Sha256Hash.hash(true, raw.toByteArray());
166-
167-
List<ByteString> srSignList = new ArrayList<>();
168-
List<ByteString> currentSrList = new ArrayList<>();
169-
for (int i = 0; i < 3; i++) {
170-
SignInterface signer = SignUtils.fromPrivate(
171-
Hex.decode(PublicMethod.getRandomPrivateKey()), true);
172-
srSignList.add(ByteString.copyFrom(signer.Base64toBytes(signer.signHash(dataHash))));
173-
currentSrList.add(ByteString.copyFrom(signer.getAddress()));
174-
}
175-
// An attacker-injected too-short signature must not bring down the whole message.
176-
srSignList.add(ByteString.copyFrom(new byte[64]));
177-
178-
boolean valid = (boolean) method.invoke(pbftDataSyncHandler, raw,
179-
srSignList, currentSrList);
180-
Assert.assertTrue("quorum of valid sigs must still pass when one sig is malformed", valid);
181-
}
18292
}

0 commit comments

Comments
 (0)