Skip to content

backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce dash-chainstate#7234

Merged
PastaPastaPasta merged 4 commits into
dashpay:developfrom
knst:bp-kernel-0
Jun 5, 2026
Merged

backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce dash-chainstate#7234
PastaPastaPasta merged 4 commits into
dashpay:developfrom
knst:bp-kernel-0

Conversation

@knst

@knst knst commented Mar 17, 2026

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

This PR is a first backport of the libdashkernel project, see for details: bitcoin#24303

What was done?

This PR introduces an example/demo dash-chainstate executable using said library which can print out information about a datadir and take in new blocks on stdin.

There are multiple PR that has been extracted from this PR and merged independently:

Beside these PR multiple other PR has been done in the past, such as: #7178, #7070, #7008, #6992, #6959, #7106 (by Udjin), #7065 (by kwvg) and many other PRs.

This PR:

There are several important notes about implementation:

  • UpdateUncommittedBlockStructures is segwit related; omitted from bitcoin-chainstate.cpp
  • call chainman.UnloadBlockIndex is removed by refactor: Move mutable globals cleared in ::UnloadBlockIndex to BlockManager bitcoin/bitcoin#22564, not backported
  • binary name is renamed from bitcoin-chainstate to dash-chainstate. Names of source files and some variables are untouched, but final artefact has the correct dashified name
  • added LIBDASHBLS to list of libraries to link
  • couple functions are reimplemented to avoid balooning dash-chainstate by including extra heavy depenencies
    • ValueFromAmount (reimplemented, core_write.cpp)
    • GetPrettyExceptionStr (reimplemented, stacktrackes.cpp)
    • g_stats_client (redefined)

