[xxo1shine]

Summary

What does this PR do?

• Validate received block witness authorization in Manager.pushBlock under the Manager lock, after PendingManager resets pending state and before KhaosDB admission. Sync defers witness authorization to this check; advertised blocks retain signature validation before broadcast and are checked again at admission.
• Reject block messages whose witness address is not 21 bytes before handing them to advertisement or sync processing.
• Update the shutdown target and observe BLOCK_TRANSACTION_COUNT only after successful block application or fork switching. Each successful pushBlock updates them once for the incoming block; rejected blocks, duplicates, and parked forks do not trigger these updates.
• Register a transaction held by rePushLoop in pushTransactionQueue atomically with peeking under the Manager lock, and remove that registration in finally. This keeps the reference visible to existing owner-marker retention and consensus-rule cache invalidation even after timeout cleanup or block-generation discard removes its repush entry.

Why are these changes required?

Witness permissions can change between network prevalidation and block admission. Pending permission changes must not authorize an incoming block against uncommitted state, and rejected blocks must not alter the shutdown target or transaction-count metrics.

A repush worker can retain a transaction after its queue entry disappears. Preserving visibility of that reference prevents owner markers from being cleared prematurely, allowing the existing rePush cache-invalidation logic to reject revoked signatures. Unchanged owners retain signature-cache reuse.

Test Plan

This PR has been tested by:

• ./gradlew clean build -x test — passed.
• ./gradlew :framework:checkstyleMain :framework:checkstyleTest -x test — passed; the clean build also checked framework/plugins Checkstyle.
• bash .github/scripts/check_math_usage.sh — passed.
• Targeted unit suites passed during implementation, including ManagerWitnessSignatureTest, ManagerTest, ManagerMockTest, and ManagerStrictEcdsaActivationTest. The latest repush changes and updated ManagerTest cases were compiled but not rerun. No full unit-test suite was run.

Regression coverage includes committed versus pending witness permissions, permission changes while waiting for the Manager lock, advertised-block validation before broadcast, invalid witness-address lengths, successful-only metrics/shutdown updates, held repush references after timeout or generation discard, cache reuse, cleanup on early exits, and fresh-transaction verification outside the admission locks.

Follow up

• Admission remains based on committed current-head permissions. Parent-state fork admission and advertisement-ingress false rejections caused by pending state are not redesigned; existing switchFork verification against rewound parent state remains unchanged.
• A dedicated metric-child regression for witness-authorization rejection remains a follow-up. The added metric checks cover unlinked-block rejection, normal success, parked forks, successful fork switching, and duplicates.

Extra details

Transaction signature verification for pending admission remains outside the Manager and transaction locks. This PR does not add an atomic permission check at pending admission; existing block-generation revalidation and received-block validation remain responsible for rejecting invalid permissions before inclusion. The trusted locally generated block bypass is unchanged. No dependency or configuration changes.

[Federico2014]

[MUST] Witness authorization needs to be checked against the block’s actual parent state.

The new Manager check can reject a valid competing branch. A and B are children of P, whose witness permission authorizes K1. A contains a K1 → K2 rotation, while B is signed by K1. B can reach ingress while A is still being processed and has not yet executed its permission-update transaction. The ingress check therefore still reads K1 and correctly accepts B’s signature. A then finishes and commits before B acquires the Manager lock. B is now a fork block, and Manager.java:1270 rejects it using the canonical branch’s K2 permission, although K1 remains valid under B’s parent P. Rejection happens before Khaos admission, so switchFork() cannot perform its parent-state validation. A regression test for this sequence passes on the base and fails with this change.

There is also a related pre-existing ingress limitation: TronNetDelegate.java:397 still checks advertised blocks before their parent has necessarily been applied. A K2-signed child of a parent rotating K1 → K2 can therefore be rejected using the old K1 permission before reaching Manager.

Please validate authorization against the actual parent state, defer/retry blocks when that state is unavailable, and add regression coverage for both sequences.

[SeriousCoding789]

