Treat reorged scripts-valid bodies as BIP22 duplicate - #596
Conversation
Core proposal LookupBlockIndex returns duplicate when IsValid(BLOCK_VALID_SCRIPTS), including after a later reorg. Use chain_tx_count != 0 (set on apply, kept on disconnect) instead of applied-chain membership. submitblock matches ProcessNewBlock !new_block. Co-authored-by: metaphorics <metaphorics@users.noreply.github.com>
|
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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 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 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_43724ad7-a6a1-45fa-9cdb-7e870e6a9cf7) |
PR Summary by QodoClassify reorged scripts-valid blocks as BIP22 duplicates
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Restart loses duplicate state
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /// `Invalid` is `BLOCK_FAILED_VALID`. A non-zero `chain_tx_count` is set | ||
| /// only after a successful apply (`record_applied_tx_count`) and survives | ||
| /// disconnect, matching Core `IsValid(BLOCK_VALID_SCRIPTS)` including | ||
| /// reorged bodies. Header-only entries stay 0 and are inconclusive — |
There was a problem hiding this comment.
1. Comment duplicates api-21 rule 📘 Rule violation ⚙ Maintainability
The expanded known_block_result comment re-specifies the API-21 scripts-valid and reorg behavior already defined in the authoritative external API contract without referencing it. This creates parallel documentation that can drift from the contract.
Agent Prompt
## Issue description
The implementation comment duplicates the scripts-valid and reorg semantics already documented by the authoritative `API-21` contract.
## Issue Context
Keep only implementation-specific information locally and reference `docs/contracts/external-api.md` section `API-21` for the complete business rule. Apply the same treatment to the nearby `submit` comment if it repeats contract semantics.
## Fix Focus Areas
- crates/node/src/mining.rs[724-730]
- crates/node/src/mining.rs[781-785]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #[test] | ||
| fn proposal_of_a_disconnected_scripts_valid_block_is_duplicate() -> anyhow::Result<()> { |
There was a problem hiding this comment.
2. Tests omit api-21 annotation 📘 Rule violation ▣ Testability
The two new permanent compatibility tests do not identify API-21, BIP22, or another named contract in their names or adjacent annotations. Although the contract catalog maps them externally, the tests themselves do not exercise an explicitly documented contract as required.
Agent Prompt
## Issue description
The new permanent tests lack a test-local reference to their named current contract.
## Issue Context
`docs/contracts/external-api.md` maps both tests to active contract `API-21`. Add an adjacent contract/spec comment such as `// CONTRACT: API-21 (BIP22 duplicate behavior)` to each test.
## Fix Focus Areas
- crates/node/tests/mining.rs[1270-1271]
- crates/node/tests/mining.rs[1307-1308]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if node.chain_tx_count != 0 { | ||
| return Some(BlockValidationResult::Duplicate); |
There was a problem hiding this comment.
3. Restart loses duplicate state 🐞 Bug ≡ Correctness
Checkpoint restore assigns chain_tx_count only to the applied tip, leaving already-applied ancestors at zero, so known_block_result returns DuplicateInconclusive for them after restart. submitblock then bypasses the duplicate fast path and attempts to process an already-applied ancestor instead of returning duplicate.
Agent Prompt
## Issue description
`known_block_result` now assumes every scripts-valid block has a nonzero per-node `chain_tx_count`, but checkpoint restore only restores that value on the applied tip. Applied ancestors consequently lose their duplicate classification after restart.
## Issue Context
Header reconstruction initializes every node's count to zero. Restore then updates only `headers.applied_tip_id`, while journal replay only assigns counts to the replayed suffix. Preserve durable per-block scripts-valid state, or retain an applied-chain fallback while introducing durable state for disconnected scripts-valid blocks.
## Fix Focus Areas
- crates/node/src/mining.rs[724-742]
- crates/node/src/checkpoint.rs[1282-1308]
- crates/chain/src/tree.rs[649-656]
- crates/node/src/chainstate_journal/replay.rs[475-508]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if node.chain_tx_count != 0 { | ||
| return Some(BlockValidationResult::Duplicate); |
There was a problem hiding this comment.
4. Pruned bodies report duplicate 🐞 Bug ≡ Correctness
A nonzero chain_tx_count records historical validation but does not prove that the body remains stored, because pruning deletes bodies without clearing the count. A stale previously-applied block whose body was pruned is therefore reported as duplicate and its submitted body is discarded instead of being treated as new storage.
Agent Prompt
## Issue description
The shared duplicate predicate conflates historical scripts-valid state with current body availability. Pruned blocks retain `chain_tx_count` but no longer have a stored body, so `submitblock` must not take the body-present duplicate fast path solely from that count.
## Issue Context
Proposal mode may continue using durable scripts-valid state, but submit mode should independently determine whether the body is currently stored. If it was pruned, process and restore the submitted body using behavior equivalent to Core's `new_block` storage result.
## Fix Focus Areas
- crates/node/src/mining.rs[713-742]
- crates/node/src/mining.rs[779-793]
- crates/node/src/state.rs[1242-1290]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (3) 🔗 Fix PR: #599 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 3 fixed
|
Checkpoint restore writes chain_tx_count only on the applied tip, so ancestors would lose BIP22 duplicate classification. Treat applied-chain membership as the restore fallback; nonzero count still covers in-process reorged bodies. Co-authored-by: metaphorics <metaphorics@users.noreply.github.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_173b45db-db83-41c5-b185-4395e9af6c67) |
2 participants
Summary
Core GBT proposal
LookupBlockIndexreturnsduplicatewhenpindex->IsValid(BLOCK_VALID_SCRIPTS), including for a body that was fully connected and later reorged off the tip. This node previously treated only the current applied chain as scripts-valid, so a disconnected (or reorged) body becameduplicate-inconclusive.API-21treats a block as scripts-valid whenchain_tx_count != 0or it is on the applied chain. The count is written byrecord_applied_tx_countafter a successful apply and is not cleared on disconnect. Checkpoint restore writes the count only on the applied tip, so applied-chain membership is the restore fallback for ancestors. Header-only nodes stay 0 and off the applied chain, sosubmitheaderthensubmitblockstill applies the body.NodeStatus::Active/Staleremain header-chain displacement and are not used.submitblockuses the same scripts-valid test. A stale scripts-valid resubmit is miner-facingduplicate, notinconclusive-not-best-prevblk. CoreBLOCK_HAVE_DATAafter prune is not modeled separately.Stack
Draft on
cursor/mining-prioritise-dust-1522(API-20). Merge after #593.Tests (local)
Includes:
proposal_of_a_disconnected_scripts_valid_block_is_duplicatesubmit_of_a_disconnected_scripts_valid_block_is_duplicateapplied_ancestor_with_unset_chain_tx_count_is_duplicate