backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce bitcoin-chainstate#7234
Draft
knst wants to merge 26 commits intodashpay:developfrom
Draft
backport: bitcoin/bitcoin#24304: [kernel 0/n] Introduce bitcoin-chainstate#7234knst wants to merge 26 commits intodashpay:developfrom
bitcoin-chainstate#7234knst wants to merge 26 commits intodashpay:developfrom
Conversation
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
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
…PackageSelection, final part)
There's new helper IsValidAndSynced in CGovernanceManager that incapsulate usages of CMasternodeSync in CMNPayment
…otifier is introduced to use it for CChainState
…r.cpp for chain-state
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
…avoid extra heavy dependencies Implemented: - ValueFromAmount (core_write.cpp) - GetPrettyExceptionStr (stacktrackes.cpp)
…dex dependency for linking
…posed to be but to fix linkage errors
…ssage for detailed notes about further todo-to-remove TODO to remove: base58.cpp \ core_write.cpp \ evo/core_write.cpp \ key_io.cpp \ node/interface_ui.cpp \ protocol.cpp \ scheduler.cpp \ node/bloom.cpp \ index/txindex.cpp \ batchedlogger.cpp \ Reduce governance significantly so far as there's no actual governance enabled Reduce llmq: dkgsession, dkgsessionmgr, dkgsessionohandler, debug Remove CSigningManager from LLMQContext. LLMQContext should be quorum-only-context
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
This PR sum up all changes that are required to backport bitcoin#24304 and proof-of-concept that it is possible after just couple extra tiny steps.
There are list of PR that are dependencies of this one:
What was done?
Beside changes that already has been merged or having PRs such as #7178 [in review], #7070, #7008, #6992, #6959, #7106 (by Udjin), #7065 (by kwvg) and many other PRs:
bitcoin-chainstatebitcoin/bitcoin#24304 which unblocks the whole multiple series of all kernel backportsThere are several dirty hacks that meant to be fixed and they will be done by extra PRs; local branches for these fixes are semi-ready:
CDKGSessionManagershould be split to network & non-network pieces. For right now a workaround for linkage is used by moving some code to net_processing.cppCQuorumManager::RequestQuorumDatashould be moved away to avoid network dependenciesThere are several dependencies of dash-chainstate that looks excessive, but they are not a blocker and may be removed in any time in the future:
- base58.cpp
- core_write.cpp
- evo/core_write.cpp
- key_io.cpp
- node/interface_ui.cpp
- protocol.cpp
- scheduler.cpp
- node/bloom.cpp
- index/txindex.cpp
- batchedlogger.cpp
- Reduce governance significantly so far as there's no actual governance enabled in chainstate mode (governance is out of blockchain anyway)
- Reduce llmq dependencies, probably to drop: dkgsession, dkgsessionmgr, dkgsessionohandler, debug
- Remove CSigningManager from LLMQContext. LLMQContext should be quorum-only-context; CSigningManager should be inside active/
How Has This Been Tested?
Build succeed with bitcoin#24304, functional & unit tests runs
Run dash-chainstate:
Breaking Changes
N/A
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.