Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,12 @@ public TransactionSignWeight getTransactionSignWeight(Transaction trx) {
tswBuilder.setPermission(permission);
if (trx.getSignatureCount() > 0) {
List<ByteString> approveList = new ArrayList<>();
// Read-only introspection: pass checkMaxSignatureSize=false so a historical tx
// committed with a sig above MAX_SIGNATURE_SIZE is still resolvable
long currentWeight = TransactionCapsule.checkWeight(permission, trx.getSignatureList(),
Sha256Hash.hash(CommonParameter.getInstance()
.isECKeyCryptoEngine(), trx.getRawData().toByteArray()), approveList);
.isECKeyCryptoEngine(), trx.getRawData().toByteArray()), approveList,
false);
tswBuilder.addAllApprovedList(approveList);
tswBuilder.setCurrentWeight(currentWeight);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,10 @@ public static ForkController instance() {
return ForkControllerEnum.INSTANCE.getInstance();
}

public static boolean signatureMaxSizeChecked() {
return instance().pass(ForkBlockVersionEnum.VERSION_4_8_2);
}

private enum ForkControllerEnum {
INSTANCE;

Expand Down
10 changes: 8 additions & 2 deletions chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.tron.common.crypto.SignUtils;
import org.tron.common.parameter.CommonParameter;
import org.tron.common.utils.ByteArray;
import org.tron.common.utils.ForkController;
import org.tron.common.utils.Sha256Hash;
import org.tron.common.utils.Time;
import org.tron.core.capsule.utils.MerkleTree;
Expand Down Expand Up @@ -185,9 +186,14 @@ private Sha256Hash getRawHash() {
public boolean validateSignature(DynamicPropertiesStore dynamicPropertiesStore,
AccountStore accountStore) throws ValidateSignatureException {
try {
ByteString witnessSig = block.getBlockHeader().getWitnessSignature();
if (!SignUtils.isValidLength(witnessSig.size(),
ForkController.signatureMaxSizeChecked())) {
throw new ValidateSignatureException(
"Witness signature size is " + witnessSig.size());
}
Comment thread
Sunny6889 marked this conversation as resolved.
byte[] sigAddress = SignUtils.signatureToAddress(getRawHash().getBytes(),
TransactionCapsule.getBase64FromByteString(
block.getBlockHeader().getWitnessSignature()),
TransactionCapsule.getBase64FromByteString(witnessSig),
CommonParameter.getInstance().isECKeyCryptoEngine());
byte[] witnessAccountAddress = block.getBlockHeader().getRawData().getWitnessAddress()
.toByteArray();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ public static long getWeight(Permission permission, byte[] address) {
* @see ForkController#init(org.tron.core.ChainBaseManager)
*/
public static long checkWeight(Permission permission, List<ByteString> sigs, byte[] hash,
List<ByteString> approveList)
List<ByteString> approveList, boolean checkMaxSignatureSize)
throws SignatureException, PermissionException, SignatureFormatException {
long currentWeight = 0;
if (sigs.size() > permission.getKeysCount()) {
Expand All @@ -240,9 +240,8 @@ public static long checkWeight(Permission permission, List<ByteString> sigs, byt
}
HashMap addMap = new HashMap();
for (ByteString sig : sigs) {
if (sig.size() < 65) {
throw new SignatureFormatException(
"Signature size is " + sig.size());
if (!SignUtils.isValidLength(sig.size(), checkMaxSignatureSize)) {
throw new SignatureFormatException("Signature size is " + sig.size());
}
String base64 = TransactionCapsule.getBase64FromByteString(sig);
byte[] address = SignUtils
Expand Down Expand Up @@ -487,7 +486,8 @@ public static boolean validateSignature(Transaction transaction,
throw new PermissionException("permission isn't exit");
}
checkPermission(permissionId, permission, contract);
long weight = checkWeight(permission, transaction.getSignatureList(), hash, null);
long weight = checkWeight(permission, transaction.getSignatureList(), hash, null,
ForkController.signatureMaxSizeChecked());
if (weight >= permission.getThreshold()) {
return true;
}
Expand Down Expand Up @@ -604,7 +604,7 @@ public void addSign(byte[] privateKey, AccountStore accountStore)
if (this.transaction.getSignatureCount() > 0) {
checkWeight(permission, this.transaction.getSignatureList(),
this.getTransactionId().getBytes(),
approveList);
approveList, true);
if (approveList.contains(ByteString.copyFrom(address))) {
throw new PermissionException(encode58Check(address) + " had signed!");
}
Expand All @@ -620,8 +620,9 @@ public void addSign(byte[] privateKey, AccountStore accountStore)
.signHash(getTransactionId().getBytes())));
this.transaction = this.transaction.toBuilder().addSignature(sig).build();
}

private static void checkPermission(int permissionId, Permission permission, Transaction.Contract contract) throws PermissionException {

private static void checkPermission(int permissionId, Permission permission,
Transaction.Contract contract) throws PermissionException {
if (permissionId != 0) {
if (permission.getType() != PermissionType.Active) {
throw new PermissionException("Permission type is error");
Expand Down Expand Up @@ -703,7 +704,7 @@ public boolean validateSignature(AccountStore accountStore,
}
}
isVerified = true;
}
}
return true;
}

Expand Down
5 changes: 5 additions & 0 deletions common/src/main/java/org/tron/core/config/Parameter.java
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ public class ChainConstant {
public static final int MAX_VOTE_NUMBER = 30;
public static final int SOLIDIFIED_THRESHOLD = 70; // 70%
public static final int PRIVATE_KEY_LENGTH = 64;
public static final int MIN_SIGNATURE_SIZE = 65;
// Canonical ECDSA signature is 65 bytes (r||s||v). 68 = 65 + up to 3 trailing
// padding bytes; this window accommodates historical non-canonical encodings
// observed on chain. Long-term goal is to tighten this back to a strict 65.
public static final int MAX_SIGNATURE_SIZE = 68;
public static final int BLOCK_SIZE = 2_000_000;
public static final long CLOCK_MAX_DELAY = 3600000; // 3600 * 1000 ms
public static final int BLOCK_PRODUCE_TIMEOUT_PERCENT = 50; // 50%
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@
import org.tron.consensus.base.Param;
import org.tron.consensus.base.Param.Miner;
import org.tron.consensus.dpos.MaintenanceManager;
import org.tron.common.utils.ForkController;
import org.tron.consensus.pbft.message.PbftBaseMessage;
import org.tron.consensus.pbft.message.PbftMessage;
import org.tron.core.ChainBaseManager;
import org.tron.protos.Protocol.PBFTMessage.DataType;

@Slf4j(topic = "pbft")
Expand Down Expand Up @@ -72,8 +72,6 @@ public List<ByteString> load(String s) throws Exception {
private PbftMessageAction pbftMessageAction;
@Setter
private MaintenanceManager maintenanceManager;
@Autowired
private ChainBaseManager chainBaseManager;

@PostConstruct
public void init() {
Expand Down Expand Up @@ -129,7 +127,7 @@ public void onPrePrepare(PbftMessage message) {
PbftMessage paMessage = message.buildPrePareMessage(miner);
forwardMessage(paMessage);
try {
paMessage.analyzeSignature();
paMessage.analyzeSignature(ForkController.signatureMaxSizeChecked());
} catch (SignatureException e) {
logger.error("", e);
}
Expand Down Expand Up @@ -175,7 +173,7 @@ public synchronized void onPrepare(PbftMessage message) {
doneMsg.put(message.getNo(), cmMessage);
forwardMessage(cmMessage);
try {
cmMessage.analyzeSignature();
cmMessage.analyzeSignature(ForkController.signatureMaxSizeChecked());
} catch (SignatureException e) {
logger.error("", e);
}
Expand Down Expand Up @@ -316,4 +314,4 @@ public void run() {
}
}, 10, 1000);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
package org.tron.consensus.pbft.message;

import com.google.protobuf.ByteString;
import com.google.protobuf.InvalidProtocolBufferException;
import java.io.IOException;
import java.security.SignatureException;
import java.util.stream.Collectors;
import org.bouncycastle.util.encoders.Hex;
import org.tron.common.crypto.ECKey;
import org.tron.common.crypto.SignUtils;
import org.tron.common.overlay.message.Message;
import org.tron.common.utils.ByteUtil;
import org.tron.common.utils.Sha256Hash;
import org.tron.common.utils.StringUtil;
import org.tron.core.capsule.TransactionCapsule;
import org.tron.core.exception.P2pException;
import org.tron.core.store.DynamicPropertiesStore;
import org.tron.protos.Protocol.PBFTMessage;
import org.tron.protos.Protocol.PBFTMessage.DataType;
import org.tron.protos.Protocol.SRL;
Expand Down Expand Up @@ -94,10 +97,15 @@ public DataType getDataType() {

public abstract String getNo();

public void analyzeSignature() throws SignatureException {
public void analyzeSignature(boolean checkMaxSignatureSize)
throws SignatureException {
ByteString signature = getPbftMessage().getSignature();
if (!SignUtils.isValidLength(signature.size(), checkMaxSignatureSize)) {
throw new SignatureException("PBFT signature size is " + signature.size());
}
byte[] hash = Sha256Hash.hash(true, getPbftMessage().getRawData().toByteArray());
publicKey = ECKey.signatureToAddress(hash, TransactionCapsule
.getBase64FromByteString(getPbftMessage().getSignature()));
.getBase64FromByteString(signature));
}

@Override
Expand Down
6 changes: 6 additions & 0 deletions crypto/src/main/java/org/tron/common/crypto/SignUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@
import org.tron.common.crypto.ECKey.ECDSASignature;
import org.tron.common.crypto.sm2.SM2;
import org.tron.common.crypto.sm2.SM2.SM2Signature;
import org.tron.core.config.Parameter.ChainConstant;

public class SignUtils {

public static boolean isValidLength(int size, boolean checkMaxSignatureSize) {
return size >= ChainConstant.MIN_SIGNATURE_SIZE
&& (!checkMaxSignatureSize || size <= ChainConstant.MAX_SIGNATURE_SIZE);
}

public static SignInterface getGeneratedRandomSign(
SecureRandom secureRandom, boolean isECKeyCryptoEngine) {
if (isECKeyCryptoEngine) {
Expand Down
13 changes: 12 additions & 1 deletion framework/src/main/java/org/tron/core/Wallet.java
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,16 @@ public GrpcAPI.Return broadcastTransaction(Transaction signedTransaction) {
trx.setTime(System.currentTimeMillis());
Sha256Hash txID = trx.getTransactionId();
try {
for (ByteString sig : signedTransaction.getSignatureList()) {
if (!SignUtils.isValidLength(sig.size(), true)) {
String info = "Signature size is " + sig.size();
logger.warn("Broadcast transaction {} has failed, {}.", txID, info);
return builder.setResult(false).setCode(response_code.SIGERROR)
Comment on lines +508 to +512
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] Now, if a node upgrades before VERSION_4_8_2 is activated, the networking layer will immediately and unconditionally reject oversized-signature transactions, while the consensus layer will only start rejecting them after VERSION_4_8_2 is activated. Was this inconsistency between the networking-layer and consensus-layer rejection conditions intentional by design?

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.

Yes — this is intentional. The asymmetry follows from what each path actually sees:

  • Consensus / block / PBFT validation must be able to replay history. Some historical on-chain signatures carry trailing padding within [66, 68] bytes that ECDSA recovery silently ignores. If we unconditionally enforced [65, 68] on these paths, a node that upgraded before activation would reject otherwise-valid historical blocks and stall. That's why the upper bound is gated by ForkController.signatureMaxSizeChecked() (i.e. VERSION_4_8_2 fork stats), so the network reaches the new rule together.
  • Admission paths (Wallet.broadcastTransaction, TransactionsMsgHandler.check, RelayService.checkHelloMessage) only see freshly-produced signatures from clients and peers — never historical chain state. A client emitting a 69-byte signature today is misbehaving regardless of fork state, so there is no replay risk in rejecting it at the edge.

Net effect on an upgraded-but-not-yet-activated node: it stops accepting freshly-broadcast oversized signatures, but still replays historical blocks correctly. This is documented in the "Compatibility" section of the PR description; happy to fold a shorter version into the code comment near the admission check if that would make the intent more discoverable.

.setMessage(ByteString.copyFromUtf8("Validate signature error: " + info))
.build();
}
}

if (tronNetDelegate.isBlockUnsolidified()) {
logger.warn("Broadcast transaction {} has failed, block unsolidified.", txID);
return builder.setResult(false).setCode(response_code.BLOCK_UNSOLIDIFIED)
Expand Down Expand Up @@ -644,7 +654,8 @@ public TransactionApprovedList getTransactionApprovedList(Transaction trx) {
byte[] hash = Sha256Hash.hash(CommonParameter
.getInstance().isECKeyCryptoEngine(), trx.getRawData().toByteArray());
for (ByteString sig : trx.getSignatureList()) {
if (sig.size() < 65) {
// Read-only path: skip the upper bound so historical txs stay resolvable.
if (!SignUtils.isValidLength(sig.size(), false)) {
throw new SignatureFormatException(
"Signature size is " + sig.size());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;
import org.tron.common.crypto.ECKey;
import org.tron.common.crypto.SignUtils;
import org.tron.common.es.ExecutorServiceManager;
import org.tron.common.utils.ByteArray;
import org.tron.common.utils.ForkController;
import org.tron.common.utils.Sha256Hash;
import org.tron.consensus.base.Param;
import org.tron.core.ChainBaseManager;
Expand Down Expand Up @@ -173,6 +175,10 @@ private class ValidPbftSignTask implements Callable<Boolean> {
@Override
public Boolean call() throws Exception {
try {
if (!SignUtils.isValidLength(sign.size(), ForkController.signatureMaxSizeChecked())) {
logger.error("viewN {} pbft signature size {} is invalid", viewN, sign.size());
return false;
}
byte[] srAddress = ECKey.signatureToAddress(dataHash,
TransactionCapsule.getBase64FromByteString(sign));
if (!srSet.contains(ByteString.copyFrom(srAddress))) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.concurrent.locks.Lock;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import org.tron.common.utils.ForkController;
import org.tron.consensus.base.Param;
import org.tron.consensus.pbft.PbftManager;
import org.tron.consensus.pbft.message.PbftBaseMessage;
Expand Down Expand Up @@ -50,7 +51,7 @@ public void processMessage(PeerConnection peer, PbftMessage msg) throws Exceptio
&& currentEpoch - msg.getEpoch() > expireEpoch) {
return;
}
msg.analyzeSignature();
msg.analyzeSignature(ForkController.signatureMaxSizeChecked());
String key = buildKey(msg);
Lock lock = striped.get(key);
try {
Expand Down Expand Up @@ -79,4 +80,4 @@ private String buildKey(PbftBaseMessage msg) {
return msg.getKey() + msg.getPbftMessage().getRawData().getMsgType().toString();
}

}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.tron.core.net.messagehandler;

import com.google.protobuf.ByteString;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand All @@ -13,6 +14,7 @@
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import org.tron.common.crypto.SignUtils;
import org.tron.common.es.ExecutorServiceManager;
import org.tron.common.utils.Sha256Hash;
import org.tron.core.ChainBaseManager;
Expand Down Expand Up @@ -142,6 +144,12 @@ private void check(PeerConnection peer, TransactionsMessage msg) throws P2pExcep
throw new P2pException(TypeEnum.BAD_TRX,
"tx " + item.getHash() + " contract size should be greater than 0");
}
for (ByteString sig : trx.getSignatureList()) {
if (!SignUtils.isValidLength(sig.size(), true)) {
throw new P2pException(TypeEnum.BAD_TRX,
"tx " + item.getHash() + " signature size is " + sig.size());
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,16 @@ public boolean checkHelloMessage(HelloMessage message, Channel channel) {

boolean flag;
try {
ByteString signature = msg.getSignature();
if (!SignUtils.isValidLength(signature.size(), true)) {
logger.warn("HelloMessage from {}, signature size {} is invalid.",
channel.getInetAddress(), signature.size());
return false;
}
Sha256Hash hash = Sha256Hash.of(CommonParameter
.getInstance().isECKeyCryptoEngine(), ByteArray.fromLong(msg.getTimestamp()));
String sig =
TransactionCapsule.getBase64FromByteString(msg.getSignature());
TransactionCapsule.getBase64FromByteString(signature);
byte[] sigAddress = SignUtils.signatureToAddress(hash.getBytes(), sig,
Args.getInstance().isECKeyCryptoEngine());
if (manager.getDynamicPropertiesStore().getAllowMultiSign() != 1) {
Expand Down
20 changes: 20 additions & 0 deletions framework/src/test/java/org/tron/core/ForkControllerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,26 @@ public void testPass() {
Assert.assertTrue(flag);
}

@Test
public void testSignatureMaxSizeChecked() {
Parameter.ForkBlockVersionEnum forkVersion = Parameter.ForkBlockVersionEnum.VERSION_4_8_2;
dynamicPropertiesStore.saveLatestBlockHeaderTimestamp(forkVersion.getHardForkTime());

byte[] stats = new byte[5];
dynamicPropertiesStore.statsByVersion(forkVersion.getValue(), stats);
Assert.assertFalse(ForkController.signatureMaxSizeChecked());

stats[0] = 1;
stats[1] = 1;
stats[2] = 1;
dynamicPropertiesStore.statsByVersion(forkVersion.getValue(), stats);
Assert.assertFalse(ForkController.signatureMaxSizeChecked());

stats[3] = 1;
dynamicPropertiesStore.statsByVersion(forkVersion.getValue(), stats);
Assert.assertTrue(ForkController.signatureMaxSizeChecked());
}

@Test
public void testReset() {
List<ByteString> list = new ArrayList<>();
Expand Down
Loading
Loading