feat(net): validate merkle root before block broadcast#6716
feat(net): validate merkle root before block broadcast#6716xxo1shine wants to merge 1 commit intotronprotocol:developfrom
Conversation
…fied on permission revocation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
xxo1shine
force-pushed
the
fix/block-merkle-broadcast-and-sig-bypass-1
branch
from
d13259c to
193f361
Compare
| try { | ||
| block.validateMerkleRoot(); | ||
| } catch (BadBlockException e) { | ||
| throw new P2pException(TypeEnum.BLOCK_MERKLE_ERROR, e.getMessage()); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if (ownerAddressSet.contains(ownerAddress)) { | ||
| tx.setVerified(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
[INT] Give code comment why should setVerified as false here.
There was a problem hiding this comment.
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.
| import org.tron.core.capsule.utils.MerkleTree; | ||
| import org.tron.core.config.Parameter.ChainConstant; | ||
| import org.tron.core.exception.BadBlockException; | ||
| import static org.tron.core.exception.BadBlockException.TypeEnum.CALC_MERKLE_ROOT_FAILED; |
There was a problem hiding this comment.
[SHOULD] Static import is interleaved with regular imports. Move it above all import org... lines as a separate group, matching the convention used in TronNetDelegate.java:4 and ManagerTest.java:11. Likely a Checkstyle violation.
| Assert.assertEquals(TronError.ErrCode.EVENT_SUBSCRIBE_ERROR, thrown.getErrCode()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
[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.
7 participants
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details