-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: assumeutxo M4 — evo snapshot format v3 and LLMQ reconstruction #7579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
1d3b001
8c3485a
52d6a92
e396b35
8baac21
d3880a5
3cfd05d
34fc56a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1668,9 +1668,10 @@ std::string Chainstate::EvoDbInconsistencyMessage() | |
| const CBlockIndex* Chainstate::SnapshotBase() | ||
| { | ||
| if (!m_from_snapshot_blockhash) return nullptr; | ||
| // Unlike upstream, a missing base block is not Assert()ed away: synthetic | ||
| // unit fixtures activate a snapshot chainstate before inserting its base | ||
| // into the block index, and ChainstateManager::LoadBlockIndex() reports a | ||
| // Unlike upstream, a missing base block is not Assert()ed away: snapshot | ||
| // detection precedes LoadBlockIndex during startup, synthetic unit | ||
| // fixtures activate a snapshot chainstate before inserting its base into | ||
| // the block index, and ChainstateManager::LoadBlockIndex() reports a | ||
| // missing on-disk base as a startup error rather than an abort. Callers | ||
| // that require existence Assert at the call site. | ||
| if (!m_cached_snapshot_base) m_cached_snapshot_base = m_chainman.m_blockman.LookupBlockIndex(*m_from_snapshot_blockhash); | ||
|
|
@@ -3917,7 +3918,8 @@ void Chainstate::TryAddBlockIndexCandidate(CBlockIndex* pindex) | |
| // For the background chainstate, we only consider connecting blocks | ||
| // towards the snapshot base (which can't be nullptr or else we'll | ||
| // never make progress). | ||
| const CBlockIndex* snapshot_base{Assert(m_chainman.GetSnapshotBaseBlock())}; | ||
| const CBlockIndex* snapshot_base{m_chainman.GetSnapshotBaseBlock()}; | ||
| if (!snapshot_base) return; | ||
| if (snapshot_base->GetAncestor(pindex->nHeight) == pindex) { | ||
| setBlockIndexCandidates.insert(pindex); | ||
| } | ||
|
|
@@ -5699,6 +5701,7 @@ bool ChainstateManager::ActivateSnapshot( | |
| } | ||
| if (!snapshot_ok) { | ||
| LOCK(::cs_main); | ||
| this->ReleaseSnapshotPruneLock(); | ||
| this->MaybeRebalanceCaches(); | ||
|
|
||
| // PopulateAndValidateSnapshot commits the snapshot best-block and | ||
|
|
@@ -5807,6 +5810,11 @@ bool ChainstateManager::PopulateAndValidateSnapshot( | |
| return false; | ||
| } | ||
|
|
||
| // Protect the full base block before the long-running population step. | ||
| // Snapshot activation is not visible yet, so use the resolved base directly. | ||
| WITH_LOCK(::cs_main, m_blockman.UpdatePruneLock( | ||
| "assumeutxo", {.height_first = snapshot_start_block->nHeight})); | ||
|
|
||
| int base_height = snapshot_start_block->nHeight; | ||
| auto maybe_au_data = ExpectedAssumeutxo(base_height, GetParams()); | ||
|
|
||
|
|
@@ -6273,6 +6281,7 @@ bool ChainstateManager::HandleSnapshotStateMismatch( | |
| m_ibd_chainstate->m_mempool = m_snapshot_chainstate->m_mempool; | ||
| m_snapshot_chainstate->m_mempool = nullptr; | ||
|
Comment on lines
+5790
to
+5795
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Preserve the mempool lock through invalid-snapshot cleanup Background ActivateBestChain enters with LOCK(MempoolMutex()), but that lock is a no-op because the background chainstate has m_mempool == nullptr. HandleSnapshotStateMismatch() then transfers the active snapshot's mempool pointer to the background chainstate and returns to ActivateBestChainStep, whose failed ConnectTip path calls MaybeUpdateMempoolForReorg(). That function now operates on the transferred mempool without its mutex, violating its explicit lock precondition, triggering lock assertions in checked builds, and racing concurrent mempool users otherwise. Defer ownership transfer until ActivateBestChain has unwound, or restructure the failure path so the snapshot mempool mutex remains owned through MaybeUpdateMempoolForReorg and the rest of the failed-step cleanup. source: ['coderabbit'] |
||
| m_snapshot_chainstate->m_disabled = true; | ||
| ReleaseSnapshotPruneLock(); | ||
| assert(!IsUsable(m_snapshot_chainstate.get())); | ||
| assert(IsUsable(m_ibd_chainstate.get())); | ||
|
Comment on lines
+5790
to
+5799
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Inspect MaybeUpdateMempoolForReorg and every call reachable after a failed background ConnectTip.
set -euo pipefail
ast-grep run --pattern 'void Chainstate::MaybeUpdateMempoolForReorg($$$) { $$$ }' --lang cpp src/validation.cpp
rg -nP -C 8 '\bMaybeUpdateMempoolForReorg\s*\(' src/validation.cpp
rg -nP -C 6 '\bMempoolMutex\s*\(' src/validation.cpp src/validation.hRepository: dashpay/dash Length of output: 150 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- validation.cpp locations ---'
rg -n -C 12 'HandleSnapshotStateMismatch|MaybeUpdateMempoolForReorg|ActivateBestChainStep|MempoolMutex|m_mempool' src/validation.cpp src/validation.h | head -n 1200
printf '%s\n' '--- relevant source ranges ---'
sed -n '3000,3420p' src/validation.cpp
sed -n '5680,5785p' src/validation.cpp
printf '%s\n' '--- declarations and call sites ---'
rg -n -C 8 'MaybeUpdateMempoolForReorg|HandleSnapshotStateMismatch|ActivateBestChainStep' src --glob '*.{cpp,h}'Repository: dashpay/dash Length of output: 50369 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- ConnectTip and ActivateBestChainStep ---'
sed -n '3025,3105p' src/validation.cpp
sed -n '3215,3305p' src/validation.cpp
sed -n '3360,3475p' src/validation.cpp
printf '%s\n' '--- snapshot mismatch implementation and callers ---'
rg -n -C 30 'HandleSnapshotStateMismatch|MaybeCompleteSnapshotValidation|ReleaseSnapshotPruneLock|m_active_chainstate|m_ibd_chainstate|m_snapshot_chainstate' src/validation.cpp | tail -n 500
printf '%s\n' '--- lock macro definitions and nullable-lock behavior ---'
rg -n -C 12 '`#define` LOCK|class?UniqueLock|UniqueLock|MempoolMutex\(\)' src sync | head -n 500Repository: dashpay/dash Length of output: 50368 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- lock headers and macros ---'
git ls-files | rg '(^|/)(sync|lock|thread).*\.(h|cpp)$|lock'
rg -n -C 15 '`#define` (LOCK|LOCK2|WITH_LOCK)|MakeUniqueLock|UniqueLock<|class UniqueLock|AssertLockHeld' src --glob '*.{h,cpp}' | head -n 1000
printf '%s\n' '--- tests for mismatch and shutdown handoff ---'
rg -n -C 20 'HandleSnapshotStateMismatch|SnapshotStateMismatch|invalid assumeutxo|m_mempool.*nullptr|GetMempool\(\)' src/test --glob '*.{cpp,h}' | head -n 1200
printf '%s\n' '--- exact declarations and construction paths ---'
sed -n '430,510p' src/validation.h
sed -n '1070,1100p' src/validation.h
rg -n -C 15 'MakeChainstate|m_ibd_chainstate\s*=|m_ibd_chainstate\)|new Chainstate|Chainstate\(' src/validation.cpp src/*.cpp src/*.h | head -n 1000Repository: dashpay/dash Length of output: 50369 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '175,325p' src/sync.h
printf '%s\n' '--- mismatch-specific tests ---'
rg -n -C 30 'HandleSnapshotStateMismatch|SnapshotStateMismatch|invalid assumeutxo|evo state mismatch' src/test --glob '*.{cpp,h}'
printf '%s\n' '--- snapshot completion handoff and mempool use ---'
sed -n '517,620p' src/evo/snapshot_load.cpp
rg -n -C 12 'm_mempool|MempoolMutex|MaybeUpdateMempoolForReorg' src/evo/snapshot_load.cpp src/test/validation_chainstatemanager_tests.cppRepository: dashpay/dash Length of output: 19783 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
validation = Path("src/validation.cpp").read_text()
sync = Path("src/sync.h").read_text()
header = Path("src/validation.h").read_text()
def section(text, start, end):
a = text.index(start)
b = text.index(end, a)
return text[a:b]
activate = section(validation, "bool Chainstate::ActivateBestChainStep", "static SynchronizationState")
mismatch = section(validation, "bool ChainstateManager::HandleSnapshotStateMismatch", "Chainstate& ChainstateManager::ActiveChainstate")
mempool_mutex = section(header, "RecursiveMutex* MempoolMutex()", "private:")
assert "LOCK(MempoolMutex());" in section(validation, "bool Chainstate::ActivateBestChain", "bool Chainstate::PreciousBlock")
assert "if (!pmutexIn) return;" in sync
assert "m_ibd_chainstate->m_mempool = m_snapshot_chainstate->m_mempool;" in mismatch
assert "m_snapshot_chainstate->m_mempool = nullptr;" in mismatch
assert "MaybeUpdateMempoolForReorg(disconnectpool, false);" in activate
handoff = mismatch.index("m_ibd_chainstate->m_mempool =")
failure_cleanup = activate.index("MaybeUpdateMempoolForReorg(disconnectpool, false);")
assert handoff >= 0 and failure_cleanup >= 0
print("MempoolMutex() returns m_mempool ? &m_mempool->cs : nullptr:", "m_mempool ? &m_mempool->cs : nullptr" in mempool_mutex)
print("LOCK(nullptr) constructs without owning a lock:", "if (!pmutexIn) return;" in sync)
print("Mismatch hands off mempool before failed-step cleanup:", True)
print("ActivateBestChainStep performs failed-step cleanup after ConnectTip:", True)
PYRepository: dashpay/dash Length of output: 403 Keep the mempool mutex held across the mempool handoff. 🤖 Prompt for AI Agents |
||
|
|
||
|
|
@@ -6406,7 +6415,10 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( | |
| } | ||
|
|
||
| // The base block is necessarily available after background validation | ||
| // reaches it. Complete any CbTx checks that could not run at snapshot load. | ||
| // reaches it. The assumeutxo prune lock is held until this check completes, | ||
| // so the shared BlockManager cannot prune the base out from under the | ||
| // snapshot chainstate. Complete any CbTx checks deferred at snapshot load. | ||
| assert(index_new.nStatus & BLOCK_HAVE_DATA); | ||
| if (DeploymentActiveAt(index_new, GetConsensus(), Consensus::DEPLOYMENT_DIP0003)) { | ||
| evo::CEvoSnapshot retained_snapshot; | ||
| CBlock base_block; | ||
|
|
@@ -6481,6 +6493,7 @@ SnapshotCompletionResult ChainstateManager::MaybeCompleteSnapshotValidation( | |
| snapshot_blockhash.ToString()); | ||
|
|
||
| m_ibd_chainstate->m_disabled = true; | ||
| ReleaseSnapshotPruneLock(); | ||
| this->MaybeRebalanceCaches(); | ||
|
|
||
| return SnapshotCompletionResult::SUCCESS; | ||
|
|
@@ -6608,6 +6621,24 @@ void ChainstateManager::ResetChainstates() | |
| m_active_chainstate = nullptr; | ||
| } | ||
|
|
||
| void ChainstateManager::ProtectSnapshotBaseFromPruning() | ||
| { | ||
| AssertLockHeld(::cs_main); | ||
| const CBlockIndex* base{GetSnapshotBaseBlock()}; | ||
| if (!base) return; | ||
|
|
||
| // The generic prune-lock buffer makes this conservative: automatic and | ||
| // manual pruning both stop below the base, keeping its full block available | ||
| // for Dash's deferred CbTx/evo check at background-validation completion. | ||
| m_blockman.UpdatePruneLock("assumeutxo", {.height_first = base->nHeight}); | ||
| } | ||
|
|
||
| void ChainstateManager::ReleaseSnapshotPruneLock() | ||
| { | ||
| AssertLockHeld(::cs_main); | ||
| m_blockman.DeletePruneLock("assumeutxo"); | ||
| } | ||
|
|
||
| ChainstateManager::~ChainstateManager() | ||
| { | ||
| LOCK(::cs_main); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this commit is a fix for already merged M3 and it is currently broken at develop somehow, let's have a commit
fix: assumeutxo init-order integration (B6)as a new PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split out, with one correction to the premise: develop is not actually broken here —
DeleteSnapshotChainstateFromDisk()already removeschainstate_snapshoton reindex. What survives are_INVALIDand_todelete, plus an ordering wart (cleanup after EvoDB recreation). That standalone piece is now #7588, which also removes the old partial-cleanup function it obsoletes. The prune-lock half of this commit only exists for the completion-time CbTx read introduced later in the series, so it travels with that PR instead of standing alone.🤖 Posted autonomously by Claude on behalf of pasta.