Skip to content

fix(db): validate block signatures against committed state #7076

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 7 commits into
base: release_v4.8.3
Choose a base branch
from
+930 −57
Open

fix(db): validate block signatures against committed state #7076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1dad6be
fix(db): validate block witness signatures against committed state
Oct 30, 2026
9ee6ee7
fix(db): invalidate expired transaction verification caches
Nov 2, 2026
98e0eab
fix(net): reject malformed block witness addresses
Nov 3, 2026
442f32b
test(net): use valid witness addresses in block fixtures
Nov 3, 2026
b1ebc3b
fix(db): revalidate repush transactions against current permissions
Nov 4, 2026
1bc52a6
fix(db): defer block side effects until successful application
Nov 5, 2026
fe1d4fc
perf(db): keep transaction signature verification outside admission l…
Nov 5, 2026
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
52 changes: 36 additions & 16 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 @@ -283,7 +283,12 @@ public class Manager {
while (isRunRePushThread) {
TransactionCapsule tx = null;
try {
tx = getRePushTransactions().peek();
synchronized (this) {
tx = getRePushTransactions().peek();
if (tx != null) {
pushTransactionQueue.add(tx);
}
}
if (tx != null) {
this.rePush(tx);
} else {
Expand All @@ -302,9 +307,12 @@ public class Manager {
throw e;
});
} finally {
if (tx != null && getRePushTransactions().remove(tx)) {
Metrics.gaugeInc(MetricKeys.Gauge.MANAGER_QUEUE, -1,
MetricLabels.Gauge.QUEUE_REPUSH);
if (tx != null) {
if (getRePushTransactions().remove(tx)) {
Metrics.gaugeInc(MetricKeys.Gauge.MANAGER_QUEUE, -1,
MetricLabels.Gauge.QUEUE_REPUSH);
}
pushTransactionQueue.remove(tx);
}
}
}
Expand Down Expand Up @@ -1228,7 +1236,8 @@ public List<TransactionCapsule> getVerifyTxs(BlockCapsule block) {

/**
* Save a block. Untrusted callers must first use network prevalidation to check its timestamp,
* transaction structure, Merkle root and block signature.
* transaction structure and Merkle root. Check witness signatures before broadcast as well.
* Received block witness authorization is checked against committed state before admission.
*/
public void pushBlock(final BlockCapsule block)
throws ValidateSignatureException, ContractValidateException, ContractExeException,
Expand All @@ -1242,11 +1251,6 @@ public void pushBlock(final BlockCapsule block)
synchronized (this) {
Metrics.histogramObserve(blockedTimer.get());
blockedTimer.remove();
if (Metrics.enabled()) {
Metrics.histogramObserve(MetricKeys.Histogram.BLOCK_TRANSACTION_COUNT,
block.getTransactions().size(),
StringUtil.encode58Check(block.getWitnessAddress().toByteArray()));
}
long headerNumber = getDynamicPropertiesStore().getLatestBlockHeaderNumber();
if (block.getNum() <= headerNumber && khaosDb.containBlockInMiniStore(block.getBlockId())) {
logger.info("Block {} is already exist.", block.getBlockId().getString());
Expand All @@ -1261,15 +1265,15 @@ public void pushBlock(final BlockCapsule block)
block.getNum(), rePushTransactions.size(), pendingTransactions.size(),
block.getTransactions().size(), txs.size());

if (CommonParameter.getInstance().getShutdownBlockTime() != null
&& CommonParameter.getInstance().getShutdownBlockTime()
.isSatisfiedBy(new Date(block.getTimeStamp()))) {
latestSolidityNumShutDown = block.getNum();
}

try (PendingManager pm = new PendingManager(this)) {

if (!block.generatedByMyself) {
// Pending changes have been reset; use committed witness permissions for admission.
// switchFork also verifies admitted fork blocks against their own parent state.
if (!block.validateSignature(getDynamicPropertiesStore(), getAccountStore())) {

@waynercheung waynercheung Nov 2, 2026

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.

[MUST] Agree with Federico2014's parent-state comment. Two additions: rejectsForkWitnessNotAuthorizedAtCurrentHead asserts that incorrect rejection and should be inverted; and whatever fix is chosen, consensus.receiveBlock and the shutdown target update must stay behind the parent-state authorization, not just be skipped for fork blocks.

@xxo1shine xxo1shine Nov 3, 2026

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.

rejectsForkWitnessNotAuthorizedAtCurrentHead covers the current implementation's admission check against committed permissions on the canonical chain; it does not establish that a fork block is authorized under its parent state. Changing the expected result to acceptance requires implementing parent-state validation as well, rather than only changing the test assertion.

The current implementation does not defer fork authorization. Both consensus.receiveBlock and the shutdown target update still run after successful authorization in Manager. If authorization is later deferred until the parent state is available, both operations will also need to run after that validation succeeds.

For this PR, I would retain the current implementation and address parent-state validation and the corresponding test changes as a separate improvement, consistent with the scope described in my earlier review reply.

@bladehan1 bladehan1 Nov 3, 2026

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.

[NIT] Null witness account surfaces as NPE instead of a clean signature-validation failure

This new call site invokes BlockCapsule.validateSignature unwrapped. On the multi-sign path that method dereferences accountStore.get(witnessAccountAddress) without a null check (BlockCapsule.java:198) and only catches SignatureException, so a block whose witness address has no account yet is rejected via an unchecked NPE rather than a clean signature-validation failure. The pre-PR ingress call site in TronNetDelegate wrapped this same call in catch (Exception) and classified it as BLOCK_SIGN_INVALID; here the block is still rejected (all current pushBlock callers catch broadly, so no crash), but error classification and logs degrade to a generic NPE, and any future caller that only catches ValidateSignatureException would miss it.

Suggestion: normalize the failure at the source — either null-check the account lookup in BlockCapsule.validateSignature and return false on absence (this also covers the switchFork call site), or wrap this call and rethrow as ValidateSignatureException. A small post-merge follow-up is fine; not blocking.

@xxo1shine xxo1shine Nov 4, 2026

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.

The missing account null check is existing behavior in BlockCapsule.validateSignature(), and the new call site in this PR does expose that case as an NPE. The current call chain still catches the exception and rejects the block before admission, so the impact is primarily on exception classification and logging; it does not change the authorization decision enforced by this fix.

This PR focuses on validating block authorization under the Manager lock after resetting the pending state. Normalizing exception handling in the shared signature-validation method affects other call sites and is better handled separately with the corresponding regression coverage. Since the current rejection behavior remains effective, I would leave this non-blocking robustness improvement outside this PR.

throw new ValidateSignatureException(
"block " + block.getNum() + " signature invalid");
}
consensus.receiveBlock(block);
}

Expand Down Expand Up @@ -1324,6 +1328,7 @@ public void pushBlock(final BlockCapsule block)
synchronized (forkLock) {
switchFork(newBlock);
}
onBlockApplied(block);
logger.info(SAVE_BLOCK, newBlock);

logger.warn(
Expand Down Expand Up @@ -1353,6 +1358,7 @@ public void pushBlock(final BlockCapsule block)
long newSolidNum = getDynamicPropertiesStore().getLatestSolidifiedBlockNum();
blockTrigger(newBlock, oldSolidNum, newSolidNum);
}
onBlockApplied(block);
logger.info(SAVE_BLOCK, newBlock);
}
//clear ownerAddressSet
Expand Down Expand Up @@ -1381,6 +1387,20 @@ public void pushBlock(final BlockCapsule block)
}
}

private void onBlockApplied(BlockCapsule block) {
if (Metrics.enabled()) {
Metrics.histogramObserve(MetricKeys.Histogram.BLOCK_TRANSACTION_COUNT,
block.getTransactions().size(),
StringUtil.encode58Check(block.getWitnessAddress().toByteArray()));
}

if (CommonParameter.getInstance().getShutdownBlockTime() != null
&& CommonParameter.getInstance().getShutdownBlockTime()
.isSatisfiedBy(new Date(block.getTimeStamp()))) {
latestSolidityNumShutDown = block.getNum();
}
}

void blockTrigger(final BlockCapsule block, long oldSolid, long newSolid) {
// post block and logs for jsonrpc
try {
Expand Down
6 changes: 5 additions & 1 deletion 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 @@ -353,6 +353,10 @@ public void validSignature(BlockCapsule block) throws P2pException {
}
}

/**
* Validate block structure and Merkle root. Advertisement also checks witness authorization;
* sync defers it to Manager, which checks committed state and revalidates forks after rewind.
*/
public void validBlock(BlockCapsule block) throws P2pException {
for (Transaction transaction : block.getInstance().getTransactionsList()) {
int contractCount = transaction.getRawData().getContractCount();
Expand All @@ -373,7 +377,6 @@ public void validBlock(BlockCapsule block) throws P2pException {
} catch (BadBlockException e) {
throw new P2pException(TypeEnum.BLOCK_MERKLE_INVALID, e.getMessage());
}
validSignature(block);
}

public boolean validAdvBlock(PeerConnection peer, BlockCapsule block) throws P2pException {
Expand All @@ -391,6 +394,7 @@ public boolean validAdvBlock(PeerConnection peer, BlockCapsule block) throws P2p
}
}
validBlock(block);
validSignature(block);
return activeWitness;
}

Expand Down
12 changes: 10 additions & 2 deletions framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,25 @@ public void processMessage(PeerConnection peer, TronMessage msg) throws P2pExcep
BlockId blockId = blockMessage.getBlockId();

BlockCapsule blockCapsule = blockMessage.getBlockCapsule();
int witnessAddressSize = blockCapsule.getWitnessAddress().size();
if (witnessAddressSize != Constant.TRON_ADDRESS_SIZE) {
throw new P2pException(TypeEnum.BAD_BLOCK,
"Invalid witness address size: " + witnessAddressSize);
}

if (blockCapsule.getInstance().getSerializedSize() > maxBlockSize) {
logger.error("Receive bad block {} from peer {}, block size over limit",
blockMessage.getBlockId(), peer.getInetSocketAddress());
blockId, peer.getInetSocketAddress());
throw new P2pException(TypeEnum.BAD_MESSAGE, "block size over limit");
}

long gap = blockCapsule.getTimeStamp() - System.currentTimeMillis();
if (gap >= BLOCK_PRODUCED_INTERVAL) {
logger.error("Receive bad block {} from peer {}, block time error",
blockMessage.getBlockId(), peer.getInetSocketAddress());
blockId, peer.getInetSocketAddress());
throw new P2pException(TypeEnum.BAD_MESSAGE, "block time error");
}

