refactor: remove llmq::chainLocksHandler, llmq::quorum{InstantSend,Snapshot}Manager, move to LLMQContext
#6504
Conversation
kwvg
changed the title
llmq::chainLocksHandler, llmq::quorum{InstantSend,Snapshot}Manager global, move to LLMQContextllmq::chainLocksHandler, llmq::quorum{InstantSend,Snapshot}Manager, move to LLMQContext
…educe flakiness c67d5ee test: increase timeout in `p2p_invalid_messages.py` to reduce flakiness (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6504 * `p2p_invalid_messages.py` has a tendency to occasionally fail and when working on [dash#6504](#6504), the test had a higher failure rate than usual (despite the rest of the test suite passing and with exception to the `test_buffer` subtest, the rest of `p2p_invalid_messages.py` passing) This pull request bumps the timeout from 1s to 2s only in the `test_buffer` subtest. ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas (note: N/A) - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation (note: N/A) - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK c67d5ee knst: utACK c67d5ee UdjinM6: utACK c67d5ee Tree-SHA512: 88c13efad733d4685c65343fd40e1b80603c24955dd3f0f97aceedef8dd4e060d6e2bb3c3a1a2f18f288dc2ef55ec3f4df6a18b524674c2964ecc1724b6adf00
|
This pull request has conflicts, please rebase. |
WalkthroughThe pull request introduces significant architectural changes to the Dash Core codebase, focusing on the integration of a new The primary objective appears to be enhancing the modularity and management of quorum-related data by introducing a dedicated snapshot management mechanism. This is achieved by adding the Key changes include:
These modifications suggest a comprehensive refactoring of the quorum management system, aimed at improving code organization, flexibility, and maintainability. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (66)
src/evo/chainhelper.h (2)
41-43: Constructor Parameter Ordering
Check that parameter ordering is consistent across the codebase. If changing future constructors, maintain consistent ordering for better readability.
54-59: InstantSendManager Passthrough and Optional Usage
Returning anstd::optional<std::pair<uint256, uint256>>is clear. Consider creating a small struct instead of a pair for improved readability.src/llmq/utils.h (2)
34-37: GetAllQuorumMembers with qsnapman
Ensure thatqsnapmanusage is required. If not strictly needed, consider a lighter signature.
41-43: GetQuorumConnections
Parameter list is getting long. Consider grouping related parameters in a structure if it grows further.src/evo/specialtxman.cpp (1)
148-148: CheckSpecialTxInner in Block Loop
Looks correct. Consider error logging if repeated calls fail.src/node/chainstate.cpp (2)
231-231: Null Check for LLMQ Context
When callingmempool->ConnectManagers(dmnman.get(), llmq_ctx->isman.get()), ensure thatllmq_ctxand itsismanfield are not null. If there's any scenario in whichllmq_ctxmight remain uninitialized, add the necessary checks or initialization routines.
236-238: Constructor Parameter Overload
TheCChainstateHelperconstructor is given numerous parameters, including multiple references fromllmq_ctx. This can reduce readability and maintainability. Consider refactoring into a builder pattern or a structured configuration object to reduce the potential for confusion and parameter mismatch.src/llmq/dkgsessionmgr.cpp (2)
299-299: Quorum Members Retrieval
auto members = utils::GetAllQuorumMembers(llmqType, m_dmnman, m_qsnapman, pQuorumBaseBlockIndex);now usesm_qsnapmanfor retrieving quorum membership data. Ensure that all usage of the old approach (without snapshots) has been fully replaced.
345-345: Encrypted Contributions with Snapshot Manager
utils::GetAllQuorumMembers(llmqType, m_dmnman, m_qsnapman, pQuorumBaseBlockIndex)also appears here. Verify that Snapshots are indeed required for encryption logic. If not, consider clarifying the interface for better separation of concerns.src/evo/cbtx.cpp (1)
65-66: Updated Merkle Roots Check
CheckCbTxMerkleRoots(...)now acceptsllmq::CQuorumSnapshotManager& qsnapmanfor deeper integration into the merkle root validation. Verify that snapshots are relevant for final block checks to avoid redundant computations or concurrency race conditions.src/llmq/chainlocks.cpp (2)
186-186: Thread-Safe Signing
UpdatedBlockTip(const llmq::CInstantSendManager& isman)schedulesTrySignChainTip(...)from a separate context. This avoids nested locks but double-check for any possible race conditions whereismandata might change mid-scheduling.
306-306: Null InstantSend Handling
Ensureismanis not null or disabled at runtime. IfIsInstantSendEnabled()returns false or the manager is not properly initialized, the subsequent checks might skip essential block signing. Consider logging or short-circuiting for clarity.src/test/util/setup_common.cpp (1)
124-125: New parameter*node.llmq_ctx->ismanto the CJContext
This injects the InstantSend manager into the coinjoin context. If the manager is optional, consider checking for null prior to usage to avoid potential runtime errors.src/rest.cpp (1)
134-153: New helperGetLLMQContext
This static utility function ensures consistent retrieval of the LLMQ context, returning an error if missing. For clarity, consider returning a reference instead of a pointer if it is always expected to exist once retrieved.src/llmq/utils.cpp (20)
27-31: Consider merging or dropping redundant namespace blocks
These two separate namespace blocks forllmqappear to bracket only a small portion of the file. If there is no functional difference or clear organizational need, you might remove one or co-locate definitions in a singlellmqnamespace.
48-50: Maintain a consistent namespace structure
The nested namespacesllmq::utilsare fine, but consider using inline namespaces or a single nested block (e.g.namespace llmq::utils { ... }) if C++17 or later is guaranteed. This simplifies code readability.
60-65: Add brief function comments
The forward declaration ofComputeQuorumMembersmight benefit from a short comment indicating expected input parameters and possible return states (e.g., empty vector). This helps clarify usage at first glance.
66-69: Document new quarter-members builder
BuildNewQuorumQuarterMembersis non-trivial. Consider appending a brief description summarizing how it constructs sets of members in different quarters.
80-82: Clarify optional usage
GetMNUsageBySnapshotreturns a pair of lists. If the snapshot is invalid, ensure the returned lists are consistently empty or handle errors gracefully.
83-86: Use consistent naming or grouping
BuildQuorumSnapshotusesquorumSnapshotto store MN skip lists. Consider a short doc comment near the forward declaration that clarifies these data structures and skip modes for future maintainers.
174-174: Check for empty results
auto q = ComputeQuorumMembersByQuarterRotation(...)can return an empty container if no members are found. Ensure subsequent logic gracefully handles empty quorums.
190-191: Skip logic
InsideComputeQuorumMembers, consider makingASSERT_IF_DEBUG(false)a standard error path or a suitable runtime check (e.g. throwing) if the function is misused in production builds.
236-237: Validate result ofBuildNewQuorumQuarterMembers
WhennewQuarterMembersis empty, the caller might want to log or handle it to avoid silent failures in subsequent processing.
290-290: Improve docstring
GetPreviousQuorumQuarterMembers(...): The function’s complexity merits a top-level comment clarifying how old blocks are determined for -C, -2C, and -3C calculations.
305-306: Potential repeated code
Indicator: repeated usage ofqsnapman.GetSnapshotForBlockin consecutive conditionals might be abstracted or refactored. For example, a utility function that fetches snapshots based on height offsets.
325-327: Consider separate data structure
BuildNewQuorumQuarterMembersis quite specialized, building quarter-based membership. Consider whether a specialized data object might simplify passing around the required partial results.
480-480: Check store result
qsnapman.StoreSnapshotForBlock(...)returns void, but consider verifying post-conditions or logging success/failure paths for better debugging.
487-488: Input vs return
BuildQuorumSnapshotmodifiesskipList. If this is the only usage pattern, returning it as a function result might be more intuitive.
521-523: Unified approach
GetQuorumQuarterMembersBySnapshotis quite similar toBuildNewQuorumQuarterMembers. Consider whether these can share partial logic or use a helper function to reduce duplication.
704-706: Clarify router logic
GetQuorumRelayMembers: The gap logic for the ring might be more readable if extracted into a separate function for clarity, e.g.ComputeOutboundRelays(index, ringSize).
708-708: Single usage
auto mns = GetAllQuorumMembers(...): If used only once, consider inlining or skipping the local var for conciseness.
791-791: Dead code check
if (members.empty()) { return false; }ensures we skip further logic. Confirm that no subsequent lines need to handle an empty members list.
808-809: Naming style
connections = GetQuorumConnections(...)is typically straightforward, but variable naming could beoutConnectionsor something explicit if further expansions are planned.
841-843: Pluggable concurrency
AddQuorumProbeConnectionsusesmn_metaman. If concurrency or usage patterns expand, consider decoupling metaman calls from direct usage here to ensure single responsibility.src/txmempool.h (1)
322-325: Namespace-based forward declaration
Forward-declaringllmq::CInstantSendManageris fine. Just confirm that additional forward declarations are consistently placed, e.g. near other LLMQ types.src/llmq/dkgsession.cpp (2)
94-94: Clarify local session status
dkgDebugManager.InitLocalSessionStatus(...): If debug info is optional, consider guarding calls with a debug-level check to avoid overhead in production builds.
1276-1276: Post-commit verification
if (!fqc.Verify(m_dmnman, m_qsnapman, m_quorum_base_block_index, true)) { ... }ensures final-check. If verification fails, consider logging more context for diagnosing potential rotation or snapshot mismatch.src/llmq/quorums.cpp (1)
410-410: Error logging
WhenBuildQuorumFromCommitmentfails to build the VVec, log an error but continue. Confirm higher-level code does not silently rely on nonexistent verification vectors.src/txmempool.cpp (1)
468-473: Add a short code comment
ConnectManagers(gsl::not_null<...>, gsl::not_null<...>)is a new overload withCInstantSendManager* isman. Briefly mention in docstring that both manager pointers must be fresh or null before calling, to avoid double-init.src/evo/deterministicmns.cpp (2)
705-707: Refine debug logging strategy.
The newdebugLogsparameter toggles verbose output. Consider using a global or config-driven debug level instead of passing a boolean down multiple calls to improve readability and reduce parameters.
1002-1003: Check for error handling on invalid quorum data.
The newHandleQuorumCommitmentfunction processes final commitments. Consider adding explicit early returns if the input data fails strict validation before penalizing any members.src/coinjoin/client.cpp (2)
594-594: Validate concurrency around IsValidInOuts checks.
Passingm_ismantoIsValidInOutsmight introduce concurrency risk if the manager is used by multiple threads. Ensure locking or thread-safe operations are in place as needed.
1920-1921: Review concurrency with multiple wallet managers.
Thestd::make_unique<CCoinJoinClientManager>(..., m_isman, ...)usage is fine. However, if multiple managers reference the sameCInstantSendManager, ensure robust thread-safety for all shared data structures.src/validation.cpp (2)
638-646: Check optional usage and variable naming
The usage of structured bindings with_for the first component can be slightly confusing. Consider renaming_to something likeconflict_islockto reflect the contained data. Overall, the conflict check logic on lines 638–646 is correct for detecting conflicting InstantSend locks.
2176-2190: InstantSend conflict resolution...
- The loop’s use of
while (auto conflictLockOpt = ... )is clear and readable.- Consider log consistency in lines 2185 and 2190 (
LogPrintvsLogPrintf, plus “ERROR” in log might be better asBCLog::CHAINLOCKSor a specific category).- Returning
state.Invalid(...)is correct to halt block connection on conflicts.src/llmq/context.h (1)
65-65: Unique pointer usage
Thestd::unique_ptr<llmq::CQuorumSnapshotManager> qsnapman;ensures proper ownership and lifetime. If external references are required, consider ashared_ptr.src/evo/specialtxman.h (1)
55-58: Constructor signatureIncorporates the snapshot manager into the special transaction processor. This looks correct. Ensure unit tests cover possible
qsnapmanusage scenarios.src/llmq/debug.h (2)
80-80: Extended parameterEnsure that additional snapshot-related context is utilized or documented if it’s central to debugging output.
93-93: Extended debug methodSame suggestion as above: verify meaningful usage of
qsnapman.src/evo/cbtx.h (2)
23-24: Snapshot manager introductionCheck if
CQuorumSnapshotManageris always used with the block processor, or can remain separate for clarity.
94-95: CalcCbTxMerkleRootMNList changesAdditional argument to provide snapshot context. Confirm any error handling or fallback logic if snapshot is unavailable or partial.
src/llmq/blockprocessor.h (1)
44-44: New reference memberm_qsnapmanensures direct interaction withCQuorumSnapshotManager.Keeping the reference form prevents null usage. Just confirm that
m_qsnapmanhas a suitable lifetime ensuring it remains valid duringCQuorumBlockProcessor's lifetime.src/llmq/commitment.cpp (1)
30-31: Consistent usage ofqsnapmaninVerify.Adding
CQuorumSnapshotManager& qsnapmanextends verification scope to quorum snapshots. Ensure its usage aligns with the rest of the code (e.g., no partial/duplicate checks). Track potential performance impacts if the snapshot manager performs disk or network operations.src/coinjoin/coinjoin.h (1)
312-314:IsValidInOutsexpansion to includeCInstantSendManager.Including the IS logic can help detect double spends or locked inputs. Validate that the added parameter is used consistently and that performance remains acceptable, especially in higher-throughput scenarios of mixing sessions.
src/node/miner.cpp (1)
79-79: [TEAM] Initializingm_qsnapmanin the constructor ensures the snapshot manager is mandatory. Consider logging or providing a clearer error if the snapshot manager is unexpectedly unavailable.src/llmq/dkgsessionhandler.cpp (2)
39-39: [TEAM] Addingm_qsnapman(qsnapman)is consistent with the new constructor parameter. Optionally, log an identifier for easier debugging when snapshot manager issues arise.
554-554: [TEAM] InvokingAddQuorumProbeConnectionswithm_qsnapmanoffers more up-to-date data, but consider how incomplete snapshots might affect this optional probe process.src/test/evo_deterministicmns_tests.cpp (1)
732-732: [TEAM] The repeatedConnectManagersinvocation could be confusing. Consider clarifying in a comment why it is necessary or if it’s a leftover from refactoring.src/rpc/quorums.cpp (2)
299-299: [TEAM] Passing*llmq_ctx.qsnapmantostatus.ToJsonshifts the data source to snapshot-based logic. Confirm backward compatibility if any code or tools rely on the previous JSON format.
862-863: [TEAM] WhenBuildQuorumRotationInfofails, throwing anRPC_INVALID_REQUESTwithstrErroruses a generic error code. For user clarity, consider more descriptive error handling or logging.src/rpc/blockchain.cpp (2)
690-690: Maintain consistent handling ofisManpointer usage.
While usingllmq_ctx.isman.get()is valid, double-check for scenarios in which the pointer could be null or uninitialized.
798-798: Possible minor improvement: pass by reference if always non-null.
Ifllmq_ctx.ismanis guaranteed to exist, consider passing a reference to simplify usage and avoid.get().src/coinjoin/context.cpp (1)
25-25: Evaluate pointer lifetime forpeerman.
peermanis taken as astd::unique_ptr<PeerManager>&. Ensure that the downstream code doesn't store a raw pointer for extended usage beyond the owner's lifetime.src/coinjoin/context.h (1)
38-40: Constructor signature updated to includeCInstantSendManager.
This integration into theCJContextconstructor is consistent with the design of the coin join flow. Consider documenting any new assumptions aboutismanusage.src/llmq/context.cpp (1)
36-37: Ensure newqmanusage in unit tests.
The Quorum Manager is expanded with snapshot integration and additional parameters. Confirm that test coverage includes these expansions.src/llmq/quorums.h (1)
267-268: Extended constructor signature.Including
CQuorumSnapshotManager& qsnapmanexpands this manager’s responsibilities to handle snapshot data. Confirm that all calling sites provide a valid instance and that the extra parameter is not left unused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
src/coinjoin/client.cpp(6 hunks)src/coinjoin/client.h(6 hunks)src/coinjoin/coinjoin.cpp(4 hunks)src/coinjoin/coinjoin.h(3 hunks)src/coinjoin/context.cpp(1 hunks)src/coinjoin/context.h(2 hunks)src/coinjoin/server.cpp(3 hunks)src/coinjoin/server.h(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/evo/cbtx.cpp(4 hunks)src/evo/cbtx.h(2 hunks)src/evo/chainhelper.cpp(1 hunks)src/evo/chainhelper.h(2 hunks)src/evo/deterministicmns.cpp(5 hunks)src/evo/deterministicmns.h(2 hunks)src/evo/specialtxman.cpp(6 hunks)src/evo/specialtxman.h(3 hunks)src/init.cpp(3 hunks)src/llmq/blockprocessor.cpp(6 hunks)src/llmq/blockprocessor.h(3 hunks)src/llmq/chainlocks.cpp(6 hunks)src/llmq/chainlocks.h(3 hunks)src/llmq/commitment.cpp(4 hunks)src/llmq/commitment.h(3 hunks)src/llmq/context.cpp(3 hunks)src/llmq/context.h(2 hunks)src/llmq/debug.cpp(4 hunks)src/llmq/debug.h(3 hunks)src/llmq/dkgsession.cpp(5 hunks)src/llmq/dkgsession.h(3 hunks)src/llmq/dkgsessionhandler.cpp(3 hunks)src/llmq/dkgsessionhandler.h(3 hunks)src/llmq/dkgsessionmgr.cpp(4 hunks)src/llmq/dkgsessionmgr.h(3 hunks)src/llmq/instantsend.cpp(0 hunks)src/llmq/instantsend.h(0 hunks)src/llmq/quorums.cpp(3 hunks)src/llmq/quorums.h(3 hunks)src/llmq/snapshot.cpp(6 hunks)src/llmq/snapshot.h(1 hunks)src/llmq/utils.cpp(17 hunks)src/llmq/utils.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/chainstate.cpp(4 hunks)src/node/chainstate.h(0 hunks)src/node/miner.cpp(3 hunks)src/node/miner.h(2 hunks)src/rest.cpp(5 hunks)src/rpc/blockchain.cpp(4 hunks)src/rpc/quorums.cpp(3 hunks)src/test/evo_deterministicmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/test/validation_chainstatemanager_tests.cpp(5 hunks)src/test/validation_flush_tests.cpp(1 hunks)src/txmempool.cpp(2 hunks)src/txmempool.h(4 hunks)src/validation.cpp(7 hunks)src/validation.h(0 hunks)test/lint/lint-circular-dependencies.py(0 hunks)
💤 Files with no reviewable changes (5)
- test/lint/lint-circular-dependencies.py
- src/llmq/instantsend.cpp
- src/llmq/instantsend.h
- src/validation.h
- src/node/chainstate.h
👮 Files not reviewed due to content moderation or server errors (3)
- src/llmq/chainlocks.h
- src/llmq/snapshot.cpp
- src/net_processing.cpp
🔇 Additional comments (178)
src/init.cpp (8)
75-75: Include usage check for <walletinitinterface.h>.
This header file is likely necessary for wallet initialization procedures. If the wallet is fully disabled, ensure that no references remain to this interface to avoid dead code.
77-77: Confirm that <bls/bls.h> is required.
Verify that BLS cryptographic functionality is indeed utilized in this file. If not, the include may be removed to optimize compilation times.
82-84: Evo/DIP2 includes usage.
These includes for deterministic masternodes (Evo) are consistent with the evolution-related logic below. No issues found.
87-90: LLMQ header additions.
These newly added LLMQ headers appear necessary for session management and signing. Ensure that any cross-dependencies are resolved to prevent circular includes.
92-92: Masternode node header inclusion.
This header helps integrate masternode-specific node definitions. No immediate concerns.
100-104: Wallet-specific CoinJoin includes.
These CoinJoin headers are well-guarded by #ifdef ENABLE_WALLET, ensuring they remain compiled out when the wallet is disabled. Implementation appears clean.
327-328: Ensure closing sequence is error-free.
The call to DashChainstateSetupClose references multiple node components alongside Assert(node.mempool.get()). This is appropriate for ensuring the mempool pointer is valid. No logic or concurrency issues detected.
2024-2025: Pointer usage verification in CJContext constructor.
Passing node.llmq_ctx->isman presupposes that llmq_ctx and its isman member are fully initialized. This is likely correct given the preceding code, but consider adding safety checks or clear ordering guarantees to avoid potential dereference issues if the code evolves.
src/evo/chainhelper.h (7)
8-9: Include Utilization Looks Fine
No issues with these includes.
11-11: Confirm C++17 Availability
Including <optional> requires C++17. Ensure the build environment is set accordingly.
22-22: Forward Declaration for CTransaction
The forward declaration is fine.
27-27: Forward Declaration for CInstantSendManager
No concerns here.
30-30: Forward Declaration for CQuorumSnapshotManager
No concerns here.
35-38: Potential Lifetime Risk with Reference Members
Holding references to manager objects (lines 36-37) is valid if they're guaranteed to outlive CChainstateHelper. Otherwise, consider using smart pointers.
49-53: ChainLocksHandler Passthrough
Functions look straightforward. Just ensure concurrency or re-entrancy is handled in clhandler.
src/evo/chainhelper.cpp (11)
9-10: Includes for ChainLocks and InstantSend
No issues spotted.
14-19: Constructor Implementation
Passing isman and clhandler by reference is fine as long as object lifetimes are guaranteed.
23-23: Blank Line
No action needed.
24-29: HasConflictingChainLock
Simple passthrough. Ensure clhandler remains synchronized with chain state.
30-34: HasChainLock
Logic is concise. Confirm concurrency safety of HasChainLock.
35-39: GetBestChainLockHeight
GetBestChainLock() must be valid. If there's a scenario where no chainlock exists, handle potential null or default.
40-40: Documentation for InstantSend Manager
Clear doc comment.
41-47: HasConflictingISLock
Usage of SerializeHash is correct. Make sure the islock's lifecycle is stable while hashing.
48-52: IsInstantSendWaitingForTx
Direct passthrough to isman. Looks good.
53-60: RemoveConflictingISLockByTx
Consider robust error checks if RemoveConflictingLock fails or throws.
61-64: ShouldInstantSendRejectConflicts
Function is small and straightforward. No further issues.
src/llmq/utils.h (5)
29-30: Forward Declaration of CQuorumSnapshotManager
Introduction of CQuorumSnapshotManager is consistent with new architecture.
32-32: Namespace utils
No issues in opening a nested namespace.
44-45: GetQuorumRelayMembers
No immediate issues. Keep an eye on potential concurrency with qsnapman.
49-53: EnsureQuorumConnections
Implementation detail not shown, but signature suggests expanded scope. Confirm thorough testing.
55-56: AddQuorumProbeConnections
Similar to above: verify concurrency and test coverage for qsnapman.
src/llmq/debug.cpp (4)
18-18: CDKGDebugSessionStatus::ToJson Updated Signature
Adding qsnapman clarifies snapshot usage. Validate concurrency if qsnapman is referenced.
31-31: GetAllQuorumMembers Invoked With qsnapman
Call is consistent with the updated signature.
112-112: CDKGDebugStatus::ToJson Updated Signature
Signatures remain consistent with other changes.
130-130: ToJson Called With qsnapman
Ensures new parameter flow through nested calls.
src/evo/specialtxman.cpp (5)
22-22: CheckSpecialTxInner Signature
Replacing chainman parameter with qsnapman is consistent with the new approach.
50-50: CheckLLMQCommitment
Updated call to pass qsnapman. Confirm the internal usage is thoroughly tested.
69-69: CheckSpecialTxInner Invocation
Ensures the new signature is used consistently.
173-173: ProcessBlock with m_qsnapman
Ensure that combining ProcessBlock with qsnapman does not introduce undefined dependencies.
182-182: CheckCbTxMerkleRoots with m_qsnapman
Updated function call is consistent. Confirm test coverage for new parameter usage.
src/node/chainstate.cpp (3)
7-7: Header Inclusion Looks Good
The new header <chainparamsbase.h> is presumably required for references to base chain parameters. This is acceptable and aligns with the usage pattern in the file.
64-64: Ensure Proper Initialization Order
chainman.InitializeChainstate(mempool, *evodb, chain_helper) now receives chain_helper as an argument. Verify that chain_helper is properly set or reset beforehand to avoid passing an uninitialized pointer-like object.
75-75: New Setup Invocation
DashChainstateSetup(...) is invoked with the updated parameters and no direct LLMQ manager references. Ensure that the removed manager references are properly handled inside or after this call if required by downstream logic.
src/llmq/dkgsessionmgr.cpp (3)
32-34: Constructor Parameter Expansion
The inclusion of CQuorumSnapshotManager& qsnapman in the constructor ensures that snapshot logic is now fully integrated. Confirm that m_qsnapman is always valid during the lifetime of CDKGSessionManager to prevent potential null references.
41-41: Member Initialization
m_qsnapman(qsnapman) is an important addition. If m_qsnapman is fundamental to the internal DKG session flow, verify that it is used consistently in all relevant methods (e.g., various DKG phases) to avoid partial usage or contradictory scenarios.
55-55: Session Handlers Tuple Update
Adding m_qsnapman to the dkgSessionHandlers.emplace call ensures consistent snapshot participation in the DKG process. Confirm that the upstream code calling DKG states expects the newly appended parameter to avoid version mismatches between calls.
src/coinjoin/coinjoin.cpp (4)
210-212: InstantSend Manager Integration (IsValidInOuts)
Replacing references to the now-removed global InstantSend manager with const llmq::CInstantSendManager& isman is logical. Confirm that callers pass the correct instance of isman to avoid potential mismatch.
273-273: Check for MemPool Input Lock
The check (coin.nHeight == MEMPOOL_HEIGHT && !isman.IsLocked(txin.prevout.hash)) ensures inputs from the mempool are locked by InstantSend. Confirm thorough coverage in unit tests, especially for edge cases where a transaction might transition from unlocked to locked.
318-319: CoinJoin Collateral Validation with InstantSend
The updated signature IsCollateralValid(ChainstateManager& chainman, const llmq::CInstantSendManager& isman, ...) ensures the presence of the manager for locked input checks. Confirm performance overhead remains acceptable when re-checking large collateral sets.
340-340: Mempool Input Lock
if (mempool.isSpent(txin.prevout) || !isman.IsLocked(txin.prevout.hash)) { ... } ties the collateral check strongly to InstantSend status. Keep an eye on potential false-negatives if the lock status is not instantly updated in all network conditions.
src/evo/cbtx.cpp (3)
90-90: CalcCbTxMerkleRootMNList Usage
if (!CalcCbTxMerkleRootMNList(block, pindex->pprev, calculatedMerkleRoot, state, dmnman, qsnapman, view)) { ... } ensures MN list is built with snapshots. Confirm that the snapshot logic is always valid for consecutive blocks to avoid partial updates.
120-121: Method Signature Expansion
bool CalcCbTxMerkleRootMNList(...) with the new parameters BlockValidationState& state before CDeterministicMNManager& dmnman, llmq::CQuorumSnapshotManager& qsnapman clarifies error handling. Ensure no unintended call-site breakage from re-ordered parameters.
131-131: Snapman in BuildNewListFromBlock
dmnman.BuildNewListFromBlock(...) now includes qsnapman. Ensure that if snapshot generation or usage fails, the logic sets an appropriate state error to keep the chain tip consistent.
src/llmq/chainlocks.cpp (4)
55-55: InstantSend Manager in Start
The new parameter const llmq::CInstantSendManager& isman in Start(...) clarifies that chain lock checks must consider InstantSend. Confirm consistent usage of isman in subsequent scheduling tasks.
62-62: Regularly Retry Signing
Inside the lambda, TrySignChainTip(isman) is called every 5 seconds. This periodic check helps ensure that newly locked transactions get included quickly, but watch out for potential concurrency if locks take longer than expected.
196-196: Atomic Scheduling in UpdatedBlockTip
tryLockChainTipScheduled.compare_exchange_strong ensures this block runs only once at a time. This approach is clean but confirm that missed triggers are not an issue if concurrency is high.
218-218: TrySignChainTip
Integration with isman.IsInstantSendEnabled() and isman.RejectConflictingBlocks() ensures only safe blocks are chain-locked. Validate that IsLocked(txid) calls remain performant under heavy concurrency or large mempools.
src/evo/deterministicmns.h (4)
38-40: Forward declaration of CQuorumSnapshotManager
This refactoring effectively separates the interface from the implementation by forward-declaring the CQuorumSnapshotManager. Ensure that only the header that truly requires the class definition includes "quorumsnapshotmanager.h", thus minimizing compile-time dependencies.
601-602: New parameter llmq::CQuorumSnapshotManager& qsnapman in ProcessBlock
The added parameter improves modularization by introducing snapshot management into the block processing flow. Verify that all callers are updated accordingly to supply the correct qsnapman instance.
608-610: Updated signature of BuildNewListFromBlock
Appending llmq::CQuorumSnapshotManager& qsnapman indicates that snapshot management is now integral to building the new MN list. This change appears consistent with existing code patterns.
611-612: Added qsnapman parameter to HandleQuorumCommitment
Including the snapshot manager in the quorum commitment handler centralizes all quorum-related management. Ensure that all upstream calls provide a valid, long-lived reference to qsnapman.
src/test/util/setup_common.cpp (8)
55-63: Reintroducing multiple includes
These headers restore or add references to flat-database.h, governance/governance.h, etc. The approach looks correct for re-enabling or cleaning up forward declarations. Confirm no cyclical dependency is introduced.
64-66: Wallet-conditional includes
Surrounding <interfaces/coinjoin.h> with #ifdef ENABLE_WALLET ensures that coinjoin-specific wallet functionality remains guarded. The approach aligns with typical conditional compilation patterns.
111-112: Passing node.llmq_ctx to DashChainstateSetup
The function signature now accepts the LLMQ context. Make sure references to llmq_ctx are valid in all code paths to prevent null pointer usages.
117-118: DashChainstateSetupClose updated parameters
The extended function call includes node.llmq_ctx. Confirm the order and usage remain consistent with the function's new signature and that the teardown sequence is correct.
Line range hint 343-346: Introducing llmq_ctx in rest_block
The code properly checks llmq_ctx for null. Then passes *llmq_ctx->clhandler and *llmq_ctx->isman. Ensure future references to these pointers remain protected by the same guard.
475-475: Extended argument list in CalcCbTxMerkleRootMNList
The qsnapman parameter is added for quorum snapshot management. This looks correct. Still, confirm no side effects if qsnapman is absent or uninitialized.
Line range hint 609-612: Retrieving llmq_ctx before building mempool info
This follows the new pattern of verifying the context pointer prior to usage. The references to *llmq_ctx->isman appear consistent.
Line range hint 635-638: Using llmq_ctx->isman for mempool content JSON
Similarly, consistent usage of the new LLMQ context for coinjoin/instant send operations. No issues seen.
src/llmq/blockprocessor.cpp (6)
29-34: Integration of qsnapman in PreComputeQuorumMembers
The additional parameter allows retrieving and caching snapshot data while precomputing quorum members. Verify that repeated calls handle the reset_cache flag correctly to avoid stale data or performance issues.
47-52: Constructor now requires CQuorumSnapshotManager& qsnapman
By passing in a reference during construction, the block processor can reliably manage snapshot references. Ensure that the lifetime of qsnapman matches or exceeds that of CQuorumBlockProcessor.
139-139: qc.Verify with m_qsnapman
Forwarding the snapshot manager to Verify ensures that quorum computations remain consistent. Validate that any concurrency or thread-safety concerns are addressed when verifying multiple commitments concurrently.
165-165: Call to PreComputeQuorumMembers with m_qsnapman
This improves performance by caching membership data at the start of block processing. Keep an eye on potential race conditions if block processing can be parallelized.
276-276: Passing m_qsnapman to qc.Verify during commitment processing
This is consistent with the earlier usage in line 139. The extended verification is essential to ensure the block’s commitments are fully validated with snapshot data.
320-320: PreComputeQuorumMembers invoked with reset_cache = true
Triggering a cache reset upon block undo is logical. Confirm that data structures are rebuilt properly in subsequent calls.
src/rest.cpp (4)
270-272: Utilizing llmq_ctx in rest_headers
The code captures llmq_ctx and checks for null. Next usage is within blockheaderToJSON. If clhandler is expected to be non-null, confirm robust error-handling or fallback if it’s unexpectedly nullptr.
Also applies to: 275-275
343-346: Integration with blockToJSON in rest_block
Ensuring clhandler and isman are used only after the null check on llmq_ctx. This addition is consistent, but keep an eye on concurrency if llmq_ctx is modified in parallel.
609-612: Passing *llmq_ctx->isman to MempoolInfoToJSON
Extracting the IS Manager from llmq_ctx is consistent with the pattern established for block and header calls. No issues noted.
635-638: Retrieving llmq_ctx within rest_mempool_contents
Again, the code checks for null, then calls MempoolToJSON. Good defensive programming. This is an appropriate expansion of the existing pattern.
src/coinjoin/server.cpp (3)
586-586: Invocation of CoinJoin::IsCollateralValid with m_isman
Passing the instant send manager enables deeper checks of input existence or security. Implementation looks correct if the manager is guaranteed to be non-null here.
620-620: IsValidInOuts now uses m_isman
Ensures the coinjoin server can access the instant send manager for final validation. Watch out for any possible performance overhead if invoked frequently.
697-697: Collateral verification with m_isman in IsAcceptableDSA
This extension of the final check ensures uniform usage of the instant send manager. The code remains straightforward; no additional concurrency or security issues appear.
src/llmq/utils.cpp (9)
70-79: Ensure robust error handling
GetPreviousQuorumQuarterMembers returns partial results in case some snapshots are missing. Consider clarifying how callers should handle incomplete data (e.g., by throwing an exception or returning an empty structure).
112-115: Evaluate concurrency on static caches
static std::map<Consensus::LLMQType, unordered_lru_cache<...>> mapQuorumMembers is guarded by a RecursiveMutex, but be cautious about potential reentry or usage in multi-thread contexts. Confirm that higher-level locking (e.g., cs_main) or usage patterns are consistent.
209-211: Double-check rotation block indexing
When pCycleQuorumBaseBlockIndex->nHeight % llmqParams.dkgInterval != 0, this function returns an empty vector. Verify that calling code is prepared for that scenario.
228-231: Avoid unexpected exceptions
GetPreviousQuorumQuarterMembers calls qsnapman.GetSnapshotForBlock(...). If the snapshot is missing or partial, ensure the returned structures do not cause index out-of-bounds in subsequent usage.
299-299: std::optional usage
std::optional<llmq::CQuorumSnapshot> quSnapshotHMinusC: make sure to handle the case where .has_value() is false consistently before subsequent usage.
312-313: Graceful fallback
Similarly, ensure that if quSnapshotHMinus3C is missing, the code does not proceed with partial or invalid data.
679-681: Check invariants
GetQuorumConnections ensures all members have the same length input vectors. If vector sizes differ, this throws. Confirm that all truncated or invalid vectors are handled earlier.
701-701: Consistent fallback
return GetQuorumRelayMembers(...) is a fine fallback. Just ensure that relayed members are a subset or superset of normal members as the design intends.
781-785: Edge-case for watch quorum
EnsureQuorumConnections: If is_masternode is false and IsWatchQuorumsEnabled() is also false, we short-circuit. Validate that the function indeed covers all watch-only scenarios properly.
src/llmq/dkgsession.cpp (3)
74-76: Extended constructor signature
Adding CQuorumSnapshotManager& qsnapman is helpful for direct snapshot usage. Ensure all call sites pass consistent references.
85-85: Check dependency injection
Storing m_qsnapman in CDKGSession is fine. Confirm that session lifetimes never exceed or mismatch the manager’s lifetime.
139-139: relayMembers logic
The utils::GetQuorumRelayMembers() call might produce differing sets if rotation is enabled. Confirm that the DKGSessions handle partial members or older indexes consistently.
src/llmq/quorums.cpp (2)
214-216: Injecting CQuorumSnapshotManager&
Constructing CQuorumManager now takes qsnapman. Confirm all references remain valid if the manager is restarted or replaced (e.g., in unit tests or new contexts).
368-368: Ensure no concurrency conflict
if (utils::EnsureQuorumConnections(llmqParams, connman, m_dmnman, m_sporkman, m_qsnapman, ...): This is a multi-manager call. Confirm locking order or concurrency strategies to avoid deadlocks.
src/txmempool.cpp (1)
1584-1584: Skip locked knobs
Inside Expire(...), if (m_isman && m_isman->IsLocked(it->GetTx().GetHash())) { ... } is good for preventing locked instant-send transactions from expiry. If you add other lock mechanisms in the future, unify them under a single check.
src/evo/deterministicmns.cpp (4)
598-600: Ensure all call sites pass the new parameters correctly.
This signature now expects both a reference to llmq::CQuorumSnapshotManager and an optional MNListUpdates output parameter. Verify that all invocations of ProcessBlock handle the new parameters consistently and that no null reference is ever passed for qsnapman.
615-615: Confirm the debug flag usage in BuildNewListFromBlock.
The additional true value for debugLogs will emit more detailed logs. Double-check that the logs do not inadvertently expose sensitive data and that debug mode remains configurable.
930-930: Validate block index consistency.
When calling HandleQuorumCommitment, ensure pQuorumBaseBlockIndex is always a valid ancestor. Adding a quick assertion or check could help detect mismatch errors early.
1007-1007: Confirm quorum membership retrieval.
llmq::utils::GetAllQuorumMembers is used to retrieve all members for the specified LLMQ type. Ensure that edge cases (e.g. empty sets or partially invalid membership) are safely handled to avoid potential out-of-range issues.
src/coinjoin/client.cpp (4)
169-169: Persist the InstantSend manager reference carefully.
Storing a reference to CInstantSendManager in the constructor (isman) means its lifetime must exceed the CCoinJoinClientSession object’s. Verify that the manager remains valid to prevent any use-after-free conditions.
178-178: Initialize the instant send manager reference consistently.
The member initializer m_isman{isman} is straightforward. Confirm that no concurrency issues arise if different sessions share the same CInstantSendManager entity.
958-958: Confirm collateral validation flow.
Invoking CoinJoin::IsCollateralValid(chainman, m_isman, mempool, ...) expands the checks for instant send aspects. Double-check that all return paths properly handle potential invalid states.
1017-1017: Ensure the new session receives the instant send manager securely.
While adding a new CCoinJoinClientSession, confirm concurrency and lifetime for m_isman remain consistent, preventing potential race conditions if sessions access the same manager concurrently.
src/validation.cpp (4)
61-61: Clarify necessity of masternode includes
It looks like <masternode/payments.h> is introduced here. Verify that all references from this header are indeed used to avoid long-term maintenance overhead.
650-650: Validate condition usage
The condition m_chain_helper.IsInstantSendWaitingForTx(hash) neatly handles special cases. No immediate issues found.
1879-1879: Chainlock conflict check
Adding a check via m_chain_helper->HasConflictingChainLock is consistent with the chain lock mechanism. Ensure thorough unit tests cover edge cases of partial chainlocks.
2218-2218: Superblock condition
The new variable check_superblock is logically clear. Double-check for alignment with the existing superblock logic to avoid off-by-one errors.
src/llmq/context.h (2)
31-31: New forward declaration
Introduction of class CQuorumSnapshotManager; is straightforward. Confirm usage synergy in .cpp files.
71-72: Instance ordering
Placing clhandler and isman after qsnapman is fine as long as constructor dependencies are satisfied. Verify that any needed objects are already initialized before usage.
src/evo/specialtxman.h (2)
43-43: Introduce snapshot manager reference
Good approach to pass a reference. Confirm lifetime safety in upstream code.
28-31: Consistency with existing includes
...Ensure forward declarations remain consistent with the rest of the codebase. Verify no missing includes in the .cpp files.
✅ Verification successful
Let me verify the consistency of forward declarations by checking the codebase.
Let me check the corresponding cpp file to verify the includes.
Let me check where these classes are used in the codebase.
Looking at the code and includes, I can now verify the forward declarations.
Forward declarations are consistent with codebase usage
The forward declarations in src/evo/specialtxman.h are correctly used as the corresponding classes are included via llmq/blockprocessor.h, llmq/quorums.h, and llmq/chainlocks.h in the implementation file specialtxman.cpp. The classes are used in function parameters and member variables within the implementation.
Explanation:
- The forward declarations are needed as these classes are used in function parameters and member variables
- The actual class definitions are properly included in the cpp file through:
#include <llmq/blockprocessor.h>(for CQuorumBlockProcessor)#include <llmq/quorums.h>(for CQuorumManager)#include <llmq/chainlocks.h>(for CChainLocksHandler)#include <llmq/snapshot.h>(for CQuorumSnapshotManager)
- This follows good practice of using forward declarations in headers and including full definitions in cpp files
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the corresponding cpp file
fd "specialtxman" --type f
# Check for includes of these classes in cpp files
rg "CChainLocksHandler|CQuorumBlockProcessor|CQuorumManager|CQuorumSnapshotManager" -A 2 --type cpp
Length of output: 65713
Script:
#!/bin/bash
# Find and check the corresponding cpp file
cat src/evo/specialtxman.cpp
Length of output: 13849
Script:
#!/bin/bash
# Check if these classes are used in specialtxman.cpp and other files
rg "class CChainLocksHandler" --type cpp
rg "class CQuorumBlockProcessor" --type cpp
rg "class CQuorumManager" --type cpp
rg "class CQuorumSnapshotManager" --type cpp
# Check includes in files that use these classes
rg "#include.*chainlocks" --type cpp
rg "#include.*blockprocessor" --type cpp
rg "#include.*quorums" --type cpp
rg "#include.*snapshot" --type cpp
Length of output: 6190
src/llmq/debug.h (1)
23-23: Forward declaration
Simple forward declaration. No apparent issues.
src/evo/cbtx.h (1)
91-92: CheckCbTxMerkleRoots adjustments
Revised parameter order might affect existing calls. Ensure consistent usage across the code to avoid confusion.
src/llmq/blockprocessor.h (2)
34-35: New forward declaration of CQuorumSnapshotManager looks correct.
Adding a forward declaration is a clean way to reduce header interdependencies and speed up compilation times.
53-54: Constructor signature updated to accept CQuorumSnapshotManager&.
This is consistent with the new member reference. Validate that all code paths constructing CQuorumBlockProcessor are updated to supply a valid CQuorumSnapshotManager.
src/llmq/dkgsessionmgr.h (3)
35-35: Introduced forward declaration of CQuorumSnapshotManager.
Forward declarations help reduce build dependencies; this is good practice.
49-49: New reference member m_qsnapman.
This maintains a direct link to the snapshot manager. Verify that the lifespan of m_qsnapman is guaranteed throughout the manager’s usage.
75-77: Constructor updated to include CQuorumSnapshotManager& qsnapman.
Ensure that all call sites are updated to pass an appropriate CQuorumSnapshotManager object and that the argument is never null.
src/coinjoin/server.h (3)
39-39: Added m_isman reference to CInstantSendManager.
This is consistent with the pass-by-reference approach and helps to avoid potential null pointer usage.
98-98: Constructor now includes CInstantSendManager& isman parameter.
Check if all instantiation points are updated accordingly to provide a valid reference.
107-107: Initializing m_isman with the passed-in isman.
Everything looks consistent. Verify that isman outlives CCoinJoinServer.
src/test/validation_chainstate_tests.cpp (1)
44-44: Removed llmq::chainLocksHandler and llmq::quorumInstantSendManager from InitializeChainstate().
Streamlines code by dropping references to removed components. Confirm that any functionality depending on these LLMQ managers is still covered or no longer needed.
src/dsnotificationinterface.cpp (1)
94-94: Clean integration with the new InstantSend manager
Invoking UpdatedBlockTip(*m_llmq_ctx->isman) provides a clear interface for updating both the ChainLock handler and the InstantSend manager in tandem. No issues identified.
src/test/validation_flush_tests.cpp (1)
24-24: Constructor signature updated to remove LLMQ dependencies
Removing llmq::chainLocksHandler and llmq::quorumInstantSendManager streamlines CChainState and aligns with the PR objectives of decoupling the validation logic from these managers. Tests appear intact and should maintain coverage.
src/llmq/commitment.h (3)
30-30: Forward declaration for CQuorumSnapshotManager
A pragmatic approach to avoid unnecessary header inclusions, keeping compilation units lean.
69-70: New parameter CQuorumSnapshotManager& qsnapman in Verify
Ensure downstream call sites are updated to pass a valid snapshot manager, preventing runtime errors.
180-180: Updated CheckLLMQCommitment to include CQuorumSnapshotManager& qsnapman
Confirms broader refactoring to unify quorum snapshot logic across LLMQ processes. Recommend verifying test coverage to ensure correctness.
src/llmq/dkgsessionhandler.h (3)
41-41: Forward declare CQuorumSnapshotManager
Keeps compile times lower by deferring inclusion until necessary.
138-138: New member variable m_qsnapman
Implements snapshot management within the DKG session handler, enabling consistent snapshot interactions.
161-163: Constructor updated to accept CQuorumSnapshotManager& qsnapman
Reflects the refactoring effort to centralize and streamline quorum snapshot logic.
src/llmq/snapshot.h (1)
210-211: Ensure existing callers are updated to use the new qsnapman parameter.
The signature of BuildQuorumRotationInfo now expects a reference to CQuorumSnapshotManager&. Verify all invocations are updated accordingly and that the function’s responsibilities are clearly documented, especially regarding how the snapshot manager is utilized.
src/node/miner.h (2)
41-41: Confirm forward declaration to guarantee compilation order.
Adding class CQuorumSnapshotManager; as a forward declaration helps avoid unnecessary includes. Ensure that any usages requiring the full definition include the corresponding header to prevent incomplete type issues.
178-178: Validate the snapshot manager’s usage and life cycle.
Introducing m_qsnapman as a reference implies it must outlive BlockAssembler. Confirm the CQuorumSnapshotManager object stays valid for the assembler’s lifetime, especially if BlockAssembler is created/destroyed frequently.
src/llmq/commitment.cpp (3)
85-85: Check snapshot-based membership queries for correctness.
utils::GetAllQuorumMembers now uses qsnapman. Verify it returns the correct set of members, including edge cases like rotating or partially updated snapshots. Consider caching or verifying that all needed data is pre-fetched.
170-170: Validate newly added parameter in CheckLLMQCommitment.
Including qsnapman in the signature indicates snapshot data is required for checking the commitment. Ensure internal logic is updated to handle possible snapshot unavailability or concurrency issues with the snapshot manager.
223-223: Re-check signature constraints post-refactor.
The commitment verification now leverages snapshots. Confirm that the signature logic used here isn’t silently bypassed or changed by snapshot-based modifications. If concurrency is relevant, ensure consistent locking around CQuorumSnapshotManager.
src/coinjoin/coinjoin.h (2)
36-36: Confirm correct scoping for CInstantSendManager.
Declaring the class in llmq ensures logical grouping but watch for potential circular dependencies or multiple forward declarations. Validate that header inclusions adequately cover required definitions.
371-372: Preserve atomic checks while adding CInstantSendManager in IsCollateralValid.
Inserting isman must not block or slow collateral checks. Verify concurrency is properly managed if CInstantSendManager interacts with other threads or shared data structures.
src/node/miner.cpp (2)
39-39: [TEAM] The new include for <llmq/snapshot.h> aligns with the broader refactoring to integrate the snapshot manager.
227-227: [TEAM] The updated function signature with the snapshot manager parameter requires verification of all call sites to ensure compliance.
src/llmq/dkgsessionhandler.cpp (4)
30-31: [TEAM] The constructor now explicitly takes a reference to CQuorumSnapshotManager, suggesting deeper usage of snapshot data. Make sure this requirement is documented for calling code.
45-45: [TEAM] Instantiating CDKGSession with the snapshot manager may increase complexity within DKG sessions. Confirm thorough unit test coverage for this new dependency.
193-193: [TEAM] The usage of std::make_unique with m_qsnapman retains consistent memory management. Keep an eye on any potential session reuse or concurrency concerns that might arise.
552-552: [TEAM] Adding the snapshot manager to EnsureQuorumConnections can improve alignment with latest consensus data. Verify that partial or out-of-sync snapshots don’t block the connection logic.
src/test/evo_deterministicmns_tests.cpp (2)
25-26: [TEAM] The new includes <llmq/context.h> and <llmq/instantsend.h> indicate extended coverage for LLMQ context and InstantSend in testing.
658-658: [TEAM] Connecting testPool to both dmnman and isman helps ensure that InstantSend logic is tested in conjunction with deterministic masternodes. Validate concurrency and reorg edge cases.
src/rpc/quorums.cpp (1)
327-328: [TEAM] Both invocations of GetQuorumConnections with m_qsnapman underscore the snapshot manager’s role in establishing connections. Verify synchronization with spork-based conditions to prevent mismatched data.
src/rpc/blockchain.cpp (2)
627-627: Consider verifying pointer non-nullness.
Accessing llmq_ctx.isman.get() without a null check can be risky if llmq_ctx.isman might be null. If it's guaranteed to be non-null, consider adding a brief comment for clarity.
758-758: Confirm pointer validity in extended contexts.
Ensure that any code calling entryToJSON with isman respects the same pointer lifetime guarantees.
src/coinjoin/context.cpp (2)
15-15: Ensure new parameter is properly integrated across all callers.
The addition of const llmq::CInstantSendManager& isman requires all constructor callers to supply this dependency. Verify that all call sites provide valid references.
18-18: Correctly threading isman through wallet manager.
Passing isman into CoinJoinWalletManager is consistent with the broader code changes. Confirm coverage with unit tests.
src/coinjoin/context.h (1)
25-27: Forward declaration of CInstantSendManager is appropriate.
The new forward declaration within llmq keeps compile times low and addresses usage only inside the constructor.
src/llmq/context.cpp (8)
20-20: Header addition is consistent with usage of CQuorumSnapshotManager.
Importing llmq/snapshot.h ensures correct instantiation of the snapshot manager.
29-29: Initialization of qsnapman is well integrated.
Creating a CQuorumSnapshotManager with evo_db aligns with its usage patterns in the rest of the code.
30-31: Check error handling for load failures.
When creating CQuorumBlockProcessor, consider ensuring robust error handling or fallback if data is missing or corrupted.
33-34: Thoroughly test DKG session manager with new parameters.
Changes might affect chain state sync and debug logic. Confirm coverage via DKG and chain synchronization tests.
40-41: Initialize chain locks handler in a safe order.
Make sure CChainLocksHandler is fully set up before referencing the mempool or shareman. The ordering here looks correct but re-check concurrency.
56-57: Graceful interrupt sequence.
The interrupt calls for shareman and sigman appear in a logical order. Place caution on shared resources between them.
69-70: Startup order consideration.
clhandler->Start(*isman) occurs just before isman->Start(peerman). Check if reversed startup order is needed or if there's a dependency that requires chain locks to be up first.
74-76: Praise for clean shutdown.
isman->Stop() and then clhandler->Stop() is symmetrical to the initialization. Properly shutting down major concurrency modules in reverse startup order is best practice.
src/coinjoin/client.h (6)
80-87: Ensure lifetime alignment for isman reference.
Storing isman by reference in the constructor is acceptable if its lifetime spans that of CoinJoinWalletManager. Otherwise, a pointer might be safer to avoid potential dangling references.
129-129: Validate reference usage.
Same note: confirm the m_isman reference remains valid across the entire lifecycle of this object.
147-147: Same lifetime concern as above.
207-207: Constructor parameter consistency.
This addition of const llmq::CInstantSendManager& isman matches the pattern used elsewhere. Verify that call sites now supply the reference properly.
273-273: Same lifetime concern as in previous reference fields.
310-317: Constructor parameter alignment in CCoinJoinClientManager.
The usage of the reference to isman is consistent with other classes. Ensure that class owners properly manage the lifetime of isman, particularly if this manager gets shut down or reinitialized.
src/llmq/quorums.h (2)
51-51: Forward declaration of CQuorumSnapshotManager.
Ensure that the corresponding implementation include is provided in the .cpp file so that the class definitions are fully visible where needed.
245-245: Reference field m_qsnapman.
Keep an eye on the lifetime of the referenced object to prevent pointer invalidation. A reference is fine if guaranteed to outlive this class.
src/llmq/dkgsession.h (3)
39-39: Forward declaration for the new manager.
As with other forward declarations, confirm we include the correct header in the associated .cpp for full definitions.
290-290: Reference to m_qsnapman.
As before, ensure that the lifetime of CQuorumSnapshotManager aligns with CDKGSession, or consider safely managing it through a pointer if that constraint cannot be guaranteed.
334-335: Constructor extends to include qsnapman.
This is consistent with the redesigned architecture. Review invocation sites to confirm they properly pass in the new snapshot manager.
src/test/validation_chainstatemanager_tests.cpp (5)
46-46: Parameters to InitializeChainstate simplified.
The removal of llmq::chainLocksHandler and llmq::quorumInstantSendManager aligns with the PR objective to refactor out these references. Verify that no calling code still relies on them.
82-82: Reduced argument list in InitializeChainstate.
This change removes unneeded LLMQ parameters, further decoupling the chainstate code from LLMQ functionalities.
145-145: Same removal of LLMQ parameters.
163-163: Same removal of LLMQ parameters.
404-404: Same removal of LLMQ parameters.
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (18)
src/llmq/dkgsessionmgr.h (1)
75-77: LGTM: Constructor modification aligns with refactoring goalsThe addition of
CQuorumSnapshotManagerparameter to the constructor is well-placed and maintains consistent style. This change supports the PR's objective of refactoring LLMQ components.Consider documenting the dependency relationship between
CDKGSessionManagerandCQuorumSnapshotManagerin the class documentation, particularly highlighting:
- The lifecycle management expectations
- Any assumptions about the lifetime of the snapshot manager
- Key interactions between these components
src/llmq/context.cpp (4)
20-20: Add an explanatory comment for the snapshot header inclusion
Consider adding a brief comment clarifying the purpose of the new#include <llmq/snapshot.h>statement. This can help maintainers quickly identify why the snapshot functionality is now needed in this file.
36-37: Check synergy between QuorumManager and QuorumSnapshotManager
Now thatCQuorumManageralso receives a reference to*qsnapman, verify that methods inCQuorumManagerdo not inadvertently create concurrency or data consistency issues when reading or updating snapshot data.
56-57: Consider interrupt ordering
Interruptingsigmanimmediately followed byismanmight be safe, but ifCInstantSendManagerdepends on ongoing signing operations, you could introduce a race. Ensure proper sequencing to avoid incomplete operations or deadlocks.
74-76: Shutdown ordering
Stoppingisman, then stoppingclhandler, thensigmanis typical. Verify that this sequence does not need reversing ifCChainLocksHandlerorCSigningManagerhold references toCInstantSendManager. A mismatch could leave references dangling.src/rpc/quorums.cpp (1)
327-328: Improve clarity of pre-conditions
BothallConnectionsandoutboundConnectionsrely onGetQuorumConnectionsusing*llmq_ctx.qsnapman. If the snapshot manager is uninitialized, these calls could fail. Consider adding an assert or an early check forqsnapmanreadiness to make your assumptions explicit.src/init.cpp (2)
75-92: Group includes meaningfully
Multiple includes were added or moved (e.g.,walletinitinterface.h, BLS headers, various LLMQ and EVO headers). For readability, consider grouping them by respective functionality (wallet, LLMQ, Evo, etc.) if permitted by project-style guidelines. This helps locate related dependencies faster.
100-104: Conditional wallet includes
Enclosing the coinjoin headers within#ifdef ENABLE_WALLETis correct to avoid compilation issues without wallet support. Ensure each coinjoin-related include is strictly needed ininit.cppor if it can be narrowed further to reduce build dependencies.src/rpc/mempool.cpp (2)
267-267: Confirm entryToJSON usage
entryToJSONnow gets a raw pointer to instant send manager. Validate that the function gracefully handles nullptr scenario ifismanis not available. Possibly add a quick check (e.g.,if (!isman) ...) inside.
375-375: Clarify “unknown” status
Whenismanis nullptr, the field is reported as"unknown". Document or clarify in code comments that anunknownstatus means the manager was not active or available at the time of query, rather than a lock failure.src/evo/chainhelper.h (1)
41-43: Consider improving parameter list readabilityThe constructor parameter list is quite long. Consider breaking it into logical groups using newlines and consistent indentation.
- explicit CChainstateHelper(CCreditPoolManager& cpoolman, CDeterministicMNManager& dmnman, CMNHFManager& mnhfman, CGovernanceManager& govman, - llmq::CInstantSendManager& isman, llmq::CQuorumBlockProcessor& qblockman, llmq::CQuorumSnapshotManager& qsnapman, - const ChainstateManager& chainman, const Consensus::Params& consensus_params, const CMasternodeSync& mn_sync, - const CSporkManager& sporkman, const llmq::CChainLocksHandler& clhandler, const llmq::CQuorumManager& qman); + explicit CChainstateHelper( + // Core managers + CCreditPoolManager& cpoolman, + CDeterministicMNManager& dmnman, + CMNHFManager& mnhfman, + CGovernanceManager& govman, + // LLMQ components + llmq::CInstantSendManager& isman, + llmq::CQuorumBlockProcessor& qblockman, + llmq::CQuorumSnapshotManager& qsnapman, + // Chain and consensus + const ChainstateManager& chainman, + const Consensus::Params& consensus_params, + // Additional components + const CMasternodeSync& mn_sync, + const CSporkManager& sporkman, + const llmq::CChainLocksHandler& clhandler, + const llmq::CQuorumManager& qman);src/llmq/context.h (1)
72-72: Address TODO comment about splitting CInstantSendManagerThe comment indicates a need to split
CInstantSendManagerandCInstantSendLockinto separate files. This aligns with good design principles of single responsibility and could improve maintainability.Would you like me to help create a plan for this split or open a GitHub issue to track this task?
src/evo/chainhelper.cpp (2)
41-46: Consider moving serialization logic to CInstantSendManagerThe serialization of the islock hash should ideally be handled within
CInstantSendManagerto maintain better encapsulation and separation of concerns.-std::optional<std::pair</*islock_hash=*/uint256, /*txid=*/uint256>> CChainstateHelper::HasConflictingISLock(const CTransaction& tx) const -{ - const auto islock = isman.GetConflictingLock(tx); - if (!islock) return std::nullopt; - return std::make_pair(::SerializeHash(*islock), islock->txid); -} +std::optional<std::pair</*islock_hash=*/uint256, /*txid=*/uint256>> CChainstateHelper::HasConflictingISLock(const CTransaction& tx) const +{ + return isman.GetConflictingLockWithHash(tx); +}
53-59: Improve error handling and memory safetyThe
RemoveConflictingISLockByTxfunction could benefit from additional error handling and memory safety improvements:
- The islock pointer should be checked for validity before dereferencing
- Consider using RAII or smart pointers if ownership transfer is involved
-bool CChainstateHelper::RemoveConflictingISLockByTx(const CTransaction& tx) -{ - const auto islock = isman.GetConflictingLock(tx); - if (!islock) return false; - isman.RemoveConflictingLock(::SerializeHash(*islock), *islock); - return true; -} +bool CChainstateHelper::RemoveConflictingISLockByTx(const CTransaction& tx) +{ + try { + const auto islock = isman.GetConflictingLock(tx); + if (!islock || !islock->IsValid()) return false; + isman.RemoveConflictingLock(::SerializeHash(*islock), *islock); + return true; + } catch (const std::exception& e) { + // Log error + return false; + } +}src/llmq/utils.h (1)
34-37: Consider adding documentation for the new parameterThe integration of
qsnapmanparameter across multiple functions is consistent and well-structured. However, it would be beneficial to add documentation explaining the role ofCQuorumSnapshotManagerin these functions.Add documentation like this:
std::vector<CDeterministicMNCPtr> GetAllQuorumMembers(Consensus::LLMQType llmqType, CDeterministicMNManager& dmnman, + // qsnapman: Manages quorum snapshots for consistent quorum state CQuorumSnapshotManager& qsnapman, gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex, bool reset_cache = false);Also applies to: 41-43, 44-46, 49-53, 54-56
src/llmq/snapshot.cpp (1)
Line range hint
208-214: Consider consolidating duplicate snapshot retrieval logic.The snapshot retrieval and error handling pattern is repeated multiple times with only minor differences in error messages and variable names.
Consider extracting the common pattern into a helper function:
+static std::optional<CQuorumSnapshot> GetAndValidateSnapshot( + llmq::CQuorumSnapshotManager& qsnapman, + Consensus::LLMQType llmqType, + const CBlockIndex* pBlockIndex, + const std::string& heightLabel, + std::string& errorRet) +{ + auto snapshot = qsnapman.GetSnapshotForBlock(llmqType, pBlockIndex); + if (!snapshot.has_value()) { + errorRet = strprintf("Can not find quorum snapshot at %s", heightLabel); + return std::nullopt; + } + return snapshot; +} - auto snapshotHMinusC = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinusCIndex); - if (!snapshotHMinusC.has_value()) { - errorRet = strprintf("Can not find quorum snapshot at H-C"); - return false; - } else { - response.quorumSnapshotAtHMinusC = std::move(snapshotHMinusC.value()); - } + auto snapshotHMinusC = GetAndValidateSnapshot(qsnapman, llmqType, pBlockHMinusCIndex, "H-C", errorRet); + if (!snapshotHMinusC.has_value()) return false; + response.quorumSnapshotAtHMinusC = std::move(snapshotHMinusC.value());Also applies to: 220-226, 232-238, 248-254
src/evo/cbtx.cpp (1)
120-121: Consider documenting the parameter reordering.The reordering of parameters (moving
statebeforedmnman) might affect existing callers.Add a comment explaining the parameter order convention:
+// Parameters ordered by: validation state, managers (deterministic, snapshot), view bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, BlockValidationState& state, CDeterministicMNManager& dmnman, llmq::CQuorumSnapshotManager& qsnapman, const CCoinsViewCache& view)Also applies to: 131-132
src/validation.cpp (1)
2475-2475: Consider improving error message formattingThe error message could be more informative by including both the conflicting lock and transaction IDs.
- LogPrintf("ERROR: ConnectBlock(DASH): transaction %s conflicts with transaction lock %s\n", tx->GetHash().ToString(), conflict_txid.ToString()); + LogPrintf("ERROR: ConnectBlock(DASH): transaction %s conflicts with transaction lock %s (conflicting tx: %s)\n", + tx->GetHash().ToString(), conflict_islock.ToString(), conflict_txid.ToString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
src/coinjoin/client.cpp(6 hunks)src/coinjoin/client.h(6 hunks)src/coinjoin/coinjoin.cpp(4 hunks)src/coinjoin/coinjoin.h(3 hunks)src/coinjoin/context.cpp(1 hunks)src/coinjoin/context.h(2 hunks)src/coinjoin/server.cpp(3 hunks)src/coinjoin/server.h(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/evo/cbtx.cpp(4 hunks)src/evo/cbtx.h(2 hunks)src/evo/chainhelper.cpp(1 hunks)src/evo/chainhelper.h(2 hunks)src/evo/deterministicmns.cpp(5 hunks)src/evo/deterministicmns.h(2 hunks)src/evo/specialtxman.cpp(6 hunks)src/evo/specialtxman.h(3 hunks)src/init.cpp(3 hunks)src/llmq/blockprocessor.cpp(6 hunks)src/llmq/blockprocessor.h(3 hunks)src/llmq/chainlocks.cpp(6 hunks)src/llmq/chainlocks.h(3 hunks)src/llmq/commitment.cpp(4 hunks)src/llmq/commitment.h(3 hunks)src/llmq/context.cpp(3 hunks)src/llmq/context.h(2 hunks)src/llmq/debug.cpp(4 hunks)src/llmq/debug.h(3 hunks)src/llmq/dkgsession.cpp(5 hunks)src/llmq/dkgsession.h(3 hunks)src/llmq/dkgsessionhandler.cpp(3 hunks)src/llmq/dkgsessionhandler.h(3 hunks)src/llmq/dkgsessionmgr.cpp(4 hunks)src/llmq/dkgsessionmgr.h(3 hunks)src/llmq/instantsend.cpp(0 hunks)src/llmq/instantsend.h(0 hunks)src/llmq/quorums.cpp(3 hunks)src/llmq/quorums.h(3 hunks)src/llmq/snapshot.cpp(6 hunks)src/llmq/snapshot.h(1 hunks)src/llmq/utils.cpp(17 hunks)src/llmq/utils.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/chainstate.cpp(4 hunks)src/node/chainstate.h(0 hunks)src/node/miner.cpp(3 hunks)src/node/miner.h(2 hunks)src/rest.cpp(5 hunks)src/rpc/mempool.cpp(4 hunks)src/rpc/quorums.cpp(3 hunks)src/test/evo_deterministicmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/test/validation_chainstatemanager_tests.cpp(5 hunks)src/test/validation_flush_tests.cpp(1 hunks)src/txmempool.cpp(2 hunks)src/txmempool.h(4 hunks)src/validation.cpp(7 hunks)src/validation.h(0 hunks)test/lint/lint-circular-dependencies.py(0 hunks)
💤 Files with no reviewable changes (5)
- src/llmq/instantsend.h
- src/llmq/instantsend.cpp
- test/lint/lint-circular-dependencies.py
- src/node/chainstate.h
- src/validation.h
🚧 Files skipped from review as they are similar to previous changes (24)
- src/dsnotificationinterface.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/test/validation_chainstate_tests.cpp
- src/node/miner.h
- src/coinjoin/context.h
- src/test/validation_chainstatemanager_tests.cpp
- src/test/validation_flush_tests.cpp
- src/rest.cpp
- src/llmq/blockprocessor.h
- src/llmq/chainlocks.h
- src/llmq/dkgsession.h
- src/coinjoin/coinjoin.cpp
- src/llmq/debug.h
- src/llmq/commitment.h
- src/llmq/debug.cpp
- src/coinjoin/server.h
- src/llmq/commitment.cpp
- src/llmq/dkgsessionmgr.cpp
- src/coinjoin/client.h
- src/net_processing.cpp
- src/coinjoin/coinjoin.h
- src/txmempool.h
- src/llmq/dkgsessionhandler.h
- src/llmq/dkgsessionhandler.cpp
👮 Files not reviewed due to content moderation or server errors (5)
- src/test/util/setup_common.cpp
- src/llmq/blockprocessor.cpp
- src/coinjoin/server.cpp
- src/txmempool.cpp
- src/coinjoin/client.cpp
🧰 Additional context used
📓 Learnings (1)
src/llmq/quorums.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#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.
🔇 Additional comments (47)
src/llmq/dkgsessionmgr.h (2)
35-35: LGTM: Forward declaration properly placed
The forward declaration of CQuorumSnapshotManager is correctly placed within the llmq namespace and follows the codebase's conventions.
49-49: LGTM: Member variable follows conventions
The m_qsnapman reference member is properly declared and follows the existing naming conventions. Being a reference member, it requires proper initialization in the constructor.
Let's verify the initialization in the implementation file:
✅ Verification successful
Reference member m_qsnapman is properly initialized
The m_qsnapman reference member is correctly initialized in the constructor's initialization list with the qsnapman parameter, following the class's initialization pattern for other reference members.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the constructor implementation to verify proper initialization
ast-grep --pattern 'CDKGSessionManager::CDKGSessionManager($_) : m_qsnapman($_)'
Length of output: 248
Script:
#!/bin/bash
# Let's try a broader search for the constructor implementation
rg -l "CDKGSessionManager::CDKGSessionManager"
Length of output: 73
Script:
#!/bin/bash
# Search for the header file to understand the constructor declaration
fd "dkgsessionmgr.h" --type f
Length of output: 54
Script:
#!/bin/bash
# Let's look at the implementation file if it exists
fd "dkgsessionmgr.cpp" --type f
Length of output: 58
Script:
#!/bin/bash
# If we find the files, let's examine their content
rg "CDKGSessionManager::CDKGSessionManager" -A 5
Length of output: 849
Script:
#!/bin/bash
# Let's see more of the constructor initialization list
rg "CDKGSessionManager::CDKGSessionManager" -A 15
Length of output: 1442
src/llmq/context.cpp (5)
29-31: Ensure exception-safety of snapshot manager instantiation
The qsnapman and quorum_block_processor objects are being created with std::make_unique in the member initializer list. This is typically safe, but if either constructor throws, the object’s partially constructed state might interrupt the normal shutdown logic. Ensure that potential exceptions (e.g. from I/O or memory allocations) are handled gracefully.
33-34: Validate references to snapshot manager in the DKGSessionManager
Here, *qsnapman is passed to the CDKGSessionManager. Confirm that direct references to qsnapman remain valid throughout the entire lifetime of qdkgsman. If there's any conditional teardown of qsnapman before qdkgsman, it could lead to dangling references.
40-41: Confirm correct ordering of chain locks handler dependencies
CChainLocksHandler depends on qman, sigman, shareman, and sporkman. Now, it's also constructed before the CInstantSendManager. Ensure that there is no startup ordering conflict—e.g. if instant send code depends on chain locks code being fully initialized or vice versa, it might lead to race conditions during node startup or shutdown.
42-43: Instantiate InstantSend manager correctly
Passing *clhandler and other references into the CInstantSendManager constructor can be beneficial for tight integration. Make sure that clhandler is fully functional prior to usage in CInstantSendManager initialization to avoid runtime issues.
69-70: Validate the chaining of Start operations
clhandler->Start(*isman) then isman->Start(peerman). Confirm that clhandler->Start does not rely on isman->Start having already run. If there's a shared resource or mode that requires the isman to be started first, reorder calls or add doc comments to reflect the dependence.
src/rpc/quorums.cpp (2)
299-299: Confirm data consistency
When invoking status.ToJson(*CHECK_NONFATAL(node.dmnman), *llmq_ctx.qsnapman, chainman, detailLevel), ensure that DPN (node.dmnman) and qsnapman remain valid throughout the call. This is especially important if the code is multi-threaded or depends on prior initialization.
862-863: Validate snapshot usage in quorum rotation
BuildQuorumRotationInfo is now called with *llmq_ctx.qsnapman. Confirm that the snapshot manager is up-to-date with relevant quorums for accurate rotation info. If there’s an out-of-sync possibility, add synchronization or logs to track usage correctness.
src/init.cpp (2)
327-328: Verify chainstate close parameters
DashChainstateSetupClose(node.chain_helper, node.cpoolman, node.dmnman, node.mnhf_manager, node.llmq_ctx, Assert(node.mempool.get())) includes node.llmq_ctx for additional teardown logic. Double-check the function’s parameter usage to ensure correct finalization order, especially around LLMQ resources.
2024-2025: Double-check the new CJContext parameter
CJContext now takes node.llmq_ctx->isman. Verify that the instant send manager remains in scope as long as the coinjoin context is active. A mismatch in lifetimes could lead to invalid references or shutdown crashes if isman is released prematurely.
src/coinjoin/context.cpp (2)
15-15: Enforce ordering constraints of isman
CJContext accepts an isman reference before peerman. If peerman internally depends on CInstantSendManager, confirm that the order of arguments and subsequent usage is well-defined to avoid partial initialization.
25-25: Confirm correct usage of *dstxman
CCoinJoinServer receives references to dstxman, mn_metaman, mempool, and now isman. Validate that newly introduced instant send checks are integrated properly without creating cyclical dependencies between the server and the manager.
src/rpc/mempool.cpp (2)
204-204: Ensure pointer validity
Using llmq_ctx.isman.get() is safe as long as isman remains in scope. If the LLMQ context or isman can be reset, either protect these calls or ensure a reference is guaranteed valid for the RPC’s lifetime.
335-335: Consistent usage of isman.get()
You now consistently pass llmq_ctx.isman.get() across multiple calls. This is good for uniform code style. Alternatively, if you frequently need CInstantSendManager*, a local alias or reference might simplify repeated calls.
src/evo/chainhelper.h (1)
35-58: Verify architectural direction: Integration of handlers seems to contradict PR objectives
The PR objectives state the removal of llmq::chainLocksHandler and related managers, but this file introduces tight coupling with these components through private members and passthrough functions. This seems to contradict the stated goal of removing these handlers.
Please clarify if:
- This is a transitional state in the refactoring process
- The PR objectives need to be updated
- The implementation needs to be aligned with the objectives
src/llmq/context.h (1)
65-72: Review member initialization order
The order of member declarations is critical as noted in the comments. Verify that the initialization order in the constructor (in the cpp file) matches this declaration order to avoid potential initialization issues:
qsnapmanquorum_block_processorqdkgsmanqmansigmansharemanclhandlerisman
✅ Verification successful
Member initialization order matches declaration order
The initialization order in the constructor matches exactly with the declaration order in the header file:
qsnapmanquorum_block_processorqdkgsmanqmansigmansharemanclhandlerisman
The initialization list in the constructor follows the same sequence as the member declarations, which is the correct practice to avoid potential initialization issues. Note that ehfSignalsHandler is initialized last and appears to be declared after the members we were asked to verify.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check constructor initialization order in cpp file
rg -A 20 "LLMQContext::LLMQContext.*\{" src/llmq/context.cpp
Length of output: 62
Script:
#!/bin/bash
# Let's try to find the file with a more flexible pattern
fd "context.cpp" src/llmq/
# Then let's check the content of the file if found
fd "context.cpp" src/llmq/ -x cat {}
Length of output: 3801
src/evo/specialtxman.h (1)
43-43: Constructor changes look good
The addition of m_qsnapman and its initialization in the constructor is properly implemented. The parameter ordering follows the member variable declaration order, which is a good practice.
Also applies to: 55-58
src/llmq/utils.h (1)
29-31: LGTM! Forward declaration properly added
The CQuorumSnapshotManager class is properly forward declared in the llmq namespace.
src/evo/cbtx.h (1)
93-95: Verify impact of parameter reordering
The parameter order in CalcCbTxMerkleRootMNList has been significantly changed, moving BlockValidationState& state before CDeterministicMNManager& dmnman. This reordering might affect existing callers.
Let's verify the impact:
src/llmq/snapshot.h (1)
210-211: LGTM! Verify function calls are updated.
The updated function signature properly integrates the CQuorumSnapshotManager while maintaining thread safety with the EXCLUSIVE_LOCKS_REQUIRED attribute.
Let's verify that all callers have been updated:
✅ Verification successful
Function calls are properly updated
The verification shows that all callers of BuildQuorumRotationInfo have been updated to include the new qsnapman parameter:
- In
src/net_processing.cpp: Usingm_llmq_ctx->qsnapman - In
src/rpc/quorums.cpp: Usingllmq_ctx.qsnapman
Both instances correctly pass the CQuorumSnapshotManager instance from the LLMQ context, which aligns with the changes in the function signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to BuildQuorumRotationInfo
ast-grep --pattern 'BuildQuorumRotationInfo($$$)'
Length of output: 689
src/evo/specialtxman.cpp (3)
22-23: LGTM! Function signature properly updated.
The CheckSpecialTxInner function signature has been updated to include qsnapman while maintaining all necessary parameters and thread safety requirements.
50-50: LGTM! Proper integration with LLMQ commitment checks.
The call to llmq::CheckLLMQCommitment has been updated to include the snapshot manager, maintaining consistency with the architectural changes.
148-151: Verify error handling in CheckSpecialTxInner.
The function maintains proper error handling with state validation and detailed error messages. However, let's verify the error paths are properly tested.
✅ Verification successful
Let me try a different approach to verify the error handling in CheckSpecialTxInner.
Let me try one more search strategy to find the error handling implementation and tests.
Let me try one final search to find the specific error handling cases.
Error handling in CheckSpecialTxInner is properly implemented and verified
Based on the code analysis, the error handling in CheckSpecialTxInner is robust and well-implemented:
-
The function properly validates special transactions and returns appropriate error states:
- Uses
TX_BAD_SPECIALfor validation failures that don't warrant instant banning - Uses
TX_CONSENSUSfor consensus rule violations - Provides detailed error messages through
GetRejectReason()andGetDebugMessage()
- Uses
-
The error handling covers multiple scenarios:
- Invalid transaction types
- Payload validation failures
- Version checks
- Consensus rule violations
- Special transaction-specific validations
-
The error propagation is consistent:
- Errors are properly propagated up through
BlockValidationResult::BLOCK_CONSENSUS - The assertion
assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS || tx_state.GetResult() == TxValidationResult::TX_BAD_SPECIAL)ensures correct error categorization
- Errors are properly propagated up through
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering error scenarios
rg -A 5 "CheckSpecialTxInner.*Invalid" src/test/
Length of output: 50
Script:
#!/bin/bash
# Search for the implementation of CheckSpecialTxInner to understand error handling paths
ast-grep --pattern 'bool CheckSpecialTxInner($$$) {
$$$
}'
# Also search for test files related to special transactions
fd -e cpp -e h "special.*test"
Length of output: 95
Script:
#!/bin/bash
# Search for CheckSpecialTxInner implementation and its usage
rg -A 10 "CheckSpecialTxInner" src/
# Look for special transaction test files with a broader pattern
fd "test" | rg -i "special"
Length of output: 2984
Script:
#!/bin/bash
# Search for the switch statement handling different transaction types and their validation
rg -B 5 -A 15 "switch.*nType" src/evo/
# Look for the validation state error codes
rg "TX_BAD_SPECIAL|TX_CONSENSUS" src/
Length of output: 24688
src/llmq/quorums.h (3)
51-51: LGTM! Proper forward declaration.
The forward declaration of CQuorumSnapshotManager helps minimize header dependencies.
245-245: LGTM! Member variable properly integrated.
The m_qsnapman member is correctly placed with other manager references and follows the established naming convention.
267-268: Verify constructor updates across the codebase.
The constructor signature has been updated to include the snapshot manager. Let's verify all instantiations have been updated.
src/llmq/snapshot.cpp (1)
87-89: LGTM! Function signature updated to use instance-based snapshot management.
The change from global snapshot manager to instance-based approach via qsnapman parameter improves modularity and testability.
src/node/chainstate.cpp (2)
64-66: LGTM! Chainstate initialization simplified by removing direct LLMQ dependencies.
The changes align with the PR objective of moving LLMQ functionality to LLMQContext.
231-238: Verify the order of manager initialization in CChainstateHelper.
The initialization order of managers in CChainstateHelper is critical. Ensure that all required managers from llmq_ctx are properly initialized before use.
✅ Verification successful
The initialization order in CChainstateHelper is correct and properly managed
Based on the verification of both the header file (chainhelper.h) and implementation (chainhelper.cpp), the initialization order is correct and safe:
- The class members are properly declared in the header file with clear dependencies.
- The constructor implementation in
chainhelper.cppshows correct initialization:- Reference members (
isman,clhandler) are initialized first in the initialization list - Unique pointers (
mn_payments,special_tx) are initialized after, using the constructor parameters
- Reference members (
- All required managers from
llmq_ctxare passed as references, ensuring they are already initialized beforeCChainstateHelperis constructed.
The code in the review snippet shows these managers are indeed initialized before being passed to CChainstateHelper, making the initialization order safe and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the initialization order in CChainstateHelper constructor
ast-grep --pattern 'class CChainstateHelper {
$$$
CChainstateHelper($$$) {
$$$
}
$$$
}'
Length of output: 101
Script:
#!/bin/bash
# Let's try to find the CChainstateHelper class definition and implementation
rg -l "CChainstateHelper" --type cpp
# Then let's look at the content of these files
rg "CChainstateHelper::" -A 10 --type cpp
Length of output: 3781
Script:
#!/bin/bash
# Let's check the header file for the class definition and member initialization order
cat src/evo/chainhelper.h
Length of output: 2381
src/evo/cbtx.cpp (1)
65-66: LGTM! Consistent parameter updates for snapshot management.
The changes maintain the existing functionality while transitioning to instance-based snapshot management through qsnapman.
Also applies to: 90-91
src/llmq/chainlocks.cpp (3)
55-63: LGTM: Parameter addition is consistent with refactoring goals
The addition of the isman parameter and its usage in the scheduled task is well-structured and maintains the existing functionality while aligning with the refactoring objectives.
Line range hint 186-198: LGTM: Thread-safe parameter propagation
The changes correctly propagate the isman parameter while maintaining thread-safety through atomic operations.
Line range hint 218-306: Verify the InstantSend locking behavior change
The change from global state to using isman.IsLocked is architecturally sound, but we should verify that all transaction locking checks behave consistently.
✅ Verification successful
Transaction locking behavior is consistent across the codebase
The verification shows that IsLocked is used consistently throughout the codebase. The change from global state to using isman.IsLocked in CChainLocksHandler::TrySignChainTip aligns with other usages:
- In mempool management:
m_isman->IsLockedfor checking transaction locks - In net processing:
m_llmq_ctx->isman->IsLockedfor transaction validation - In block mining:
m_isman.IsLockedfor transaction selection - In RPC interfaces:
isman.IsLockedfor transaction status reporting - In CoinJoin:
isman.IsLockedfor input validation
The architectural change maintains consistent behavior while removing global state dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other IsLocked calls to ensure consistent usage
rg "IsLocked\(" --type cpp -B 2 -A 2
Length of output: 15078
src/node/miner.cpp (1)
39-39: LGTM: Clean snapshot manager integration
The addition of the snapshot manager header and member initialization is clean and follows the established pattern for manager integration in the BlockAssembler class.
Also applies to: 79-79
src/evo/deterministicmns.h (2)
38-40: LGTM: Clean forward declaration
The forward declaration of CQuorumSnapshotManager is properly placed in the llmq namespace.
601-602: LGTM: Consistent parameter propagation
The addition of qsnapman parameter to ProcessBlock, BuildNewListFromBlock, and HandleQuorumCommitment methods is consistent with the refactoring goals and maintains proper const-correctness.
Also applies to: 608-612
src/llmq/utils.cpp (2)
112-115: LGTM: Parameter addition aligns with refactoring goals
The addition of qsnapman parameter to GetAllQuorumMembers is consistent with the PR's objective of integrating snapshot management into quorum operations.
Line range hint 368-368: LGTM: Proper integration of snapshot manager in connection handling
The addition of m_qsnapman to EnsureQuorumConnections maintains consistency with the refactoring pattern and properly integrates snapshot management into quorum connection handling.
src/llmq/dkgsession.cpp (1)
Line range hint 74-85: LGTM: Clean integration of snapshot manager in DKG session
The constructor properly initializes the snapshot manager as a member variable and maintains the initialization order in the member initialization list.
src/llmq/quorums.cpp (1)
214-224: LGTM: Proper initialization sequence confirmed
The addition of qsnapman to CQuorumManager constructor is safe as the manager is fully initialized by LLMQContext, as confirmed by previous discussions.
src/evo/deterministicmns.cpp (3)
598-600: LGTM! Parameter addition looks good.
The addition of qsnapman parameter to ProcessBlock method is well-integrated and properly propagated to BuildNewListFromBlock call.
705-707: LGTM! Parameter propagation looks good.
The addition of qsnapman parameter to BuildNewListFromBlock method is well-integrated and properly propagated to HandleQuorumCommitment call.
1002-1003: LGTM! Parameter usage looks good.
The addition of qsnapman parameter to HandleQuorumCommitment method is well-integrated and used for retrieving quorum members.
src/validation.cpp (3)
725-734: LGTM! Improved conflict detection handling
The changes improve the InstantSend conflict detection by:
- Using std::optional for clearer API semantics
- Properly extracting both the lock and conflicting txid when a conflict is found
- Adding notification for double spend attempts through GetMainSignals()
Line range hint 2461-2479: LGTM! Improved chainlock and InstantSend conflict handling
The changes correctly implement the consensus rules by:
- Allowing chainlocked transactions to override InstantSend locks
- Handling multiple conflicts per transaction
- Properly removing overridden InstantSend locks
Line range hint 2164-2168: LGTM! Early chainlock validation in ConnectBlock
The changes improve block validation by checking chainlock conflicts early in the block connection process, before performing expensive validation steps. This is more efficient and uses the correct BlockValidationResult::BLOCK_CHAINLOCK error code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/txmempool.cpp (1)
Line range hint
1584-1588: Consider adding error handling for null m_ismanWhile the implementation is correct, it would be more robust to add a null check for m_isman before dereferencing.
- if (m_isman && m_isman->IsLocked(it->GetTx().GetHash())) { + if (!m_isman) { + LogPrint(BCLog::MEMPOOL, "Warning: m_isman is null in Expire()\n"); + } else if (m_isman->IsLocked(it->GetTx().GetHash())) {🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 1585-1585: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
src/llmq/snapshot.cpp (1)
209-211: Consider extracting repeated snapshot retrieval patternThe snapshot retrieval and error handling pattern is repeated multiple times with only slight variations. Consider extracting this into a helper function to reduce duplication.
Example implementation:
+ bool GetAndValidateSnapshot(CQuorumSnapshotManager& qsnapman, + Consensus::LLMQType llmqType, + const CBlockIndex* pBlockIndex, + const std::string& heightDesc, + CQuorumSnapshot& outSnapshot, + std::string& errorRet) { + auto snapshot = qsnapman.GetSnapshotForBlock(llmqType, pBlockIndex); + if (!snapshot.has_value()) { + errorRet = strprintf("Can not find quorum snapshot at %s", heightDesc); + return false; + } + outSnapshot = std::move(snapshot.value()); + return true; +} // Usage example: - auto snapshotHMinusC = qsnapman.GetSnapshotForBlock(llmqType, pBlockHMinusCIndex); - if (!snapshotHMinusC.has_value()) { - errorRet = strprintf("Can not find quorum snapshot at H-C"); - return false; - } else { - response.quorumSnapshotAtHMinusC = std::move(snapshotHMinusC.value()); - } + if (!GetAndValidateSnapshot(qsnapman, llmqType, pBlockHMinusCIndex, "H-C", + response.quorumSnapshotAtHMinusC, errorRet)) { + return false; + }Also applies to: 221-223, 233-235, 249-251, 305-307
src/llmq/snapshot.h (2)
Line range hint
213-234: Consider improvements to theCQuorumSnapshotManagerimplementationWhile the basic structure is sound, consider the following improvements:
- Document the rationale for the hardcoded cache size of 32 entries
- Add documentation for public methods explaining their behavior and thread safety guarantees
- Mark
GetSnapshotForBlockasconstif it doesn't modify internal state (beyond cache)- Clarify error handling strategy (e.g., when should
GetSnapshotForBlockreturn nullopt?)Example documentation format:
/** * Retrieves a snapshot for the specified quorum type and block. * Thread-safe. May populate internal cache. * * @param llmqType The type of LLMQ to get snapshot for * @param pindex Block index to get snapshot at * @return Optional containing the snapshot if found, nullopt otherwise */ std::optional<CQuorumSnapshot> GetSnapshotForBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex) const;
Line range hint
89-93: Consider implementing the suggested class split in TODO commentThe TODO comment suggests splitting this class into:
CQuorumSnapshotfor core snapshot dataCSnapshotInfofor snapshot + mnListDiff dataThis split would improve:
- Code organization and maintainability
- Separation of concerns
- Data structure clarity
Would you like me to propose a detailed design for this split?
src/rest.cpp (1)
636-639: Inconsistent pointer access pattern.While the integration is correct, the use of
get()onismanis inconsistent with other similar calls in the file. For consistency, consider using the same pattern as other calls:-UniValue mempoolObject = MempoolToJSON(*mempool, llmq_ctx->isman.get(), true); +UniValue mempoolObject = MempoolToJSON(*mempool, *llmq_ctx->isman, true);src/validation.cpp (2)
725-734: Improved error handling for instant send conflictsThe code now uses an optional return value to handle instant send conflicts more safely, with proper error logging and signal notifications.
Consider extracting the conflict notification logic into a separate helper method to improve readability:
-if (txConflict) { - GetMainSignals().NotifyInstantSendDoubleSpendAttempt(ptx, txConflict); -} +NotifyInstantSendConflict(ptx, txConflict);
Line range hint
2461-2479: Improved instant send conflict handling in block validationThe code now handles instant send conflicts more robustly during block validation:
- Only checks when instant send reject conflicts is enabled
- Properly handles chainlocked blocks overriding instant send locks
- Provides clear error messages
Consider extracting the conflict resolution logic into a separate method to improve readability:
+bool ResolveISLockConflict(const CTransaction& tx, bool has_chainlock) { + while (auto conflictLockOpt = m_chain_helper->HasConflictingISLock(tx)) { + auto [conflict_islock_hash, conflict_txid] = conflictLockOpt.value(); + if (has_chainlock) { + LogPrint(BCLog::ALL, "Chain-locked transaction %s overrides islock %s\n", + tx.GetHash().ToString(), conflict_islock_hash.ToString()); + m_chain_helper->RemoveConflictingISLockByTx(tx); + } else { + LogPrintf("Transaction %s conflicts with lock %s\n", + tx.GetHash().ToString(), conflict_txid.ToString()); + return false; + } + } + return true; +}src/coinjoin/server.h (1)
39-39: Document InstantSend integration with CoinJoin.The integration of
CInstantSendManagerintoCCoinJoinServerrepresents a significant architectural change. Consider adding documentation that explains:
- The relationship between CoinJoin and InstantSend
- How InstantSend affects CoinJoin transaction validation
- Any new requirements or constraints this integration introduces
This will help maintainers understand the design decisions and implications.
Also applies to: 98-107
src/llmq/chainlocks.h (1)
Line range hint
1-24: Documentation improvements neededGiven the significant architectural changes and complex interactions between chain locks and instant send:
- Add class-level documentation for
CChainLocksHandler- Document the relationship and interaction between chain locks and instant send
- Consider adding examples or sequence diagrams for complex flows
src/evo/chainhelper.cpp (1)
41-60: Consider adding error logging for conflict resolutionThe InstantSend passthrough functions handle conflicts well, but consider adding debug logging when conflicts are detected or resolved, especially in
RemoveConflictingISLockByTx.bool CChainstateHelper::RemoveConflictingISLockByTx(const CTransaction& tx) { const auto islock = isman.GetConflictingLock(tx); if (!islock) return false; + LogDebug("instantsend", "Removing conflicting IS lock for tx=%s", tx.GetHash().ToString()); isman.RemoveConflictingLock(::SerializeHash(*islock), *islock); return true; }src/llmq/context.cpp (1)
55-70: Ensure proper thread interruption orderThe order of thread interruption and startup is critical. Consider documenting the dependency order and why this specific sequence is chosen.
void LLMQContext::Interrupt() { + // Thread interruption order: + // 1. shareman - Handle share management cleanup first + // 2. sigman - Clean up signing operations + // 3. isman - Finally stop instant send operations shareman->InterruptWorkerThread(); sigman->InterruptWorkerThread(); isman->InterruptWorkerThread(); }src/llmq/blockprocessor.h (1)
53-54: Consider documenting the ownership modelWhile the reference member is correctly used, it would be helpful to document the ownership expectations of
qsnapman.+ /// @param qsnapman Reference to the snapshot manager. Lifetime must exceed this instance. explicit CQuorumBlockProcessor(CChainState& chainstate, CDeterministicMNManager& dmnman, CEvoDB& evoDb, CQuorumSnapshotManager& qsnapman);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
src/coinjoin/client.cpp(6 hunks)src/coinjoin/client.h(6 hunks)src/coinjoin/coinjoin.cpp(4 hunks)src/coinjoin/coinjoin.h(3 hunks)src/coinjoin/context.cpp(1 hunks)src/coinjoin/context.h(2 hunks)src/coinjoin/server.cpp(3 hunks)src/coinjoin/server.h(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/evo/cbtx.cpp(4 hunks)src/evo/cbtx.h(2 hunks)src/evo/chainhelper.cpp(1 hunks)src/evo/chainhelper.h(2 hunks)src/evo/deterministicmns.cpp(5 hunks)src/evo/deterministicmns.h(2 hunks)src/evo/specialtxman.cpp(6 hunks)src/evo/specialtxman.h(3 hunks)src/init.cpp(3 hunks)src/llmq/blockprocessor.cpp(6 hunks)src/llmq/blockprocessor.h(3 hunks)src/llmq/chainlocks.cpp(6 hunks)src/llmq/chainlocks.h(3 hunks)src/llmq/commitment.cpp(4 hunks)src/llmq/commitment.h(3 hunks)src/llmq/context.cpp(3 hunks)src/llmq/context.h(2 hunks)src/llmq/debug.cpp(4 hunks)src/llmq/debug.h(3 hunks)src/llmq/dkgsession.cpp(4 hunks)src/llmq/dkgsession.h(3 hunks)src/llmq/dkgsessionhandler.cpp(4 hunks)src/llmq/dkgsessionhandler.h(3 hunks)src/llmq/dkgsessionmgr.cpp(5 hunks)src/llmq/dkgsessionmgr.h(3 hunks)src/llmq/instantsend.cpp(0 hunks)src/llmq/instantsend.h(0 hunks)src/llmq/quorums.cpp(4 hunks)src/llmq/quorums.h(3 hunks)src/llmq/snapshot.cpp(6 hunks)src/llmq/snapshot.h(1 hunks)src/llmq/utils.cpp(17 hunks)src/llmq/utils.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/chainstate.cpp(4 hunks)src/node/chainstate.h(0 hunks)src/node/miner.cpp(3 hunks)src/node/miner.h(2 hunks)src/rest.cpp(5 hunks)src/rpc/mempool.cpp(4 hunks)src/rpc/quorums.cpp(3 hunks)src/test/evo_deterministicmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/test/validation_chainstatemanager_tests.cpp(5 hunks)src/test/validation_flush_tests.cpp(1 hunks)src/txmempool.cpp(2 hunks)src/txmempool.h(4 hunks)src/validation.cpp(7 hunks)src/validation.h(0 hunks)test/lint/lint-circular-dependencies.py(0 hunks)
💤 Files with no reviewable changes (5)
- src/llmq/instantsend.h
- test/lint/lint-circular-dependencies.py
- src/llmq/instantsend.cpp
- src/node/chainstate.h
- src/validation.h
🚧 Files skipped from review as they are similar to previous changes (32)
- src/dsnotificationinterface.cpp
- src/llmq/context.h
- src/node/miner.h
- src/test/evo_deterministicmns_tests.cpp
- src/rpc/mempool.cpp
- src/test/validation_flush_tests.cpp
- src/llmq/debug.h
- src/test/validation_chainstatemanager_tests.cpp
- src/test/validation_chainstate_tests.cpp
- src/llmq/debug.cpp
- src/coinjoin/context.h
- src/coinjoin/context.cpp
- src/node/chainstate.cpp
- src/llmq/dkgsession.h
- src/llmq/dkgsessionhandler.h
- src/evo/cbtx.h
- src/llmq/dkgsessionmgr.h
- src/rpc/quorums.cpp
- src/llmq/commitment.cpp
- src/coinjoin/coinjoin.h
- src/evo/specialtxman.h
- src/llmq/dkgsession.cpp
- src/llmq/quorums.h
- src/coinjoin/coinjoin.cpp
- src/evo/chainhelper.h
- src/txmempool.h
- src/evo/cbtx.cpp
- src/coinjoin/client.h
- src/init.cpp
- src/coinjoin/client.cpp
- src/llmq/dkgsessionhandler.cpp
- src/net_processing.cpp
🧰 Additional context used
📓 Learnings (1)
src/llmq/quorums.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#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.
🔇 Additional comments (53)
src/txmempool.cpp (1)
468-473: LGTM: ConnectManagers implementation is correct
The updated implementation properly initializes both managers and maintains the double-initialization protection.
src/llmq/chainlocks.cpp (3)
55-63: LGTM: Clean refactoring of dependency injection
The changes properly implement dependency injection by passing CInstantSendManager as a parameter instead of relying on global state, which aligns with the PR objectives.
Line range hint 186-199: LGTM: Thread-safe implementation of UpdatedBlockTip
The implementation correctly:
- Uses atomic operations to prevent parallel scheduling
- Maintains proper execution order through the scheduler
- Passes the InstantSendManager consistently
Line range hint 218-307: LGTM: Consistent refactoring of TrySignChainTip
The changes maintain the existing behavior while properly integrating the injected InstantSendManager. The WAIT_FOR_ISLOCK_TIMEOUT concern was previously addressed (retries occur every few seconds).
src/llmq/snapshot.cpp (1)
87-88: LGTM: Good refactoring of dependency injection
The updated function signature now explicitly accepts CQuorumSnapshotManager as a reference parameter, removing dependency on global state. This change improves modularity and testability.
src/llmq/snapshot.h (1)
210-211: LGTM: Function signature update aligns with refactoring goals
The addition of CQuorumSnapshotManager parameter is consistent with the PR's objective of moving functionality to dedicated managers. The locking requirements and parameter ordering are well-structured.
src/rest.cpp (4)
135-154: LGTM! Well-structured helper function.
The implementation follows the established pattern of other context getters in the file, with proper error handling and clear documentation.
271-276: LGTM! Clean integration of LLMQContext.
The changes properly handle the LLMQ context retrieval and integrate it into the JSON response generation.
344-347: LGTM! Proper integration of LLMQContext components.
The changes correctly integrate both the chain locks handler and instant send manager from the LLMQ context.
610-613: LGTM! Clean integration of InstantSend manager.
The changes properly integrate the instant send manager for mempool info JSON generation.
src/test/util/setup_common.cpp (4)
55-62: LGTM: Header inclusions are well-organized
The added headers are necessary and properly organized, supporting the core functionality being tested.
111-112: LGTM: Proper integration of LLMQ context
The addition of node.llmq_ctx to the setup call aligns with the PR's objective of moving functionality to LLMQContext.
124-125: LGTM: Proper integration of InstantSend manager
The addition of *node.llmq_ctx->isman to the CJContext constructor properly integrates the InstantSend manager through the LLMQ context.
475-475: Verify all callers of CalcCbTxMerkleRootMNList
The change to use *m_node.llmq_ctx->qsnapman for quorum snapshot management is correct, but let's verify all callers are updated.
✅ Verification successful
All callers of CalcCbTxMerkleRootMNList are properly updated
Based on the search results, there are only three callers of CalcCbTxMerkleRootMNList:
src/test/util/setup_common.cpp: Uses*m_node.llmq_ctx->qsnapmansrc/node/miner.cpp: Usesm_qsnapmansrc/evo/cbtx.cpp: Usesqsnapmanparameter directly
All callers are properly updated to use the quorum snapshot manager parameter, either through direct parameter passing or through the node's LLMQ context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to CalcCbTxMerkleRootMNList to verify they use the new parameter
rg -A 3 "CalcCbTxMerkleRootMNList" --type cpp
Length of output: 2563
src/llmq/utils.h (3)
29-30: LGTM! Clean forward declaration.
The forward declaration of CQuorumSnapshotManager in the llmq namespace is well-placed and follows good practices for header organization.
34-37: LGTM! Consistent parameter additions across related functions.
The addition of CQuorumSnapshotManager& qsnapman parameter is implemented consistently across all related functions, with proper const-correctness and naming conventions.
Also applies to: 41-43, 44-45, 49-53, 54-56
34-37: Verify impact of API changes.
While the PR objectives state "no breaking changes are expected", the modification of these public function signatures in a header file could potentially break downstream code that calls these functions.
Let's verify the usage of these functions across the codebase:
Also applies to: 41-43, 44-45, 49-53, 54-56
✅ Verification successful
API changes are properly handled across the codebase
Based on the search results, all calls to the modified functions across the codebase are already using the correct parameter list that matches the new signatures:
GetAllQuorumMembers: All 13 call sites pass the required parameters includingdmnman,qsnapman, andpQuorumBaseBlockIndexGetQuorumConnections: All calls include the newqsnapmanparameterGetQuorumRelayMembers: All invocations properly pass the required parametersEnsureQuorumConnections: The 2 call sites correctly include all parameters includingqsnapmanAddQuorumProbeConnections: Both calls in the codebase pass the complete parameter list
The changes appear to be part of a coordinated update where all callers were modified along with the function signatures. No breaking changes or incompatible calls were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes by finding all calls to the modified functions
# that might need updates due to the new parameter requirements.
echo "Checking usage of modified functions..."
# Search for function calls that might need updates
for func in "GetAllQuorumMembers" "GetQuorumConnections" "GetQuorumRelayMembers" "EnsureQuorumConnections" "AddQuorumProbeConnections"; do
echo "=== Checking $func ==="
rg "\b${func}\(" --type cpp -A 2
done
Length of output: 10681
src/validation.cpp (3)
Line range hint 737-750: LGTM: Improved mempool conflict handling for instant send transactions
The code correctly handles mempool conflicts by:
- Removing conflicting transactions when waiting for instant send
- Checking for conflicts with mempool transactions otherwise
2164-2167: LGTM: Added chainlock conflict validation in ConnectBlock
The code properly validates that the block does not conflict with existing chainlocks before connecting it.
5494-5494: LGTM: Updated CChainState constructor call
The code correctly passes the chain helper to the CChainState constructor.
src/coinjoin/server.h (2)
39-39: LGTM! Member variable follows conventions.
The new member m_isman is properly declared as a const reference and follows the established naming conventions.
Line range hint 98-107: Verify CoinJoin server instantiation sites.
The constructor changes look good, following proper initialization patterns. However, we should verify all instantiation sites of CCoinJoinServer have been updated with the new isman parameter.
✅ Verification successful
Let me search for actual instantiations of the class, as the previous result only showed the declaration.
Let me try a different approach to find instantiations by looking for variable declarations of CCoinJoinServer type.
Let me check the actual instantiation in the context implementation file.
Constructor changes are properly integrated
The verification shows that the only instantiation of CCoinJoinServer in src/coinjoin/context.cpp has been correctly updated to include the new isman parameter. The instantiation follows the same parameter order as defined in the constructor, and all required parameters are present.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CCoinJoinServer instantiations to verify they include the new isman parameter
rg -l "CCoinJoinServer" | xargs rg "CCoinJoinServer\s*\(" -A 10
Length of output: 953
Script:
#!/bin/bash
# Search for new CCoinJoinServer object creation
rg "new CCoinJoinServer\s*\(" -A 10
Length of output: 37
Script:
#!/bin/bash
# Search for CCoinJoinServer variable declarations and instantiations
rg "CCoinJoinServer\s+\w+" -B 2 -A 5
# Also search for std::unique_ptr with CCoinJoinServer
rg "unique_ptr\s*<\s*CCoinJoinServer\s*>" -B 2 -A 5
Length of output: 411
Script:
#!/bin/bash
# Search for the implementation of CoinJoin context where server is instantiated
rg -l "coinjoin/context" | xargs rg "server\s*\{.*CCoinJoinServer" -A 10
Length of output: 416
src/llmq/chainlocks.h (2)
34-34: LGTM: Forward declaration aligns with refactoring goals
The forward declaration of CInstantSendManager is properly placed and follows codebase conventions.
93-93: Architectural concern: Consider dependency direction
The addition of CInstantSendManager dependencies to these methods seems to contradict the PR's goal of removing these handlers and moving functionality to LLMQContext. This change might make future refactoring more challenging.
Consider:
- Whether these dependencies could be moved to
LLMQContextdirectly - If the dependency injection is necessary, whether it should be done at construction time rather than method level
Let's verify the usage of these methods across the codebase:
Also applies to: 104-104, 109-109
src/evo/chainhelper.cpp (2)
13-23: LGTM: Constructor implementation follows best practices
The constructor properly initializes all components with clear member initialization order. The separation of concerns between payment processing and special transaction handling is well-structured.
28-39: LGTM: Well-implemented chainlock passthrough functions
The chainlock-related passthrough functions are properly implemented with:
- Clear documentation
- Consistent error handling
- Proper const correctness
src/llmq/context.cpp (1)
40-43: Verify thread safety in manager initialization
The initialization of clhandler and isman needs careful consideration of thread safety, especially since they share resources. This was previously flagged in an earlier review.
Run this verification script to analyze potential thread safety issues:
#!/bin/bash
# Search for mutex usage and potential race conditions
echo "Checking mutex usage in instantsend and chainlock handlers..."
rg -A 5 "std::mutex|std::lock_guard|std::unique_lock"
echo "Checking for concurrent access patterns..."
ast-grep --pattern 'class $CLASS {
$$$
std::mutex $_
$$$
}'src/llmq/blockprocessor.h (1)
34-34: LGTM: Clean integration of CQuorumSnapshotManager
The addition of CQuorumSnapshotManager and its integration into CQuorumBlockProcessor is well-designed:
- Forward declaration properly placed
- Member variable follows class member ordering
- Constructor parameter properly added
Also applies to: 44-44, 53-54
src/llmq/commitment.h (3)
30-30: LGTM: Forward declaration of CQuorumSnapshotManager
The forward declaration is properly placed within the llmq namespace.
69-70: LGTM: Updated Verify method signature
The addition of CQuorumSnapshotManager& qsnapman parameter maintains const correctness and aligns with the refactoring goals.
180-182: LGTM: Updated CheckLLMQCommitment function signature
The addition of CQuorumSnapshotManager& qsnapman parameter is consistent with the changes in the Verify method.
src/evo/specialtxman.cpp (4)
22-26: LGTM: Updated CheckSpecialTxInner signature
The function signature update properly integrates CQuorumSnapshotManager while maintaining the existing parameter order and lock requirements.
51-51: LGTM: Updated CheckLLMQCommitment call
The call site correctly passes qsnapman to match the updated function signature.
150-151: LGTM: Updated CheckSpecialTxInner call in ProcessSpecialTxsInBlock
The call site correctly passes m_qsnapman to match the updated function signature.
176-176: LGTM: Updated ProcessBlock and CheckCbTxMerkleRoots calls
Both call sites correctly integrate m_qsnapman parameter.
Also applies to: 185-185
src/llmq/dkgsessionmgr.cpp (3)
34-34: LGTM: Updated constructor signature and member initialization
The constructor properly integrates CQuorumSnapshotManager and initializes m_qsnapman member.
Also applies to: 43-43
57-58: LGTM: Updated dkgSessionHandlers initialization
The initialization correctly passes m_qsnapman to the session handlers.
301-301: LGTM: Updated GetAllQuorumMembers calls
Both call sites correctly pass m_qsnapman to the utility function.
Also applies to: 347-347
src/node/miner.cpp (1)
39-39: LGTM: Integration of CQuorumSnapshotManager
The changes correctly integrate the CQuorumSnapshotManager by adding the necessary header and member variable.
Also applies to: 79-79
src/evo/deterministicmns.h (2)
38-40: LGTM: Forward declaration of CQuorumSnapshotManager
The forward declaration is correctly placed in the llmq namespace.
601-602: LGTM: Method signature updates for CQuorumSnapshotManager integration
The method signatures are correctly updated to include the CQuorumSnapshotManager parameter, maintaining consistency with the refactoring objectives.
Let's verify that all call sites have been updated to pass the new parameter:
Also applies to: 608-610, 611-612
✅ Verification successful
Let me search for the call sites of these methods to verify the parameter updates.
Let me try a different search pattern to find the call sites.
All call sites correctly updated with CQuorumSnapshotManager parameter
Based on the search results, all call sites have been properly updated to include the qsnapman parameter:
- In
src/evo/specialtxman.cpp:ProcessBlockis called withm_qsnapman - In
src/evo/cbtx.cpp:BuildNewListFromBlockis called withqsnapman - In
src/evo/deterministicmns.cpp: Both internal calls toBuildNewListFromBlockandHandleQuorumCommitmentare properly passing theqsnapmanparameter
The method signature changes have been consistently applied across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to these methods to ensure they've been updated
rg -A 5 "ProcessBlock|BuildNewListFromBlock|HandleQuorumCommitment" --type cpp
Length of output: 49907
Script:
#!/bin/bash
# Search for calls to ProcessBlock, BuildNewListFromBlock, and HandleQuorumCommitment
ast-grep --pattern 'ProcessBlock($$$)'
ast-grep --pattern 'BuildNewListFromBlock($$$)'
ast-grep --pattern 'HandleQuorumCommitment($$$)'
Length of output: 1382
Script:
#!/bin/bash
# Search for calls to these methods with ripgrep for better context
rg -U "ProcessBlock.*qsnapman|BuildNewListFromBlock.*qsnapman|HandleQuorumCommitment.*qsnapman" --type cpp
Length of output: 626
src/llmq/blockprocessor.cpp (4)
29-34: LGTM: PreComputeQuorumMembers function update
The function signature and implementation are correctly updated to use CQuorumSnapshotManager for getting quorum members.
47-52: LGTM: CQuorumBlockProcessor constructor update
The constructor correctly initializes the new m_qsnapman member variable.
139-139: LGTM: qc.Verify calls update
The qc.Verify calls are correctly updated to pass the m_qsnapman parameter.
Also applies to: 276-276
165-165: LGTM: PreComputeQuorumMembers calls update
The calls to PreComputeQuorumMembers are correctly updated to pass the m_qsnapman parameter.
Also applies to: 320-320
src/coinjoin/server.cpp (1)
586-586: LGTM: Parameter updates for InstantSend integration
The changes correctly integrate the new m_isman parameter into the collateral and input/output validation calls, maintaining consistency with the PR's refactoring objectives.
Also applies to: 620-621
src/llmq/utils.cpp (2)
60-86: LGTM: Consistent parameter integration
The function signature updates correctly integrate the qsnapman parameter across all relevant functions, maintaining consistency with the refactoring objectives.
238-240:
Implement proper error handling for external triggers
Multiple TODO comments indicate missing error handling for externally triggered scenarios. Consider:
- Implementing proper exception handling for P2P and block validation cases
- Converting assertions to runtime checks with appropriate error responses
Let me verify the error handling patterns in the codebase:
Also applies to: 303-305, 310-312, 316-318
src/llmq/quorums.cpp (1)
215-215: LGTM: Proper initialization through LLMQContext
The constructor changes correctly integrate qsnapman parameter. Based on previous feedback, the CQuorumManager is fully initialized by LLMQContext, addressing any initialization sequence concerns.
Also applies to: 225-225
src/evo/deterministicmns.cpp (4)
598-601: LGTM: Parameter addition aligns with refactoring goals
The addition of qsnapman parameter is consistent with the PR's objective of moving quorum functionality to LLMQContext.
706-709: LGTM: Consistent parameter addition
The addition of qsnapman parameter maintains consistency with the ProcessBlock method changes.
1004-1007: LGTM: Consistent parameter addition for quorum handling
The addition of qsnapman parameter completes the consistent pattern of quorum snapshot manager integration.
932-932: Verify quorum member retrieval changes
The modification to pass qsnapman to handle quorum commitments is a critical change affecting quorum member retrieval.
Run the following script to verify the quorum member retrieval implementation:
✅ Verification successful
Let me gather more information about the implementation to verify the consistency.
Let me try a different pattern to locate the implementation.
Quorum member retrieval implementation is consistent
Based on the verification results, the modification to pass qsnapman to HandleQuorumCommitment is correct and consistent with the codebase usage. The evidence shows:
- All calls to
GetAllQuorumMembersconsistently use four parameters:llmqType,dmnman,qsnapman, andpQuorumBaseBlockIndex - The function is used across multiple components (commitment, quorums, dkg sessions) with the same parameter pattern
- The implementation in
src/evo/deterministicmns.cppfollows the same pattern as other callers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the quorum member retrieval implementation
# Search for all calls to GetAllQuorumMembers to ensure consistent parameter usage
ast-grep --pattern 'GetAllQuorumMembers($$$)'
Length of output: 693
Script:
#!/bin/bash
# Check the implementation of GetAllQuorumMembers
ast-grep --pattern 'GetAllQuorumMembers(LLMQType $_, $_, $_, $_) {
$$$
}'
Length of output: 77
Script:
#!/bin/bash
# Search for the GetAllQuorumMembers function definition and its usage context
rg "GetAllQuorumMembers" -A 5 --type cpp
Length of output: 5979
| @@ -34,3 +37,24 @@ bool CChainstateHelper::HasChainLock(int nHeight, const uint256& blockHash) cons | |||
| } | |||
|
|
|||
| int32_t CChainstateHelper::GetBestChainLockHeight() const { return clhandler.GetBestChainLock().getHeight(); } | |||
|
|
|||
| /** Passthrough functions to CInstantSendManager */ | |||
| std::optional<std::pair</*islock_hash=*/uint256, /*txid=*/uint256>> CChainstateHelper::HasConflictingISLock( | |||
There was a problem hiding this comment.
nit: consider making this a struct
There was a problem hiding this comment.
Unlikely to house any functions and is used in a very limited manner (invoked twice in validation), think it's fine.
There was a problem hiding this comment.
nit: maybe rename it to CChainstateHelper::ConflictingISLockIfAny(...) ?
| const auto islock = isman.GetConflictingLock(tx); | ||
| if (!islock) return std::nullopt; | ||
| return std::make_pair(::SerializeHash(*islock), islock->txid); |
There was a problem hiding this comment.
why not simply
return isman.GetConflictingLock(tx);
then we are just returning a shared_ptr to the conflicting lock, and the caller can do whatever they need with it
There was a problem hiding this comment.
This has been addressed in the PR description:
Care has been taken to avoid returning or accepting as arguments, types related to a particular manager (like CInstantSendLockPtr) to avoid tying it down to implementation-specific details but are otherwise passthrough.
We don't want CChainstateHelper and friends to expose implementation-specific details, it'll do what you ask but won't let you do anything more. The idea being that if we need it to do more, it would be defined in a new function and it's immediately apparent what kinds of interactions are happening between validation code and the managers.
TestBlockValidity is the exception to this as it still has wholesale access to CChainLocksHandler but this is because TestBlockValidity is used pervasively in RPC and mining code, which readily use LLMQ managers while CChainstateHelper is meant to primarily reside in validation code.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/rest.cpp (3)
344-347:⚠️ Potential issueProtect against null pointer dereference
Similar to the headers endpoint, accessing
llmq_ctx->clhandlerandllmq_ctx->ismandirectly could cause a crash if they are null.
610-613:⚠️ Potential issueProtect against null pointer dereference
Similar to previous endpoints, accessing
llmq_ctx->ismandirectly could cause a crash if it is null.
636-639:⚠️ Potential issueProtect against null pointer dereference
Similar to previous endpoints, accessing
llmq_ctx->ismandirectly could cause a crash if it is null.
🧹 Nitpick comments (8)
src/llmq/utils.h (1)
34-37: Consistent integration ofCQuorumSnapshotManageracross utility functionsThe refactoring consistently integrates the snapshot manager across all relevant utility functions. This architectural change improves the modularity of quorum management by centralizing snapshot-related operations.
Consider documenting the responsibility split between
CQuorumManagerandCQuorumSnapshotManagerin the class documentation to clarify their distinct roles.Also applies to: 41-43, 44-46, 49-53, 55-56
src/llmq/snapshot.h (1)
Line range hint
216-231: Review the hardcoded cache size and consider configuration optionsThe implementation of
CQuorumSnapshotManagerlooks solid with proper thread safety usingRecursiveMutex. However:
- The LRU cache size is hardcoded to 32 entries
- The cache configuration might need to be adjustable based on system resources
Consider making the cache size configurable through:
- A constructor parameter with a default value
- A configuration option
- Runtime adjustment methods
- explicit CQuorumSnapshotManager(CEvoDB& evoDb) : - m_evoDb(evoDb), quorumSnapshotCache(32) {} + explicit CQuorumSnapshotManager(CEvoDB& evoDb, size_t cache_size = 32) : + m_evoDb(evoDb), quorumSnapshotCache(cache_size) {}src/llmq/blockprocessor.cpp (1)
Line range hint
1-1: Well-structured integration of CQuorumSnapshotManagerThe integration of
CQuorumSnapshotManageracross the codebase follows a consistent pattern and maintains proper encapsulation. The changes preserve thread safety through consistent use of reference parameters and proper locking annotations.Consider documenting the responsibilities and lifecycle of
CQuorumSnapshotManagerin the codebase to help future maintainers understand its role in the quorum management system.src/llmq/utils.cpp (2)
112-115: Consider adding parameter validationThe function accepts critical parameters but lacks validation. Consider adding checks for null pointers and invalid parameters.
std::vector<CDeterministicMNCPtr> GetAllQuorumMembers(Consensus::LLMQType llmqType, CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman, gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex, bool reset_cache) { + // Validate parameters + if (!Params().HasLLMQ(llmqType)) { + LogPrint(BCLog::LLMQ, "GetAllQuorumMembers: Invalid LLMQ type %d\n", static_cast<int>(llmqType)); + return {}; + }
480-488: Consider adding detailed loggingThe function handles critical quorum data but could benefit from more detailed logging to aid debugging.
+ LogPrint(BCLog::LLMQ, "BuildQuorumSnapshot: Storing snapshot for quorum type %d at height %d\n", + static_cast<int>(llmqParams.type), pCycleQuorumBaseBlockIndex->nHeight); qsnapman.StoreSnapshotForBlock(llmqParams.type, pCycleQuorumBaseBlockIndex, quorumSnapshot);src/llmq/quorums.cpp (1)
369-371: Consider adding error handling for connection failuresWhile the integration is correct, the function could benefit from more robust error handling for connection failures.
if (utils::EnsureQuorumConnections(llmqParams, connman, m_dmnman, m_sporkman, m_qsnapman, m_dmnman.GetListAtChainTip(), quorum->m_quorum_base_block_index, myProTxHash, /* is_masternode = */ m_mn_activeman != nullptr)) { + LogPrint(BCLog::LLMQ, "Successfully ensured quorum connections for quorum %s\n", + quorum->qc->quorumHash.ToString()); + } else { + LogPrint(BCLog::LLMQ, "Failed to ensure quorum connections for quorum %s\n", + quorum->qc->quorumHash.ToString()); }src/validation.cpp (2)
725-734: Improved InstantSend conflict handlingThe refactoring improves error handling by using structured bindings to extract conflict details and adds notification for double spend attempts.
Consider adding debug logging before the GetTransaction call to help with troubleshooting:
+ LogPrint(BCLog::INSTANTSEND, "Checking transaction %s against locked TX %s\n", hash.ToString(), conflict_txid.ToString()); CTransactionRef txConflict = GetTransaction(/* block_index */ nullptr, &m_pool, conflict_txid, chainparams.GetConsensus(), hashBlock);
2460-2477: Enhanced InstantSend conflict resolutionThe code now properly handles the case where chainlocks can override conflicting InstantSend locks, with improved error handling and logging.
Consider adding metrics to track chainlock overrides:
+ ::g_stats_client->inc("instantsend.chainlock_overrides", 1.0f); LogPrint(BCLog::ALL, "ConnectBlock(DASH): chain-locked transaction %s overrides islock %s\n", tx->GetHash().ToString(), conflict_islock_hash.ToString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
src/coinjoin/client.cpp(6 hunks)src/coinjoin/client.h(6 hunks)src/coinjoin/coinjoin.cpp(4 hunks)src/coinjoin/coinjoin.h(3 hunks)src/coinjoin/context.cpp(1 hunks)src/coinjoin/context.h(2 hunks)src/coinjoin/server.cpp(3 hunks)src/coinjoin/server.h(3 hunks)src/dsnotificationinterface.cpp(1 hunks)src/evo/cbtx.cpp(4 hunks)src/evo/cbtx.h(2 hunks)src/evo/chainhelper.cpp(1 hunks)src/evo/chainhelper.h(2 hunks)src/evo/deterministicmns.cpp(5 hunks)src/evo/deterministicmns.h(2 hunks)src/evo/specialtxman.cpp(6 hunks)src/evo/specialtxman.h(3 hunks)src/init.cpp(3 hunks)src/llmq/blockprocessor.cpp(6 hunks)src/llmq/blockprocessor.h(3 hunks)src/llmq/chainlocks.cpp(6 hunks)src/llmq/chainlocks.h(3 hunks)src/llmq/commitment.cpp(4 hunks)src/llmq/commitment.h(3 hunks)src/llmq/context.cpp(3 hunks)src/llmq/context.h(2 hunks)src/llmq/debug.cpp(4 hunks)src/llmq/debug.h(3 hunks)src/llmq/dkgsession.cpp(4 hunks)src/llmq/dkgsession.h(3 hunks)src/llmq/dkgsessionhandler.cpp(4 hunks)src/llmq/dkgsessionhandler.h(3 hunks)src/llmq/dkgsessionmgr.cpp(5 hunks)src/llmq/dkgsessionmgr.h(3 hunks)src/llmq/instantsend.cpp(0 hunks)src/llmq/instantsend.h(0 hunks)src/llmq/quorums.cpp(4 hunks)src/llmq/quorums.h(3 hunks)src/llmq/snapshot.cpp(6 hunks)src/llmq/snapshot.h(1 hunks)src/llmq/utils.cpp(17 hunks)src/llmq/utils.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/chainstate.cpp(4 hunks)src/node/chainstate.h(0 hunks)src/node/miner.cpp(3 hunks)src/node/miner.h(2 hunks)src/rest.cpp(5 hunks)src/rpc/mempool.cpp(4 hunks)src/rpc/quorums.cpp(3 hunks)src/test/evo_deterministicmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/validation_chainstate_tests.cpp(1 hunks)src/test/validation_chainstatemanager_tests.cpp(5 hunks)src/test/validation_flush_tests.cpp(1 hunks)src/txmempool.cpp(2 hunks)src/txmempool.h(4 hunks)src/validation.cpp(7 hunks)src/validation.h(0 hunks)test/lint/lint-circular-dependencies.py(0 hunks)
💤 Files with no reviewable changes (5)
- src/llmq/instantsend.h
- test/lint/lint-circular-dependencies.py
- src/llmq/instantsend.cpp
- src/node/chainstate.h
- src/validation.h
🚧 Files skipped from review as they are similar to previous changes (38)
- src/node/miner.h
- src/coinjoin/server.h
- src/dsnotificationinterface.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/llmq/context.cpp
- src/llmq/context.h
- src/llmq/blockprocessor.h
- src/llmq/dkgsessionmgr.h
- src/llmq/chainlocks.cpp
- src/coinjoin/context.cpp
- src/llmq/snapshot.cpp
- src/test/validation_chainstate_tests.cpp
- src/coinjoin/context.h
- src/coinjoin/coinjoin.h
- src/evo/specialtxman.h
- src/rpc/quorums.cpp
- src/llmq/commitment.cpp
- src/llmq/quorums.h
- src/llmq/debug.h
- src/test/validation_chainstatemanager_tests.cpp
- src/llmq/dkgsessionhandler.h
- src/rpc/mempool.cpp
- src/evo/cbtx.h
- src/llmq/commitment.h
- src/node/chainstate.cpp
- src/test/validation_flush_tests.cpp
- src/net_processing.cpp
- src/llmq/dkgsessionmgr.cpp
- src/llmq/debug.cpp
- src/coinjoin/client.h
- src/coinjoin/server.cpp
- src/txmempool.h
- src/llmq/dkgsession.h
- src/coinjoin/coinjoin.cpp
- src/llmq/dkgsession.cpp
- src/evo/chainhelper.h
- src/test/util/setup_common.cpp
- src/evo/chainhelper.cpp
🧰 Additional context used
📓 Learnings (1)
src/llmq/quorums.cpp (1)
Learnt from: kwvg
PR: dashpay/dash#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.
🔇 Additional comments (42)
src/llmq/utils.h (1)
29-30: LGTM: Clean introduction ofCQuorumSnapshotManagerclassThe new class is appropriately placed in the
llmqnamespace.src/llmq/chainlocks.h (2)
34-34: LGTM: Introduction ofCInstantSendManagerforward declarationThe forward declaration is appropriately placed in the
llmqnamespace.
93-93: Verify the integration ofCInstantSendManagerwith chain locksThe modified methods now require
CInstantSendManager, indicating a tighter coupling between chain locks and instant send functionality. While this aligns with the PR objectives, we should verify:
- The const reference usage is appropriate for all cases
- The dependency flow between these components is unidirectional to avoid circular dependencies
Also applies to: 104-104, 109-109
✅ Verification successful
The const reference usage and dependency structure are appropriate
Based on the code analysis:
The const reference usage is correct as
CInstantSendManageris only used for reading state inStart(),UpdatedBlockTip(), andTrySignChainTip()methods, with no modifications to the manager.The dependency structure is well-designed:
CChainLocksHandlerhas a forward declaration ofCInstantSendManagerCInstantSendManagerholds a reference toCChainLocksHandlerbut only for coordination purposes- No circular header dependencies were found
- The interaction between components is properly managed through constructor injection
The integration follows good architectural practices with clear separation of concerns and appropriate const-correctness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential circular dependencies between chainlocks and instantsend rg -l "class.*ChainLocks.*;\s*class.*InstantSend|class.*InstantSend.*;\s*class.*ChainLocks"Length of output: 93
Script:
#!/bin/bash # Check for instantsend dependencies in chainlocks header and implementation rg -A 5 "CInstantSendManager" src/llmq/chainlocks.* # Check for chainlocks dependencies in instantsend files rg -A 5 "ChainLocks" src/llmq/instantsend.* # Find all files that include both chainlocks and instantsend headers rg -l "#include.*chainlocks\.h.*#include.*instantsend\.h|#include.*instantsend\.h.*#include.*chainlocks\.h" # Check class definitions and their dependencies ast-grep --pattern 'class $_ { $$$ CInstantSendManager $$$ $$$ }'Length of output: 6434
src/llmq/snapshot.h (1)
210-213: LGTM: Updated signature ofBuildQuorumRotationInfoThe function signature change aligns with the architectural changes, replacing direct quorum manager usage with the new snapshot manager.
src/evo/deterministicmns.h (3)
38-40: LGTM! Forward declaration is properly placed.The forward declaration of
CQuorumSnapshotManagerfollows the existing pattern and is correctly scoped within thellmqnamespace.
601-602: LGTM! Method signature update maintains thread safety.The addition of
qsnapmanparameter preserves the existing locking requirements and correctly uses reference semantics.
608-612: LGTM! Consistent parameter updates across related methods.The signature updates for both methods maintain consistency with ProcessBlock changes and preserve the existing locking requirements.
src/llmq/dkgsessionhandler.cpp (2)
30-30: LGTM! Constructor changes follow best practices.The addition of
qsnapmanparameter and member initialization follows proper initialization order and reference semantics.Also applies to: 40-40
553-557: LGTM! Connection management updates preserve existing behavior.The addition of
m_qsnapmanto connection management functions maintains the existing control flow while integrating the new snapshot manager functionality.src/llmq/blockprocessor.cpp (3)
29-35: LGTM! PreComputeQuorumMembers update is well-structured.The addition of
qsnapmanparameter and its integration withGetAllQuorumMembersmaintains the function's static nature and follows a logical parameter order.
47-52: LGTM! Constructor implementation matches declaration.The constructor properly initializes the new
m_qsnapmanmember while maintaining the existing initialization order.
139-139: LGTM! Commitment verification updates are consistent.The addition of
m_qsnapmanto commitment verification calls is consistent and preserves existing validation logic.Also applies to: 276-276
src/rest.cpp (1)
135-154: Well-structured context getter implementationThe new
GetLLMQContextfunction follows the established pattern of other context getters in the file, with proper error handling and descriptive messages.src/llmq/utils.cpp (2)
60-86: Clean forward declarations with consistent parameter namingThe forward declarations are well-structured with consistent parameter naming conventions.
236-239:⚠️ Potential issueImplement proper error handling for external triggers
The TODO comments indicate missing error handling for externally triggered scenarios. Based on the codebase patterns shown in previous reviews, proper exception handling should be implemented.
Replace the TODO comment and assertion with proper error handling:
- //TODO Check if it is triggered from outside (P2P, block validation). Throwing an exception is probably a wiser choice - //assert (!newQuarterMembers.empty()); + if (newQuarterMembers.empty()) { + throw std::runtime_error("Invalid quorum members: empty quarter members set"); + }Likely invalid or redundant comment.
src/llmq/quorums.cpp (1)
Line range hint
215-225: Proper integration of CQuorumSnapshotManagerThe constructor correctly integrates the snapshot manager. Based on the retrieved learning, we know that CQuorumManager is fully initialized by LLMQContext, addressing any initialization concerns.
src/txmempool.cpp (2)
468-473: LGTM! Initialization of managers looks good.The changes properly initialize both the deterministic masternode manager and instant send manager, with appropriate null checks in the assertion.
1584-1587: LGTM! Proper handling of locked transactions.The code correctly skips expiration for transactions that are locked by the instant send manager, which is essential for maintaining transaction finality.
🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 1585-15851585: Prefer prefix ++/-- operators for non-primitive types.
(postfixOperator)
src/evo/deterministicmns.cpp (4)
598-601: LGTM! Parameter addition looks good.The
qsnapmanparameter is properly added to the method signature with appropriate type and reference.
706-709: LGTM! Consistent parameter propagation.The
qsnapmanparameter is correctly propagated through the call chain.
1004-1007: LGTM! Parameter addition and usage looks good.The
qsnapmanparameter is properly added to HandleQuorumCommitment and used for quorum snapshot management.
932-932: LGTM! Proper parameter passing at call site.The
qsnapmanparameter is correctly passed to HandleQuorumCommitment.src/coinjoin/client.cpp (5)
Line range hint
169-178: LGTM! Constructor initialization looks good.The
m_ismanparameter is properly added to the constructor and initialized in the initializer list.
594-595: LGTM! Proper parameter passing for validation.The
m_ismanparameter is correctly passed to IsValidInOuts for transaction validation.
959-959: LGTM! Proper collateral validation.The
m_ismanparameter is correctly passed to IsCollateralValid for collateral validation.
1018-1018: LGTM! Proper constructor parameter passing.The
m_ismanparameter is correctly passed to the CCoinJoinClientSession constructor.
1921-1922: LGTM! Proper manager initialization.The
m_ismanparameter is correctly passed through to initialize the CCoinJoinClientManager.src/init.cpp (2)
2024-2025: Clean architectural change moving InstantSend manager access through LLMQ contextThe refactoring improves encapsulation by accessing the InstantSend manager through the LLMQ context rather than passing it directly. This aligns with the PR's goal of consolidating LLMQ-related functionality.
327-328: Appropriate adaptation of chainstate setup to new LLMQ architectureThe chainstate setup has been properly updated to work with the refactored LLMQ structure, maintaining functionality while aligning with the architectural changes.
src/validation.cpp (3)
2163-2166: Simplified chainlock conflict checkThe chainlock conflict check has been simplified using the helper method. The error logging provides clear context.
2502-2503: Optimized superblock validationThe code now skips superblock validation for blocks below the best chainlock height, which is a valid optimization.
Let's verify the chainlock height check is consistent:
✅ Verification successful
Chainlock height validation is consistently implemented
The code review is accurate. The chainlock height check in the superblock validation is consistent with other chainlock-related checks in the codebase. Key findings:
- The
GetBestChainLockHeight()is properly implemented inCChainstateHelper(src/evo/chainhelper.cpp) and delegates to the chainlock handler- The
HasChainLock()checks throughout the codebase follow a consistent pattern of comparing block height and hash- The optimization to skip superblock validation for blocks below chainlock height aligns with other chainlock-based optimizations in the codebase, such as:
- Skipping islock checks for chainlocked blocks in instantsend.cpp
- Early returns in various RPC methods when chainlocks are present
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other chainlock height comparisons rg "GetBestChainLockHeight|HasChainLock" -A 2Length of output: 5568
Line range hint
736-749: Special handling for InstantSend transactionsThe code correctly handles the special case where a transaction is waiting for InstantSend by removing conflicts from the mempool.
Let's verify that all InstantSend-related code paths are covered:
✅ Verification successful
Let me gather more information about the InstantSend conflict handling to ensure complete coverage.
Let me verify one more aspect of InstantSend handling to ensure complete coverage.
InstantSend transaction handling is properly implemented with comprehensive conflict resolution
The code correctly implements InstantSend transaction handling with proper conflict resolution across all relevant components:
In
validation.cpp, InstantSend transactions are handled by:
- Checking for conflicting ISLocks via
HasConflictingISLock- Removing conflicts from mempool via
removeConflictsandremoveProTxConflictswhen a transaction is waiting for InstantSend- Rejecting transactions that conflict with mempool entries when not in InstantSend mode
The implementation is backed by:
CInstantSendManagerwhich provides core InstantSend functionality throughIsWaitingForTxandGetConflictingLockCTxMemPoolwhich handles conflict removal throughremoveConflictsand specialized ProTx conflict handling- Proper synchronization with EXCLUSIVE_LOCKS_REQUIRED annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all InstantSend-related function calls to ensure consistent handling rg "IsInstantSendWaitingForTx|HasConflictingISLock" -A 5Length of output: 2980
Script:
#!/bin/bash # Search for related InstantSend conflict handling functions rg "removeConflicts|removeProTxConflicts|GetConflictTx" -A 3Length of output: 3452
Script:
#!/bin/bash # Search for IsWaitingForTx implementation and related code rg "IsWaitingForTx|GetConflictingLock" -A 3Length of output: 2821
src/evo/specialtxman.cpp (4)
22-26: LGTM: Function signature update aligns with architectural changesThe addition of
qsnapmanparameter is consistent with the PR's objective of moving quorum snapshot management toLLMQContext.
51-51: LGTM: Consistent parameter passingThe
qsnapmanparameter is correctly passed toCheckLLMQCommitment, maintaining consistency with the architectural changes.
176-176: LGTM: Consistent integration of snapshot managerThe
m_qsnapmanparameter is correctly integrated into theProcessBlockcall.
185-185: LGTM: Consistent parameter passing in merkle root verificationThe
m_qsnapmanparameter is correctly integrated into the merkle root verification process.src/evo/cbtx.cpp (3)
65-66: LGTM: Function signature update aligns with architectural changesThe addition of
qsnapmanparameter maintains consistency with the refactoring objectives.
90-90: LGTM: Consistent parameter passing in merkle root calculationThe
qsnapmanparameter is correctly integrated into the merkle root calculation process.
120-121: Verify the impact of parameter reorderingThe parameter order has been changed to move
statebeforedmnman. While this is valid, we should verify all call sites have been updated accordingly.✅ Verification successful
All call sites have been updated with the correct parameter order
The verification shows that all call sites of
CalcCbTxMerkleRootMNListare consistent with the new parameter order. The function is called in three locations:
src/test/util/setup_common.cppsrc/node/miner.cppsrc/evo/cbtx.cppAll calls pass the parameters in the correct order, with
statebeforedmnman. The function declaration in the header file (src/evo/cbtx.h) also matches the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all calls to CalcCbTxMerkleRootMNList to verify parameter order rg -A 2 "CalcCbTxMerkleRootMNList\("Length of output: 1623
src/node/miner.cpp (3)
39-39: LGTM: Required include for snapshot managementThe addition of
<llmq/snapshot.h>is necessary for theCQuorumSnapshotManagerfunctionality.
79-79: LGTM: Proper member initialization with safety checksThe
m_qsnapmanmember is correctly initialized with appropriate null checks usingAssert.
227-227: LGTM: Updated parameter order in merkle root calculationThe call to
CalcCbTxMerkleRootMNListhas been correctly updated to match the new parameter order.
| @@ -34,3 +37,24 @@ bool CChainstateHelper::HasChainLock(int nHeight, const uint256& blockHash) cons | |||
| } | |||
|
|
|||
| int32_t CChainstateHelper::GetBestChainLockHeight() const { return clhandler.GetBestChainLock().getHeight(); } | |||
|
|
|||
| /** Passthrough functions to CInstantSendManager */ | |||
| std::optional<std::pair</*islock_hash=*/uint256, /*txid=*/uint256>> CChainstateHelper::HasConflictingISLock( | |||
There was a problem hiding this comment.
nit: maybe rename it to CChainstateHelper::ConflictingISLockIfAny(...) ?
| void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) EXCLUSIVE_LOCKS_REQUIRED(!cs); | ||
| void BlockConnected(const std::shared_ptr<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs); | ||
| void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> pindexDisconnected) EXCLUSIVE_LOCKS_REQUIRED(!cs); | ||
| void CheckActiveState() EXCLUSIVE_LOCKS_REQUIRED(!cs); | ||
| void TrySignChainTip() EXCLUSIVE_LOCKS_REQUIRED(!cs); | ||
| void TrySignChainTip(const llmq::CInstantSendManager& isman) EXCLUSIVE_LOCKS_REQUIRED(!cs); |
There was a problem hiding this comment.
[nit + out-of-scope of this PR] - it seems as possible to make TrySignChainTip as private
| { | ||
| // Do not allow double-initialization | ||
| assert(m_dmnman == nullptr); | ||
| assert(m_dmnman == nullptr && m_isman == nullptr); |
There was a problem hiding this comment.
[can be addressed in follow-up PR]
2 asserts are better than 1 assert with 2 conditions.
In case if assert will happen in current implementation it is impossible to understand which exactly variable has been not nullptr
assert(m_dmnman == nullptr);
assert(m_isman == nullptr);| @@ -1580,7 +1581,7 @@ int CTxMemPool::Expire(std::chrono::seconds time) | |||
| setEntries toremove; | |||
| while (it != mapTx.get<entry_time>().end() && it->GetTime() < time) { | |||
| // locked txes do not expire until mined and have sufficient confirmations | |||
| if (llmq::quorumInstantSendManager->IsLocked(it->GetTx().GetHash())) { | |||
| if (m_isman && m_isman->IsLocked(it->GetTx().GetHash())) { | |||
There was a problem hiding this comment.
@PastaPastaPasta @UdjinM6 should we interrupt instead logic of "expiration"?
such as:
if (!m_isman) return 0;
if txmempool is initialized before LLMQContext and this clean-up process will be triggered before ConnectManager [can it actually happen?] we may expire some instant-send-locked transactions which is not good
There was a problem hiding this comment.
Good point 👍 There should be an assert(m_isman); outside of the loop instead I think.
|
|
||
| assert(isman == llmq::quorumInstantSendManager.get()); | ||
| llmq::quorumInstantSendManager->InterruptWorkerThread(); | ||
| sigman->InterruptWorkerThread(); |
There was a problem hiding this comment.
sigman thread starts before isman. It means to be stopped in "reversed" order. Firstly, isman, after that - sigman. Same for shareman - if the thread started the first (before sigman and isman) - it means to be stopped last.
This place requires either clarification why order is inversed, or fixing inerruption order.
isman->InterruptWorkerThread();
sigman->InterruptWorkerThread();
shareman->InterruptWorkerThread();
Additional Information
llmq::CChainLocksHandlerandllmq::CInstantSendManagerhad their usage trimmed by moving functions used in validation logic toCChainstateHelper, meaning thatCChainstateHelpernow implements logic of its own alongside initializingCMNPaymentsProcessorandCSpecialTxProcessor.CInstantSendLockPtr) to avoid tying it down to implementation-specific details but are otherwise passthrough.Breaking Changes
None expected.
Checklist