Repository navigation
fix(block): validate witnesses against candidate-parent state - #60
Closed
Federico2014 wants to merge 2 commits into
Closed
Federico2014 wants to merge 2 commits into
Federico2014 wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/main/java/org/tron/core/net/TronNetDelegate.java">
<violation number="1" location="framework/src/main/java/org/tron/core/net/TronNetDelegate.java:354">
P0: This unconditional return removes active-witness schedule validation, while `Manager.validateBlockSignature` only checks account permissions and the cryptographic signature. A validly signed block from a non-scheduled witness can therefore be applied and propagated; validate the active witness against the selected candidate-parent schedule in `Manager` before application.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| return witnessScheduleStore.getActiveWitnesses().contains(block.getWitnessAddress()); | ||
| // Witness permissions, recovery rules and schedules belong to the candidate-parent state. | ||
| // Manager checks them after selecting that state, before executing the block. | ||
| return true; |
There was a problem hiding this comment.
P0: This unconditional return removes active-witness schedule validation, while Manager.validateBlockSignature only checks account permissions and the cryptographic signature. A validly signed block from a non-scheduled witness can therefore be applied and propagated; validate the active witness against the selected candidate-parent schedule in Manager before application.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At framework/src/main/java/org/tron/core/net/TronNetDelegate.java, line 354:
<comment>This unconditional return removes active-witness schedule validation, while `Manager.validateBlockSignature` only checks account permissions and the cryptographic signature. A validly signed block from a non-scheduled witness can therefore be applied and propagated; validate the active witness against the selected candidate-parent schedule in `Manager` before application.</comment>
<file context>
@@ -367,8 +349,9 @@ public boolean validBlock(BlockCapsule block) throws P2pException {
- return witnessScheduleStore.getActiveWitnesses().contains(block.getWitnessAddress());
+ // Witness permissions, recovery rules and schedules belong to the candidate-parent state.
+ // Manager checks them after selecting that state, before executing the block.
+ return true;
}
</file context>
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java">
<violation number="1" location="framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java:164">
P2: Keep `validWitnessProductTwoBlock` outside this APPLIED-only guard. `ALREADY_KNOWN` still carries the stored block, and skipping this only call site prevents witness double-production tracking for blocks first persisted by sync and later seen as redundant advertisements; restrict the guard to `broadcast`.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/db/Manager.java">
<violation number="1" location="framework/src/main/java/org/tron/core/db/Manager.java:1261">
P2: Once four signatures are cached, this drops every later variant, so invalid arrivals can exhaust the pool before the valid witness signature arrives. The valid branch is then rejected or forced through another sync attempt instead of being applied; retain a recoverable candidate or otherwise prevent untrusted variants from exhausting the pool.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Comment on lines
+164
to
+171
| if (result.getStatus() == BlockPushResult.Status.APPLIED) { | ||
| for (BlockCapsule applied : result.getValidatedBlocks()) { | ||
| BlockMessage message = new BlockMessage(applied); | ||
| message.sanitize(); // Normalize stored branch blocks before forwarding. | ||
| broadcast(message); | ||
| witnessProductBlockService.validWitnessProductTwoBlock(applied); | ||
| } | ||
| } |
There was a problem hiding this comment.
P2: Keep validWitnessProductTwoBlock outside this APPLIED-only guard. ALREADY_KNOWN still carries the stored block, and skipping this only call site prevents witness double-production tracking for blocks first persisted by sync and later seen as redundant advertisements; restrict the guard to broadcast.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At framework/src/main/java/org/tron/core/net/messagehandler/BlockMsgHandler.java, line 164:
<comment>Keep `validWitnessProductTwoBlock` outside this APPLIED-only guard. `ALREADY_KNOWN` still carries the stored block, and skipping this only call site prevents witness double-production tracking for blocks first persisted by sync and later seen as redundant advertisements; restrict the guard to `broadcast`.</comment>
<file context>
@@ -161,11 +161,13 @@ private void processBlock(PeerConnection peer, BlockCapsule block) throws P2pExc
- message.sanitize(); // Also normalize previously stored historical blocks before forwarding.
- broadcast(message);
- witnessProductBlockService.validWitnessProductTwoBlock(applied);
+ if (result.getStatus() == BlockPushResult.Status.APPLIED) {
+ for (BlockCapsule applied : result.getValidatedBlocks()) {
+ BlockMessage message = new BlockMessage(applied);
</file context>
Suggested change
| if (result.getStatus() == BlockPushResult.Status.APPLIED) { | |
| for (BlockCapsule applied : result.getValidatedBlocks()) { | |
| BlockMessage message = new BlockMessage(applied); | |
| message.sanitize(); // Normalize stored branch blocks before forwarding. | |
| broadcast(message); | |
| witnessProductBlockService.validWitnessProductTwoBlock(applied); | |
| } | |
| } | |
| for (BlockCapsule applied : result.getValidatedBlocks()) { | |
| if (result.getStatus() == BlockPushResult.Status.APPLIED) { | |
| BlockMessage message = new BlockMessage(applied); | |
| message.sanitize(); // Normalize stored branch blocks before forwarding. | |
| broadcast(message); | |
| } | |
| witnessProductBlockService.validWitnessProductTwoBlock(applied); | |
| } |
| if (signatures.contains(signature)) { | ||
| return true; | ||
| } | ||
| if (signatures.size() >= MAX_DEFERRED_SIGNATURES) { |
There was a problem hiding this comment.
P2: Once four signatures are cached, this drops every later variant, so invalid arrivals can exhaust the pool before the valid witness signature arrives. The valid branch is then rejected or forced through another sync attempt instead of being applied; retain a recoverable candidate or otherwise prevent untrusted variants from exhausting the pool.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At framework/src/main/java/org/tron/core/db/Manager.java, line 1261:
<comment>Once four signatures are cached, this drops every later variant, so invalid arrivals can exhaust the pool before the valid witness signature arrives. The valid branch is then rejected or forced through another sync attempt instead of being applied; retain a recoverable candidate or otherwise prevent untrusted variants from exhausting the pool.</comment>
<file context>
@@ -1216,6 +1248,22 @@ private void clearDeferredBlocks() {
+ if (signatures.contains(signature)) {
+ return true;
+ }
+ if (signatures.size() >= MAX_DEFERRED_SIGNATURES) {
+ return false;
+ }
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Validate external block witness signatures in Manager after establishing the candidate-parent state, for both direct application and fork switching. Defer competing branches until validation is possible, and forward blocks only after successful validation.
Propagate application results to broadcast and synchronization handlers, support retrying signature variants, and avoid blaming a child provider for an invalid ancestor. The database and network changes are coupled through the new block-processing result and signature-failure attribution.
At most four normalized witness signatures are retained per deferred block; overflow requests resynchronization without disconnecting the provider. The Khaos graph node is preserved, and its block is replaced only after candidate-parent signature validation. Already-applied blocks no longer repeat application metrics or broadcasts.
Why are these changes required?
Ingress verification currently reads local state that may differ from the candidate-parent state. This can accept stale signatures or reject valid competing branches after witness-key rotation or strict-ECDSA activation.
Related: #59
This PR has been tested by:
./gradlew clean build -x testpassed on x86_64 / JDK 8../gradlew :framework:checkstyleMain :framework:checkstyleTest :plugins:checkstyleMainpassed.bash .github/scripts/check_math_usage.shandgit diff --checkpassed.BlockWitnessParentStateTest,ManagerSignaturePreValidationTest,ManagerStrictEcdsaActivationTest,TronNetDelegateTest,BlockAlreadyKnownCounterTest,BlockMsgHandlerTest,SyncServiceTestandKhaosDatabaseTest, run with--max-workers=2. Coverage includes both signature-arrival orders on broadcast and synchronization, bounded signature alternatives and cleanup, duplicate handling, and rejection of signed blocks from an unscheduled witness../gradlew testwas reattempted and stopped after five timeout failures: twoDbBackfillBloomTestcases, two RocksDB DbLite cases andStatusLifecycleTest.statusFirstRemainsAValidProbe. These plugin/P2P files are unchanged by this PR. Full-suite success is not established; failures have not been checked against the base branch.Follow up
Complete multi-node convergence testing and measure validation and propagation costs.
Extra details
Consensus and compatibility risk: this changes block-acceptance decisions around witness-permission updates and strict-ECDSA activation. Mixed-version nodes may disagree on blocks previously accepted using stale state or rejected before candidate-parent validation. Historical replay compatibility must also be verified.
Broadcasting now waits for successful block application, which may increase propagation latency. Deferred-branch handling and synchronization retries require load and convergence testing.
No dependency, configuration or database schema changes. Coordinated rollout or activation gating must be assessed before deployment; safe rolling-upgrade compatibility has not yet been established.
Summary by cubic
Validates witness signatures against the candidate-parent state instead of the current local state, fixing acceptance of stale signatures and rejection of valid competing branches after witness-key rotation or strict-ECDSA activation.
Managerafter the candidate-parent state is established, for both direct application and fork switching.BlockPushResultandBlockSignatureExceptionto distinguish deferred, applied, and invalid blocks, and avoids blaming a peer for an invalid ancestor block.Written for commit 281d716. Summary will update on new commits.