There are several modules that should be removed in the near future from dash-chainstate binary, but out-of-scope of this PR:

  • protocol.cpp -- move NetMsgType inline helpers out of chainlock/clsig.h, governance/object.h, evo/simplifiedmns.h into their .cpp files
  • llmq/{dkgsession,dkgsessionhandler,dkgsessionmgr,debug}.cpp and batchedlogger.cpp -- CQuorumManager already null-guards m_qdkgsman; drop once ConnectManagers() is never called on the kernel side
  • llmq/signing.cpp -- split CSigningManager out of LLMQContext into a post-construction Connect() so the context becomes quorum-only
  • governance/*.cpp -- promote the 4 methods used by CMNPaymentsProcessor (IsValid, IsSuperblockTriggered, IsValidSuperblock, GetSuperblockPayments) to a base interface; instantiate a NullGovernanceManager in bitcoin-chainstate.cpp

How Has This Been Tested?

New binary dash-chainstate is produced, functional & unit tests succeed.

Run dash-chainstate:

$ src/dash-chainstate /home/knst/.dashcore/
Hello! I'm going to print out some information about your datadir.
        Path: "/home/knst/.dashcore"
        Reindexing: false
        Snapshot Active: false
        Active Height: 2294070
        Active IBD: true
        CBlockIndex(pprev=0x5654a3bdb218, nHeight=2294070, merkle=5c87e557b9e697e9308a4a6bd684e362910fe418ca60e59ae1376e91c487df77, hashBlock=0000000000000010938b2d1c2f1ff08163dae17f4196a41d73b9e6e3c4114cc2)

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

@github-actions

github-actions Bot commented Mar 17, 2026

Copy link
Copy Markdown

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

Please coordinate with the authors of these PRs to avoid merge conflicts.

PastaPastaPasta added a commit that referenced this pull request Mar 31, 2026
eb2142d fix: added missing thread annotation for CJWalletManagerImpl (Konstantin Akimov)
40cd08d refactor: rename CCoinJoinBaseManager to CoinJoinQueueManager and make it non-virtual (Konstantin Akimov)
6fae09a refactor: remove inheritance from CCoinJoinBaseManager in src/test/coinjoin_basemanager_tests.cpp (Konstantin Akimov)
a0f39f2 refactor: removed CCoinJoinBaseManager from inheritance of CCoinJoinServer and make it a member (Konstantin Akimov)
72229ce refactor: inline class CCoinJoinClientQueueManager to CJWalletManagerImpl (Konstantin Akimov)
3b1c2da refactor: inline the middleman class CoinJoinWalletManager to CJWalletManagerImpl (Konstantin Akimov)
e2d8247 refactor: pass directly reference to CCoinJoinClientManager& instead unique_ptr inside ForEachCJClientMan (Konstantin Akimov)
689b4ec refactor: remove passing pointer to unique_ptr to more clear sequence of object initializtion in CJ (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  Current coinjoin subsystem have multiple classes that inherits from each other but some of them doesn't provide any real abstraction level yet complicate architecture.

  CJWalletManagerImpl, CCoinJoinClientManager, and CCoinJoinClientQueueManager are 2 separate pieces of implementation of CJWalletManager while CJWalletManagerImpl is already private and could keep all implementation inside.

  ## What was done?

  This PR simplify CJ architecture by removing boiler code and unneeded abstraction levels to make code easier to support, read and improve.

  The changes for this PR has been spotted and half-done during #7234 but it's not a direct dependency of final solution, so far as it was possible to fully cut CJ from dash-chainstate implementation.

   - replaces all references to unique_ptr to just a reference or just unique_ptr to more clear sequence of object initialization in CJ and ownership
   - inline the middleman class CoinJoinWalletManager to CJWalletManagerImpl which is already private and hidden
   - remove class CCoinJoinClientQueueManager; logic inlined into CJWalletManagerImpl
   - remove interface CoinJoinQueueNotify - que notification no longer needed as a separate abstraction
   - replaced inheritance of CCoinJoinServer : public CCoinJoinBaseManager to composition (m_queueman member)
   - remove inheritance of regression tests from CCoinJoinBaseManager
   - rename CCoinJoinBaseManager to CoinJoinQueueManager and make it non-virtual

   ## CoinJoin Class Diagram — Before vs After [by claude]

      BEFORE

      CCoinJoinBaseManager          CoinJoinQueueNotify
       (virtual ctor/dtor)           (interface)
       - vecCoinJoinQueue             - OnQueueUpdate() [pure]
       - cs_vecqueue
              ▲                              ▲
              │ inherits              │ implements
              ├─────────────────┐     │
      CCoinJoinClientQueueManager  CCoinJoinClientManager
       (owned by CJWalletManagerImpl)  (map entry per wallet)
              │
              │ inherits
              │
       CCoinJoinServer ──also inherits──▶ CCoinJoinBaseSession
                                               ▲
                                               │ inherits
                                         CCoinJoinClientSession

      CJWalletManager (abstract)
       └── CJWalletManagerImpl
            ├── unique_ptr<CCoinJoinClientQueueManager>  m_basequeueman
            └── map<wallet* → CCoinJoinClientManager>

      ---
      AFTER

      CoinJoinQueueManager           ← renamed (no C prefix), no virtual ctor/dtor
       - vecCoinJoinQueue               no longer a base class at all
       - cs_vecqueue
       + TryHasQueueFromMasternode()    new TRY_LOCK helpers
       + TryCheckDuplicate()
       + TryAddQueue()

      CCoinJoinServer  ──inherits──▶  CCoinJoinBaseSession
       - m_queueman: CoinJoinQueueManager   ← OWNED by value (composition)
                                               ▲
                                               │ inherits
                                         CCoinJoinClientSession

      CJWalletManager (abstract)
       └── CJWalletManagerImpl
            ├── unique_ptr<CoinJoinQueueManager>  m_queueman  ← nullptr if !relay_txes
            └── map<wallet* → CCoinJoinClientManager>
                     │
                     │  CCoinJoinClientManager
                     │   - m_queueman: CoinJoinQueueManager*   ← non-owning ptr
                     │                 (points into CJWalletManagerImpl::m_queueman)

  ## How Has This Been Tested?
  Run unit & functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK eb2142d

Tree-SHA512: 03a648885d86c6e251296b3315ddf2ffb0ab3839526cf10a52bb36f53dc7ff1888bc45f69bbdcddd8a32bd940955cf6e8487e84062d0dde9daa32317721eeb5a
PastaPastaPasta added a commit that referenced this pull request Apr 5, 2026
…andleNewRecoveredSig to variant

eeb84cb refactor: replace usages of MessageProcessingResult in HandleNewRecoveredSig to variant (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  The public interface `HandleNewRecoveredSig` which is widely used on develop returns `MessageProcessingResult`

  Using MessageProcessingResult is incorrect using here; because `HandleNewRecoveredSig` may return only CInv or CTransactionRef.

  Using MessageProcessingResult here spreads including of heavy `msg_result.h` all over codebase; it makes dependency of llmq/ code on network code.

  This PR is a direct dependency for kernel / dash-chainstate project, see #7234

  ## What was done?
  Replaced MessageProcessingResult to std::variant<CInv, CTransactionRef, std::monostate>.

  Potentially, even CTransactionRef is overhead and should be replaced to special case of CInv, but it's out-of-scope of this refactoring.
  Also, `m_clhandler.ProcessNewChainLock` should be refactored and split to network and non-network part and avoid leaking `MessageProcessingResult` abstraction here too, but I kept it out-of-scope of this refactoring too.

  ## How Has This Been Tested?
  Run unit tests & functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  thepastaclaw:
    crACK eeb84cb

Clean refactoring that replaces the generic `MessageProcessingResult` with a precise `std::variant<std::monostate, CInv, CTransactionRef>` for `HandleNewRecoveredSig`. This addresses the TODO that was already in the code.

Verified:
- **ChainLock handler**: `ProcessNewChainLock(-1, ...)` can only produce a single `CInv` or empty result (no `MisbehavingError` when `from == -1`, no `m_transactions`), so extracting `m_inventory.front()` is correct. The TODO comment about splitting `ProcessNewChainLock` is a good note for follow-up.
- **EHF handler**: Now returns `CTransactionRef` directly instead of pushing `GetHash()` into `m_transactions`. The caller in `net_signing.cpp` calls `PeerRelayTransaction(hash)` which routes through `RelayTransaction` → `_RelayTransaction` — same codepath as the old `PostProcessMessage` loop over `m_transactions`.
- **InstantSend + SigShares handlers**: Always returned empty `MessageProcessingResult{}`, now correctly return `std::monostate{}`.
- **Dispatch in `net_signing.cpp`**: Clean `std::get_if` dispatch replacing the opaque `PeerPostProcessMessage` call. Each variant arm calls the appropriate relay function.
- `PeerPostProcessMessage` remains in `net_processing.h` for other callers — no dangling references.
  UdjinM6:
    utACK eeb84cb

Tree-SHA512: 6158fc345ad9a70d2b0d4da0b5c3cfef76642e16d5738fab3aebdc771f68f4b3074bb7638864a3574ae66ad58123341dcd5dd83d39d7fb0737881129809b6427
@github-actions

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

PastaPastaPasta added a commit that referenced this pull request Apr 28, 2026
…uorum participant

863ee6b doc: clarify masternode late InitializeCurrentBlockTip comment (UdjinM6)
dc8c6cf fix: replay startup tip into ActiveContext/ObserverContext (UdjinM6)
21fe75f fix: guard GetProTxHash() behind is_masternode check, add CQuorum safety assert (UdjinM6)
27129d9 fix: defer full InitializeCurrentBlockTip broadcast on masternodes (UdjinM6)
55d8025 refactor: safety belt to be sure that CQuorum is initialized properly and no empty qc member (Konstantin Akimov)
8bc5744 fix: review comments - missing forward declaration, protection for no valid members (Konstantin Akimov)
e2df648 refactor: drop argument request_limit_exceeded in ProcessContribQGETDATA (Konstantin Akimov)
b47de25 refactor: remove net related code ProcessContribQGETDATA and ProcessContribQDATA to NetQuorum (Konstantin Akimov)
1be8863 refactor: move out network code to new NetQuorum from CQuorumManager (Konstantin Akimov)
c86e4f6 refactor: it's always either observer_ctx or active_ctx, not both created together (Konstantin Akimov)
6a2f291 refactor: remove duplicated code between QuorumRole's UpdatedBlockTip and InitializeQuorumConnections (Konstantin Akimov)
6c99cb0 fix: parameter 'params' shadows member inherited from type 'CDKGSession' [-Werror,-Wshadow-field] (Konstantin Akimov)
0f1174a refactor: remove multiple friends of CDKGSession (Konstantin Akimov)
f4cbf92 refactor: inline QuorumParticipant to ActiveContext (Konstantin Akimov)
5ded711 refactor: rename src/llmq/observer/context to src/llmq/observer (Konstantin Akimov)
93f168a refactor: remove QuorumRoleBase by moving its functionality directly to QuorumRole (Konstantin Akimov)
9bb50db refactor: merge QuorumObserver and ObserverContext classes to the one entity to reduce amount of abstractions (Konstantin Akimov)
7978822 refactor: quorum members: remove QuorumObserverParent; participant is no more child of 'observer' (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  It's a follow-up for #7066 and related PRs

  This PR is reducing mental concentration when reading quorum-related code by significant simplification amount of levels of abstractions and inheritance classes from each other.

  This PR is a direct dependency of #7234 (kernel / chainstate project)

  Performance is not expected to be improved by any noticeable margin.

  Compilation time is expected to be improved marginally due to fewer files (-2) to compile and less transitive includes over `src/llmq/quorumsman.h`.

  ## What was done?

  This changes has been spotted during #7234 and extracted to own PR.

   - Remove QuorumObserverParent; decouple QuorumParticipant from QuorumObserver inheritance — they become independent implementations of a common QuorumRole interface.
   - Merge QuorumObserver and ObserverContext into a single class, then remove the transitional QuorumRoleBase by folding it into QuorumRole.
   - Rename src/llmq/observer/context.{h,cpp} to src/llmq/observer.{h,cpp} — flatten the now-single-file subdirectory.
   - Inline QuorumParticipant into ActiveContext — it only applies to active masternode mode; src/active/quorums.{h,cpp} deleted.
   - Remove four friend declarations from CDKGSession (ActiveDKGSession, ActiveDKGSessionHandler, CDKGSessionHandler, CDKGSessionManager); affected members changed from private to protected.
   - Extract network code from CQuorumManager and ActiveContext into new NetQuorum (src/llmq/net_quorum.{h,cpp}) — handles QGETDATA/QDATA processing, quorum peer connections, data recovery, and periodic cleanup.

  ###  Class hierarchy — before vs. after
   *by claude

    Before:
    QuorumObserverParent (interface, implemented by CQuorumManager)
        │
    QuorumObserver (base class with shared state)
        ├── ObserverContext  (observer mode, also CValidationInterface)
        └── QuorumParticipant (active mode, in src/active/quorums.h)

    After:
    QuorumRole (pure virtual interface in quorumsman.h)
        ├── ObserverContext  (observer mode, in src/llmq/observer.h)
        └── ActiveContext    (active mode, QuorumParticipant inlined)

  ## How Has This Been Tested?
  Run unit & functional tests

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 863ee6b

Tree-SHA512: 13a3d8e533bf811fd5d438e0c2f3007b633e990b725767907d5c4b8eb2da32d06f282f3f61319ce78a17e40ca41dac1b3c791039c5672321e1b43a7174712b2d
@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown

This pull request has conflicts, please rebase.

PastaPastaPasta added a commit that referenced this pull request May 12, 2026
0bcb345 refactor: move VerifyChainLock and ChainLockSig between modules to untangle chain-state dependencies (Konstantin Akimov)
3441f04 refactor: move multiple msg_result.h usages headers to implementation (Konstantin Akimov)
69d4a06 feat: better loging of p2p governance objects, separate local and pendings (Konstantin Akimov)
f4ee8b4 refactor: drop dependency of CGovernanceManager on net.cpp (Konstantin Akimov)
646c2c8 fix: circular dependency over rpc/evo_util (Konstantin Akimov)
2b2d0a4 refactor: move CDeterministicMN::ToJson to core_write to avoid g_txindex dependency for linking (Konstantin Akimov)
312ca8e refactor: drop pfrom from CGovernanceManager::ProcessVote (Konstantin Akimov)
d0414cc refactor: drop dependency of governance/object on core_write (ScriptToAsmStr) (Konstantin Akimov)
aa4e692 fix: add guards if CTxMempool is nullptr in chainstate (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  That's some final preparation with various refactors for kernel-0 project, splitted out from #7234

  ## What was done?
   - drop pfrom from CGovernanceManager::ProcessVote
   - drop dependency of CGovernanceManager on net.cpp
   - drop dependency of governance/object on core_write (ScriptToAsmStr)
   - add guards for CTxMempool is nullptr [which could be for chainstate case]
   - removed multiple includes of msg_result.h over codebase
   - moved CDeterministicMN::ToJson to core_write to avoid g_txindex
   - move VerifyChainLock and ChainLockSig between modules to untangle chain-state dependencies

  NOTE: the circular dependency over `dkgsessionhandler <-> net_processing` is not new, it's pre-existing one but discovered by replacing removing `llmq/dkgsessionhandler.h` from `llmq/dkgsessionmgr.h` and direct adding this include to llmq/observer

          "llmq/dkgsessionhandler -> net_processing -> llmq/dkgsessionmgr -> llmq/dkgsessionhandler",
          "llmq/dkgsessionhandler -> net_processing -> llmq/observer -> llmq/dkgsessionhandler",

  ## How Has This Been Tested?
  Run unit & functional tests; proof-of-concept of final PR is #7234

  Compilation time is not affected by this PR: removing msg_result.h from the header have too small positive impact; replacing chainlock/clsig.h to chainlock/chainlock.h doesn't give any visible change either.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 0bcb345

Tree-SHA512: 944adb2da273ac4f72f4c63a5be19511c1cabcee601ea03594a5bfe8cd65ff82512011be2e644fb12bf16c20a4149fcfa1c641df539a32c80cd511335a25c2e3
PastaPastaPasta added a commit that referenced this pull request May 12, 2026
4e6f844 refactor: collapse spork signer recovery to one ECDSA op, restore peer info in logs (UdjinM6)
6a4ee52 fix: replace assert to if-return. Less probability to get troubles in the future (Konstantin Akimov)
7460211 refactor: address review feedback on spork ProcessMessage refactor (UdjinM6)
7c899a2 refactor: drop dependency of spork manager on network code (Konstantin Akimov)
bd2bff1 refactor: move network implementation of spork to net_processing (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  It helps to break circular dependencies over spork <-> net.
  It helps to get spork manager ready kernel project.

  Apparantly, chainstate implementation uses sporks for validation of blockchain but network dependencies could be avoided there.

  This changes is PR-ready changes from #7234 for spork manager.

  ## What was done?
  Dropped dependency of spork manager on network code by moving network code to net_processing.

  There's no need dedicated NetHandler because imlementation is trivial, short and doesn't have any async worker or threads.

  ## How Has This Been Tested?
  Run unit & functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK 4e6f844

Tree-SHA512: 5b06e49c716cf66b66ffe9902fa0f987d0492236b87c45eb285d30def49a2bc66f8ef21b0ae4372465de4598b02f17605c5e127bab79bf2f35c83f0e819b7caf
@knst knst force-pushed the bp-kernel-0 branch 2 times, most recently from 2b2d776 to 6b9abe4 Compare May 13, 2026 18:43
@knst knst changed the title backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce bitcoin-chainstate backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce dash-chainstate May 13, 2026
@knst knst added this to the 24 milestone May 13, 2026
@knst knst marked this pull request as ready for review May 13, 2026 19:05
@thepastaclaw

thepastaclaw commented May 13, 2026

Copy link
Copy Markdown

✅ Review complete (commit 72b66af)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 99d2079bfc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/Makefile.am Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/bitcoin-chainstate.cpp (1)

87-87: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add error handling for directory creation.

The std::filesystem::create_directories call can fail (e.g., due to permission errors or disk space issues), but there's no check for success. If directory creation fails, subsequent operations will encounter less-clear error messages.

🛡️ Proposed fix to check directory creation
 std::filesystem::path abs_datadir = std::filesystem::absolute(argv[1]);
-std::filesystem::create_directories(abs_datadir);
+if (!std::filesystem::exists(abs_datadir) && !std::filesystem::create_directories(abs_datadir)) {
+    std::cerr << "Failed to create directory: " << abs_datadir << std::endl;
+    return 1;
+}
 gArgs.ForceSetArg("-datadir", abs_datadir.string());
🤖 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/bitcoin-chainstate.cpp` at line 87, The call to
std::filesystem::create_directories(abs_datadir) can fail and currently has no
error handling; wrap the call in a try/catch for
std::filesystem::filesystem_error (or check its boolean return on non-throwing
overload) and on failure log a clear error message including abs_datadir and the
exception message, then return an appropriate error/exit status so subsequent
code does not run on a missing directory. Target the
create_directories(abs_datadir) call in bitcoin-chainstate.cpp and ensure the
error path uses existing logging/error reporting used elsewhere in the module.
🤖 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.

Duplicate comments:
In `@src/bitcoin-chainstate.cpp`:
- Line 87: The call to std::filesystem::create_directories(abs_datadir) can fail
and currently has no error handling; wrap the call in a try/catch for
std::filesystem::filesystem_error (or check its boolean return on non-throwing
overload) and on failure log a clear error message including abs_datadir and the
exception message, then return an appropriate error/exit status so subsequent
code does not run on a missing directory. Target the
create_directories(abs_datadir) call in bitcoin-chainstate.cpp and ensure the
error path uses existing logging/error reporting used elsewhere in the module.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 19ba7c83-f01c-4ab5-bfd7-d78a543b892f

📥 Commits

Reviewing files that changed from the base of the PR and between 99d2079 and 0aaadb0.

📒 Files selected for processing (12)
  • .gitignore
  • ci/dash/matrix.sh
  • ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh
  • configure.ac
  • src/Makefile.am
  • src/active/dkgsessionhandler.cpp
  • src/active/dkgsessionhandler.h
  • src/bitcoin-chainstate.cpp
  • src/llmq/net_quorum.cpp
  • src/llmq/net_quorum.h
  • src/llmq/utils.cpp
  • src/llmq/utils.h
💤 Files with no reviewable changes (2)
  • src/llmq/utils.h
  • src/llmq/utils.cpp
✅ Files skipped from review due to trivial changes (2)
  • .gitignore
  • src/active/dkgsessionhandler.h
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/Makefile.am
  • configure.ac
  • src/active/dkgsessionhandler.cpp
  • src/llmq/net_quorum.cpp
  • ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0aaadb0dd6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +160 to +163
if (rv.has_value()) {
std::cerr << "Failed to load Chain state from your datadir." << std::endl;
goto epilogue;
} else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return non-zero status when load/validation fails

This failure branch jumps to epilogue, but main() never sets or returns an error code afterward, so the process exits with status 0 even when chainstate loading or verification fails. The same pattern is used for malformed stdin blocks (break to epilogue), so callers and CI scripts cannot distinguish success from failure by exit status and may treat failed runs as successful.

Useful? React with 👍 / 👎.

@knst knst requested review from PastaPastaPasta, UdjinM6 and kwvg May 14, 2026 17:06

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

I verified the queued worktree at commit 0aaadb0dd6b72846d6c6605554f8b8f8ff36f187 against the verifier artifacts and the touched source files. There are no remaining valid findings from Claude, Codex, or CodeRabbit on this head; the earlier src/Makefile.am issues and the dash-chainstate shutdown-order concern are fixed in the current code.

Reviewed commit: 0aaadb0

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fe2a4472f0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/bitcoin-chainstate.cpp Outdated

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Carried-forward prior findings: none (prior review at 0aaadb0 was clean). New latest-delta finding: PR #7212 (merged into develop after the prior review) dropped the CMasternodeSync& mn_sync parameter from node::LoadChainstate, but the new src/bitcoin-chainstate.cpp was not updated and still passes mn_sync as the 5th positional argument. This breaks the build of the dash-chainstate experimental binary that this PR introduces and gates in CI via linux64_nowallet_libbitcoinkernel. Both Claude and Codex independently flagged the same issue with very high confidence; I verified the live signatures in src/node/chainstate.h:78-106 and the call site in src/bitcoin-chainstate.cpp:130-159.

🔴 1 blocking

🤖 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/bitcoin-chainstate.cpp`:
- [BLOCKING] src/bitcoin-chainstate.cpp:130-159: `dash-chainstate` call to `LoadChainstate` uses the pre-#7212 signature
  After PR #7212 (`refactor: remove dependencies of CChainState on CMasternodeSync`) landed on develop between the prior reviewed head (0aaadb0d) and this head (fe2a4472), `CMasternodeSync& mn_sync` was removed from both `node::LoadChainstate` and `node::DashChainstateSetup`. The current declaration in `src/node/chainstate.h:78-106` has `CSporkManager& sporkman` as the 5th parameter (immediately after `CMasternodeMetaMan& mn_metaman`), not `mn_sync`.

  However, the new caller in `src/bitcoin-chainstate.cpp:130-159` still passes `mn_sync` as the 5th positional argument, ahead of `sporkman`. This shifts every following argument into the wrong slot and the call no longer matches any overload:

  - `mn_sync` (`CMasternodeSync&`) cannot bind to `CSporkManager&`.
  - `sporkman` would then try to bind to `chainlock::Chainlocks&`, etc.
  - The argument count is also off by one.

  The `CMasternodeSync mn_sync{...}` local on line 121 is still needed to construct `CGovernanceManager` on line 124, so only the argument in the `LoadChainstate(...)` call needs to be removed. Because this PR also enables `--enable-experimental-util-chainstate` in CI (`ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh`), this breakage will fail the dedicated `linux64_nowallet_libbitcoinkernel` job and blocks the headline deliverable of the PR.

Comment thread src/bitcoin-chainstate.cpp
knst and others added 3 commits June 2, 2026 20:51
BACKPORT NOTES:

 - UpdateUncommittedBlockStructures is segwit related; omitted from bitcoin-chainstate.cpp
 - call chainman.UnloadBlockIndex is removed by bitcoin#22564
 - binary name is renamed from bitcoin-chainstate to dash-chainstate. Names of source files and some variables are untouched
 - added LIBDASHBLS to list of libraries to link
 - couple functions are reimplemented to avoid balooning dash-chainstate by including extra heavy depenencies
      - ValueFromAmount (reimplemented, core_write.cpp)
      - GetPrettyExceptionStr (reimplemented, stacktrackes.cpp)
      - g_stats_client (redefined)

There are several modules that should be removed in the near future from dash-chainstate binary

      - protocol.cpp        move NetMsgType inline helpers out of
                            chainlock/clsig.h, governance/object.h,
                            evo/simplifiedmns.h into their .cpp files
      - llmq/dkgsession.cpp,
        llmq/dkgsessionhandler.cpp,
        llmq/dkgsessionmgr.cpp,
        llmq/debug.cpp,
        batchedlogger.cpp   CQuorumManager already null-guards m_qdkgsman; drop once ConnectManagers() is
                            never called on the kernel side
      - llmq/signing.cpp    split CSigningManager out of LLMQContext into a post-construction Connect() so the
                            context becomes quorum-only
      - governance/*.cpp    promote the 4 methods used by CMNPaymentsProcessor (IsValid, IsSuperblockTriggered,
                            IsValidSuperblock, GetSuperblockPayments) to a base interface;
                            instantiate a NullGovernanceManager in bitcoin-chainstate.cpp

ORIGINAL PR Description:

2c03cec ci: Build bitcoin-chainstate (Carl Dong)
095aa6c build: Add example bitcoin-chainstate executable (Carl Dong)

Pull request description:

  Part of: bitcoin#24303

  This PR introduces an example/demo `bitcoin-chainstate` executable using said library which can print out information about a datadir and take in new blocks on stdin.

  Please read the commit messages for more details.

  -----

  #### You may ask: WTF?! Why is `index/*.cpp`, etc. being linked in?

  This PR is meant only to capture the state of dependencies in our consensus engine as of right now. There are many things to decouple from consensus, which will be done in subsequent PRs. Listing the files out right now in `bitcoin_chainstate_SOURCES` is purely to give us a clear picture of the task at hand, it is **not** to say that these dependencies _belongs_ there in any way.

  ### TODO

  1. Clean up `bitcoin-chainstate.cpp`
     It is quite ugly, with a lot of comments I've left for myself, I should clean it up to the best of my abilities (the ugliness of our init/shutdown might be the upper bound on cleanliness here...)

ACKs for top commit:
  ajtowns:
    ACK 2c03cec
  ryanofsky:
    Code review ACK 2c03cec. Just rebase, comments, formatting change since last review
  MarcoFalke:
    re-ACK 2c03cec 🏔

Tree-SHA512: 86e7fb5718caa577df8abc8288c754f4a590650d974df9d2f6476c87ed25c70f923c4db651c6963f33498fc7a3a31f6692b9a75cbc996bf4888c5dac2f34a13b
@knst

knst commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator Author

@knst knst force-pushed the bp-kernel-0 branch from fe2a447 to ff7a110 now

force-pushed to fix compilation error and unrelated CI failure (by bumping base branch version) @thepastaclaw

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/bitcoin-chainstate.cpp`:
- Around line 159-173: The startup error paths (e.g., when rv.has_value() after
LoadChainstate, when maybe_verify_error.has_value() from
node::VerifyLoadedChainstate, and failures returned by ActivateBestChain)
currently jump to epilogue and let main() fall through returning zero; change
the error handling so fatal initialization failures set or return a non-zero
exit status (e.g., set an int exit_status = EXIT_FAILURE before jumping to
epilogue or replace the goto with an immediate return EXIT_FAILURE) so main()
exits with failure on those conditions; update the epilogue handling to
return/exit with exit_status and ensure all paths that print fatal errors
(LoadChainstate, VerifyLoadedChainstate, ActivateBestChain) result in a non-zero
process exit code.
🪄 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

Run ID: fa68d3b3-3797-4f16-acfa-c81483caca99

📥 Commits

Reviewing files that changed from the base of the PR and between fe2a447 and ff7a110.

📒 Files selected for processing (12)
  • .gitignore
  • ci/dash/matrix.sh
  • ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh
  • configure.ac
  • src/Makefile.am
  • src/active/dkgsessionhandler.cpp
  • src/active/dkgsessionhandler.h
  • src/bitcoin-chainstate.cpp
  • src/llmq/net_quorum.cpp
  • src/llmq/net_quorum.h
  • src/llmq/utils.cpp
  • src/llmq/utils.h
💤 Files with no reviewable changes (2)
  • src/llmq/utils.cpp
  • src/llmq/utils.h
✅ Files skipped from review due to trivial changes (2)
  • .gitignore
  • src/active/dkgsessionhandler.h
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/Makefile.am
  • ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh
  • src/llmq/net_quorum.h
  • src/active/dkgsessionhandler.cpp
  • src/llmq/net_quorum.cpp
  • configure.ac

Comment thread src/bitcoin-chainstate.cpp

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Carried-forward prior finding (mn_sync arg in LoadChainstate call) is now RESOLVED — the offending positional argument was removed in this delta and the signature matches the post-#7212 LoadChainstate. The only net-new change is adding $(BOOST_CPPFLAGS) to dash_chainstate_CPPFLAGS in src/Makefile.am, which legitimately addresses missing Boost include flags needed by dash-chainstate. No new in-scope issues in the latest delta.

Comment thread src/Makefile.am Outdated
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Incremental review from ff7a110 to 72b66af. No carried-forward prior findings (the previously-resolved LoadChainstate signature issue remains correctly addressed — verified no regression). The only delta in 72b66af adds $(GMP_LIBS) to dash_chainstate_LDADD, which is a correct fix since LIBDASHBLS depends on GMP and matches the existing libdashconsensus_la_LIBADD pattern. One nitpick-level observation on the Dash-specific GetPrettyExceptionStr stub introduced earlier in this PR remains worth noting.

💬 1 nitpick(s)

Comment on lines +61 to +71
std::string GetPrettyExceptionStr(const std::exception_ptr& e)
{
try {
// rethrow and catch the exception as there is no other way to reliably cast to the real type (not possible with RTTI)
std::rethrow_exception(e);
} catch (const std::exception& e2) {
return e2.what();
} catch (...) {
throw;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: GetPrettyExceptionStr stub re-throws on non-std exceptions

This Dash-specific simplified replacement for GetPrettyExceptionStr re-throws any non-std::exception via throw; in the catch(...) branch. The real implementation in src/stacktraces.cpp:737 routes through GetCrashInfoFromException and never propagates. Because this helper is called from PrintExceptionContinue (src/util/system.cpp:866) — which is best-effort error logging — re-throwing here would abort the reporting path and obscure the original failure if a non-std::exception ever reaches it. The binary is experimental and demo-only so practical impact is small, but returning a fallback string is a trivial improvement that matches the upstream contract.

Suggested change
std::string GetPrettyExceptionStr(const std::exception_ptr& e)
{
try {
// rethrow and catch the exception as there is no other way to reliably cast to the real type (not possible with RTTI)
std::rethrow_exception(e);
} catch (const std::exception& e2) {
return e2.what();
} catch (...) {
throw;
}
}
std::string GetPrettyExceptionStr(const std::exception_ptr& e)
{
if (!e) {
return "unknown exception";
}
try {
// rethrow and catch the exception as there is no other way to reliably cast to the real type (not possible with RTTI)
std::rethrow_exception(e);
} catch (const std::exception& e2) {
return e2.what();
} catch (...) {
return "unknown non-std exception";
}
}

source: ['codex']

@UdjinM6 UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 72b66af

@PastaPastaPasta PastaPastaPasta merged commit baac304 into dashpay:develop Jun 5, 2026
43 of 46 checks passed
PastaPastaPasta added a commit that referenced this pull request Jun 5, 2026
58ab8b3 fix: register NetGovernance unconditionally and tie SuperblockManager lifetime to govman (UdjinM6)
28f339e refactor: move validators to governance/object.h; remove lonely empty header and cpp (Konstantin Akimov)
8d4989f refactor: replace class CProposalValidator with a simple function (Konstantin Akimov)
7b08205 refactor: drop friend CGovernanceObject from CGovernanceManager (Konstantin Akimov)
853c81d refactor: pass reference to CDeterministicMNManager instead of reference to unique_ptr (Konstantin Akimov)
abced0f refactor: simplify after governance refactoring: drop unused forward declarations & includes, move more network code to NetGovernance (Konstantin Akimov)
c08fcae refactor: merge governance/exceptions into governance/object where it is used (Konstantin Akimov)
2366761 refactor: move code from governance/classes to governance/superblock where it belongs (Konstantin Akimov)
8579a1a refactor: cleanup governance/governance includes and forward declarations (Konstantin Akimov)
b7baf0b fmt: clang-apply (Konstantin Akimov)
9237c62 refactor: break circular dependency between governance/object <-> governance/governance (Konstantin Akimov)
95d5dcc refactor: extract SuperblockManager from CGovernanceManager (Konstantin Akimov)
57ba90f refactor: drop useless GovernanceSignerParent (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  The `dash-chainstate` (see #7234) requires as dependency full governance implementation, even if it is not functional or relevant to dash-chainstate (the governance uses off-chain objects and they can't be retrieved without p2p communication).

  It brings extra files to compilation unit of kernel / dash-chainstate:

      governance/classes.cpp \
      governance/common.cpp \
      governance/exceptions.cpp \
      governance/governance.cpp \
      governance/object.cpp \
      governance/validators.cpp \
      governance/vote.cpp \
      governance/votedb.cpp \

  Also, Governance blocks backporting https://github.com/bitcoin/bitcoin/pull/25064/files because it uses timedata.cpp

  Also, Governance requires extra dependency of node/txindex (and recursively index/base which is out-of-scope for kernel)

  ## What was done?
  The only usage of governance in coinsensus code is a validation of superblocks (CMNPaymentsProcessor); superblock validator has been extracted from CGovernanceManager to own file and class `governance::SuperblockManager`.
  It can be simply replaced to stub NullSuperblockmanager, same as `class NullNodeSyncNotifier final : public NodeSyncNotifier` for kernel project.

  There are also several relevant refactorings:
   - governance/exception files are removed; `CGovernanceException` moved to `governance/object.h` where it's used
   - governance/validators files are removed; the class `CProposalValidator` is replaced by a simple helper `governance::ValidateProposal` and moved to `governance/object.h'
   - the dependency CGovermentManager -> GovernanceSigner is reverted; `CGovermentManager` should not know anything about signer if it's not a masternode. Abstract interface `GovernanceSignerParent` is removed.

  ## How Has This Been Tested?
  2 circular dependencies disappeared:
  -    "governance/classes -> governance/object -> governance/governance -> governance/classes",
  -    "governance/governance -> governance/signing -> governance/object -> governance/governance",

  Unit & functional tests succeed.
  Mock proposal is created on testnet with Dash-Qt.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 58ab8b3

Tree-SHA512: 1090981c656d4ba98e8676bf38a8e22d837b7830837406f58532208021d212662028899348e474436a06262be4548bb48e6433ec10f2a6e105e41434fdeae406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants