Repository navigation
backport: bitcoin#25193, bitcoin#33212, bitcoin#34897 and index migration fix for v24.0.x - #8033
PastaPastaPasta wants to merge 8 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
8608aab to
02a0eeb
Compare
|
✅ Final review complete — no blockers (commit 8a1fa43) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8608aabd2f
ℹ️ 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".
…t, allow interaction with reindex-chainstate
BACKPORT NOTE:
tests that delete blocks/ and chainstate/ must delete indexes/ too: txindex is on by default in Dash and BaseIndex::Init now refuses to start when the index's best block is unknown ("txindex: best block of the index not found. Please rebuild the index.")
-------
97844d9 index: Enable reindex-chainstate with active indexes (Martin Zumsande)
60bec3c index: Use first block from locator instead of looking for fork point (Martin Zumsande)
Pull request description:
This makes two improvements to the index init phase:
**1) Prevent index corruption in case a reorg happens when the index was switched off**:
This is done by reading in the top block stored in the locator instead of looking for a fork point already in `BaseIndex::Init()`.
Before, we'd just go back to the fork point by calling `FindForkInGlobalIndex()`, which would have corrupted the coinstatsindex because its saved muhash needs to be reverted step by step by un-applying all blocks in between, which wasn't done before. This is now being done a bit later in `ThreadSync()`, which has existing logic to call the custom `Rewind()` method when going back along the chain to the forking point (thanks ryanofsky for pointing this out to me!).
**2) Allow using the `-reindex-chainstate` option without needing to disabling indexes**:
With `BaseIndex::Init()` not calling `FindForkInGlobalIndex()` anymore, we can allow `reindex-chainstate` with active indexes. `reindex-chainstate` deletes the chain and rebuilds it later in `ThreadImport`, so there is no chain available during `BaseIndex::Init()`, which would lead to problems (see bitcoin#24789).
But now we'll only need the chain a bit later in `BaseIndex::ThreadSync`, which will wait for the reindex-chainstate in `ThreadImport` to finish and will continue syncing after that.
ACKs for top commit:
ryanofsky:
Code review ACK 97844d9. Just simple rebase since last review
Tree-SHA512: e24973fc22e0b87a49026f4820aecb0a4e415f4d381bade9969dd31cf97afecfea0449dce7fcc797343b792199cc8287276d1f5ffa4433dcb54fb24a808db6fb
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
(cherry picked from commit 8588588)
a602f6f test: index with an unclean restart after a reorg (Martin Zumsande) 01b95ac index: don't commit state in BaseIndex::Rewind (Martin Zumsande) Pull request description: The committed state of an index should never be ahead of the flushed chainstate. Otherwise, in the case of an unclean shutdown, the blocks necessary to revert from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular. Instead, the index state will be committed with the next ChainStateFlushed notification. Fixes bitcoin#33208 ACKs for top commit: achow101: ACK a602f6f stickies-v: re-ACK a602f6f Tree-SHA512: 2559ea3fe066caf746a54ad7daac5031332f3976848e937c3dc8b35fa2ce925674115d8742458bf3703b3916f04f851c26523b6b94aeb1da651ba5a1b167a419 (cherry picked from commit 254fe94) BACKPORT NOTE: The unclean-restart scenario calls syncwithvalidationinterfacequeue before the kill: the index stays "synced" across the reorg, so without it the kill can land before Rewind() and the test passes without the fix. Dash's TestNode has no kill_process(), so the functional test kills the process directly and expects the same exit code (-9, or 1 on Windows) that upstream's helper checks.
…state e9ed898 validation: Don't use m_chain.Tip() in FlushStateToDisk (Martin Zumsande) 3679f1e index: Don't commit ahead of the flushed chainstate (Martin Zumsande) 6573572 index: Remove return value from Commit() (Martin Zumsande) 09c0696 validation: track last flushed block (Martin Zumsande) 13c02b5 test: add test for index commits ahead of the last flushed block (Martin Zumsande) Pull request description: If indexes commit their data ahead of the flushed chainstate, and there is an unclean shutdown, the index will be corrupted. This is especially the case for the coinstatsindex, which has state (the muhash) which can't easily be rolled back without access to the blocks. This was only partly fixed in bitcoin#33212 (for reorg scenarios) but could still happen during initial sync. Fix this more thoroughly by having the node keep track of the last flushed block, and skipping index commits if the current block of the index is not an ancestor of the node's last flushed block (similar to the suggestion by stickies-v in bitcoin#33212 (review). Fixes bitcoin#33208 Fixes bitcoin#34261 ACKs for top commit: achow101: ACK e9ed898 sedited: Re-ACK e9ed898 fjahr: re-ACK e9ed898 Tree-SHA512: 7f4dc6fb942d6726587eb75dece24c79c679d8630320502aca9fa2d2b03b1d25999cd11255ec20344ebbb8985552747e2554e2557b9d2ad0c75db71652d615ab BACKPORT NOTE: - Commit() keeps Dash's existing LogError call; LogDebug becomes LogPrint(BCLog::COINDB, ...). - In FlushStateToDisk the last flushed block is recorded right after the coins flush, as upstream does; Dash then commits EvoDB in the same flush before ChainStateFlushed is sent. - src/test/chainstate_write_tests.cpp does not exist in Dash, so its two assertions are not ported. - baseindex_tests uses Dash's index API: Init() and Sync() are private here, so the test starts the index and holds back the sync thread with g_indexes_ready_to_sync to read the committed best block. (cherry picked from commit 582821e)
02a0eeb to
246b450
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
c10397f to
e489121
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
e489121 to
68c2dff
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 68c2dff and the relevant upstream test changes; one non-blocking test-coverage suggestion remains, with no confirmed production blocker. This was a static review; no builds or tests were run locally. Current CI shows passing Linux source build, lint, and cppcheck jobs, while Linux tests and several additional builds remain pending; no failed code checks were reported.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The combined changes intricately alter persistent index rewind and flush ordering in src/index/base.cpp and storage migration finalization and legacy flags in src/txdb.cpp, meeting the storage-migration critical-surface bar. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/validation_flush_tests.cpp`:
- [SUGGESTION] src/test/validation_flush_tests.cpp:185-204: Adapt the omitted bitcoin#34897 flush-tracking checks
Upstream commit 09c06960c6a2b4d037dedbf727dcd5eca53b7c7a adds two GetLastFlushedBlock() assertions around an activation-time flush. The backport note explicitly documents their omission. The new validation test checks notification ordering, but does not verify the internally recorded flushed block; the baseindex tests check forced-flush state and commit behavior, not tracking during an activation-time flush. Extend this existing critical-cache scenario with equivalent tracking assertions using Dash's supported flush paths. In particular, verify that GetLastFlushedBlock() records the newly flushed block even though the wallet-facing locator must remain at its already-connected ancestor. That distinction could regress without failing the current ordering checks. This is a non-blocking coverage gap, not evidence of an incorrect production backport, and does not require importing the unrelated periodic-flush changes.
…egacy data On a datadir upgraded from v23, the legacy "addressindex", "spentindex" and "timestampindex" flags in the block index database stay set after the first v24 start has moved the legacy entries into indexes/. If the index directories are later deleted to rebuild them, the next start finds nothing to migrate, but MigrateOldIndexData() still finalizes the empty database with the chainstate tip. The index then reports itself synced while every query returns nothing (zero balances, no txids, no block hashes), and the same would happen to a rebuild that was interrupted before its first commit. Only finalize a target that actually holds migrated entries, and clear each enabled index's legacy flag once its migration step has run, since the legacy entries no longer exist. A deleted or partially rebuilt index is now rebuilt from genesis. Older versions see the cleared flag and ask for the -reindex that the release notes already require after a downgrade, instead of serving the erased synchronous index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit a0e35d3)
… from it A node that runs without -addressindex across a reorg and then enables it again must rewind the index past the orphaned blocks instead of keeping their entries. Before bitcoin#25193 (8588588) the index resumed from the fork point of its locator and never removed them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bitcoin#25193 lets -reindex-chainstate run with optional indexes enabled and removed "Deactivate all optional indexes before running this." from the option's help text. Update the dashd and dash-qt man pages to match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bitcoin#34897 announces the last flushed block in ChainStateFlushed. Upstream only indexes listen to that signal, since bitcoin#30221 stopped the wallet from storing its locator. Dash's wallet still writes the announced locator as its best block. A flush inside ConnectTip() happens after the coins are updated but before the block is part of m_chain and before its BlockConnected is queued, so the wallet could record the block it is about to scan as the chain tip. A crash in that window would skip the rescan on restart and miss the block's transactions. Announce the most recent flushed block that is part of the active chain. That is the parent while ConnectTip() is connecting a block, which is what was announced before bitcoin#34897, and the flushed parent after DisconnectTip(), which keeps the upstream fix for that case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit e8325a1)
The address, spent, timestamp and governance collateral indexes erase the entries of rewound blocks in CustomRewind. Since bitcoin#33212 a rewind no longer commits its locator, and since bitcoin#34897 a later commit only writes it once the index tip is an ancestor of the last flushed block. If the chainstate's last flush is below the rewind target, the erasures were persisted while the committed locator stayed above them, on the rewound branch. After an unclean restart in which that branch wins again, the index resumed from the old locator and never appended the erased blocks again. The governance collateral index used to write its locator in the same batch as the erasures; skipping it there opened the same gap. When the committed locator is above the new tip on the same branch, move it to the new tip. The committed tip was an ancestor of a flushed block when it was written, so the new tip below it is in the block index after a restart. The locator then matches what the index holds, whichever branch wins afterwards. A committed locator at or below the new tip, or on another branch, is left alone: a restart resumes or rewinds from it correctly. The governance collateral index writes it in the same batch as its erasures, the others right after theirs, as the full commit did before bitcoin#33212. This also covers a rewind whose new tip is already flushed, so BaseIndex::Rewind() stays as upstream left it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 337c464)
68c2dff to
8a1fa43
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1fa4309d
ℹ️ 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".
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Updated after discussion: 1 finding(s) of this review were settled in the inline threads; no blocking finding remains.
- ✅ Withdrawn: 🔴 Persist the rewind locator atomically with each rewind batch
At 8a1fa43, the Dash-specific rewind repair still leaves a crash window between index erasures and the locator update; the activation-time flush-tracking coverage suggestion also remains open. This was static verification only, with no local builds or tests. Current CI has passing lint, cppcheck, several builds and the nowallet tests, while additional Linux/sanitizer builds and sqlite tests remain running; check_merge fails its separate fast-forward-to-master policy check, not compilation or tests.
🔴 1 0 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff combines intricate index persistence, rewind, and crash-recovery changes with storage migration changes in src/txdb.cpp's CBlockTreeDB::MigrateOldIndexData and TargetNeedsLocator, meeting both the complexity and critical-surface criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/index/addressindex.cpp`:
- [BLOCKING → WITHDRAWN in discussion; do not fix] src/index/addressindex.cpp:401-403: Persist the rewind locator atomically with each rewind batch
The locator update is a separate database write after all per-block rewind writes have completed. Start with chainstate and the index committed at A100, reorg without flushing chainstate or the replacement block index, and terminate the process after RewindBatch() removes A100's entries but before this locator write. Restart can restore A100 as the active tip, while the index still advertises its unchanged A100 locator. BaseIndex::Init() then considers it synced and never rebuilds the deleted history and unspent entries. SpentIndex::CustomRewind() and TimestampIndex::CustomRewind() have the same separation. These are entries for an already committed block, not the newly appended, uncommitted entries covered by the PR's documented limitation. Include the conditionally lowered locator in each block's rewind batch, targeting that block's parent, so every persisted intermediate state remains recoverable. Extend the restart coverage to exercise interruption between rewind batches; the current test completes CustomRewind() before restarting and cannot expose this boundary.
In `src/test/validation_flush_tests.cpp`:
- [SUGGESTION] src/test/validation_flush_tests.cpp:198-204: Adapt the omitted bitcoin#34897 flush-tracking checks
(existing thread: https://github.com/dashpay/dash/pull/8033#discussion_r4452656189)
This scenario still verifies notification ordering without reading GetLastFlushedBlock(), so it does not check the internal tracking invariant covered by the two omitted assertions in upstream commit 09c06960c6a2b4d037dedbf727dcd5eca53b7c7a. During ConnectTip(), the recorded flushed block must be the new block even though the announced wallet locator identifies its connected ancestor. Add a known pre-flush baseline and verify that distinction during activation. An assertion only after CreateAndProcessBlock() returns is insufficient for the proposed m_chain.Tip() regression: with this zero cache budget, ActivateBestChain() can perform another PERIODIC flush after advancing the chain tip, overwriting the incorrect earlier marker. Capture the tracker before that later flush, for example in SynchronousUpdatedBlockTip(), and check the announced ancestor separately. This remains a non-blocking coverage gap, not a demonstrated production defect; if deferred, link the follow-up issue or PR.
| CDBBatch locator_batch(GetDB()); | ||
| WriteRewindLocator(locator_batch, new_tip_index); | ||
| return GetDB().WriteBatch(locator_batch); |
There was a problem hiding this comment.
✅ Withdrawn at
8a1fa430; see the replies below.
🔴 Blocking: Persist the rewind locator atomically with each rewind batch
The locator update is a separate database write after all per-block rewind writes have completed. Start with chainstate and the index committed at A100, reorg without flushing chainstate or the replacement block index, and terminate the process after RewindBatch() removes A100's entries but before this locator write. Restart can restore A100 as the active tip, while the index still advertises its unchanged A100 locator. BaseIndex::Init() then considers it synced and never rebuilds the deleted history and unspent entries. SpentIndex::CustomRewind() and TimestampIndex::CustomRewind() have the same separation. These are entries for an already committed block, not the newly appended, uncommitted entries covered by the PR's documented limitation. Include the conditionally lowered locator in each block's rewind batch, targeting that block's parent, so every persisted intermediate state remains recoverable. Extend the restart coverage to exercise interruption between rewind batches; the current test completes CustomRewind() before restarting and cannot expose this boundary.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
You're right that the window exists, but this head doesn't introduce or widen it. On v24.0.0-rc.6 (8d195f189d6), the base of this PR, AddressIndex::CustomRewind commits its erasures with RewindBatch(), and BaseIndex::Rewind then writes the locator through a separate Commit(). That is the same erasure-then-locator gap. It's also the same with the Commit() this push dropped. Spent and timestamp indexes work the same way. The window is between two back-to-back LevelDB writes on the same thread, so it needs a process kill at exactly that point.
The same finding is open on #8030 as r4452414669, where the reviewer rated it a non-blocking completeness gap that predates the PR. Writing the locator for each block into that block's rewind batch is on the follow-up list. It needs the database helpers to accept a caller-owned batch, so it changes more than this release backport should. Leaving this thread open for the maintainers to decide, together with the other open #8030 blocker.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
You're right about the baseline: at 8d195f189d6, all three indexes persist their erasures inside CustomRewind(), and only afterward does BaseIndex::Rewind() call the separate Commit() that writes the locator. This head restores that post-rewind locator write without introducing the interruption window I flagged. I overstated this as a blocker for the release backport; atomic per-block locator updates remain a worthwhile follow-up, but I'm withdrawing this finding.
Withdrawing this finding.
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review after discussion — commit 8a1fa43
Standing review was CHANGES_REQUESTED; this re-review is COMMENT: no blocking findings remain.
Not approved: 1 non-blocking finding(s) from the full review of 8a1fa430 (run 8631) still stand.
No longer blocking:
- Persist the rewind locator atomically with each rewind batch
Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.
Review provenance
Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed
…se notes Fold doc/release-notes-8018.md and doc/release-notes-8029.md into the consolidated release notes and remove them: - dashpay#8018: the IPv4-primary and mainnet standard-port rules go under Extended addresses, with a pointer from the masternode upgrade notes. Mainnet keeps requiring them; testnet and devnets allow other primaries from height 0. - dashpay#8029: dashd no longer creates an empty dash.conf, under Updated settings. This also settles the pending note on starting without an existing default data directory, so that TODO is removed. Describe the index fixes carried from dashpay#8033: the one-time rebuild for nodes that ran a release candidate (replacing the release-candidate TODO), the -reindex-chainstate change from bitcoin#25193, older versions refusing a migrated index until -reindex, and the known limitation shared with earlier versions. The other rc.7 picks fix code that is new in v24 and need no release note. No man page changes: the only help text change, the ipv4_optional name in -testactivationheight, is debug-only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Superseded by #8023. Its eight commits (head 8a1fa43) are now in #8023 unchanged, so v24.0.x needs only one backport merge. When #8042 merges to develop, #8023 will replace the bitcoin#33212/bitcoin#34897 and locator commits with #8042's v24.0.x port. The open review threads here move with them. 🤖 Posted autonomously by Claude on behalf of pasta. |
…se notes Fold doc/release-notes-8018.md and doc/release-notes-8029.md into the consolidated release notes and remove them: - dashpay#8018: the IPv4-primary and mainnet standard-port rules go under Extended addresses, with a pointer from the masternode upgrade notes. Mainnet keeps requiring them; testnet and devnets allow other primaries from height 0. - dashpay#8029: dashd no longer creates an empty dash.conf, under Updated settings. This also settles the pending note on starting without an existing default data directory, so that TODO is removed. Describe the index fixes carried from dashpay#8033: the one-time rebuild for nodes that ran a release candidate (replacing the release-candidate TODO), the -reindex-chainstate change from bitcoin#25193, older versions refusing a migrated index until -reindex, and the known limitation shared with earlier versions. The other rc.7 picks fix code that is new in v24 and need no release note. No man page changes: the only help text change, the ipv4_optional name in -testactivationheight, is debug-only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The bitcoin#33212/bitcoin#34897 and locator commits from dashpay#8033 are replaced by the v24.0.x port of dashpay#8042 (merge 9e5e3d5), ported commit by commit, so the index paragraph now cites dashpay#8042 and dashpay#8031 instead of dashpay#8033. dashpay#8031 is cited for older versions refusing a migrated index, and bitcoin#25193 alone for -reindex-chainstate. dashpay#8042 also makes startup fail with "<index>: the block the index was last synced to is not found. Please rebuild the index." when an unclean shutdown loses blocks whose entries an index had already written. Mention that error next to the known limitation, which still applies in the other cases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2bf133c docs: cite #8042 for the index fixes in the v24.0.0 release notes (pasta) 8aa11f6 docs: add the v24.0.0-rc.7 picks and index fixes to the v24.0.0 release notes (pasta) ad4e5d6 docs: drop the index caveat from the -reindex-chainstate man page entry (pasta) 19a90bb test: blocks orphaned while the address index is disabled are removed from it (pasta) d2f2a1f fix(index): don't adopt an empty or rebuilt async index as migrated legacy data (pasta) e3bafdb fix(index): reject an uncommitted tip the block index lost (pasta) 9198962 fix(index): check the blocks governance resumes from at startup (pasta) baff347 fix(index): resume from the tip a skipped commit leaves behind (pasta) 2dbf5ca fix(index): write a rewound block's erasures in one batch with its locator (pasta) 252acbe fix(index): move the committed locator below the entries a rewind erases (Konstantin Akimov) e9f9c8a Merge bitcoin#36150: indexes: set prune lock to genesis before first block (Konstantin Akimov) 2edeb98 partial Merge bitcoin#34897: indexes: Don't commit ahead of the flushed chainstate (Konstantin Akimov) 3eba84c Merge bitcoin#32875: index: handle case where pindex_prev equals chain tip in NextSyncBlock() (Konstantin Akimov) 399125c Merge bitcoin#32882: index: remove unnecessary locator cleaning in BaseIndex::Init() (Konstantin Akimov) bb724c6 Merge bitcoin#33212: index: Don't commit state in BaseIndex::Rewind (Konstantin Akimov) d5ff431 Merge bitcoin#32878: index: fix wrong assert of current_tip == m_best_block_index (Konstantin Akimov) 31c7d8f partial Merge bitcoin#32580: wallet, test: best block locator matches scan state follow-ups (Konstantin Akimov) 94f5c58 partial Merge bitcoin#30221: wallet: Ensure best block matches wallet scan state (Konstantin Akimov) 8e0b1e8 partial Merge bitcoin#32069: test: fix intermittent failure in wallet_reorgsrestore.py (Konstantin Akimov) 4d17947 Merge bitcoin#25193: indexes: Read the locator's top block during init, allow interaction with reindex-chainstate (Konstantin Akimov) 0127df8 Merge #8045: test: scale the mempool-loaded wait by timeout_factor so TSAN reindex restarts stop timing out (pasta) 62d0cef Merge #8036: fix(qt): let a shared masternode participant follow a revised invitation that keeps their share (merge-script) 775d31c Merge #8018: fix(consensus): require IPv4 masternode primaries until IPv4 is optional, pin mainnet ports (merge-script) e1b826e Merge #8026: fix(qt): keep shared masternode contribution fields locked while coins are reserved (merge-script) 9fa3df3 Merge #8020: fix(rpc): make protx shared_sign sign only dissolutions that return every principal (merge-script) 246a465 Merge #8024: fix(qt): keep the shared masternode dissolve dialog consistent with what it will send (merge-script) 590ad0a Merge #8021: fix(qt): don't load a newer shared masternode session copy that changes your own share (merge-script) 974aaa3 Merge #8034: fix(qt): check a shared masternode's service and refund addresses before coins are reserved (merge-script) 0026e3e Merge #8027: fix(qt): fund a shared masternode's service update from this wallet by default (merge-script) 6a5d2dc Merge #8035: fix(qt): don't strand coins reserved for a shared masternode session (merge-script) df6083a Merge #8032: fix(qt): don't offer a new operator key when reopening a shared masternode session (merge-script) 2b5bf30 Merge #8029: fix: let dashd -version, -help and first start work without an existing data directory (merge-script) e33d9b1 Merge #8037: docs: list the config file path each Linux init system uses (merge-script) 6030ea0 docs: add the #8014 migratewallet fixes to the v24.0.0 release notes (pasta) 1b1ea95 Merge #8014: backport: bitcoin#28609, bitcoin#34215, bitcoin#34226, bitcoin#34370, partials bitcoin#31423, bitcoin#34156 (migratewallet fixes from 28.x) (merge-script) 608ff63 Merge bitcoin#28542: wallet: Check for uninitialized last processed and conflicting heights in MarkConflicted (DCG-Claude) 3af6f63 Merge bitcoin#28038: wallet: address book migration bug fixes (DCG-Claude) 2fe41fb docs: add ecosystem notes on Asset Unlock txids, coinbase payees and RPC errors (pasta) 0e3f653 docs: correct operator guidance in the v24.0.0 release notes for rc.7 (pasta) 11c8140 Merge #8019: fix(qt): stop reporting refused shared masternode broadcasts as sent (merge-script) Pull request description: ## Issue being fixed or feature implemented Backports and release prep for v24.0.0-rc.7, on top of `v24.0.x` at 8d195f1 (v24.0.0-rc.6, #8013). This PR collects the develop merges labeled `backport-candidate-24.0.x`, the v24.0.x port of #8042 with the index fixes from #8033, and the release notes for them, so rc.7 has a single backport PR to merge. ## What was done? ### Picked Cherry-picked with `git cherry-pick -m1 -x`, in develop merge order, except that bitcoin#28038 goes before bitcoin#28542 as in Bitcoin Core. Each pick changes exactly the same lines as its develop merge, except as noted below for bitcoin#28542, #8014, #8020 and #8018. - #8019 fix(qt): stop reporting refused shared masternode broadcasts as sent - bitcoin#28038 (from #7863): prerequisite of #8014 - bitcoin#28542 (from #7762): prerequisite of #8014. Its `test_conflict_txs` migrates through `self.migrate_wallet`, as on develop since bitcoin#28868. - #8014 backport: bitcoin#28609, bitcoin#34215, bitcoin#34226, bitcoin#34370, partials bitcoin#31423, bitcoin#34156 (migratewallet fixes from 28.x), with the Dash adaptations below - #8037 docs: list the config file path each Linux init system uses - #8029 fix: let dashd -version, -help and first start work without an existing data directory - #8032 fix(qt): don't offer a new operator key when reopening a shared masternode session - #8035 fix(qt): don't strand coins reserved for a shared masternode session - #8027 fix(qt): fund a shared masternode's service update from this wallet by default - #8034 fix(qt): check a shared masternode's service and refund addresses before coins are reserved - #8021 fix(qt): don't load a newer shared masternode session copy that changes your own share - #8024 fix(qt): keep the shared masternode dissolve dialog consistent with what it will send - #8020 fix(rpc): make protx shared_sign sign only dissolutions that return every principal. Its `doc/release-notes-7437.md` sentence goes into the same paragraph of `doc/release-notes.md`, since that file was folded in for v24.0.0. - #8026 fix(qt): keep shared masternode contribution fields locked while coins are reserved (merged on develop as cc73a04; GitHub shows the PR as closed) - #8018 fix(consensus): require IPv4 masternode primaries until IPv4 is optional, pin mainnet ports, with the adaptation below - #8036 fix(qt): let a shared masternode participant follow a revised invitation that keeps their share - #8045 test: scale the mempool-loaded wait by timeout_factor so TSAN reindex restarts stop timing out ### Index fixes: #8042 port and #8033 The index commits are #8042 (merge 9e5e3d5), ported commit by commit to `v24.0.x`, plus the parts of #8033 that #8042 doesn't cover. #8042's v24.0.x port replaces #8033's bitcoin#33212, bitcoin#34897 and rewind-locator commits, and #8033's flushed-block announcement fix (e6a687b) is dropped: bitcoin#30221 removes the wallet's `ChainStateFlushed` locator write, and nothing else on `v24.0.x` consumes that notification. This PR replaces #8033, and #8031 comes in through it (adapted to the `v24.0.x` `CCoinsViewDB` constructor), not as a separate pick. - bitcoin#25193 (from #7883, via #8033): indexes: read the locator's top block during init, allow interaction with `-reindex-chainstate` - From #8042: partial bitcoin#32069 (only `TestNode.kill_process()`, with develop's `wait_until_stopped(**kwargs)` handling it needs), partial bitcoin#30221, partial bitcoin#32580, bitcoin#32878, bitcoin#33212, bitcoin#32882, bitcoin#32875, partial bitcoin#34897, bitcoin#36150, and #8042's own fixes: move the committed locator below the entries a rewind erases; write a rewound block's erasures in one batch with its locator; resume from the tip a skipped commit leaves behind; check the blocks an index resumes from at startup; reject an uncommitted tip the block index lost - From #8033: #8031 fix(index): don't adopt an empty or rebuilt async index as migrated legacy data; test: blocks orphaned while the address index is disabled are removed from it; docs: drop the index caveat from the `-reindex-chainstate` man page entry #### Dash adaptations for #8042 - The governance collateral index doesn't exist on `v24.0.x`, so its hunks are dropped; from the startup resume check only the `base.{h,cpp}` part is kept. - `feature_pruning.py`: node 5 syncs before node 0's pruned restart, and `test_rescan_blockchain` expects the governance-disabled stderr a pruned node prints on `v24.0.x`. - The wallet's `blockConnected` has no birthday skip on `v24.0.x`. - `TestNode.wait_until_stopped()` keeps `v24.0.x`'s `None` default for a clean exit. Apart from these, the `v24.0.x` index, validation and index test sources differ from develop after #8042 only by differences that already existed before it. #### Dash adaptations for #8018 `v24.0.x` predates bitcoin#26177, so it has no `src/kernel/chainparams.cpp` and no `GetBuriedDeployment()`. The four `IPv4OptionalHeight` values (mainnet not scheduled, testnet 0, devnet 0, regtest only through `-testactivationheight=ipv4_optional@<height>`) go into the matching classes in `src/chainparams.cpp`, and `ipv4_optional` is parsed in its `MaybeUpdateHeights()`. The `GetBuriedDeployment()` hunk is dropped; the `DeploymentName()` case is kept. Every other file, including all tests, changes exactly the same lines as the develop merge. The same notes are in the pick's commit message. #### Dash adaptations for #8014 #8014 doesn't cherry-pick cleanly onto `v24.0.x`. The conflicts were resolved against `v24.0.x` APIs. No prerequisite was backported, and no hunk or test was dropped. The same notes are in the pick's commit message. - `src/wallet/bdb.{cpp,h}`, `dump.cpp`, `sqlite.{cpp,h}`, `transaction.{cpp,h}`, `wallet.cpp`: the changed lines are identical to the develop merge. In `wallet.cpp`, the conflict was with develop-only context (`CanGrindR`), which stays out. - `src/wallet/db.h`: #8014 makes `WalletDatabase::Files()` pure virtual. `v24.0.x` still defines `DummyDatabase` in `db.h`, so it gets the `Files()` override there. - `src/wallet/test/wallet_tests.cpp`: the test database mock on `v24.0.x` is `FailDatabase`, so it gets the same `Files()` override. `src/wallet/salvage.cpp` and `src/wallet/test/util.h` keep the `v24.0.x` side, since develop's `DummyDatabase`/`MockableDatabase` placement doesn't exist here. - `test/functional/tool_wallet.py`, `wallet_backup.py`, `wallet_migration.py`: the test framework on `v24.0.x` has no `TestNode.wallets_path`/`datadir_path` (bitcoin#27884), so the new tests build these paths with `os.path.join` and pass RPC paths as strings. `wallet_migration.py` takes only #8014's new tests, not the develop-only tests around them. - `test/functional/wallet_migration.py`: `unsynced_wallet_on_pruned_node_fails` stops its pruned node with `EXPECTED_STDERR_NO_GOV_PRUNE`, because on `v24.0.x` a pruned node still prints the governance-disabled warning. The rc.6 picks handled the same warning the same way. ### Separate v24.0.x PRs, merged separately - #8016 backport: v23.1.10 fixes for v24.0.x - #8022 backport: bind CoinJoin signatures to the approved transaction (#7815) for v24.0.x, which depends on #7815 merging on develop #8016 and #8022 each merge cleanly with this branch in either order, and all three merge to the same tree in any order. ### Not picked - #8025: closed; superseded by #8014 (picked above) - #8031 is not picked separately; it comes through #8033's commits above. - #8039, #8040, #8041, #8043, #8044 (CI and tooling), #7905, follow-ups to #8042, and the PRs in the 24.1 milestone. - Explicitly deferred: #7870, #7679, #7675, #7220, #7217, #7109, #6988, #4974, #8028. ### Release notes `doc/release-notes.md` corrections, checked against the code on this branch: - **Version 2 Asset Unlocks:** version 1 Asset Unlocks stay valid after `v24` activation. Platform withdrawals keep using version 1 until a future Dash Platform release issues version 2, so the stable txid, InstantSend locking and instance-hash relay don't apply to them yet. - **Masternode requirements:** the startup refusals (#7754) and the DKG bad votes for missing transaction relay or compact filters (#7755) apply from the first start of v24.0.0, not from activation. Operators should remove conflicting settings and upgrade before the `v24` start date (November 9, 2026), so their masternodes are running when the activation signal is signed. `-peerblockfilters=0` is the new refusal; `-blockfilterindex=0` already failed on v23. - **How to Upgrade:** a pruned node with `-addressindex` or `-spentindex` now refuses to start; v23 accepted that combination. - **Downgrading:** there is no supported downgrade after `v24` activation: older versions can't follow the network, and they misread the masternode list data v24.0.0 writes. Before activation, downgrading to v23.1.x works with the listed caveats. Dash-Qt's dust-protection setting is migrated to the shared settings, so it reads as disabled after a downgrade. - **Dash Platform:** Platform/dashmate v5.0.0 and later handle the hidden `service` and `softforks` RPC fields. Older Platform versions need both `-deprecatedrpc` options. - **Compatibility:** users on macOS 13 or older must upgrade macOS, or move their node, before activation. - **Legacy wallets:** in v24, `createwallet` with `descriptors=false` also needs `load_on_startup`; before v24 this applied only to `descriptors=true`. The notes now give a working example. - **Wallet migration (#8014):** failure handling in `migratewallet`, `restorewallet` and `dash-wallet createfromdump` is fixed, and the new backup and watch-only/solvables wallet names are listed. - **Ecosystem notes:** the txid of a version 2 Asset Unlock zeroes `requestedHeight`, `quorumHash` and `quorumSig` before hashing (the full-bytes hash is `instanceHash`); pools should emit one coinbase output per `masternode[]` and `superblock[]` template entry, since merged or deduplicated payees are invalid after activation; client libraries that send numbers or booleans as strings fail (#7099); and some wallet RPC error messages changed with upstream backports. - **rc.7 picks:** `doc/release-notes-8018.md` and `doc/release-notes-8029.md` are folded in and removed. #8018's IPv4-primary and mainnet standard-port rules are under Extended addresses, with a pointer from the masternode upgrade notes; #8029's `dash.conf` change is under Updated settings. The other rc.7 picks fix code that is new in v24 and need no release note. - **Index fixes (#8042, #8031):** nodes that ran a release candidate with the address, spent or timestamp index should rebuild them once (with `-reindex`, or by starting once with them disabled, deleting their `indexes/` directories and starting with them enabled). The notes also cover `-reindex-chainstate` with indexes enabled, older versions refusing a migrated index until `-reindex`, the new "the block the index was last synced to is not found. Please rebuild the index." startup error, and the known limitation shared with earlier versions in the remaining cases. - All rc.7 TODO comments are resolved. `configure.ac` is already 24.0.0 with `CLIENT_VERSION_IS_RELEASE` false, so the version string comes from the `v24.0.0-rc.7` tag. The man pages carry #8033's `-reindex-chainstate` change; no other pick changes non-debug help text (#8018's `-testactivationheight` change is debug-only). ## How Has This Been Tested? On this head (2bf133c): - Every develop pick compared with its develop merge (`git patch-id --stable` and the set of changed lines). #8019, #8037, #8032, #8035, #8027, #8034, #8021, #8024, #8026, #8036 and #8045: identical. #8029: the same changed lines; only context differs in `feature_config_args.py`. #8020: the same changed lines, in `doc/release-notes.md` instead of `doc/release-notes-7437.md`. #8018: identical except the adaptation above, checked file by file against 900f085 and reviewed separately for consensus equivalence on mainnet, testnet, devnet and regtest. #8014 and its prerequisites: as in the earlier rounds (adaptations above). - #8042 port: `v24.0.x`'s divergence from develop, before #8042 (058b060) versus after it (9e5e3d5), is unchanged in `src/index/*`, `src/validation.{h,cpp}`, the index unit tests and `feature_coinstatsindex.py`, apart from the adaptations listed above. The #8031, test and man page commits have the same patch-id and message as on #8033. - Built with `--enable-debug --enable-werror` and Qt: no warnings. - Unit suites pass: `baseindex_tests`, `index_migration_tests`, `txindex_tests`, `blockfilter_index_tests`, `coinstatsindex_tests`, `validation_flush_tests`, `wallet_tests`, `interfaces_tests`, `evo_netinfo_tests` and `evo_snapshot_tests`. With the develop picks (7275005), every `evo_*` suite and `test_dash-qt` (15 classes, 152 passed, 0 failed) passed too. - Functional tests pass: `rpc_netinfo.py`, `feature_masternode_shares.py`, `wallet_migration.py`, `feature_coinstatsindex.py` (including the unclean restart after a reorg), `feature_addressindex.py`, `feature_config_args.py` and `feature_index_prune.py`. On the same C++ tree, these pass as well: `feature_spentindex.py`, `feature_timestampindex.py`, `feature_pruning.py`, `feature_txindex.py`, `feature_reindex.py`, `feature_init.py`, `feature_abortnode.py` and `wallet_reorgsrestore.py`. - Regtest upgrade gates, run on a build of this exact head: all pass. They cover v23.1.10 index migration, index contents vs v23 and `-reindex`, a kill mid-migration, 9 wallets, a pre-activation downgrade and round trip, and v22.1.3/v23.0.2 upgrades. The crash-after-reorg probe and the deleted-index probe are both required to be not reproduced: 0/10 stale (rc.6: about 4/10), and the plain-delete rebuild matches. - Man pages: `help2man` output of the built `dashd`, `dash-cli`, `dash-tx`, `dash-wallet` and `dash-util` matched `doc/man` on the develop picks (apart from the debug-build-only `lock` log category). No later commit changes non-debug help text, so nothing is regenerated beyond #8033's `-reindex-chainstate` change. - `git merge-tree` against #8016 (cdb250c) and #8022 (62609bf), heads fetched 05:23 UTC: clean in both orders, and all three merge to the same tree in every order. From the earlier rounds: the wallet suites and functional tests listed for #8014, bitcoin#28038 and bitcoin#28542 passed, and the `createwallet` example was checked against the official v24.0.0-rc.6 binary. ## Breaking Changes - From #8014: migration backups are now named `<name>_<time>.legacy.bak`, the unnamed wallet's watch-only and solvables wallets are named `default_wallet_watchonly` and `default_wallet_solvables`, and `restorewallet` accepts an existing empty directory. - From #8018: on mainnet, a version 3 ProRegTx or ProUpServTx must list an IPv4 address on the standard port as the first entry of each address list; testnet and devnets are unaffected. - From #8029: `dashd` no longer creates an empty `dash.conf`. - From #8031: older versions refuse an index migrated by v24.0.0 until they run `-reindex`; nodes that ran a release candidate should rebuild these indexes once. - From #8042: an index whose last synced block was lost in an unclean shutdown makes startup fail with a rebuild instruction. The release notes describe these. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone 🤖 Generated with [Claude Code](https://claude.com/claude-code) Top commit has no ACKs. Tree-SHA512: 3c405ef570fc1c5b46d1ba8bb2e344213cf65018abe5ef4b94b7cf2b9aa7160ba2675f71a0a365c72bc8249a2f9a5defe774af2a4e33537245aeb420c5f66711
Issue being fixed or feature implemented
On v24.0.0-rc.1 through rc.6, the asynchronous address, spent and timestamp indexes (#7101) can become inconsistent with the active chain, in two ways:
-reindexround trip. The node keeps serving the stale balances and txids.synced: true.This brings the fixes for both to v24.0.x, ahead of rc.7.
What was done?
Cherry-picked, in order:
85885880a22: indexes: Read the locator's top block during init, allow interaction with reindex-chainstate bitcoin/bitcoin#25193 from backport: bitcoin#25193, #25862, #25943, #28123 and relevant fixes #7883, the init change only. The other backport: bitcoin#25193, #25862, #25943, #28123 and relevant fixes #7883 backports are not needed here. On its own this change brings back Indexes stuck on unknown best block after unclean shutdown bitcoin/bitcoin#33208 (a node that refuses to start after a crash), which the next two picks fix.feature_addressindexregression test, all from backport: bitcoin#33212, bitcoin#34897 (don't commit indexes ahead of the flushed chainstate) #8030. The governance-collateral commit in backport: bitcoin#33212, bitcoin#34897 (don't commit indexes ahead of the flushed chainstate) #8030 is develop-only, because that index doesn't exist on v24.0.x.ChainStateFlushedlocator fix and the rewind-locator fix from backport: bitcoin#33212, bitcoin#34897 (don't commit indexes ahead of the flushed chainstate) #8030. The governance collateral part is develop-only. Dash's wallet still stores that locator, unlike upstream after wallet: Ensure best block matches wallet scan state bitcoin/bitcoin#30221.-reindex-chainstateindashd.1anddash-qt.1, matching the indexes: Read the locator's top block during init, allow interaction with reindex-chainstate bitcoin/bitcoin#25193 help text. No other help text changes.The
(cherry picked from …)trailers name the commits in #8030/#8031. If those PRs merge first, this can be redone as-m1picks of their merges, as for the other v24.0.x backports. The test-only Dash changes adapt to v24.0.x APIs (CCoinsViewDBconstructor).How Has This Been Tested?
On a build of this branch:
baseindex_tests,index_migration_testsandvalidation_flush_testspass. Each new test fails with its fix reverted; the rewind-locator cases also reject an earlier common-ancestor variant.feature_addressindex,feature_coinstatsindex,feature_index_prune,feature_reindex,feature_spentindex,feature_timestampindex,mempool_persist,p2p_blockfilters,rpc_getblockfilter,rpc_misc,wallet_backup,wallet_hd. The newfeature_addressindexcheck fails on rc.6. The index: Don't commit state in BaseIndex::Rewind bitcoin/bitcoin#33212feature_coinstatsindexscenario fails 4/4 with only indexes: Read the locator's top block during init, allow interaction with reindex-chainstate bitcoin/bitcoin#25193 ("best block of the index not found").Commit()inRewindper review on backport: bitcoin#33212, bitcoin#34897 (don't commit indexes ahead of the flushed chainstate) #8030): all 8 baseline cases pass (v23.1.10 migration, index contents, kills, wallets, pre-activation round trip, v22.1.3 and v23.0.2 upgrades), and both the crash-after-reorg probe (0/10 vs about 4/10 on rc.6) and the deleted-index probe are not reproduced.ChainStateFlushedlocator change; the runs were offline with the wallet disabled):synced:falseat height 0, rebuild in about 13 minutes, and match both rc.6's rebuild and their own migration exactly. rc.6 reports empty indexes as synced here; this case had not been run on real data before.-reindexis given.Known limitation (not fixed here)
These changes make the committed index state follow the flushed chainstate and make rewinds durable. They do not cover one case that v23's synchronous indexes share: the entries of a block connected shortly before an unclean shutdown stay in the address, spent and timestamp indexes if that block is lost in the crash and never received again, because a competing block won. Nothing on disk records those entries after the crash, so no rewind can find them. A full
-reindex(or starting once with these indexes disabled, stopping, deleting theirindexes/directories and starting again with them enabled) removes them. Closing this would need the index to track the blocks it wrote past its committed locator, which is a follow-up rather than part of this fix.Breaking Changes
-reindex. The release notes already required this.-reindex, or start once with them disabled, stop, delete theirindexes/directories, and start again with them enabled.Checklist:
🤖 Generated with Claude Code