I agree with adding the witness-signature check inside Manager.pushBlock: checking against committed state after the pending session is reset fixes the pending-state problem. My concern is with removing it from the sync network layer. After this change the in-Manager check is the only witness-signature check on the sync path, and it runs only after work the old check used to avoid.

1. Work moves into the critical section. A sync block is only buffered on receipt and validated later on the sync handler thread. At base, a block with a bad witness signature was rejected in validBlock before processBlock. With this PR it first enters pushBlock: it raises blockWaitLock (Manager.java:1241, which makes pushTransaction callers wait), takes the Manager monitor that block production also uses (:1243), runs getVerifyTxs (:1259) and resets the pending session (:1265), so all pending transactions are re-queued and executed again. Only then is it rejected at :1270. The sender still gets a BAD_BLOCK disconnect and a 1-hour ban, but that limit is per IP, so it can be repeated from many addresses.

2. The check in front of consensus.receiveBlock becomes the only guard. consensus.receiveBlock (:1274) acts on the block's witness address (duplicate-producer detection for this node's SR keys), so it should only see blocks whose signature has been verified. Today the new check runs right before it. But the fix for the fork-state issue in the comment above (validate against the block's parent state, defer when that state is unavailable) would likely skip or defer the check for some fork blocks at this point. Without a sync-layer check, those blocks would reach consensus.receiveBlock and khaosDb.push with no witness-signature check at all.

3. Runtime exceptions are no longer wrapped. validateSignature can throw unchecked exceptions on malformed input, e.g. a witness signature shorter than 65 bytes while strict ECDSA validation is off (sanitize() only trims overlong signatures). The old network wrapper turned any exception into BLOCK_SIGN_INVALID. Inside pushBlock they now escape unwrapped, skip failProcessBlock, and are logged with a full stack trace on every attempt.

4. Known block ids (minor). BlockId covers only the raw header, and both the freshBlockId shortcut (TronNetDelegate.java:272) and the "already exist" return (Manager.java:1252-1254) run before the new check. A sync copy of a known block with a garbage signature now returns success instead of BAD_BLOCK.

Restoring the old sync check unchanged would bring back the pending-state false rejections this PR fixes; on the sync path that means banning an honest peer. So rather than reverting, I'd suggest keeping a sync-layer pre-check that does not depend on pending state:

• before processBlock, a state-free check that the witness signature has a valid length and a signer can be recovered (same strictness as validateSignature), rejected with BAD_BLOCK on failure; this cannot reject a valid block;
• if the fork-state fix defers validation for some blocks, making sure a block reaches consensus.receiveBlock only after its signature has been verified, or moving receiveBlock after the deferred validation.

It might also be worth converting runtime exceptions from validateSignature into ValidateSignatureException inside pushBlock, and adding tests for: a garbage-signature sync block with a non-empty pending pool, a short witness signature with strict ECDSA off, and a known block id carrying a garbage signature.

[halibobo1205]

[MUST] Manager.java:1246-1249: with the signature check removed from sync validBlock, this observation now runs for sync blocks before any witness check, and its label is computed from the unverified witness_address header field.

Please move the BLOCK_TRANSACTION_COUNT observation after the witness check at :1270 (or after the block is applied), and have TronNetDelegate.validBlock reject a witness_address that is not 21 bytes. A signature-length/recoverability pre-check alone does not bound this field.

Test: with Prometheus enabled, a sync block that fails the witness check (unknown witness, or a witness_address of the wrong length) must not create a block_transaction_count child.

[xxo1shine]

Moved the BLOCK_TRANSACTION_COUNT observation after successful witness authorization and pushed the update. Sync blocks that fail validation now exit before the metric label is computed, so they do not create a corresponding metric series.

[halibobo1205]

[MUST] Witness authorization needs to be checked against the block’s actual parent state.

The new Manager check can reject a valid competing branch. A and B are children of P, whose witness permission authorizes K1. A contains a K1 → K2 rotation, while B is signed by K1. B can reach ingress while A is still being processed and has not yet executed its permission-update transaction. The ingress check therefore still reads K1 and correctly accepts B’s signature. A then finishes and commits before B acquires the Manager lock. B is now a fork block, and Manager.java:1270 rejects it using the canonical branch’s K2 permission, although K1 remains valid under B’s parent P. Rejection happens before Khaos admission, so switchFork() cannot perform its parent-state validation. A regression test for this sequence passes on the base and fails with this change.

There is also a related pre-existing ingress limitation: TronNetDelegate.java:397 still checks advertised blocks before their parent has necessarily been applied. A K2-signed child of a parent rotating K1 → K2 can therefore be rejected using the old K1 permission before reaching Manager.

Please validate authorization against the actual parent state, defer/retry blocks when that state is unavailable, and add regression coverage for both sequences.

On the regression coverage: a third sequence is a witness-permission update that is still pending locally. validAdvBlock reads the pending-merged view, so a block signed with the key that is valid at its parent can be rejected before broadcast. Validating against the parent state covers this too; please include it in the tests.

[waynercheung]

Request changes. I agree with the open items from Federico2014, SeriousCoding789 and halibobo1205; only additions below.

1. [MUST] Metrics (halibobo1205, :1247): the observation should move after the parent-state authorization, including deferred fork authorization, not just after the current :1270 check.
2. [SHOULD] Pending-update sequence (halibobo1205): the ingress rejection is BLOCK_SIGN_INVALID -> BAD_BLOCK disconnect of an honest sender, and acceptsCommittedPermissionAfterPendingRevocationReset applies the pending change after ingress validation, so it misses this case. Only blocks that pass state-independent structural and signature checks should be deferred, without broadcast or peer penalty.
3. [SHOULD] Besides the short-signature case (SeriousCoding789), a recoverable signature for a non-existent witness account throws an NPE when multi-sign is enabled; please handle it explicitly and add a test.

[waynercheung]

[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]

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.

[xxo1shine]

@Federico2014, the limitation where the state used for ingress validation differs from a block's parent state during a permission change also exists in the previous version. For example, if B arrives only after A has completed the K1 → K2 update, the previous version also uses K2 at ingress and rejects B signed by K1. A child signed by K2 that arrives before its parent has been applied faces a similar issue.

In production, SRs have a defined procedure for rotating their block-signing key: stop block production, update the on-chain witness permission, verify the permission and configure the corresponding key, then resume block production. This procedure is intended to avoid continuing to produce blocks with a mismatched key during the transition. See the official witness permission guide.

This PR focuses on preventing pending state from affecting signature validation and checking committed permissions under the Manager lock. Given the operational procedure above and the scope of this fix, I would prefer to retain the current implementation rather than extend this PR to include parent-state lookup, deferred processing, and retries. Comprehensive handling of forks during permission transitions can be discussed as a separate issue.

[xxo1shine]

@SeriousCoding789 Regarding the review above, I agree that sync-layer pre-checks and permission validation against committed state inside Manager should be considered separately. My thoughts on the four points are:

1. Work performed before signature validation. An invalid sync block triggers a BAD_BLOCK disconnect and a one-hour IP ban. The sync handler also removes queued blocks belonging to disconnected peers, so the same connection cannot continuously trigger pending-session resets with invalid blocks. The additional work does exist, but whether repeated attempts from multiple IPs materially affect transaction processing or block production still needs measurement. A signature-format and recovery pre-check only filters malformed signatures; it cannot reject a well-formed signature from an unauthorized signer, so it would not eliminate this overhead. For now, I would prefer to retain the current implementation and quantify the impact first.

2. Validation before consensus.receiveBlock. The current implementation does not adopt deferred validation for fork blocks. Received blocks must still pass witness authorization inside Manager before reaching consensus.receiveBlock and khaosDb.push, so the unverified path described here does not exist in the current implementation.

3. Runtime exception handling. Runtime exceptions are still caught by the sync layer: the block is rejected, and the peer is disconnected and banned for one hour. Keeping the full stack trace helps diagnose the underlying exception and does not itself compromise the correctness of error handling. The remaining difference is inconsistent failure accounting. I would treat that as a follow-up observability improvement and leave the exception path unchanged in this PR.

4. Early return for a known BlockId. This is duplicate-block handling. The received copy does not replace the stored block, get executed, or get inserted into the fork cache. Changing the signature while retaining the same BlockId only causes the copy to be ignored; it does not bypass signature validation for an unknown block. I would therefore retain the early return rather than add signature-verification work solely to identify an invalid signature on an already ignored duplicate.

Overall, I would retain the current permission validation inside Manager and duplicate-block handling. Failure-accounting consistency can be addressed separately, and an additional sync-layer signature pre-check can be evaluated based on measured overhead and impact.

[xxo1shine]

@halibobo1205, this pending permission-update scenario does exist at the advertisement ingress, but the previous version has the same limitation: validAdvBlock ultimately validates witness authorization against an account-state view that includes pending changes, so it can reject a block signed with the key authorized by its parent state.

This PR revalidates authorization inside Manager after resetting the pending session, preventing blocks that reach Manager from being authorized solely by pending permissions. The sync path also no longer depends on the pending permission state at ingress. The advertisement path retains authorization validation before forwarding, so the current change does not cover the ingress false-rejection scenario you described.

I would prefer to keep this PR focused and address parent-state validation at advertisement ingress, together with the corresponding regression coverage, as a separate improvement. The existing Manager-level tests do not substitute for a test of this ingress path.

[xxo1shine]

@waynercheung Regarding the review above, the metric observation has been moved after the current witness authorization check in Manager, and the update has been pushed. This prevents sync blocks that fail that check from creating metric labels. The case you describe—a fork block that passes authorization against the current state but is unauthorized under its actual parent state—remains part of the parent-state validation issue and is not fully covered by this adjustment.

The advertisement-ingress false rejection and penalty caused by a pending permission update also exist in the previous version. The existing test verifies pending-session reset after a block reaches Manager; it does not cover rejection before Manager at ingress. I would prefer to address parent-state validation, deferred processing, and the related fork and ingress tests together as a separate improvement.

A non-existent witness account can indeed cause a null-pointer exception when multi-sign is enabled. The sync layer still catches the exception, rejects the block, and disconnects and bans the peer; the block is not accepted as a result. Explicit null handling would improve error classification and diagnostics and can be addressed as a follow-up exception-handling improvement. I would retain the current handling in this PR.

[bladehan1]

[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]

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.

[bladehan1]

[NIT] Documentation backlog — 2 small follow-ups on side-effect ordering semantics

This is a cross-cutting documentation note, not a bug in this line. Grouped as one comment since both items are doc/notes asks about the new ordering of side effects in pushBlock.

1. BLOCK_TRANSACTION_COUNT observation semantics changed (Manager.java:1272-1276). Blocks that fail witness authorization no longer produce a data point for this histogram; the child label is now derived only from witness-authorized blocks. Impact: monitoring口径 shifts silently and can affect alert thresholds; the dedicated metric-child regression test is still pending per the PR description. Suggestion: note the monitoring change in the PR description or release notes, and keep the metric-child test on the follow-up list.

2. Shutdown-target update timing changed (Manager.java:1278-1282). latestSolidityNumShutDown is now updated only after witness authorization passes, even if the block later fails to apply (e.g. UnLinkedBlockException); previously it updated on receipt. Impact: shutdown-scheduling semantics differ from historical versions — arguably more correct, but operators should know. Suggestion: add a one-line note to the PR description or release notes.

Suggestion: add brief notes covering both ordering changes to the PR description (or release notes) so operators can adjust monitoring and shutdown-scheduling expectations; the missing metric-child regression test is already tracked as a follow-up.

[xxo1shine]

The latest version records the transaction-count metric and updates the shutdown target only after the block is successfully applied or the fork switch succeeds. Rejected blocks, duplicates, and parked forks do not trigger either update. The PR description now documents these semantics.

The dedicated regression test verifying that witness-authorization failures do not create a metric child remains explicitly listed as a follow-up.

Approved by mistake. Review not completed yet

[halibobo1205]

[SHOULD] PendingManager.java:32: this covers the timeout removal, but rePushTransactions has another removal point. generateBlock polls entries (Manager.java:1639, :1647) and can drop them without packing them, so a reference rePushLoop already peeked keeps its cached flag. "Must be revalidated even after its owner marker is pruned" therefore holds only for timeouts. The same path exists on the base, so this is not a regression, but the description claims the broader property.

Suggest covering every removal point in one place, inside the existing monitor in rePush (Manager.java:2131-2135):

if (ownerAddressSet.contains(ownerAddress) || rePushTransactions.peek() != tx) {
  tx.setVerified(false);
}

This keeps the cache for the normal case. I tried it on this head: ManagerTest passes, the two new tests still pass without the PendingManager line, and ManagerMockTest.testRePush1 needs its queue set because it builds Manager without init().

Test: an entry already peeked by rePushLoop, then dropped by generateBlock while its owner marker is pruned, must not enter pending on re-push.

If you prefer to keep this PR narrow, please scope that sentence to the timeout path and track this as a follow-up.

[xxo1shine]

The latest implementation performs peek() and registration in pushTransactionQueue under the same Manager lock, and removes the registration in the repush loop's finally block. This keeps the held reference visible to owner-marker retention even if timeout cleanup or block-generation discard removes its repush entry. The existing check in rePush() then invalidates the verification cache when the owner is marked.

The timeout-specific invalidation has been removed. Regression coverage now includes actual block-generation discard, timeout removal, and cache reuse when permissions are unchanged. Signature verification remains in its original location; removal from the repush queue alone does not force re-verification.

[halibobo1205]

[SHOULD] Manager.java:907: this check now runs for every pushTransaction caller, not only the repush path. A fresh transaction from an owner in ownerAddressSet has just been verified outside the locks at :878, so clearing the flag here makes :1515 verify it again while holding transactionLock and the Manager monitor. On the base that path verifies once, outside both locks. I counted this with a single-signature transaction: one verification on the base with none under the monitor, two on b1ebc3bcd8 with one under the monitor. The description doesn't mention this cost.

A capsule with a cached result needs this check. A fresh capsule needs it only if the permission view changed after its outer check, so I'd suggest:

final boolean cachedOnEntry = trx.isVerified();
final long generationOnEntry = permissionViewGeneration;
// outer validateSignature and locks unchanged
if ((cachedOnEntry || generationOnEntry != permissionViewGeneration)
    && ownerAddressSet.contains(ByteArray.toHexString(trx.getOwnerAddress()))) {
  trx.setVerified(false);
}

permissionViewGeneration is a volatile counter, incremented after a permission update merges in pushTransaction and at the start of PendingManager.close(). I tried this on this head: the fresh case goes back to one verification outside the locks, a fresh transaction racing a committed permission change is still rejected, and ManagerTest.rePush*, ManagerMockTest.rePushLoop* and ManagerStrictEcdsaActivationTest pass.

A smaller option is to use only cachedOnEntry. Fresh transactions then behave as on the base: one racing a permission change can still enter pending, and the marker check in generateBlock keeps it out of the block.

Test: a fresh transaction from a marked owner verifies once, outside the Manager monitor.

If you decide the cost is acceptable, please say so in the description.

[xxo1shine]

Removed the newly added cache-invalidation check inside the admission locks in pushTransaction() and restored the original check in rePush(). Fresh transactions no longer incur an additional cryptographic verification under the locks solely because their owner is marked.

This fix is now scoped to registering references held by the repush thread early, so they remain tracked after removal from the repush queue. It does not introduce a permission-version mechanism. The existing concurrency window between verification outside the locks and pending admission retains the baseline behavior, and the PR description explicitly documents this boundary.

Added a regression test checking that a fresh transaction from a marked owner performs exactly one cryptographic verification, outside both admission locks. These changes are in fe1d4fc.