if (!fastForward && !peer.isRelayPeer()) {
check(peer, blockMessage);
}
Expand Down
40 changes: 40 additions & 0 deletions 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 @@ -420,6 +420,46 @@ public void testRePush1() {
dbManager.rePush(trx);
}

@Test
public void rePushLoopCleansRegistrationForKnownTransaction() throws Exception {
assertRePushLoopCleansRegistration(true);
}

@Test
public void rePushLoopCleansRegistrationForUnsupportedAuths() throws Exception {
assertRePushLoopCleansRegistration(false);
}

private void assertRePushLoopCleansRegistration(boolean alreadyKnown) throws Exception {
Manager manager = new Manager();
TransactionCapsule tx = new TransactionCapsule(Protocol.Transaction.newBuilder()
.setRawData(Protocol.Transaction.raw.newBuilder()
.addAuths(Protocol.authority.getDefaultInstance())).build());
BlockingQueue<TransactionCapsule> repush = new LinkedBlockingQueue<>();
BlockingQueue<TransactionCapsule> pushing = new LinkedBlockingQueue<>();
repush.add(tx);
setField(manager, "rePushTransactions", repush);
setField(manager, "pushTransactionQueue", pushing);
ChainBaseManager chain = mock(ChainBaseManager.class);
TransactionStore transactions = mock(TransactionStore.class);
setField(manager, "chainBaseManager", chain);
when(chain.getTransactionStore()).thenReturn(transactions);
List<TransactionCapsule> registeredAtLookup = new ArrayList<>();
when(transactions.has(any())).thenAnswer(invocation -> {
manager.stopRePushThread();
registeredAtLookup.addAll(pushing);
return alreadyKnown;
});
Field loop = Manager.class.getDeclaredField("rePushLoop");
loop.setAccessible(true);

((Runnable) loop.get(manager)).run();

Assert.assertEquals(Collections.singletonList(tx), registeredAtLookup);
assertTrue(repush.isEmpty());
assertTrue(pushing.isEmpty());
}

@Test
public void testPostSolidityFilter() throws Exception {
Manager dbManager = spy(new Manager());
Expand Down
Loading
Loading