refactor: move ownerships of mnhfman, cpoolman inside CChainstateHelper where it belongs to #7104
Conversation
…from detemrinisticmns to specialtxman
|
WalkthroughThis PR refactors the dependency management and initialization of masternode-related managers in the Dash codebase. The primary changes involve removing Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/node/miner.cpp (2)
295-313: Avoid potential null-deref onm_chain_helper.credit_pool_managerin V20 coinbase path.
Ifcredit_pool_manageris ever optional (e.g., certain test setups / partial init),*m_chain_helper.credit_pool_managerwill crash. Consider making the invariant explicit before deref.Proposed fix
if (CalcCbTxBestChainlock(m_clhandler, pindexPrev, cbTx.bestCLHeightDiff, cbTx.bestCLSignature)) { LogPrintf("CreateNewBlock() h[%d] CbTx bestCLHeightDiff[%d] CLSig[%s]\n", nHeight, cbTx.bestCLHeightDiff, cbTx.bestCLSignature.ToString()); } else { // not an error LogPrintf("CreateNewBlock() h[%d] CbTx failed to find best CL. Inserting null CL\n", nHeight); } BlockValidationState state; + CHECK_NONFATAL(m_chain_helper.credit_pool_manager); const auto creditPoolDiff = GetCreditPoolDiffForBlock(*m_chain_helper.credit_pool_manager, m_blockman, m_qman, *pblock, pindexPrev, chainparams.GetConsensus(), blockSubsidy, state); if (creditPoolDiff == std::nullopt) { throw std::runtime_error(strprintf("%s: GetCreditPoolDiffForBlock failed: %s", __func__, state.ToString())); }
461-475: Documentcs_mainlocking requirement withAssertLockHeld(::cs_main), but remove redundant null checks.The concern about hidden locking assumptions is valid—
addPackageTxs()calls manager methods (credit_pool_manager->GetCreditPool()andehf_manager->GetSignalsStage()) but only documentsmempool.csas required. Currently invoked undercs_mainfromCreateNewBlock(), but a future refactor could accidentally drop this lock.However, the managers are initialized as
const std::unique_ptrfields inCChainstateHelperand cannot be null, makingCHECK_NONFATALredundant. Add only:void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated, const CBlockIndex* const pindexPrev) { AssertLockHeld(mempool.cs); + AssertLockHeld(::cs_main);This documents the implicit assumption without false null-safety checks.
🤖 Fix all issues with AI agents
In `@src/evo/specialtxman.cpp`:
- Around line 761-785: CheckService currently asserts on
NetInfoStatus::BadInput, ::Duplicate and ::MaxLimit leaving no return in
non-debug builds; change those assert branches to return
state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "<appropriate-error>") with
distinct error strings (e.g. "bad-protx-netinfo-badinput",
"bad-protx-netinfo-duplicate", "bad-protx-netinfo-maxlimit") so every
NetInfoStatus path returns a deterministic validation failure; ensure the final
assert(false) is removed and all enum cases map to a return to avoid undefined
behavior for peer-supplied data in the CheckService template.
In `@test/lint/lint-circular-dependencies.py`:
- Around line 38-46: The EXPECTED_CIRCULAR_DEPENDENCIES allowlist contains six
stale evo/* entries that the lint tool reports as no longer present; remove the
following strings from the list: "evo/chainhelper -> evo/creditpool ->
validation -> evo/chainhelper", "evo/deterministicmns -> evo/providertx ->
validation -> evo/deterministicmns", "evo/deterministicmns -> evo/providertx ->
validation -> txmempool -> evo/deterministicmns", "evo/netinfo -> evo/providertx
-> evo/netinfo", "evo/netinfo -> evo/providertx -> validation -> txmempool ->
evo/netinfo", and "evo/providertx -> validation -> txmempool -> evo/providertx"
from the EXPECTED_CIRCULAR_DEPENDENCIES array so the allowlist matches the lint
output.
🧹 Nitpick comments (3)
src/evo/specialtxman.cpp (1)
857-912: Minor: tighten semantics aroundgsl::not_null+ fix misleading comment.
GetValidatedPayload(..., gsl::not_null<const CBlockIndex*> ...)implies non-null, but some later code paths still treatpindexPrevas optional (e.g.,if (pindexPrev)in callers). Consider removing the redundant null-check branches to reduce confusion.- In
IsVersionChangeValid(), the ExtAddr comment mentions ProRegTx, but this helper is only used by update txs here; consider adjusting the comment to reflect actual call sites.src/test/util/setup_common.cpp (1)
144-147: UseAssert(node.evodb.get())before dereferencing.This wrapper dereferences
*node.evodbwithout the usualAssert(...)guard used for other NodeContext members. If any test harness constructs NodeContext without evodb, this becomes a crash.Proposed fix
- DashChainstateSetup(chainman, *Assert(node.govman.get()), *Assert(node.mn_metaman.get()), *Assert(node.mn_sync.get()), - *Assert(node.sporkman.get()), node.chain_helper, node.dmnman, *node.evodb, + DashChainstateSetup(chainman, *Assert(node.govman.get()), *Assert(node.mn_metaman.get()), *Assert(node.mn_sync.get()), + *Assert(node.sporkman.get()), node.chain_helper, node.dmnman, *Assert(node.evodb.get()), node.llmq_ctx, Assert(node.mempool.get()), node.args->GetDataDirNet(), llmq_dbs_in_memory, llmq_dbs_wipe, llmq::DEFAULT_BLSCHECK_THREADS, llmq::DEFAULT_MAX_RECOVERED_SIGS_AGE, consensus_params);src/evo/specialtxman.h (1)
93-104: NewCheckPro*Txdeclarations are OK; consider scoping to reduce global namespace pollution.These declarations match the implementations and help decouple deterministicmns from validation plumbing. If feasible later, consider scoping them (e.g., a dedicated namespace or making them
static/member utilities) to keep the header’s global surface smaller.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
src/active/context.cppsrc/active/context.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/mnhftx.cppsrc/evo/mnhftx.hsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/init.cppsrc/llmq/context.hsrc/llmq/ehf_signals.cppsrc/llmq/ehf_signals.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/context.hsrc/node/miner.cppsrc/node/miner.hsrc/rpc/blockchain.cppsrc/rpc/rawtransaction.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cpptest/lint/lint-circular-dependencies.py
💤 Files with no reviewable changes (5)
- src/node/miner.h
- src/node/context.h
- src/llmq/context.h
- src/evo/deterministicmns.cpp
- src/evo/deterministicmns.h
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/test/evo_deterministicmns_tests.cppsrc/node/miner.cppsrc/llmq/ehf_signals.hsrc/active/context.hsrc/rpc/blockchain.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/llmq/ehf_signals.cppsrc/test/util/setup_common.cppsrc/init.cppsrc/rpc/rawtransaction.cppsrc/evo/mnhftx.cppsrc/node/chainstate.cppsrc/evo/mnhftx.hsrc/node/chainstate.hsrc/active/context.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cpp
src/node/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Files:
src/node/miner.cppsrc/node/chainstate.cppsrc/node/chainstate.h
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/llmq/ehf_signals.hsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/llmq/ehf_signals.cppsrc/evo/mnhftx.cppsrc/evo/mnhftx.h
src/{masternode,llmq}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
BLS integration must be used for cryptographic foundation of advanced masternode features
Files:
src/llmq/ehf_signals.hsrc/llmq/ehf_signals.cpp
src/llmq/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
src/llmq/**/*.{cpp,h}: LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Files:
src/llmq/ehf_signals.hsrc/llmq/ehf_signals.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/llmq/ehf_signals.hsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/llmq/ehf_signals.cppsrc/evo/mnhftx.cppsrc/evo/mnhftx.h
src/{masternode,evo}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/mnhftx.cppsrc/evo/mnhftx.h
src/evo/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Files:
src/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/mnhftx.cppsrc/evo/mnhftx.h
src/node/chainstate.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Files:
src/node/chainstate.cppsrc/node/chainstate.h
🧠 Learnings (36)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request `#6543` is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR `#6761`, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (`#6718`), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/**/*.{cpp,h} : NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: In Dash Core, `DEFAULT_SYNC_MEMPOOL` belongs with node synchronization logic (currently in src/masternode/sync.h) rather than with validation logic in src/validation.h, even though other mempool-related constants are in validation.h. The constant is conceptually tied to sync functionality, not validation.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cppsrc/init.cpp
📚 Learning: 2025-11-13T20:02:55.480Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.480Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
Applied to files:
src/test/evo_deterministicmns_tests.cppsrc/node/miner.cppsrc/evo/chainhelper.hsrc/init.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/test/evo_deterministicmns_tests.cppsrc/node/miner.cppsrc/rpc/blockchain.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/test/util/setup_common.cppsrc/init.cppsrc/rpc/rawtransaction.cppsrc/node/chainstate.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/test/evo_deterministicmns_tests.cppsrc/node/miner.cppsrc/rpc/blockchain.cppsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/test/util/setup_common.cppsrc/init.cppsrc/rpc/rawtransaction.cppsrc/evo/mnhftx.cppsrc/node/chainstate.cppsrc/evo/mnhftx.hsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/test/evo_deterministicmns_tests.cppsrc/node/miner.cppsrc/rpc/blockchain.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/evo/chainhelper.hsrc/test/util/setup_common.cppsrc/init.cppsrc/rpc/rawtransaction.cppsrc/node/chainstate.cppsrc/evo/mnhftx.hsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Applied to files:
src/test/evo_deterministicmns_tests.cppsrc/llmq/ehf_signals.hsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/evo/mnhftx.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/test/evo_deterministicmns_tests.cppsrc/node/miner.cppsrc/llmq/ehf_signals.hsrc/rpc/blockchain.cppsrc/evo/specialtxman.cppsrc/test/util/setup_common.cppsrc/init.cppsrc/rpc/rawtransaction.cppsrc/node/chainstate.cppsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/node/miner.cppsrc/rpc/blockchain.cppsrc/evo/specialtxman.cppsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/test/util/setup_common.cppsrc/init.cppsrc/rpc/rawtransaction.cppsrc/node/chainstate.cppsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/node/miner.cppsrc/test/util/setup_common.cppsrc/node/chainstate.cppsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/node/miner.cppsrc/rpc/blockchain.cppsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/test/util/setup_common.cppsrc/init.cppsrc/rpc/rawtransaction.cppsrc/node/chainstate.cppsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/node/miner.cppsrc/rpc/blockchain.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/llmq/ehf_signals.cpptest/lint/lint-circular-dependencies.py
📚 Learning: 2025-11-04T18:23:28.175Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/governance/classes.cpp:147-154
Timestamp: 2025-11-04T18:23:28.175Z
Learning: In src/governance/classes.cpp, CSuperblock::GetPaymentsLimit intentionally uses synthetic difficulty constants (nBits = 1 for mainnet, powLimit for networks with min difficulty) and simple height-based V20 activation checks instead of actual chain block data. This is by design because superblocks themselves are "synthetic" governance payment blocks, not regular mined blocks.
Applied to files:
src/node/miner.cppsrc/rpc/rawtransaction.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : LLMQ quorums must support multiple configurations (50/60, 400/60, 400/85) for different services (ChainLocks, InstantSend, governance voting)
Applied to files:
src/llmq/ehf_signals.hsrc/llmq/ehf_signals.cppsrc/evo/mnhftx.cppsrc/evo/mnhftx.hsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Applied to files:
src/llmq/ehf_signals.hsrc/evo/specialtxman.cppsrc/llmq/ehf_signals.cppsrc/rpc/rawtransaction.cppsrc/evo/mnhftx.hsrc/node/chainstate.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/ehf_signals.hsrc/evo/mnhftx.cppsrc/evo/mnhftx.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/ehf_signals.hsrc/active/context.hsrc/test/util/setup_common.cppsrc/init.cppsrc/evo/mnhftx.cppsrc/evo/mnhftx.hsrc/active/context.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/ehf_signals.hsrc/evo/mnhftx.h
📚 Learning: 2025-12-22T15:42:48.595Z
Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.
Applied to files:
src/llmq/ehf_signals.hsrc/active/context.hsrc/evo/specialtxman.hsrc/evo/chainhelper.hsrc/evo/mnhftx.hsrc/node/chainstate.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
Applied to files:
src/active/context.hsrc/active/context.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/active/context.hsrc/test/util/setup_common.cppsrc/init.cppsrc/node/chainstate.cppsrc/node/chainstate.hsrc/active/context.cpp
📚 Learning: 2025-12-02T12:59:28.247Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7026
File: src/rpc/evo.cpp:1713-1799
Timestamp: 2025-12-02T12:59:28.247Z
Learning: In the Dash codebase, CChain::operator[] has built-in safe bounds checking. Accessing chainman.ActiveChain()[height] is safe even if height exceeds the current chain height, as the operator will handle bounds checking internally. There is no need to manually check chain height before using the [] operator on CChain.
Applied to files:
src/rpc/blockchain.cppsrc/rpc/rawtransaction.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/specialtxman.cppsrc/evo/specialtxman.h
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/evo/specialtxman.cppsrc/evo/specialtxman.h
📚 Learning: 2025-06-16T17:59:55.669Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
Applied to files:
src/evo/specialtxman.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Applied to files:
src/evo/chainhelper.hsrc/test/util/setup_common.cppsrc/init.cppsrc/node/chainstate.cppsrc/node/chainstate.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/evo/chainhelper.hsrc/init.cppsrc/node/chainstate.h
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (`#6718`), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/test/util/setup_common.cpp
📚 Learning: 2025-10-28T18:36:40.263Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/test/util/setup_common.cppsrc/node/chainstate.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/test/util/setup_common.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/test/util/setup_common.cppsrc/init.cppsrc/node/chainstate.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/**/*.{cpp,h} : NodeContext must be extended with Dash-specific managers for each subsystem during initialization
Applied to files:
src/test/util/setup_common.cppsrc/init.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/rawtransaction.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/rpc/rawtransaction.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR `#6692`, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR `#6789` to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/evo/mnhftx.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/node/chainstate.h
🧬 Code graph analysis (10)
src/node/miner.cpp (2)
src/evo/creditpool.cpp (3)
creditPoolDiff(332-332)GetCreditPoolDiffForBlock(325-349)GetCreditPoolDiffForBlock(325-327)src/evo/creditpool.h (1)
pindexPrev(81-109)
src/llmq/ehf_signals.h (1)
src/llmq/ehf_signals.cpp (2)
CEHFSignalsHandler(21-29)CEHFSignalsHandler(31-34)
src/evo/specialtxman.cpp (3)
src/evo/netinfo.cpp (2)
IsNodeOnMainnet(71-71)IsNodeOnMainnet(71-71)src/deploymentstatus.h (3)
DeploymentActiveAfter(14-18)DeploymentActiveAfter(20-24)DeploymentDIP0003Enforced(53-56)src/chainparams.cpp (2)
Params(1345-1348)Params(1345-1345)
src/evo/specialtxman.h (1)
src/evo/specialtxman.cpp (8)
CheckProRegTx(914-1048)CheckProRegTx(914-916)CheckProUpServTx(1050-1127)CheckProUpServTx(1050-1051)CheckProUpRegTx(1129-1201)CheckProUpRegTx(1129-1131)CheckProUpRevTx(1203-1233)CheckProUpRevTx(1203-1204)
src/evo/chainhelper.cpp (2)
src/evo/chainhelper.h (1)
CChainstateHelper(35-71)src/evo/mnhftx.cpp (2)
GetSignalsStage(61-92)GetSignalsStage(61-61)
src/evo/chainhelper.h (3)
src/evo/creditpool.cpp (2)
CCreditPoolManager(241-245)CCreditPoolManager(247-247)src/evo/chainhelper.cpp (4)
CChainstateHelper(17-30)CChainstateHelper(32-32)GetSignalsStage(68-71)GetSignalsStage(68-68)src/evo/mnhftx.cpp (4)
GetSignalsStage(61-92)GetSignalsStage(61-61)CMNHFManager(46-53)CMNHFManager(55-59)
src/llmq/ehf_signals.cpp (1)
src/llmq/ehf_signals.h (1)
CEHFSignalsHandler(21-50)
src/test/util/setup_common.cpp (1)
src/node/chainstate.cpp (2)
DashChainstateSetupClose(241-251)DashChainstateSetupClose(241-244)
src/node/chainstate.cpp (1)
src/test/util/setup_common.cpp (2)
DashChainstateSetup(138-148)DashChainstateSetup(138-142)
src/evo/mnhftx.h (1)
src/evo/mnhftx.cpp (2)
CMNHFManager(46-53)CMNHFManager(55-59)
🪛 Cppcheck (2.19.0)
src/evo/specialtxman.cpp
[error] 22-22: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 24-24: #error No known always_inline attribute for this platform.
(preprocessorErrorDirective)
src/evo/chainhelper.cpp
[error] 22-22: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
src/llmq/ehf_signals.cpp
[error] 10-10: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (32)
src/test/evo_deterministicmns_tests.cpp (1)
10-16: Looks good: test now includes the new ProTx validation home header.
This aligns with the ProTx validation move toevo/specialtxman.*and helps keep the unit tests compiling against the refactor.src/node/miner.cpp (1)
28-35: Include move looks fine.src/active/context.h (1)
64-71: Constructor signature is consistent across header, definition, and call site—no issues found.Header declaration (lines 64–71), definition in
src/active/context.cpp(lines 26–33), and the construction site insrc/init.cpp(lines 2199–2202) all match exactly. All 21 parameters are passed in the correct order, and the removal ofCMNHFManageris properly reflected everywhere (only a forward declaration remains, no orphaned member references).src/rpc/blockchain.cpp (1)
19-20: No actionable issues. TheGetSignalsStage()call is safe and performant:
- Lock-safe: Annotated with
EXCLUSIVE_LOCKS_REQUIRED(!cs_cache), ensuring proper lock design. Calling undercs_maindoes not conflict with the lock contract.- Performance: Implementation is lightweight (block traversal and consensus checks via
GetForBlock()andGetAncestor()calls), with no file I/O or expensive operations.src/evo/specialtxman.cpp (3)
7-29: New includes look reasonable; watch for avoidable coupling.No issues spotted with the added headers in this file; they appear required for the moved validation code.
787-827: ConfirmMainParams()usage is intended for non-mainnet validation.
CheckPlatformFields()usesMainParams()for some “port must not equal core port” checks even when not on mainnet. That may be intentional, but it’s easy to accidentally bake in mainnet defaults for test/dev networks.Suggested verification:
- confirm which networks these rules are consensus-enforced on (mainnet-only vs all nets)
- confirm
NetInfoInterface::GetPrimary().GetPort()behavior whennetInfois empty and how that interacts with these checks
914-1233: Per-type ProTx validation entry points look cleanly separated.The
CheckPro*Tx()split + shared helpers is a good direction for breaking circular deps and centralizing validation entry points. OnceCheckService()is hardened (above), the rest of these flows look consistent with the existing structure (payload parse → trivial validity → semantic checks → inputsHash → sig).src/init.cpp (2)
403-405: Verify shutdown ordering insideDashChainstateSetupClose()vsCTxMemPool::DisconnectManagers().Given
DashChainstateSetupClose()now resetsllmq_ctxinternally (per snippet), please confirmmempool->DisconnectManagers()doesn’t touch llmq-owned objects afterllmq_ctx.reset(). If it does, reorder to disconnect before resetting llmq.
2199-2202: ActiveContext construction update looks consistent with the refactor intent.Call-site update aligns with removing direct mnhf_manager plumbing from init.
src/test/util/setup_common.cpp (1)
152-154: Wrapper update looks fine; keep the teardown invariants explicit.Given tests often mutate init/teardown sequences, keeping this wrapper minimal is good.
src/evo/mnhftx.h (1)
92-99: Fix typo in TODO comment on line 92: "functionallity" → "functionality"This is a simple spelling error in the TODO block.
Likely an incorrect or invalid review comment.
src/evo/chainhelper.cpp (3)
10-11: LGTM: Required includes added for manager ownership.The includes for
creditpool.handmnhftx.hare necessary sinceCChainstateHelpernow owns and constructs these manager instances directly.
17-30: LGTM: Constructor correctly initializes owned managers.The constructor properly:
- Accepts
CEvoDB& evodbto construct the managers internally- Initializes
ehf_managerandcredit_pool_managerviastd::make_uniquebeforespecial_txwhich depends on them- Passes dereferenced managers to
CSpecialTxProcessorThe initializer list order matches the member declaration order in the header, which is correct C++ practice.
68-71: Remove this suggestion—the method cannot be const.The
GetSignalsStagemethods in bothCMNHFManagerandCChainstateHelperare intentionally non-const. The abstract base class inversionbits.hexplicitly documents: "This member function is not const because it calls non-constGetFromCache()." The implementation callsGetForBlock(), which requires access to non-const cache operations. Making these methods const would violate the virtual interface contract and break the implementation.Likely an incorrect or invalid review comment.
src/evo/chainhelper.h (3)
10-15: LGTM: Forward declarations and includes properly added.The additions support the new public API:
<unordered_map>forGetSignalsStagereturn typeCBlockIndexforward declaration for the method parameterCEvoDBforward declaration for the constructor parameter
45-50: LGTM: Constructor signature simplified by internalizing manager dependencies.The constructor now takes
CEvoDB& evodband constructsehf_manager/credit_pool_managerinternally, reducing the external dependency surface. This aligns with the PR objective of moving ownership intoCChainstateHelper.
64-68: Publicconst unique_ptrmembers expose managers appropriately.The
constqualifier on theunique_ptrmembers prevents reassignment while still allowing read access and method calls on the managed objects. This is a reasonable pattern for exposing owned resources.Minor: Consider whether
GetSignalsStageshould beconst(see related comment in chainhelper.cpp).src/active/context.cpp (1)
26-43: LGTM:CMNHFManagerdependency removed fromActiveContext.The constructor signature and
CEHFSignalsHandlerinitialization correctly remove themnhfmanparameter. This aligns with the refactor moving EHF manager access through the chain helper pathway. TheCEHFSignalsHandlerwill now obtain EHF signals viaChainstateManagerrather than a direct reference.src/node/chainstate.cpp (4)
84-86: LGTM:LoadChainstatecorrectly passesevodbto setup.The call to
DashChainstateSetupnow passes*evodb(dereferenced) instead of separate manager pointers. This reflects the API change where managers are now constructed internally byCChainstateHelper.
194-196: LGTM: Signal DB update accessed through chain helper.
ForceSignalDBUpdateis now correctly accessed viachain_helper->ehf_manager, reflecting the new ownership model whereCMNHFManageris owned byCChainstateHelper.
206-238: LGTM:DashChainstateSetupsignature and implementation simplified.The function now:
- Takes
CEvoDB&by reference (appropriate since caller owns the evodb lifetime)- Constructs
dmnman,llmq_ctx, andchain_helperusing the evodb reference- No longer manages
cpoolmanormnhf_managerseparately since they're owned bychain_helperThe initialization order (
dmnman→llmq_ctx→chain_helper) is correct sincechain_helperdepends on components fromllmq_ctx.
241-251: Destruction order is correct and safe.The current order (chain_helper → llmq_ctx → mempool → dmnman) correctly handles the dependency: chain_helper holds raw references to llmq_ctx-owned components (isman and clhandler), so destroying chain_helper first ensures it no longer accesses these references before their owners are destroyed. The ehf_manager and credit_pool_manager owned by chain_helper are not accessed by llmq_ctx.
src/node/chainstate.h (2)
14-23: LGTM: Forward declarations cleaned up.The removed forward declarations for
CCreditPoolManagerandCMNHFManagercorrectly reflect that these types are no longer part of the public API signatures. They're now internal toCChainstateHelper.
108-128: LGTM: API signatures simplified.The header changes correctly reflect:
DashChainstateSetupnow takesCEvoDB&by reference instead ofunique_ptrDashChainstateSetupCloseno longer requirescpoolmanormnhf_managerparametersThis API simplification aligns with the PR objective of internalizing manager ownership within
CChainstateHelper. As per coding guidelines, chainstate initialization is properly separated in these dedicated files.src/rpc/rawtransaction.cpp (1)
49-56: Include looks fine; just watch dependency creep.
Adding<evo/chainhelper.h>is consistent with the refactor, but it does broaden RPC compile-time dependencies. Given this PR already updatedlint-circular-dependencies.py, this is probably intended.src/llmq/ehf_signals.h (1)
34-37: Ctor signature simplification is consistent with the new ownership model.
This aligns the handler with chainstate-owned EHF manager access and reduces plumbing through callers.src/evo/mnhftx.cpp (3)
204-212: ProcessBlock wiring tom_qmanlooks correct.
This keeps the verification path explicit and avoids pointer/atomic indirection.
253-260: UndoBlock wiring tom_qmanlooks correct.
Same comment as ProcessBlock—cleaner and easier to reason about.
46-53: Good refactor. Storingconst llmq::CQuorumManager&as a member eliminates the need forconnect/disconnecthandlers. The lifetime guarantee is sound:LLMQContextis initialized beforeCChainstateHelper(which ownsCMNHFManager), and destroyed after it, ensuring the referencedCQuorumManageroutlivesCMNHFManagerthroughout its lifetime.src/llmq/ehf_signals.cpp (3)
7-29: Ctor + include updates are consistent with the new access path.
Signature matchessrc/llmq/ehf_signals.hand removes the old direct manager dependency.
84-100: ehf_manager access is safe and consistent across the codebase.The
ehf_manageris aconst std::unique_ptr<CMNHFManager>member initialized in the constructor and cannot be null. The access pattern used inHandleNewRecoveredSigmatches the pattern throughout the codebase (miner.cpp, chainstate.cpp) without null checks, indicating this is intentional design. No preprocessor or cppcheck warnings were identified for this file in the current configuration.
36-49: No blocking issue identified; ehf_manager lifecycle is properly managed.The
ehf_manageris initialized viastd::make_uniquein the ChainHelper constructor and cannot be null. The ChainHelper() accessor includes an assert guard, and the CEHFSignalsHandler properly manages listener registration/unregistration through RAII (constructor registers, destructor unregisters). The code is safe as-is and poses no crashing risk.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
knst
changed the title
knst
changed the title
2 participants
Issue being fixed or feature implemented
LoadChainState is aware of EHF Manager and Credit Pool Manager that are constructed outside, but have no meaning without Chain State; and its ownership and life time is controlled by Chain State too.
What was done?
CMNHFManager), Credit Pool Manager (CCreditPoolManagerCMNHFManager::ConnectManagersm_qmaninCMNHFManagerTODO: this PR includes some extra refactorings with code move from deterministicmns.h, that's a preparation for move
dmnman(CDeterministicMNManager) inside Chain State too. Though, there's too many usages ofNodeContext::dmnmanall over code base, and many of its usages are not aware about Chain State at the moment; it will requires significant refactoring all over code base and out-of-scope of this PR; will be done in the further PRs with some extra efforts. Also, LLMQContext depends on dmnman; chainhelper depends on llmq-context; dmnman can't be easily moved inside chainhelper without moving llmq-ctx inside, which is out-of-scope of this PR.How Has This Been Tested?
Run unit and functional tests, run lint-circular-dependencies.py
Breaking Changes
N/A
Checklist: