Consensus: skip Spark state updates during VerifyDB reconnect - #1903
Consensus: skip Spark state updates during VerifyDB reconnect#1903reubenyap wants to merge 2 commits into
Conversation
CVerifyDB check level 3 disconnects recent blocks against a throwaway coins view and level 4 reconnects them through ConnectBlock, but global Spark state and the Spark entries of the block index are never rolled back in the process. Re-running ConnectBlockSpark on such a reconnect fails the used-lTag check for any block containing a Spark spend, and for mint blocks double-counts coin group sizes and mint metadata or trips the coin group ordering assertion, so -checklevel=4 either refuses to start, aborts, or corrupts in-memory Spark state that can later be flushed to the block index on disk. Thread an isVerifyDB flag through ConnectBlock and skip the Spark connect logic when reconnecting under VerifyDB. The stateless Spark checks still run through CheckBlock and per-transaction validation, whose lTag check already tolerates recorded lTags outside tip connection. Add a regression test that runs -checklevel=4 style verification over Spark mint and spend blocks and asserts the global Spark state comes out untouched. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01LphSq9aU2w13j6DWs7m6Ry
getHash() hashes the coin's full serialization, which covers the type byte and, for mint-type coins, the value v. The old comment claimed type and v were excluded. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01LphSq9aU2w13j6DWs7m6Ry
|
User [email protected] does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughThe change prevents VerifyDB block reconnects from reapplying Spark state. It adds an integration test for state preservation and updates the Coin hash documentation. ChangesSpark state preservation
Coin hash documentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change prevents Spark state from being replayed during VerifyDB reconnection and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2557181731
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, | ||
| CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck) | ||
| CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck, bool isVerifyDB) |
There was a problem hiding this comment.
When the verified range contains a Spark-name registration followed by a transfer, the live name manager still holds the transferred address, but this new flag only suppresses ConnectBlockSpark: ConnectBlock continues calling CheckTransaction(..., false, ...), which reaches CheckSparkNameTx and rejects the historical registration as “name already exists” because its original address differs from the current one. Thus -checklevel=4 can still report an unconnectable block and prevent startup; the VerifyDB mode must also bypass checks that depend on the unrewound Spark-name state while retaining the context-independent Spark checks.
Useful? React with 👍 / 👎.
| // Spark entries of the block index are never rolled back. Re-running the | ||
| // Spark connect logic on such a reconnect double-counts mints and fails the | ||
| // used-lTag check against state that still contains this block, so skip it. | ||
| if (!isVerifyDB && !spark::ConnectBlockSpark(state, chainparams, pindex, &block, fJustCheck)) |
There was a problem hiding this comment.
When the block index is missing or has corrupted Spark metadata, this blanket skip makes level-4 verification report success without comparing the Spark data reconstructed into block.sparkTxInfo with pindex->sparkMintedCoins, spentLTags, or sparkSetHash. For example, a missing spentLTags entry remains absent from the global state built at startup, so the node can subsequently treat an already-spent linking tag as unused even after the operator runs -checklevel=4; VerifyDB should validate the reconstructed entries without applying them to the live state rather than omitting this phase entirely.
Useful? React with 👍 / 👎.
2 participants
PR intention
Fix a pre-existing
-checklevel=4startup crash/corruption found while reviewing #1902.CVerifyDBlevel 3 disconnects recent blocks against a throwaway coins view and level 4 reconnects them throughConnectBlock, but global Spark state and the Spark entries of the block index are never rolled back in the process (DisconnectBlockwithpfCleanperforms "no real disconnect", andDisconnectTipSparkis never invoked). Level-4 reconnection therefore re-runsConnectBlockSparkagainst state that still contains those blocks:assert(coinGroup.lastBlock->nHeight <= index->nHeight)inAddMintsToStateAndBlockIndexwhen a later block already holds mints of the same group;CheckLTagused-lTag check, so VerifyDB reports an unconnectable block and the node refuses to start;The new regression test reproduces the abort on current master (assertion failure in
spark/state.cppAddMintsToStateAndBlockIndexduring level-4 reconnection) and passes with the fix.Code changes brief
isVerifyDBflag throughConnectBlock(defaultfalse) and pass it fromCVerifyDB::VerifyDB's level-4 reconnection loop.ConnectBlockSparkwhen the flag is set. Stateful Spark checks are meaningless against un-rewound state, while the stateless per-transaction Spark checks still run throughCheckBlock/CheckTransactioninsideConnectBlock, whose lTag check already tolerates recorded lTags outside tip connection. Real block connection (ConnectTip,TestBlockValidity) is unchanged.spark_state_tests/verifydb_preserves_spark_state: builds two Spark mint blocks and a spend block, runs-checklevel=4-style verification viaCVerifyDB, and asserts the mint map, spend map, and coin-group bookkeeping come out untouched.spark::Coin::getHash()claiming type and v are excluded from the hash; they are part of the serialization it hashes.Verified locally on Linux:
test_firo --run_test=spark_state_tests,spark_tests,spark_mintspendpasses (25 cases, no errors detected). With the validation change reverted to master, the new test aborts exactly as a-checklevel=4node would.Independent of #1902 and textually non-conflicting: the hunks touch different regions of
validation.cppand this PR does not modifyspark/state.cpplogic. If both land, #1902's active-state gate inConnectBlockSparksimply never sees a VerifyDB reconnection anymore, which is compatible with its intent.🤖 Generated with Claude Code
https://claude.ai/code/session_01LphSq9aU2w13j6DWs7m6Ry
Generated by Claude Code