Skip to content

feat(net): validate merkle root before block broadcast #6716

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
xxo1shine wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

feat(net): validate merkle root before block broadcast #6716

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
17 changes: 17 additions & 0 deletions chainbase/src/main/java/org/tron/core/capsule/BlockCapsule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

package org.tron.core.capsule;

import static org.tron.core.exception.BadBlockException.TypeEnum.CALC_MERKLE_ROOT_FAILED;

import com.google.common.primitives.Longs;
import com.google.protobuf.ByteString;
import com.google.protobuf.CodedInputStream;
Expand All @@ -37,6 +39,7 @@
import org.tron.common.utils.Time;
import org.tron.core.capsule.utils.MerkleTree;
import org.tron.core.config.Parameter.ChainConstant;
import org.tron.core.exception.BadBlockException;
import org.tron.core.exception.BadItemException;
import org.tron.core.exception.ValidateSignatureException;
import org.tron.core.store.AccountStore;
Expand All @@ -49,6 +52,7 @@
public class BlockCapsule implements ProtoCapsule<Block> {

public boolean generatedByMyself = false;
private volatile boolean merkleValidated = false;
@Getter
@Setter
private TransactionRetCapsule result;
Expand Down Expand Up @@ -225,6 +229,19 @@ public Sha256Hash calcMerkleRoot() {
return MerkleTree.getInstance().createTree(ids).getRoot().getHash();
}

public void validateMerkleRoot() throws BadBlockException {
if (merkleValidated) {
return;
}
Sha256Hash actual = calcMerkleRoot();
if (!actual.equals(getMerkleRoot())) {
throw new BadBlockException(CALC_MERKLE_ROOT_FAILED,
String.format("merkle root mismatch for block %d: expected %s, actual %s",
getNum(), getMerkleRoot(), actual));
}
merkleValidated = true;
}

public void setMerkleRoot() {
BlockHeader.raw blockHeaderRaw =
this.block.getBlockHeader().getRawData().toBuilder()
Expand Down
14 changes: 8 additions & 6 deletions framework/src/main/java/org/tron/core/db/Manager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1293,12 +1293,7 @@ public void pushBlock(final BlockCapsule block)
try (PendingManager pm = new PendingManager(this)) {

if (!block.generatedByMyself) {
if (!block.calcMerkleRoot().equals(block.getMerkleRoot())) {
logger.warn("Num: {}, the merkle root doesn't match, expect is {} , actual is {}.",
block.getNum(), block.getMerkleRoot(), block.calcMerkleRoot());
throw new BadBlockException(CALC_MERKLE_ROOT_FAILED,
String.format("The merkle hash is not validated for %d", block.getNum()));
}
block.validateMerkleRoot();
consensus.receiveBlock(block);
}

Expand Down Expand Up @@ -2106,6 +2101,13 @@ public void rePush(TransactionCapsule tx) {
return;
}

String ownerAddress = ByteArray.toHexString(tx.getOwnerAddress());
synchronized (this) {
if (ownerAddressSet.contains(ownerAddress)) {
tx.setVerified(false);
}
}
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.

[INT] Give code comment why should setVerified as false here.

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 if statement is already quite concise and clear. Transactions involving permission changes should reset this flag. Re-verification is required to prevent transactions with incorrect signatures from entering the pending queue.


try {
this.pushTransaction(tx);
} catch (ValidateSignatureException | ContractValidateException | ContractExeException
Expand Down
5 changes: 5 additions & 0 deletions framework/src/main/java/org/tron/core/net/TronNetDelegate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,11 @@ public boolean validBlock(BlockCapsule block) throws P2pException {
throw new P2pException(TypeEnum.BAD_BLOCK,
"time:" + time + ",block time:" + block.getTimeStamp());
}
try {
block.validateMerkleRoot();
} catch (BadBlockException e) {
throw new P2pException(TypeEnum.BLOCK_MERKLE_ERROR, e.getMessage());
}
Comment on lines +350 to +354
Copy link
Copy Markdown

@alan-eth alan-eth Apr 29, 2026

Choose a reason for hiding this comment

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

Nice work moving the merkle check before broadcast — that's exactly the right place to catch tampered blocks before we forward them. 🎯

Minor consistency nit: when wrapping BadBlockException here, you're passing e.getMessage() as the message string, but the existing flow in processBlock (around L302) wraps it as throw new P2pException(TypeEnum.BLOCK_MERKLE_ERROR, e) (the exception itself becomes the cause). Would it make sense to do the same here so the original stack trace is preserved when this propagates up? It also keeps the two BLOCK_MERKLE_ERROR throw sites symmetric.

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 BLOCK_MERKLE_ERROR exception is quite obvious, and the message information is sufficient. There's no need to re-throw the original exception, and the previous logic doesn't need to be changed; there's no impact whatsoever.

validSignature(block);
return witnessScheduleStore.getActiveWitnesses().contains(block.getWitnessAddress());
}
Expand Down
38 changes: 38 additions & 0 deletions framework/src/test/java/org/tron/core/capsule/BlockCapsuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.tron.common.utils.Sha256Hash;
import org.tron.core.Wallet;
import org.tron.core.config.args.Args;
import org.tron.core.exception.BadBlockException;
import org.tron.core.exception.BadItemException;
import org.tron.protos.Protocol.Transaction.Contract.ContractType;
import org.tron.protos.contract.BalanceContract.TransferContract;
Expand Down Expand Up @@ -85,6 +86,43 @@ public void testCalcMerkleRoot() throws Exception {
logger.info("Transaction[O] Merkle Root : {}", blockCapsule0.getMerkleRoot().toString());
}

@Test
public void testValidateMerkleRoot() throws Exception {
// build a fresh local block so shared blockCapsule0 is not mutated
String parentHash = "9938a342238077182498b464ac0292229938a342238077182498b464ac029222";
BlockCapsule local = new BlockCapsule(1,
Sha256Hash.wrap(ByteString.copyFrom(ByteArray.fromHexString(parentHash))),
1234,
ByteString.copyFrom("1234567".getBytes()));

// valid block: setMerkleRoot then validate — should not throw
local.setMerkleRoot();
local.validateMerkleRoot(); // no exception

// flag is set — second call must be a no-op (no recomputation)
local.validateMerkleRoot(); // still no exception

// tamper with a transaction to break merkle
TransferContract transferContract = TransferContract.newBuilder()
.setAmount(999L)
.setOwnerAddress(ByteString.copyFrom("0x0000000000000000000".getBytes()))
.setToAddress(ByteString.copyFrom(ByteArray.fromHexString(
Wallet.getAddressPreFixString() + "A389132D6639FBDA4FBC8B659264E6B7C90DB086")))
.build();
local.addTransaction(
new TransactionCapsule(transferContract, ContractType.TransferContract));
// merkle root was set before adding the tx, so it is now stale/invalid

BlockCapsule tampered = new BlockCapsule(local.getInstance());
// tampered has no merkleValidated flag set
try {
tampered.validateMerkleRoot();
Assert.fail("Expected BadBlockException for merkle mismatch");
} catch (BadBlockException e) {
Assert.assertTrue(e.getMessage().contains("merkle"));
}
}

/* @Test
public void testAddTransaction() {
TransactionCapsule transactionCapsule = new TransactionCapsule("123", 1L);
Expand Down
13 changes: 12 additions & 1 deletion framework/src/test/java/org/tron/core/db/ManagerMockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,18 @@ public void testRePush() {
@Test
public void testRePush1() {
Manager dbManager = spy(new Manager());
Protocol.Transaction transaction = Protocol.Transaction.newBuilder().build();
BalanceContract.TransferContract transferContract =
BalanceContract.TransferContract.newBuilder()
.setOwnerAddress(ByteString.copyFromUtf8("aaa"))
.setToAddress(ByteString.copyFromUtf8("bbb"))
.setAmount(1)
.build();
Protocol.Transaction transaction = Protocol.Transaction.newBuilder()
.setRawData(Protocol.Transaction.raw.newBuilder()
.addContract(Protocol.Transaction.Contract.newBuilder()
.setParameter(Any.pack(transferContract))
.setType(Protocol.Transaction.Contract.ContractType.TransferContract)))
.build();
TransactionCapsule trx = new TransactionCapsule(transaction);
TransactionStore transactionStoreMock = mock(TransactionStore.class);

Expand Down
33 changes: 31 additions & 2 deletions framework/src/test/java/org/tron/core/db/ManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,8 @@ public void pushBlockInvalidMerkelRoot() {
} catch (BadBlockException e) {
Assert.assertTrue(e instanceof BadBlockException);
Assert.assertTrue(e.getType().equals(CALC_MERKLE_ROOT_FAILED));
Assert.assertEquals("The merkle hash is not validated for "
+ blockCapsule2.getNum(), e.getMessage());
Assert.assertTrue(e.getMessage().startsWith(
"merkle root mismatch for block " + blockCapsule2.getNum() + ":"));
} catch (Exception e) {
Assert.assertFalse(e instanceof Exception);
}
Expand Down Expand Up @@ -1292,6 +1292,35 @@ public void blockTrigger() {
Assert.assertEquals(TronError.ErrCode.EVENT_SUBSCRIBE_ERROR, thrown.getErrCode());
}

@Test
Copy link
Copy Markdown
Collaborator

@waynercheung waynercheung May 6, 2026

Choose a reason for hiding this comment

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

[SHOULD] The PR title is "validate merkle root before block broadcast", but TronNetDelegate.validBlock() - the function that actually realises this - has no direct test. Consider adding a test that constructs a block with a tampered merkle root and asserts validBlock throws P2pException with TypeEnum.BLOCK_MERKLE_ERROR. The existing tests only cover BlockCapsule.validateMerkleRoot() and the Manager.pushBlock path.

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.

Agreed. BlockCapsule.validateMerkleRoot() and the Manager.pushBlock path are covered, but TronNetDelegate.validBlock() — the actual pre-broadcast entry point — has no direct test (it's mocked out in BlockMsgHandlerTest). Plan: add a unit test that constructs a BlockCapsule with a tampered merkle root, calls validBlock directly, and asserts P2pException is thrown with TypeEnum.BLOCK_MERKLE_ERROR. Will push this in the next commit. Thanks for the catch!

public void testRePushResetsVerifiedOnOwnerAddressSetHit() throws Exception {
TransferContract transferContract = TransferContract.newBuilder()
.setAmount(1L)
.setOwnerAddress(ByteString.copyFrom(
ByteArray.fromHexString(Wallet.getAddressPreFixString()
+ "548794500882809695A8A687866E76D4271A1ABC")))
.setToAddress(ByteString.copyFrom(
ByteArray.fromHexString(Wallet.getAddressPreFixString()
+ "A389132D6639FBDA4FBC8B659264E6B7C90DB086")))
.build();
TransactionCapsule tx = new TransactionCapsule(transferContract, ContractType.TransferContract);
tx.setVerified(true); // simulate mempool-cached state

String ownerAddress = ByteArray.toHexString(tx.getOwnerAddress());

// Inject ownerAddress into ownerAddressSet via reflection
Set<String> ownerAddressSet =
(Set<String>) ReflectUtils.getFieldObject(dbManager, "ownerAddressSet");
ownerAddressSet.add(ownerAddress);

// rePush should reset isVerified to false before pushTransaction
dbManager.rePush(tx);

// After rePush, isVerified must be false
Boolean verified = (Boolean) ReflectUtils.getFieldObject(tx, "isVerified");
Assert.assertFalse(verified);
}

public void adjustBalance(AccountStore accountStore, byte[] accountAddress, long amount)
throws BalanceInsufficientException {
Commons.adjustBalance(accountStore, accountAddress, amount,
Expand Down
41 changes: 41 additions & 0 deletions framework/src/test/java/org/tron/core/net/TronNetDelegateTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,24 @@

import static org.mockito.Mockito.mock;

import com.google.protobuf.ByteString;
import java.lang.reflect.Field;
import org.junit.Assert;
import org.junit.Test;
import org.mockito.Mockito;
import org.tron.common.TestConstants;
import org.tron.common.parameter.CommonParameter;
import org.tron.common.utils.ByteArray;
import org.tron.common.utils.Sha256Hash;
import org.tron.core.ChainBaseManager;
import org.tron.core.Wallet;
import org.tron.core.capsule.BlockCapsule;
import org.tron.core.capsule.TransactionCapsule;
import org.tron.core.config.args.Args;
import org.tron.core.exception.P2pException;
import org.tron.core.exception.P2pException.TypeEnum;
import org.tron.protos.Protocol.Transaction.Contract.ContractType;
import org.tron.protos.contract.BalanceContract.TransferContract;

public class TronNetDelegateTest {

Expand Down Expand Up @@ -49,4 +57,37 @@ public void test() throws Exception {

Assert.assertTrue(!tronNetDelegate.isBlockUnsolidified());
}

@Test
public void testValidBlockMerkleRoot() throws Exception {
Args.setParam(new String[] {}, TestConstants.TEST_CONF);

String parentHash = "9938a342238077182498b464ac0292229938a342238077182498b464ac029222";
BlockCapsule block = new BlockCapsule(1,
Sha256Hash.wrap(ByteString.copyFrom(ByteArray.fromHexString(parentHash))),
System.currentTimeMillis(),
ByteString.copyFrom("witness".getBytes()));
block.setMerkleRoot();

// Add a transaction after setMerkleRoot, making the stored merkle root stale.
TransferContract transferContract = TransferContract.newBuilder()
.setAmount(1L)
.setOwnerAddress(ByteString.copyFrom("0x0000000000000000000".getBytes()))
.setToAddress(ByteString.copyFrom(ByteArray.fromHexString(
Wallet.getAddressPreFixString() + "A389132D6639FBDA4FBC8B659264E6B7C90DB086")))
.build();
block.addTransaction(
new TransactionCapsule(transferContract, ContractType.TransferContract));

// Wrap in a fresh BlockCapsule so the merkleValidated flag is reset.
BlockCapsule tampered = new BlockCapsule(block.getInstance());

TronNetDelegate tronNetDelegate = new TronNetDelegate();
try {
tronNetDelegate.validBlock(tampered);
Assert.fail("Expected P2pException for tampered merkle root");
} catch (P2pException e) {
Assert.assertEquals(TypeEnum.BLOCK_MERKLE_ERROR, e.getType());
}
}
}
Loading