backport: assumeutxo M2 — EvoDB multi-chainstate isolation and subsystem gating - #7456
backport: assumeutxo M2 — EvoDB multi-chainstate isolation and subsystem gating#7456PastaPastaPasta wants to merge 13 commits into
Conversation
|
See https://gist.github.com/PastaPastaPasta/c5108775f7b126f88856cde20d7a8556 for ai generated commit by commit explanation. |
06b5c6f to
1a95697
Compare
|
This pull request has conflicts, please rebase. |
1a95697 to
a3c4476
Compare
a3c4476 to
26c8718
Compare
|
This pull request has conflicts, please rebase. |
26c8718 to
b6edcf8
Compare
11a31da to
5f3a876
Compare
|
that's not my final review feedback, I am not determined yet how exactly improve or change this PR or keep it as it is
There are several components that are logically part of chainstate, but initialized separately and behave differently.
@PastaPastaPasta what is your thoughts on And one more things: you have no plans to make evodb a part of assume-utxo, right? =================
-evodb = std::make_unique<CEvoDB>(db_params);
+m_evodb{std::make_unique<CEvoDB>(db_params, snapshot_base ? "evodb" + std::string{node::SNAPSHOT_CHAINSTATE_SUFFIX} : "evodb")}, |
|
This pull request has conflicts, please rebase. |
5f3a876 to
d47d9b8
Compare
|
This pull request has conflicts, please rebase. |
d47d9b8 to
7fabb1d
Compare
7fabb1d to
bdd5626
Compare
|
@knst Fable's raw response: This is based on the context of the full WIP assumeutxo implementation. I think it answers your questions well. |
|
⛔ Blockers found — Sonnet deferred (commit 993d53b) |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
![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: bdd5626c20
ℹ️ 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".
| if (!(baseBlockIndex->nStatus & BLOCK_HAVE_DATA)) { | ||
| errorRet = strprintf("block data for base block %s is not available (pruned or below an unvalidated snapshot base)", | ||
| baseBlockIndex->GetBlockHash().ToString()); | ||
| return false; |
There was a problem hiding this comment.
When a pruned node receives an MNLISTDIFF request whose base block has been pruned but whose target block is retained, this new check rejects the request even though the base is only used to retrieve EvoDB-backed masternode/quorum state; the only block read performed later is for blockIndex at line 229. The network handler then silently drops an otherwise serviceable request, unnecessarily preventing pruned nodes from serving recent diffs from older bases. Retain the target-block availability check, but do not require BLOCK_HAVE_DATA for baseBlockIndex.
AGENTS.md reference: AGENTS.md:L167-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed. The base block is indeed only used for EvoDB-backed masternode/quorum state (which pruned nodes retain independent of block files), and the only disk read is for the target block's cbTx/merkle tree. The snapshot case this check was named for is already handled more precisely by the exception path: GetListForBlock fails with the IsBlockDataUnavailableError sentinel when a snapshot node hasn't validated the base yet, so the request is still dropped without penalizing the peer.
The same reasoning applied one layer up: BuildQuorumRotationInfo had the identical BLOCK_HAVE_DATA requirement on the caller-supplied baseBlockHashes, which are used exclusively as inner-diff bases — removed as well. The cycle/work/tip checks there remain, since those blocks are inner-diff targets and do get read from disk.
Both removals are amended into the introducing commit (dash: guard serving unavailable snapshot history) with comments documenting why bases need no availability check.
🤖 Posted autonomously by Claude on behalf of pasta.
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (44)
WalkthroughThe change adds NORMAL and SNAPSHOT EvoDB identities with isolated transactions, best-block markers, derived-state consistency checks, and dual-chainstate markers. Chain-aware context now flows through validation, special transactions, quorum lookup, and commitment processing. Snapshot detection validates EvoDB markers and disables DKG and quorum signing during background validation. Persistence mismatches become explicit errors. Block-data availability failures avoid peer penalties. Tests cover EvoDB behavior, snapshot lifecycle, quorum state, persistence, and deterministic serialization. Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ChainstateManager
participant ActiveContext
participant DKGSessionHandler
participant CSigSharesManager
ChainstateManager->>ActiveContext: Report active unvalidated snapshot
ActiveContext->>DKGSessionHandler: Suppress DKG updates
ActiveContext->>CSigSharesManager: Block quorum signing
ChainstateManager->>ActiveContext: Report validation complete
ActiveContext->>DKGSessionHandler: Resume duty updates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/active/masternode.cpp (1)
112-138: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
InitInternalstill callsGetListForBlockwithout the new exception guard.
UpdatedBlockTip(Lines 181-193) now catches exceptions fromGetListForBlock, because this callback runs on the scheduler thread and an uncaught exception there terminates the node.InitInternalat Line 132 callsm_dmnman.GetListForBlock(pindex)without that same guard.
InitInternalis reachable from the same scheduler-thread call chain:UpdatedBlockTipcallsInit(pindexNew)at Line 224 when the manager is notREADY, and calls theresetlambda at Line 200 from inside theREADYbranch, andresetcallsInitInternal(pindexNew)directly. Both paths can still crash the node on the same failure mode the surrounding fix targets.Apply the same try/catch pattern to the
GetListForBlockcall insideInitInternal.🛡️ Proposed fix
- CDeterministicMNList mnList = m_dmnman.GetListForBlock(pindex); + CDeterministicMNList mnList; + try { + mnList = m_dmnman.GetListForBlock(pindex); + } catch (const std::exception& e) { + // GetListForBlock throws when list data is unavailable. This runs on + // the scheduler thread, where an uncaught exception terminates the + // node; skip this init attempt instead and let the next one retry. + LogPrintf("CActiveMasternodeManager::%s -- masternode list unavailable: %s\n", __func__, e.what()); + m_state = MasternodeState::SOME_ERROR; + return; + }Also applies to: 181-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/active/masternode.cpp` around lines 112 - 138, Guard the m_dmnman.GetListForBlock call in CActiveMasternodeManager::InitInternal with the same exception-handling pattern used by UpdatedBlockTip. Catch failures, preserve the existing initialization error/reset behavior, and ensure exceptions do not escape the scheduler-thread call chain.src/evo/deterministicmns.cpp (1)
640-693: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCatch missing masternode-list diffs before marking blocks consensus-invalid.
GetListForBlockInternalthrows a plainstd::runtime_errorwith the “is not available” sentinel when a DIP3-active diff is missing in a dual-chainstate run.ProcessBlockcalls this beforeBuildDiff, so the exception is caught by the genericcatch (const std::exception&)at line 690 and converted tostate.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-dmn-block"). UseEvoDbInconsistencyErrorfor this condition, or another distinguishable exception type, and route it tostate.Error(...). Also catch it in callers such asBuildSimplifiedDiffso P2P serving does not treat it as a hard failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/deterministicmns.cpp` around lines 640 - 693, Introduce a distinguishable EvoDB inconsistency exception for the “is not available” missing masternode-list diff raised by GetListForBlockInternal, and handle it in ProcessBlock by calling state.Error(...) instead of marking the block consensus-invalid. Update callers such as BuildSimplifiedDiff to catch the same exception and return an appropriate non-fatal failure for P2P serving, while preserving existing handling for unrelated std::exception failures.src/llmq/quorumsman.h (1)
167-184: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe "cs_main-free" comment is now inaccurate, and the pointer overload lacks the lock annotation.
Line 167 states that all private methods below are cs_main-free. The new private
GetQuorumat lines 182-184 declaresEXCLUSIVE_LOCKS_REQUIRED(::cs_main), which contradicts that statement.The private
ScanQuorumsoverload at lines 168-171 is annotated as cs_main-free, but it accepts a non-nullchain. On that path it performs chain-membership checks that require::cs_main. The public wrapper at lines 135-138 does require::cs_main, so the current call path is safe, and the annotation does not describe the actual requirement. A future caller could pass a non-null chain without holding::cs_mainand the analyzer would not report it.Update the comment, and document the conditional lock requirement on the pointer overload.
📝 Proposed comment and annotation update
private: - // all private methods here are cs_main-free + // Private methods here are cs_main-free unless annotated otherwise. + //! When `chain` is non-null the caller must hold ::cs_main; the chain-membership + //! checks on that path require it. The annotation cannot express this + //! conditionally, so it is stated here. std::vector<CQuorumCPtr> ScanQuorums(Consensus::LLMQType llmqType, gsl::not_null<const CBlockIndex*> pindexStart, size_t nCountRequested, const CChain* chain) const EXCLUSIVE_LOCKS_REQUIRED(!cs_db, !m_cs_maps, !m_cache_cs);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/quorumsman.h` around lines 167 - 184, Update the private-method comment above ScanQuorums to remove the claim that all methods are cs_main-free. Add a conditional ::cs_main lock annotation to the pointer-based ScanQuorums overload so calls with a non-null chain require the lock, while preserving the existing lock annotations and the separate GetQuorum overload requirements.
🧹 Nitpick comments (8)
src/evo/evodb.cpp (1)
96-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDerive the non-NORMAL marker key from the identity value.
ReadBestBlockandWriteBestBlockuse the constantuint8_t{1}for every non-NORMAL identity. Today onlySNAPSHOTexists, so the behavior is correct. If a third identity is added, it silently shares theSNAPSHOTmarker key.Encoding the identity in the key removes that failure mode and keeps the legacy
NORMALkey bytes unchanged.♻️ Proposed refactor
- return transaction.Read(std::make_pair(EVODB_BEST_BLOCK, uint8_t{1}), hash); + return transaction.Read(std::make_pair(EVODB_BEST_BLOCK, static_cast<uint8_t>(identity)), hash);Apply the matching change in
WriteBestBlock.EvoDbIdentity::SNAPSHOTis1, so the on-disk key bytes do not change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/evodb.cpp` around lines 96 - 123, Update ReadBestBlock and WriteBestBlock to derive the non-NORMAL marker key from the identity value instead of using the hardcoded uint8_t{1}; preserve the existing EVODB_BEST_BLOCK key for EvoDbIdentity::NORMAL and ensure EvoDbIdentity::SNAPSHOT continues encoding as 1.src/evo/evodb.h (1)
146-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the serialize-and-compare logic into a helper.
The same five lines build two
CDataStreamobjects and compare them twice. A small static helper removes the duplication and re-serializesvalueonly once.♻️ Proposed refactor
+ CDataStream value_stream{SER_DISK, CLIENT_VERSION}; + value_stream << value; + const auto matches_value = [&](const V& other) { + CDataStream other_stream{SER_DISK, CLIENT_VERSION}; + other_stream << other; + return other_stream.size() == value_stream.size() && + std::equal(other_stream.begin(), other_stream.end(), value_stream.begin()); + }; + V existing; bool write{true}; if (transaction.Read(key, existing)) { - CDataStream existing_stream{SER_DISK, CLIENT_VERSION}; - CDataStream value_stream{SER_DISK, CLIENT_VERSION}; - existing_stream << existing; - value_stream << value; - const bool matches = existing_stream.size() == value_stream.size() && - std::equal(existing_stream.begin(), existing_stream.end(), value_stream.begin()); - if (!matches) { + if (!matches_value(existing)) { LogPrintf("ERROR: CEvoDB::WriteDerived: block-derived payload mismatch in EvoDB\n"); return false; } write = false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/evodb.h` around lines 146 - 181, Extract the duplicated serialization and equality check from CEvoDB::WriteDerived into a small static helper that accepts the existing or pending value and value, serializes the value once, and returns whether their serialized payloads match. Replace both local comparison blocks with calls to this helper while preserving the existing mismatch handling.src/llmq/quorumsman.cpp (1)
264-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert
cs_mainin the explicit-chain branch.The private
ScanQuorumsoverload at Line 177 carries noEXCLUSIVE_LOCKS_REQUIRED(::cs_main)annotation, soNO_THREAD_SAFETY_ANALYSISis needed to call the annotatedGetQuorum. The lock is held only because the sole caller that passes a non-nullchainis the reference overload at Line 169. That invariant is not expressed in this function.Add a runtime assertion so a future caller cannot break the invariant silently.
♻️ Proposed change
CQuorumCPtr quorum; if (chain) { + AssertLockHeld(::cs_main); quorum = [&]() NO_THREAD_SAFETY_ANALYSIS { return GetQuorum(llmqType, pQuorumBaseBlockIndex, *chain, populate_cache); }(); } else { quorum = GetQuorum(llmqType, pQuorumBaseBlockIndex, populate_cache); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/quorumsman.cpp` around lines 264 - 271, Add a runtime cs_main lock assertion in the explicit-chain branch of the private ScanQuorums overload, immediately before invoking the annotated GetQuorum through the NO_THREAD_SAFETY_ANALYSIS lambda. Preserve the existing no-chain path and quorum lookup behavior.src/test/validation_chainstatemanager_tests.cpp (1)
816-821: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a named EvoDB helper instead of the raw snapshot marker key.
The test builds the snapshot best-block key as
std::make_pair(EVODB_BEST_BLOCK, uint8_t{1}). This duplicates the key layout thatCEvoDBowns. If the identity suffix encoding changes, the test erases nothing and then fails for an unrelated reason.Expose an erase or key helper on
CEvoDBkeyed byEvoDbIdentity, and call it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/validation_chainstatemanager_tests.cpp` around lines 816 - 821, Replace the raw std::make_pair(EVODB_BEST_BLOCK, uint8_t{1}) erase in the snapshot transaction with a CEvoDB helper that accepts EvoDbIdentity::SNAPSHOT and encapsulates the best-block key layout. Add or reuse the named erase/key helper on CEvoDB, then call it from this test while preserving the existing transaction and commit flow.src/llmq/blockprocessor.cpp (1)
68-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: reuse one serialized-comparison helper.
SerializedEqualrepeats the byte-wise comparison thatCEvoDB::WriteDerivedperforms insrc/evo/evodb.h(lines 148-181). Move one shared helper into a common header, for example next toCDataStream, and call it from both places. That keeps the serialization parameters (SER_DISK,CLIENT_VERSION) in one location.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/blockprocessor.cpp` around lines 68 - 77, Move the serialization-based equality logic from SerializedEqual into a shared helper in a common header near CDataStream, preserving SER_DISK and CLIENT_VERSION there. Update both SerializedEqual and CEvoDB::WriteDerived to call the shared helper and remove their duplicated byte-comparison implementations.src/llmq/blockprocessor.h (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: confirm the const relaxation on
m_chainmanis required.The member and the constructor parameter changed from
const ChainstateManager&toChainstateManager&. The uses visible insrc/llmq/blockprocessor.cppareActiveChainstate(),ActiveChain(),GetConsensus(), andm_blockman, which are const-callable. If a non-const member is needed only forGetAll()throughEraseMinedCommitmentIfUnreferenced, note that this free function receives theChainstatedirectly and does not usem_chainman. Keep the const reference if nothing requires mutation.Also applies to: 88-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/blockprocessor.h` at line 57, Revert m_chainman and its constructor parameter to const ChainstateManager& unless the BlockProcessor implementation has a demonstrated need to mutate ChainstateManager. Keep the existing const-callable uses—ActiveChainstate(), ActiveChain(), GetConsensus(), and m_blockman—unchanged, since EraseMinedCommitmentIfUnreferenced operates on Chainstate directly.src/validation.cpp (1)
1628-1634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making
EvoDbInconsistencyMessage()const.The method only reads
m_chainmanandm_evoDb. A const qualifier documents that and allows calls from const contexts.GetAll()andHasDualChainstateMarker()must also be const-callable for this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validation.cpp` around lines 1628 - 1634, Make Chainstate::EvoDbInconsistencyMessage() const and update its declaration consistently. Ensure m_chainman.GetAll() and m_evoDb.HasDualChainstateMarker() are const-callable so the method compiles and remains usable from const contexts.src/validation.h (1)
538-541: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: the member name shadows the enum type name.
Chainstate::EvoDbIdentity()has the same name as the global typeEvoDbIdentity. Every use of the type insideChainstatenow needs the::prefix, as the declaration at Line 539 shows. A name such asGetEvoDbIdentity()removes that requirement and reduces the chance of a confusing compile error in future edits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validation.h` around lines 538 - 541, Rename the Chainstate member function EvoDbIdentity() to GetEvoDbIdentity(), updating its declaration, definition, and all call sites while preserving its return value and behavior. This removes the collision with the global EvoDbIdentity type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/active/dkgsessionhandler.cpp`:
- Around line 110-113: Replace the polling calls to
ChainstateManager::IsSnapshotActiveAndUnvalidated in the DKG wait loops with an
atomic or otherwise unlocked snapshot-validation indicator, so the loop never
holds ::cs_main while sleeping. Centralize the repeated check in a small helper
and use that helper at all DKG polling sites, preserving the existing abort
behavior when background validation is incomplete.
In `@src/evo/evodb.cpp`:
- Around line 43-53: Update the non-const CEvoDB::GetContext overload to
construct the TransactionContext before inserting into transaction_contexts, so
a throwing construction cannot leave a null map entry; preserve returning the
existing context for identities already present and keep the const overload
unchanged.
In `@src/evo/evodb.h`:
- Around line 206-213: Synchronize transaction state access in
src/evo/evodb.h:206-213 by making cs mutable, requiring GetMemoryUsage() to
exclude cs on entry, and locking cs before traversing transaction_contexts. In
src/evo/evodb.h:89-99, annotate transaction_contexts, active_transaction, and
m_default_identity with GUARDED_BY(cs), and mark both GetContext overloads and
GetCurrentIdentity with EXCLUSIVE_LOCKS_REQUIRED(cs).
In `@src/evo/specialtxman.cpp`:
- Around line 786-787: Update Chainstate::ConnectBlock in validation.cpp so the
IsBlockValueValid and IsBlockPayeeValid calls use this chainstate’s m_chain
rather than m_chainman.ActiveChain(). Preserve the existing behavior and
arguments for other checks, including the CheckCbTxBestChainlock call shown in
the diff.
In `@src/test/evo_db_tests.cpp`:
- Around line 101-112: Update the EvoDB write path and the test around
db.Write(key2, ...) to disallow transaction-less writes instead of accepting
them through the default identity’s cur_transaction. Ensure writes require an
active BeginTransaction context, while reads may still resolve through the
default identity as intended; remove or revise the assertions that depend on an
unscoped write persisting across identity changes.
In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 1612-1625: The serialization-order test around
CDeterministicMNListDiff must reliably distinguish the two insertion
arrangements instead of assuming keys 1 and 2 produce different unordered_map
iteration orders. Replace the test keys with values known to iterate differently
before the change, or compare forward_stream against a canonical stream
explicitly ordered by internalId while preserving the existing serialization
comparison.
In `@src/validation.cpp`:
- Around line 5891-5894: Update DetectSnapshotChainstate handling of
ReadSnapshotBaseBlockhash so a missing or unreadable base_blockhash fails
startup instead of returning true. Preserve the existing malformed-directory log
as the failure error, and add a separate error report for I/O or trailing-data
failures before returning failure from ValidateChainstateOnStartup.
---
Outside diff comments:
In `@src/active/masternode.cpp`:
- Around line 112-138: Guard the m_dmnman.GetListForBlock call in
CActiveMasternodeManager::InitInternal with the same exception-handling pattern
used by UpdatedBlockTip. Catch failures, preserve the existing initialization
error/reset behavior, and ensure exceptions do not escape the scheduler-thread
call chain.
In `@src/evo/deterministicmns.cpp`:
- Around line 640-693: Introduce a distinguishable EvoDB inconsistency exception
for the “is not available” missing masternode-list diff raised by
GetListForBlockInternal, and handle it in ProcessBlock by calling
state.Error(...) instead of marking the block consensus-invalid. Update callers
such as BuildSimplifiedDiff to catch the same exception and return an
appropriate non-fatal failure for P2P serving, while preserving existing
handling for unrelated std::exception failures.
In `@src/llmq/quorumsman.h`:
- Around line 167-184: Update the private-method comment above ScanQuorums to
remove the claim that all methods are cs_main-free. Add a conditional ::cs_main
lock annotation to the pointer-based ScanQuorums overload so calls with a
non-null chain require the lock, while preserving the existing lock annotations
and the separate GetQuorum overload requirements.
---
Nitpick comments:
In `@src/evo/evodb.cpp`:
- Around line 96-123: Update ReadBestBlock and WriteBestBlock to derive the
non-NORMAL marker key from the identity value instead of using the hardcoded
uint8_t{1}; preserve the existing EVODB_BEST_BLOCK key for EvoDbIdentity::NORMAL
and ensure EvoDbIdentity::SNAPSHOT continues encoding as 1.
In `@src/evo/evodb.h`:
- Around line 146-181: Extract the duplicated serialization and equality check
from CEvoDB::WriteDerived into a small static helper that accepts the existing
or pending value and value, serializes the value once, and returns whether their
serialized payloads match. Replace both local comparison blocks with calls to
this helper while preserving the existing mismatch handling.
In `@src/llmq/blockprocessor.cpp`:
- Around line 68-77: Move the serialization-based equality logic from
SerializedEqual into a shared helper in a common header near CDataStream,
preserving SER_DISK and CLIENT_VERSION there. Update both SerializedEqual and
CEvoDB::WriteDerived to call the shared helper and remove their duplicated
byte-comparison implementations.
In `@src/llmq/blockprocessor.h`:
- Line 57: Revert m_chainman and its constructor parameter to const
ChainstateManager& unless the BlockProcessor implementation has a demonstrated
need to mutate ChainstateManager. Keep the existing const-callable
uses—ActiveChainstate(), ActiveChain(), GetConsensus(), and
m_blockman—unchanged, since EraseMinedCommitmentIfUnreferenced operates on
Chainstate directly.
In `@src/llmq/quorumsman.cpp`:
- Around line 264-271: Add a runtime cs_main lock assertion in the
explicit-chain branch of the private ScanQuorums overload, immediately before
invoking the annotated GetQuorum through the NO_THREAD_SAFETY_ANALYSIS lambda.
Preserve the existing no-chain path and quorum lookup behavior.
In `@src/test/validation_chainstatemanager_tests.cpp`:
- Around line 816-821: Replace the raw std::make_pair(EVODB_BEST_BLOCK,
uint8_t{1}) erase in the snapshot transaction with a CEvoDB helper that accepts
EvoDbIdentity::SNAPSHOT and encapsulates the best-block key layout. Add or reuse
the named erase/key helper on CEvoDB, then call it from this test while
preserving the existing transaction and commit flow.
In `@src/validation.cpp`:
- Around line 1628-1634: Make Chainstate::EvoDbInconsistencyMessage() const and
update its declaration consistently. Ensure m_chainman.GetAll() and
m_evoDb.HasDualChainstateMarker() are const-callable so the method compiles and
remains usable from const contexts.
In `@src/validation.h`:
- Around line 538-541: Rename the Chainstate member function EvoDbIdentity() to
GetEvoDbIdentity(), updating its declaration, definition, and all call sites
while preserving its return value and behavior. This removes the collision with
the global EvoDbIdentity type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8a257e2-bb5f-49b5-9865-417e52e11877
📒 Files selected for processing (44)
src/Makefile.test.includesrc/active/context.cppsrc/active/context.hsrc/active/dkgsessionhandler.cppsrc/active/masternode.cppsrc/dbwrapper.hsrc/evo/assetlocktx.cppsrc/evo/assetlocktx.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/creditpool.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/evodb.cppsrc/evo/evodb.hsrc/evo/mnhftx.cppsrc/evo/mnhftx.hsrc/evo/smldiff.cppsrc/evo/smldiff.hsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/governance/signing.cppsrc/llmq/blockprocessor.cppsrc/llmq/blockprocessor.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.hsrc/llmq/snapshot.cppsrc/net_processing.cppsrc/node/chainstate.cppsrc/node/miner.cppsrc/rpc/blockchain.cppsrc/rpc/masternode.cppsrc/rpc/quorums.cppsrc/test/evo_cbtx_tests.cppsrc/test/evo_db_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstatemanager_tests.cppsrc/validation.cppsrc/validation.hsrc/versionbits.h
| // Transaction-less writes land in the default identity's overlay and stay | ||
| // invisible to the other identity. | ||
| const auto key2 = PayloadKey(8); | ||
| db.Write(key2, PayloadFor(8)); | ||
| db.SetDefaultIdentity(EvoDbIdentity::NORMAL); | ||
| Payload value2; | ||
| BOOST_CHECK(!db.Read(key2, value2)); | ||
| { | ||
| auto tx = db.BeginTransaction(EvoDbIdentity::SNAPSHOT); | ||
| BOOST_REQUIRE(db.Read(key2, value2)); | ||
| BOOST_CHECK(value2 == PayloadFor(8)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Clear() semantics and find production transaction-less writes.
ast-grep run --pattern 'void Clear() { $$$ }' --lang cpp src/dbwrapper.h
rg -nP --type=cpp -C3 '\bevoDb?\.\s*Write\s*\(' -g '!src/test/**'Repository: dashpay/dash
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files matching evo/db names =="
git ls-files | rg '(^|/)(evo_db_tests|evodb|evodb|dbwrapper)\.(cpp|h|hpp)$|[^/]evo_db_tests\.cpp$|[^/].*evo.*\.cpp$|[^/].*evo.*\.h$' | sed -n '1,120p'
echo
echo "== line counts for likely files =="
while IFS= read -r f; do
wc -l "$f"
done < <(git ls-files | rg '(^|/)(evo_db_tests\.cpp|evodb\.cpp|evodb\.h|dbwrapper\.h)$')
echo
echo "== search Write/Read/BeginTransaction/RollbackCurTransaction in relevant files =="
python3 - <<'PY'
from pathlib import Path
terms = ['Write(', 'Read(', 'BeginTransaction(', 'RollbackCurTransaction', 'Clear()']
files = [Path(f) for f in __import__('subprocess').check_output(['git','ls-files'], text=True).splitlines() if f.endswith(('.cpp','.h','.hpp'))]
for term in terms:
print(f"\n-- {term!r} --")
for p in files:
try:
txt = p.read_text(errors='ignore').splitlines()
except Exception:
continue
hits = [i for i,l in enumerate(txt,1) if term in l]
if hits:
print(p)
for i in hits[:20]:
lo=max(1,i-3); hi=min(len(txt),i+3)
print(f" lines {lo}-{hi}:")
for j in range(lo,hi+1):
print(f" {j}: {txt[j-1]}")
PYRepository: dashpay/dash
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -u
echo "== concise search for CEvoDB methods and Clear transactions outside tests =="
python3 - <<'PY'
from pathlib import Path
import re
files = [Path(f) for f in __import__('subprocess').check_output(['git','ls-files'], text=True).splitlines()
if f.endswith(('.cpp','.h')) and not f.startswith('src/test/') and ('evo' in f.lower() or 'evo' in Path(f).stem.lower())]
terms = ['RollbackCurTransaction', 'Clear()']
for term in terms:
print(f"\n-- {term!r} --")
for p in files:
txt = p.read_text(errors='ignore').splitlines()
hits = [i for i,l in enumerate(txt,1) if term in l]
if hits:
print(p)
for i in hits:
lo=max(1,i-8); hi=min(len(txt),i+12)
print(f" lines {lo}-{hi}:")
for j in range(lo,hi+1):
print(f" {j}: {txt[j-1]}")
PY
echo
echo "== src/evo/evodb.cpp relevant section =="
wc -l src/evo/evodb.cpp src/evo/evodb.h src/test/evo_db_tests.cpp
sed -n '1,190p' src/evo/evodb.cpp
sed -n '80,120p' src/evo/evodb.h
sed -n '88,116p' src/test/evo_db_tests.cpp
echo
echo "== production calls to Write/BeginTransaction/RollbackCurTransaction/Commit in source excluding tests =="
rg -n --type=cpp -C2 '\b(Write|BeginTransaction|RollbackCurTransaction|Commit)\s*\(' -g '!src/test/**' | rg -n 'src/(evo|.*evo).*|src/.*\.cpp$' | sed -n '1,220p'Repository: dashpay/dash
Length of output: 30000
🏁 Script executed:
#!/bin/bash
set -u
echo "== locate TransactionContext and CurTransaction declarations =="
rg -n -C4 'struct TransactionContext|using TransactionContext|struct CurTransaction|class CurTransaction|CurTransaction::Clear|template.*class.*CurTransaction' src/evo src/dbwrapper.cpp src/dbwrapper.h
echo
echo "== evo evodb.h transaction-related section =="
sed -n '1,180p' src/evo/evodb.h
echo
echo "== dbwrapper transaction write/rollback methods =="
sed -n '480,690p' src/dbwrapper.h
echo
echo "== concise production evo Write usages outside tests =="
rg -n -C2 '\bevoDb?\.(Write|Exists|GetCurTransaction)\s*\(' -g '!src/test/**' | sed -n '1,220p'Repository: dashpay/dash
Length of output: 14541
🏁 Script executed:
#!/bin/bash
set -u
echo "== CDBTransaction remaining methods in dbwrapper.h =="
sed -n '520,680p' src/dbwrapper.h
echo
echo "== CDBTransaction Clear implementation =="
rg -n -C6 'void Clear\(\)|~CDBTransaction\(' src/dbwrapper.hRepository: dashpay/dash
Length of output: 6290
Disallow transaction-less writes in EvoDB.
Write() outside BeginTransaction writes to the current identity’s cur_transaction. The transaction destructor rolls back by clearing the entire CDBTransaction, including writes that were never covered by that scoped transaction. Line 104 demonstrates that the test accepts this path, but production code avoids transaction-less writes. Remove transaction-less write support or make only reads resolve through the default identity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/evo_db_tests.cpp` around lines 101 - 112, Update the EvoDB write
path and the test around db.Write(key2, ...) to disallow transaction-less writes
instead of accepting them through the default identity’s cur_transaction. Ensure
writes require an active BeginTransaction context, while reads may still resolve
through the default identity as intended; remove or revise the assertions that
depend on an unscoped write persisting across identity changes.
bdd5626 to
e84c379
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/llmq/snapshot.cpp`:
- Line 43: Remove the CheckBlockDataAvailable guard on ret.m_cycle_index in
snapshot construction, so EvoDB-backed snapshot metadata lookups are not gated
by BLOCK_HAVE_DATA. Retain the existing block-data check for actual block-read
targets such as BuildSimplifiedMNListDiff, and preserve snapshot construction
when the requested metadata and snapshots are available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 889a4adf-b581-423c-aa9d-80eb2623e26d
📒 Files selected for processing (31)
src/active/context.cppsrc/active/context.hsrc/active/dkgsessionhandler.cppsrc/active/masternode.cppsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/creditpool.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/evodb.cppsrc/evo/evodb.hsrc/evo/mnhftx.cppsrc/evo/smldiff.cppsrc/evo/smldiff.hsrc/evo/specialtxman.cppsrc/governance/signing.cppsrc/llmq/blockprocessor.cppsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.hsrc/llmq/snapshot.cppsrc/net_processing.cppsrc/node/miner.cppsrc/rpc/blockchain.cppsrc/rpc/masternode.cppsrc/rpc/quorums.cppsrc/test/evo_db_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cppsrc/test/validation_chainstatemanager_tests.cppsrc/validation.cppsrc/versionbits.h
🚧 Files skipped from review as they are similar to previous changes (28)
- src/llmq/signing_shares.h
- src/versionbits.h
- src/test/evo_deterministicmns_tests.cpp
- src/rpc/blockchain.cpp
- src/governance/signing.cpp
- src/evo/chainhelper.h
- src/evo/chainhelper.cpp
- src/rpc/masternode.cpp
- src/active/context.cpp
- src/evo/smldiff.h
- src/test/evo_db_tests.cpp
- src/evo/creditpool.cpp
- src/node/miner.cpp
- src/net_processing.cpp
- src/evo/deterministicmns.h
- src/active/context.h
- src/active/masternode.cpp
- src/rpc/quorums.cpp
- src/evo/mnhftx.cpp
- src/llmq/signing_shares.cpp
- src/evo/deterministicmns.cpp
- src/evo/specialtxman.cpp
- src/active/dkgsessionhandler.cpp
- src/test/util/setup_common.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/llmq/blockprocessor.cpp
- src/validation.cpp
- src/evo/evodb.h
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e84c379ee0
ℹ️ 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".
| // (for multiple chainstates to actually work in parallel) | ||
| // and not a global | ||
| if (&chainman.ActiveChainstate() == chainstate && !evodb->CommitRootTransaction()) { | ||
| if (&chainman.ActiveChainstate() == chainstate && !evodb->CommitRootTransaction(chainstate->EvoDbIdentity())) { |
There was a problem hiding this comment.
When startup replays an interrupted coins flush for the background chainstate, ReplayBlocks() immediately flushes the repaired coins DB but only commits the corresponding EvoDB transaction into that identity's in-memory root overlay. This active-chain condition then skips the physical EvoDB batch write, and committing the snapshot identity does not flush the newly isolated NORMAL overlay. If the process stops again before the background chainstate performs a later full flush, the next startup sees a consistent coins DB but a stale NORMAL EvoDB marker and reports an EvoDB inconsistency requiring reindexing. Commit each chainstate's own root transaction after replay, not only the active one.
AGENTS.md reference: AGENTS.md:L167-L167
Useful? React with 👍 / 👎.
e84c379 to
8d7b52a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d7b52ab95
ℹ️ 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".
| EvoDbIdentity CEvoDB::GetCurrentIdentity() const | ||
| { | ||
| return active_transaction.value_or(m_default_identity); |
There was a problem hiding this comment.
While the background chainstate has a NORMAL transaction open, this process-wide value makes every concurrent transaction-less EvoDB read use NORMAL instead of the active snapshot's default identity. For example, CQuorumManager::GetQuorum releases cs_main after its membership check and BuildQuorumFromCommitment subsequently calls GetMinedCommitment; if background validation starts between those operations, snapshot-only records still pending in the SNAPSHOT overlay disappear from that lookup and the active node intermittently reports a missing quorum. Track the transaction identity per validation execution context, or require explicit identities, without overriding unrelated transaction-less consumers.
AGENTS.md reference: AGENTS.md:L163-L168
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The chain-aware EvoDB changes are generally well structured, but startup has a confirmed recovery regression: both reindex modes erase the snapshot EvoDB marker before requiring it, so a node with a persisted snapshot chainstate cannot reindex. The commit stack also contains six uncompilable bisect points due to stale txindex fixture calls; two additional history rewrites would keep corrective changes atomic with the behavior they fix.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s)
3 additional finding(s) omitted (not in diff).
🤖 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/node/chainstate.cpp`:
- [BLOCKING] src/node/chainstate.cpp:57-71: Reindex cannot recover a node with a persisted snapshot chainstate
`LoadChainstate()` constructs `CEvoDB` with `wipe = true` for both `-reindex` and `-reindex-chainstate`, which removes the SNAPSHOT best-block marker. The `chainstate_snapshot` directory and its base-blockhash file remain on disk, so `DetectSnapshotChainstate()` finds the directory and `ActivateExistingSnapshot()` immediately rejects it because the marker was just erased. Startup exits before the snapshot coins database can be wiped, making the error's instruction to reindex ineffective on every retry. Explicit reindexing must remove the persisted snapshot chainstate artifacts, or otherwise avoid enforcing the erased marker while coherently discarding that chainstate, and a regression test should cover reindex with `chainstate_snapshot` present.
In `<commit:ae5e18f>`:
- [BLOCKING] <commit:ae5e18f>:1: Fold the delayed txindex cleanup into the test commit
Commit `f77de21` adds `TxIndex(1 << 20, true)` and `Start(restarted.ActiveChainstate())`, but at that commit `TxIndex` requires a `std::unique_ptr<interfaces::Chain>` as its first argument and `BaseIndex::Start()` takes no arguments. The calls therefore do not compile, leaving all six commits from `f77de21` through `92b2b98` unusable bisect points until `ae5e18f` removes them. Rewrite `f77de21` without the stale txindex restart code. The `govman` teardown-order change in `ae5e18f` is independent and should be folded into the fixture-lifetime commit that requires it or retained as a separately described teardown fix.
In `<commit:d596742>`:
- [SUGGESTION] <commit:d596742>:1: Squash the WriteDerived error-classification corrections
Commit `f75031e` adds the `WriteDerived` call sites but initially reports mismatches as consensus failures or throws generic exceptions that enclosing catches convert to consensus failures. Commit `d596742` changes these newly introduced paths to node-abort/M_ERROR handling, and `d0cda00` then fixes the remaining `GetForBlock` rethrow that lost this classification. Squash both corrective commits into `f75031e` so the feature is introduced with the final error semantics and no intermediate commit can persistently mark a valid block failed or penalize its relaying peer.
In `<commit:8d7b52a>`:
- [SUGGESTION] <commit:8d7b52a>:1: Keep the new masternode-list throw and its callers atomic
Commit `4a3c1fd` makes `GetListForBlockInternal()` throw when a DIP3-active list diff is unavailable in a dual-chainstate run, while `CActiveMasternodeManager::UpdatedBlockTip()` and `GovernanceSigner::CreateGovernanceTrigger()` still call it without catches in that commit. These callbacks execute through the scheduler-backed validation interface, so the intermediate commit can terminate the node. Move the throw and the two catches from `8d7b52a` into one atomic commit, or squash `8d7b52a` into `4a3c1fd` and update the subject to cover both the new failure mode and its caller handling.
| chainman.InitializeChainstate(options.mempool, *evodb, chain_helper); | ||
|
|
||
| // Load a chain created from a UTXO snapshot, if any exist. | ||
| chainman.DetectSnapshotChainstate(options.mempool); | ||
| bilingual_str snapshot_error; | ||
| if (!chainman.DetectSnapshotChainstate(options.mempool, snapshot_error)) { | ||
| return {ChainstateLoadStatus::FAILURE, snapshot_error}; |
There was a problem hiding this comment.
🔴 Blocking: Reindex cannot recover a node with a persisted snapshot chainstate
LoadChainstate() constructs CEvoDB with wipe = true for both -reindex and -reindex-chainstate, which removes the SNAPSHOT best-block marker. The chainstate_snapshot directory and its base-blockhash file remain on disk, so DetectSnapshotChainstate() finds the directory and ActivateExistingSnapshot() immediately rejects it because the marker was just erased. Startup exits before the snapshot coins database can be wiped, making the error's instruction to reindex ineffective on every retry. Explicit reindexing must remove the persisted snapshot chainstate artifacts, or otherwise avoid enforcing the erased marker while coherently discarding that chainstate, and a regression test should cover reindex with chainstate_snapshot present.
source: ['codex']
Pass the validating chainstate through special transaction and quorum commitment processing instead of borrowing the active chainstate. Interpret mined-commitment records and quorum resolution relative to the caller's chain. The cached values remain reusable, but chain membership is reevaluated across reorgs and chainstates while public non-validation callers retain active-chain semantics. This prevents snapshot-seeded records from suppressing commitments or satisfying MNHF and asset-unlock quorum lookups during background validation. Add dual-chainstate coverage for a commitment seeded at a block not yet contained by the background chain, including HasQuorum and GetQuorum cache-order checks.
Emit block, tip, deterministic masternode-list, UI, and flush notifications only for the active chainstate. In particular, suppressing background ChainStateFlushed prevents a background locator from regressing wallet best-block state. Keep BlockChecked ungated because its subscribers are mining/block-submit and peer validation/relay accounting; it does not reach CMNAuth. Document all 21 B3 call-site dispositions and extend the dual-chainstate test with validation-interface and UI counters.
Check local block-data availability before building masternode-list diffs and quorum rotation info. Treat failures caused by pruning or an unvalidated snapshot base like pruned getdata: log and silently drop the plausible request without increasing the peer's misbehavior score. Malformed and implausible requests retain the pre-existing penalties.
Disable DKG participation and quorum signing until snapshot background validation completes. Enforce the refusal at CreateSigShare, the actual share-production boundary, so direct RPC, async, and queued signing paths cannot bypass it. The quorum sign RPC now returns a clear JSON-RPC error for both submit modes, and masternode status exposes the disabled participation state. Add unit coverage for the shared production-gate predicate across snapshot activation.
A WriteDerived failure means independently derived block data disagrees with the copy already recorded in EvoDB. That is local state corruption (or a cross-chainstate divergence bug), never evidence about the block being processed. Previously the mismatch surfaced as BLOCK_CONSENSUS: the block was persistently marked BLOCK_FAILED_VALID (surviving restart and forking the node off the network) and the relaying peer was handed a 100-point misbehavior score via BlockChecked, which background validation also triggers. Instead, follow the existing EvoDbInconsistencyMessage convention: request node shutdown via AbortNode and fail validation with M_ERROR, which neither marks the block invalid nor punishes peers. The credit-pool and MNHF sites abort at the throw site because miner and RPC callers never pass through a validation-state catch; a typed EvoDbInconsistencyError lets the four block-path catch blocks that would otherwise swallow it into BLOCK_CONSENSUS reclassify it as M_ERROR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ethrow CMNHFManager::GetForBlock re-throws internal ProcessBlock failures as a plain runtime_error, which would let a downstream generic catch misreport an EvoDB mismatch (M_ERROR) as a consensus failure. Re-throw typed when the validation state carries M_ERROR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reads and writes outside a BeginTransaction scope previously always bound to the NORMAL identity. While a snapshot chainstate is active, transaction-less consumers (RPC, mempool, miner, P2P serving) could not see snapshot-chain records still pending in the SNAPSHOT root overlay, and transaction-less writes dirtied the wrong overlay. CEvoDB now tracks a default identity which snapshot activation (ActivateSnapshot, ActivateExistingSnapshot) sets to SNAPSHOT and ResetChainstates resets; snapshot completion must reset it to NORMAL when marker promotion lands. Also make the GetListForBlockInternal fallback loud: a missing list diff for a DIP3-active block is pending-elsewhere or corrupt data, never the pre-DIP3 genesis of the masternode list, so throw instead of silently caching an empty list and clobbering m_initial_snapshot_index. The thrown message carries the IsBlockDataUnavailableError sentinel and BuildSimplifiedMNListDiff converts it into a serve failure, so peers requesting such history are not penalized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GetListForBlock can now throw when list data is unavailable (missing diff for a DIP3-active block). CActiveMasternodeManager::UpdatedBlockTip and GovernanceSigner's trigger creation run on the scheduler thread, where an uncaught exception terminates the node; catch it there and skip the update instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
8d7b52a to
993d53b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 993d53b8f8
ℹ️ 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".
| { | ||
| auto db_tx = snapshot_chainstate.m_evoDb.BeginTransaction(EvoDbIdentity::SNAPSHOT); | ||
| snapshot_chainstate.m_evoDb.WriteBestBlock(EvoDbIdentity::SNAPSHOT, base_blockhash); | ||
| snapshot_chainstate.m_evoDb.WriteDualChainstateMarker(); |
There was a problem hiding this comment.
If PopulateAndValidateSnapshot() succeeds but the subsequent WriteSnapshotBaseBlockhash() fails—for example because the datadir becomes unwritable—this marker and the SNAPSHOT best-block marker have already been committed, while the failure path deletes only the snapshot coins directory. The node is then back to one chainstate with a persistent dual-chainstate marker: GetListForBlockInternal() consequently treats otherwise-supported legacy missing-diff bootstrapping as unavailable history, and a future snapshot directory can satisfy ActivateExistingSnapshot() using this stale marker because that function checks only its existence. Roll back both EvoDB markers whenever the overall activation is abandoned.
AGENTS.md reference: AGENTS.md:L167-L175
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All four carried-forward prior findings remain valid: snapshot reindex recovery is broken, six intermediate revisions do not compile, and two corrective changes remain separated from the commits that introduce their behavior. The latest delta is tree-equivalent to the prior head and adds no new logical change; cumulative reinspection additionally confirms the missing bitcoin#27596/bitcoin#29726 cleanup prerequisite, incorrect process-wide BLS state during background validation, and overly broad scheduler exception handling. Five canonical blockers require changes before merge.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 5 blocking | 🟡 2 suggestion(s)
3 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/node/chainstate.cpp`:
- [BLOCKING] src/node/chainstate.cpp:57-71: Missing prerequisite: bitcoin#27596 snapshot reindex cleanup
Upstream bitcoin#27596 commit `c711ca186f8` added snapshot-chainstate deletion during both reindex modes, and bitcoin#29726 commit `e57f951805b` corrected that operation to locate the on-disk snapshot directory without initialized coins views and transfer the mempool before destroying the snapshot chainstate. Neither operation exists in this PR's base or head. Because this stack now wipes the shared EvoDB before requiring the snapshot-specific marker, omitting that prerequisite makes persisted-snapshot reindexing unrecoverable. Adapt the corrected cleanup here, including the necessary Dash manager/EvoDB teardown and rebinding, and test both reindex modes.
In `<commit:0d36cebe991>`:
- [BLOCKING] <commit:0d36cebe991>:1: Fold the delayed txindex cleanup into the test commit
Commit `19da2c28add` adds `std::make_unique<TxIndex>(1 << 20, true)` and `Start(restarted.ActiveChainstate())`. At that exact revision, `TxIndex` requires a `std::unique_ptr<interfaces::Chain>` as its first constructor argument and `BaseIndex::Start()` takes no arguments, so both calls fail to compile. The six revisions from `19da2c28add` through `d36983cfba1` remain unusable bisect points until `0d36cebe991` removes the stale code. Rewrite `19da2c28add` without those calls, and fold the independent `govman` teardown-order repair into the fixture-lifetime change that requires it or retain it as a separately described commit.
In `src/validation.cpp`:
- [BLOCKING] src/validation.cpp:2518-2521: Background validation still uses the active chainstate's global BLS scheme
`bls::bls_legacy_scheme` is process-wide, but `ConnectBlock()` only saves the current global value and commits any successful transition; it never initializes the scheme from the chainstate being validated. With an active post-V19 snapshot, the global flag is basic, so background validation of pre-V19 blocks starts under the basic scheme and never switches to legacy because `ProcessSpecialTxsInBlock()` only changes the flag while crossing V19 forward. Historical BLS-serialized quorum and special-transaction data can therefore be decoded or validated under the wrong scheme. A background disconnect across V19 can conversely commit the legacy scheme and leave active-chain consumers using it. Establish the scheme from the calling chainstate before each background connect or disconnect and restore the active chainstate's scheme afterward, with dual-chainstate coverage spanning V19.
In `src/active/masternode.cpp`:
- [BLOCKING] src/active/masternode.cpp:183-192: Only swallow the intended unavailable-history exception
The new scheduler protection catches every `std::exception`, although the recoverable condition is specifically the error matched by `IsBlockDataUnavailableError`. `GetListForBlock()` can also throw for inconsistent list diffs: `ApplyDiff()` reports missing removals or updates, duplicate masternodes, and duplicate unique properties with ordinary `std::runtime_error`. Those failures are now logged as benign unavailable history, leaving the active masternode in its previous READY state; `GovernanceSigner::CreateGovernanceTrigger()` has the same broad catch. Return only for the unavailable-history sentinel, and propagate or abort for all other exceptions so local EvoDB/list corruption is not hidden.
In `<commit:03cb36c524b>`:
- [SUGGESTION] <commit:03cb36c524b>:1: Squash the WriteDerived error-classification corrections
Commit `8c1ec1e9aef` introduces `WriteDerived` call sites that classify deterministic EvoDB mismatches as consensus failures or throw generic exceptions that enclosing catches translate into consensus failures. Commits `03cb36c524b` and `7a12dba1298` later repair those paths to abort the node with `M_ERROR` and preserve that classification through `CMNHFManager::GetForBlock()`. Canonical serialization required for reliable byte comparison also does not arrive until `d36983cfba1`, six commits after first use. Introduce `WriteDerived` with its final corruption semantics and canonical payload serialization so intermediate revisions cannot reject valid blocks, penalize peers, or compare logically identical values using noncanonical bytes.
In `<commit:993d53b8f87>`:
- [SUGGESTION] <commit:993d53b8f87>:1: Keep the new masternode-list throw and its callers atomic
Commit `04c615834a0` makes `GetListForBlockInternal()` throw when a DIP3-active list diff is unavailable, but `CActiveMasternodeManager::UpdatedBlockTip()` and `GovernanceSigner::CreateGovernanceTrigger()` still call it without catches at that revision. These callbacks execute through the scheduler-backed validation interface, so the intermediate commit can terminate the node. Move the missing-diff behavior and both caller adaptations into one atomic commit, or squash `993d53b8f87` into `04c615834a0` and update the subject accordingly.
| chainman.InitializeChainstate(options.mempool, *evodb, chain_helper); | ||
|
|
||
| // Load a chain created from a UTXO snapshot, if any exist. | ||
| chainman.DetectSnapshotChainstate(options.mempool); | ||
| bilingual_str snapshot_error; | ||
| if (!chainman.DetectSnapshotChainstate(options.mempool, snapshot_error)) { | ||
| return {ChainstateLoadStatus::FAILURE, snapshot_error}; |
There was a problem hiding this comment.
🔴 Blocking: Missing prerequisite: bitcoin#27596 snapshot reindex cleanup
Upstream bitcoin#27596 commit c711ca186f8 added snapshot-chainstate deletion during both reindex modes, and bitcoin#29726 commit e57f951805b corrected that operation to locate the on-disk snapshot directory without initialized coins views and transfer the mempool before destroying the snapshot chainstate. Neither operation exists in this PR's base or head. Because this stack now wipes the shared EvoDB before requiring the snapshot-specific marker, omitting that prerequisite makes persisted-snapshot reindexing unrecoverable. Adapt the corrected cleanup here, including the necessary Dash manager/EvoDB teardown and rebinding, and test both reindex modes.
source: ['codex']
| m_evoDb.WriteBestBlock(EvoDbIdentity(), pindex->GetBlockHash()); | ||
|
|
||
| // Block is committed: keep the scheme it switched to (fJustCheck dry runs returned above). | ||
| bls_scheme_guard.Commit(); |
There was a problem hiding this comment.
🔴 Blocking: Background validation still uses the active chainstate's global BLS scheme
bls::bls_legacy_scheme is process-wide, but ConnectBlock() only saves the current global value and commits any successful transition; it never initializes the scheme from the chainstate being validated. With an active post-V19 snapshot, the global flag is basic, so background validation of pre-V19 blocks starts under the basic scheme and never switches to legacy because ProcessSpecialTxsInBlock() only changes the flag while crossing V19 forward. Historical BLS-serialized quorum and special-transaction data can therefore be decoded or validated under the wrong scheme. A background disconnect across V19 can conversely commit the legacy scheme and leave active-chain consumers using it. Establish the scheme from the calling chainstate before each background connect or disconnect and restore the active chainstate's scheme afterward, with dual-chainstate coverage spanning V19.
source: ['codex']
| try { | ||
| oldMNList = m_dmnman.GetListForBlock(pindexNew->pprev); | ||
| newMNList = m_dmnman.GetListForBlock(pindexNew); | ||
| } catch (const std::exception& e) { | ||
| // GetListForBlock throws when list data is unavailable. This | ||
| // callback runs on the scheduler thread, where an uncaught | ||
| // exception terminates the node; skip this tip update instead and | ||
| // let the next one retry. | ||
| LogPrintf("CActiveMasternodeManager::%s -- masternode list unavailable: %s\n", __func__, e.what()); | ||
| return; |
There was a problem hiding this comment.
🔴 Blocking: Only swallow the intended unavailable-history exception
The new scheduler protection catches every std::exception, although the recoverable condition is specifically the error matched by IsBlockDataUnavailableError. GetListForBlock() can also throw for inconsistent list diffs: ApplyDiff() reports missing removals or updates, duplicate masternodes, and duplicate unique properties with ordinary std::runtime_error. Those failures are now logged as benign unavailable history, leaving the active masternode in its previous READY state; GovernanceSigner::CreateGovernanceTrigger() has the same broad catch. Return only for the unavailable-history sentinel, and propagate or abort for all other exceptions so local EvoDB/list corruption is not hidden.
source: ['codex']
3 participants
Issue being fixed or feature implemented
M1 (#7451) added AssumeUTXO snapshot persistence, but Dash stores deterministic masternode, quorum, MNHF, and credit-pool state in a shared EvoDB. Running the snapshot and background chainstates concurrently therefore requires independent EvoDB transaction state and markers, chain-aware Dash validation, and protection against emitting or signing from the wrong chainstate.
This is milestone 2 of the AssumeUTXO series. It supplies the Dash-specific multi-chainstate foundation required by the later background-completion and
loadtxoutsetmilestones.What was done?
WriteDerivedfor immutable block-derived records. Independently derived values must serialize identically, including values pending in the other chainstate's overlay.BlockCheckedremains ungated because it reports validation results rather than active-tip changes.Review follow-ups (appended commits):
WriteDerivedmismatch is local EvoDB corruption, never evidence about the block. It now aborts the node withM_ERROR(matching the existingEvoDbInconsistencyMessageconvention) instead of marking the blockBLOCK_CONSENSUS-invalid and penalizing the relaying peer. A typedEvoDbInconsistencyErrorpreserves that classification through the catch blocks on the miner, RPC, and MNHF-recomputation paths.BeginTransactionscope previously always bound to the NORMAL identity, so transaction-less consumers (RPC, mempool, miner, P2P serving) could not see snapshot-chain records pending in the SNAPSHOT overlay.CEvoDBnow tracks a default identity that snapshot activation sets to SNAPSHOT andResetChainstatesresets; the background-completion milestone must reset it to NORMAL at marker promotion (TODO noted in code).GetListForBlockInternalno longer fabricates an empty "initial snapshot" masternode list when a diff for a DIP3-active block is missing; it throws instead. The message deliberately carries theIsBlockDataUnavailableErrorsentinel and is deliberately a plainruntime_errorrather thanEvoDbInconsistencyError: at that layer a missing diff can be benign (pending in the other chainstate's unflushed overlay, e.g. while serving historicalmnlistdiff), so it is reported as unavailable history without penalizing the requesting peer, and only definite mismatches abort the node. Scheduler-thread consumers (CActiveMasternodeManager::UpdatedBlockTip, governance trigger creation) catch it and skip the update, since an uncaught exception there would terminate the node.The batch contains the original eight focused commits, a rebase fixture adaptation after the txindex removal, and four review follow-up commits. The partial Bitcoin Core
BlockInfoandChainstateRoleprerequisites were intentionally moved to the laterloadtxoutsetmilestone where their APIs are first consumed.How Has This Been Tested?
transaction_less_access_uses_default_identitycovering default-identity resolution of transaction-less reads and writes. Known gap: the DIP3-active missing-diff throw has no end-to-end unit test because regtest activates DIP3 at height 432, above the unit-test chain heights; it is covered by the serving-path catch and review.make checkpassed and the following functional subset passed:feature_mnehf.py,feature_asset_locks.py,feature_dip3_deterministicmns.py(both modes),feature_llmq_signing.py(both modes),feature_llmq_rotation.py, andrpc_quorum.py.evo_db_tests,evo_deterministicmns_tests,evo_mnhf_tests,evo_assetlocks_tests, andvalidation_chainstatemanager_testspass (20 cases), andlint-circular-dependenciesis clean.git diff-tree --check.Breaking Changes
No released-interface changes. Internally,
ActivateExistingSnapshotbecomes fallible so startup can reject a snapshot chainstate whose EvoDB marker is missing. Two RPC-visible additions while a snapshot is active and unvalidated:masternode statusgains aquorumParticipationfield and appends a "DKG participation and quorum signing disabled" clause tostatus, andquorum signfails with an explanatory error.AbstractEHFManager::Signalschanged fromstd::unordered_maptostd::map(canonical serialization); the on-disk encoding is unchanged, but iteration order of EHF signals ingetblockchaininfooutput is now sorted.Checklist: