backport: assumeutxo M6 — hardening (bitcoin#29370 + A4 batch)#55
Open
PastaPastaPasta wants to merge 16 commits into
Open
backport: assumeutxo M6 — hardening (bitcoin#29370 + A4 batch)#55PastaPastaPasta wants to merge 16 commits into
PastaPastaPasta wants to merge 16 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
PastaPastaPasta
force-pushed
the
assumeutxo/m5-loadtxoutset
branch
from
a0dd520 to
5737704
Compare
PastaPastaPasta
force-pushed
the
assumeutxo/m6-hardening
branch
from
1d7c4f0 to
c1a0aa7
Compare
PastaPastaPasta
force-pushed
the
assumeutxo/m5-loadtxoutset
branch
from
5737704 to
da6888b
Compare
…lues 9d9a745 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky) ef174e9 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky) 0391458 test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky) ef29c8b assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky) 9b97d5b doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky) 0fd915e validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky) 63e8fc9 ci: add getchaintxstats ubsan suppressions (Ryan Ofsky) f252e68 assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky) Pull request description: The `PopulateAndValidateSnapshot` function introduced in f6e2da5 from bitcoin#19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (bitcoin#29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake. Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag. Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail. Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case. ACKs for top commit: Sjors: re-ACK 9d9a745 achow101: ACK 9d9a745 mzumsande: Tested ACK 9d9a745 maflcko: ACK 9d9a745 🎯 Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
Keep the dynamically authorized regtest nChainTx count scoped to the snapshot base across activation and restart. Extend the Dash lifecycle test to verify header-only nTx/nChainTx values stay unset until background validation receives the blocks.
…state after loading 9315754 test: assumeutxo: spend coin from snapshot chainstate after loading (Sebastian Falbesoner) Pull request description: This PR extends the AssumeUTXO functional test by submitting a spending transaction for an UTXO that is only available in a the snapshot chainstate (after loading via `loadtxoutset`), i.e. it hasn't been seen in a block before. With that we can verify that snapshot coins are visible to the mempool. Note that we unfortunately can't use MiniWallet here, as the only available UTXO to spend from the snapshot chainstate is at height 200, where a P2PKH created from the test framework's deterministic private key is used (see `TestNode.generate(...)` and the `PRIV_KEYS` array). Coinbase outputs with smaller heights (<= 199) would be part of the pre-generated chain and hence not qualify for the "UTXO is only in snapshot chainstate and has never been seen in a block" scenario, coinbase outputs with larger heights (>= 201) can't be spent due to immaturity, as the snapshot chainstate block height is 299. One could of course mine a different chain with outputs that MiniWallet supports (e.g. taproot anyone-can-spend), but this would change the hardcoded AssumeUTXO hash, colliding with other PRs like bitcoin#28838, so I wanted to avoid that. ACKs for top commit: maflcko: lgtm ACK 9315754 jamesob: ACK bitcoin@9315754 Tree-SHA512: 0665868e1e91fe74f408d0a239cc264bbbc11a6b55bcc0e86cc8b4b2ec1f44977884b817dbe9065a7c768332cab464636656858bc8b9c8e7d7810498e0a17d78
…ansactions fa5cd66 test: Assumeutxo with more than just coinbase transactions (MarcoFalke) Pull request description: Currently the AU tests only check that loading a txout set with only coinbase outputs works. Fix that by adding other transactions. ACKs for top commit: jamesob: ACK bitcoin@fa5cd66 glozow: concept ACK fa5cd66 Tree-SHA512: e090c41f73490ad72e36c478405bfd0716d46fbf5a131415095999da6503094a86689a179a84addae3562b760df64cdb67488f81692178c8ca8bf771b1e931ff
…n mempool not empty 8d20602 test, assumeutxo: Add test to ensure failure when mempool not empty (Hernan Marino) Pull request description: Add a test to ensure that loadtxoutset fails when the node's mempool is not empty, as suggested by maflcko here: bitcoin#27596 (comment) ACKs for top commit: maflcko: re-ACK 8d20602 BrandonOdiwuor: ACK 8d20602 Tree-SHA512: 97c9668c0f38897934bf0d326515d898d4e682ff219deba9d751b35125b5cf33d51c9df116a74117ecf0394f28995a3d0cae1266b1e5acb4365ff4f309ce3f6c
…d not be loaded df6dc2a test: Assumeutxo: snapshots with less work should not be loaded (Hernan Marino) Pull request description: This PR adds a test which checks that snapshots with less accumulated work than the node's active chain, should not be loaded and return with an error. Although in a different context of discussion the missing test was detect in a thread in bitcoin#29394 (see bitcoin#29394 (comment)) ACKs for top commit: maflcko: utACK df6dc2a kevkevinpal: utACK [df6dc2a](bitcoin@df6dc2a) achow101: ACK df6dc2a alfonsoromanz: Re ACK df6dc2a. Make is successful and the test passes. Tree-SHA512: 07a394b4b288cc8ad3f66ed4e70dcda468db18113e9442eb7215cf491768432d55efaaa5b79d633094917e05475a30f0c5e4f64f8f2da293ba306891b4485560
…ore background chain 49d569c p2p: For assumeutxo, download snapshot chain before background chain (Martin Zumsande) 7a88551 p2p: Restrict downloading of blocks for snapshot chain (Martin Zumsande) Pull request description: After loading a snapshot, `pindexLastCommonBlock` is usually already set to some block for existing peers. That means we'd continue syncing the background chain from those peers instead of prioritising the snapshot chain, which defeats the purpose of doing assumeutxo in the first place. Only existing peers are affected by this bug. ACKs for top commit: fjahr: re-ACK 49d569c achow101: ACK 49d569c Sjors: tACK 49d569c Tree-SHA512: 0eaebe1c29a8510d5ced57e14c09b128ccb34b491692815291df68bf12e2a15b52b1e7bf8d9f34808904e7f7bc20f70b0ad0f7e14df93bbdf456bd12cc02a5d2
…e shell scripts 94b0adc rpc, refactor: Prevent potential race conditions in dumptxoutset (Fabian Jahr) e868a6e doc: Improve assumeutxo guide and add more docs/comments (Fabian Jahr) b29c21f assumeutxo: Remove devtools/utxo_snapshot.sh (Fabian Jahr) 20a1c77 contrib: Remove test_utxo_snapshots.sh (Fabian Jahr) 8426850 test: Test for dumptxoutset at specific height (Fabian Jahr) 993cafe RPC: Add type parameter to dumptxoutset (Fabian Jahr) fccf4f9 RPC: Extract ReconsiderBlock helper (Fabian Jahr) 446ce51 RPC: Extract InvalidateBlock helper (Fabian Jahr) Pull request description: This adds a height parameter to the `dumptxoutset` RPC. This internalizes the workflow that was previously done by scripts: roll back the chain to the height we actually want the snapshot from, create the snapshot, roll forward to the real tip again. The nice thing about internalizing this functionality is that we can write tests for the code and it gives us more options to make the functionality robust. The shell scripts we have so far will be more cumbersome to maintain in the long run, especially since we will only notice later when we have broken them. I think it's safe to remove these `test_utxo_snapshots.sh` as well when we have this option in `dumptxoutset` because we have also added some good additional functional test coverage for this functionality. ACKs for top commit: Sjors: re-utACK 94b0adc achow101: ACK 94b0adc mzumsande: ACK 94b0adc pablomartin4btc: re-ACK 94b0adc Tree-SHA512: a4c9af5f687d1ca7bfb579a36f363882823386b5fa80c05de531b05a2782b5da6ff5baf3ada4bca8f32f63975d86f1948175abed9affe51fc958472b5f838dab
…idated b7ba60f test: add coverage for -reindex and assumeutxo (Martin Zumsande) e57f951 init, validation: Fix -reindex option with an existing snapshot (Martin Zumsande) Pull request description: In c711ca1 logic was introduced that `-reindex` and `-reindex-chainstate` will delete the snapshot chainstate. This doesn't work currently, instead of deleting the snapshot chainstate the node crashes with an assert (this can be triggered by applying the added test commit on master). Fix this, and another bug that would prevent the new active chainstate from having a mempool after `-reindex` has deleted the snapshot (also covered by the test). ACKs for top commit: fjahr: re-ACK b7ba60f hernanmarino: crACK b7ba60f . Good fix BrandonOdiwuor: re-ACK b7ba60f byaye: Tested ACK b7ba60f Tree-SHA512: c168f36997d7677d590af37b10427870f5d30123abf1c76032a16661e486735373bfa7e049e6aca439526fbcb6d619f970bf9d042196c851bf058a75a32fafdc
…a snapshot twice b259b0e [Test] Assumeutxo: ensure failure when importing a snapshot twice (Alfonso Roman Zubeldia) Pull request description: I am getting familiar with the `assume_utxo` tests and I found that the scenario of trying to activate a snapshot twice is not covered. This test is to ensure failure when loading a snapshot if there is already a snapshot-based chainstate. ACKs for top commit: fjahr: Code review ACK b259b0e kevkevinpal: tACK [b259b0e](bitcoin@b259b0e) achow101: ACK b259b0e rkrux: tACK [b259b0e](bitcoin@b259b0e) Tree-SHA512: 3510861390d0e40cdad6861b728df04827a1b63e642f3d956aee66ed2770b1cb7e3aa3eb00c62eb9da0544703c943cc5296936c9ebfcac18c719741c354421bb
…ent chain + test 5b7f70b test: loadtxoutset in divergent chain with less work (Alfonso Roman Zubeldia) d35efe1 p2p: Start downloading historical blocks from common ancestor (Martin Zumsande) Pull request description: This PR adds a test to cover the scenario of loading an assumeutxo snapshot when the current chain tip is not an ancestor of the snapshot block but has less work. During the review process, a bug was discovered where blocks between the last common ancestor and the background tip were not being requested if the background tip was not an ancestor of the snapshot block. mzumsande suggested a fix (65343ec) to start downloading historical blocks from the last common ancestor to address this issue. This fix has been incorporated into the PR with a slight modification. Related to bitcoin#28648 ACKs for top commit: fjahr: tACK 5b7f70b achow101: ACK 5b7f70b mzumsande: Code Review ACK 5b7f70b Tree-SHA512: f8957349686a6a1292165ea9e0fd8c912d21466072632a10f8ef9d852a5f430bc6b2a531e6884a4dbf2e3adb28b3d512b25919e78f5804a67320ef54c3b1aaf6
…invalid chain 2f9bde6 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr) 19ce3d4 assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr) 80315c0 refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr) Pull request description: This was discovered in a discussion in bitcoin#29996 If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain. While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state. The behavior change described above is in the second commit. The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`. The third commit removes an unnecessary restart introduced in bitcoin#29428. ACKs for top commit: mzumsande: re-ACK 2f9bde6 alfonsoromanz: Re-ACK 2f9bde6. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user. achow101: ACK 2f9bde6 Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
… the best header chain 55b6d7b validation: Don't load a snapshot if it's not in the best header chain. (Martin Zumsande) Pull request description: This was suggested by me in the discussion of bitcoin#30288, which has more context. If the snapshot is not an ancestor of the most-work header (`m_best_header`), syncing from that alternative chain leading to `m_best_header` should be prioritised. Therefore it's not useful loading the snapshot in this situation. If the other chain turns out to be invalid or the chain with the snapshot retrieves additional headers so that it's the most-work one again (see functional test), `m_best_header` will change and loading the snapshot will be possible again. Because of the work required to generate a conflicting headers chain, a situation with two conflicting chains should only be possible under extreme circumstances, such as major forks. ACKs for top commit: fjahr: re-ACK 55b6d7b achow101: ACK 55b6d7b alfonsoromanz: Re ACK 55b6d7b Tree-SHA512: 4fbea5ab1038ae353fc949a186041cf9b397e7ce4ac59ff36f881c9437b4f22ada922490ead5b2661389eb1ca0f3d1e7e7e6a4261057678643e71594a691ac36
…and add new test d63ef73 test: Add loadtxoutset test with tip on snapshot block (Fabian Jahr) c2f86d4 test: Remove already resolved assumeutxo todo comments (Fabian Jahr) Pull request description: The first commit removes three Todos that have been addressed previously (see commit message for details). The second message resolves another todo by adding the missing test case. This is a special case of "the tip has more work than the snapshot" where the tip is the same block as the snapshot base block. Related to bitcoin#28648. ACKs for top commit: jrakibi: ACK [d63ef73](bitcoin@d63ef73) achow101: ACK d63ef73 maflcko: ACK d63ef73 alfonsoromanz: Re ACK d63ef73 Tree-SHA512: 8d5a25fc0b26531db3a9740132694138f2103b7b42eeb1d4a64095bfc901c1372e23601c0855c7def84c8a4e185d10611e4e830c4e479f1b663ae6ed53abb130
PastaPastaPasta
force-pushed
the
assumeutxo/m6-hardening
branch
from
c1a0aa7 to
deaa69b
Compare
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
5 participants
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.
Issue being fixed or feature implemented
M5 (#54) made
loadtxoutsetwork end to end; this milestone hardens itagainst the failure modes upstream found after their own equivalent RPC
shipped (stale index metadata, download-priority regressions, evo rollback
correctness, ChainLock-conflicted base rejection). This is milestone 6/7. The
execution plan and per-unit ledger (M6 section) are tracked outside the repo
in a private gist, not checked in: https://gist.github.com/PastaPastaPasta/aa52b1f89fb74a0566ba3b5e15afab6a
PR 6/7 in the stacked series — base is M5 (
assumeutxo/m5-loadtxoutset).What was done?
bitcoin#29370(8 upstream commits) — removes fakednTx/nChainTxandretires
BLOCK_ASSUMED_VALID(bit 256 reserved; Dash'sCHAINLOCK=128untouched). Gate2 review found M4/M5-era persisted indexes on
already-migrated branches carry fabricated
nTx=1+ the old bit 256,which aborts
CheckBlockIndexpost-upgrade — fixed with a scopedLoadBlockIndexmigration that clears the legacy entries, withpersistence/reload tests. M5's early partial extraction of this fix was
folded into this merge commit and removed as standalone code, per the M5
integration item.
-assumeutxodata'snchaintxis now base-only per thenew semantics. A 13-row audit confirmed no governance/superblock/MN-payment
consumer reads pre-base counts.
bitcoin#29215..#30403(12 PRs), landed as chronologicalone-PR commits:
#29519preserves M5's snapshot-first-then-historicaldownload scheduling;
#29553adds Dash evo rollback capture at arequested historical base plus an E2E byte-identical oracle/rollback
dump+load proof spanning ordinary and rotated quorums;
#29726composessnapshot cleanup with Dash's EvoDB wipe/rebuild;
#30267rejects bothupstream failed-chain bases and Dash
BLOCK_CONFLICT_CHAINLOCKbases.Gate1 review fixed a disabled-index wait race found by the verifier and a
stale shell-lint exclusion; gate2 came back clean with no missing
prerequisites.
How Has This Been Tested?
make: exit 0.make check: exit 0, 0 failures.blockmanager — 45 cases, no errors.
feature_assumeutxo,feature_assumeutxo_dash,wallet_assumeutxo,rpc_dumptxoutset,feature_reindex,p2p_ibd_txrelay— all passed. Full battery (15 parallel + E2E solo) rerunat the milestone gate.
executable bit found and restored); python lint deferred to CI.
feature_assumeutxo_dash.pyis load-sensitive under-j4(8 MN nodes +repeated restarts) — must run solo/serially in CI scheduling, not
parallel.
block-index migration); 1 round for the 12-PR A4 batch; milestone gate
PASS on 2026-07-12.
Breaking Changes
None for released behavior. For branches staged mid-series (persisted
chainstate from M4/M5-era code), a one-time
LoadBlockIndexmigration clearsfabricated legacy index entries on first load after this PR — handled
automatically, no user action required.
Checklist: