refactor: follow-up dash#6742, implement review suggestions, avoid redundant transaction queries #6770
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis set of changes refactors the InstantSend subsystem by introducing an abstract interface Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/instantsend/instantsend.cpp (1)
212-212: Address the TODO commentThe TODO comment suggests investigating if leaving the current implementation is acceptable. This should be resolved or tracked in an issue.
Would you like me to help investigate this TODO and potentially open an issue to track it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/dsnotificationinterface.cpp(1 hunks)src/instantsend/db.cpp(6 hunks)src/instantsend/db.h(3 hunks)src/instantsend/instantsend.cpp(23 hunks)src/instantsend/instantsend.h(6 hunks)src/instantsend/lock.cpp(2 hunks)src/instantsend/signing.cpp(15 hunks)src/instantsend/signing.h(3 hunks)test/lint/lint-circular-dependencies.py(0 hunks)
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/dsnotificationinterface.cppsrc/instantsend/lock.cppsrc/instantsend/instantsend.hsrc/instantsend/db.cppsrc/instantsend/signing.hsrc/instantsend/db.hsrc/instantsend/signing.cppsrc/instantsend/instantsend.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.758Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: knst
PR: dashpay/dash#6533
File: test/functional/feature_llmq_singlenode.py:98-106
Timestamp: 2025-01-22T08:33:31.405Z
Learning: When reviewing PRs, ensure that suggestions are directly related to the PR's primary objectives rather than general code improvements that could be addressed separately.
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
src/dsnotificationinterface.cpp (4)
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: kwvg
PR: #6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, IsVersionChangeValid() is only called by transaction types that update existing masternode entries (like ProUpServTx, ProUpRegTx, ProUpRevTx), not by ProRegTx which creates new entries. This means validation logic in IsVersionChangeValid() only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
src/instantsend/lock.cpp (2)
Learnt from: kwvg
PR: #6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/instantsend/instantsend.h (3)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/instantsend/db.cpp (6)
Learnt from: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
src/instantsend/signing.h (2)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
src/instantsend/db.h (4)
Learnt from: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/instantsend/signing.cpp (7)
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Learnt from: kwvg
PR: #6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The CQuorumManager is fully initialized by LLMQContext, addressing any concerns about the manager’s initialization sequence.
src/instantsend/instantsend.cpp (6)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
🪛 GitHub Actions: Clang Diff Format Check
src/instantsend/signing.h
[error] 69-78: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
[error] 104-110: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
src/instantsend/instantsend.cpp
[error] 368-377: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
[error] 706-715: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
💤 Files with no reviewable changes (1)
- test/lint/lint-circular-dependencies.py
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/dsnotificationinterface.cppsrc/instantsend/lock.cppsrc/instantsend/instantsend.hsrc/instantsend/db.cppsrc/instantsend/signing.hsrc/instantsend/db.hsrc/instantsend/signing.cppsrc/instantsend/instantsend.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.758Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: knst
PR: dashpay/dash#6533
File: test/functional/feature_llmq_singlenode.py:98-106
Timestamp: 2025-01-22T08:33:31.405Z
Learning: When reviewing PRs, ensure that suggestions are directly related to the PR's primary objectives rather than general code improvements that could be addressed separately.
Learnt from: kwvg
PR: dashpay/dash#6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
src/dsnotificationinterface.cpp (4)
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: kwvg
PR: #6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, IsVersionChangeValid() is only called by transaction types that update existing masternode entries (like ProUpServTx, ProUpRegTx, ProUpRevTx), not by ProRegTx which creates new entries. This means validation logic in IsVersionChangeValid() only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
src/instantsend/lock.cpp (2)
Learnt from: kwvg
PR: #6532
File: src/test/fuzz/netaddress.cpp:83-84
Timestamp: 2025-01-14T09:06:19.717Z
Learning: In fuzzer harness tests, CServiceHash can be used with both default constructor (CServiceHash()) and parameterized constructor (CServiceHash(salt_k0, salt_k1)) to test different variants. The usage pattern CServiceHash()(service) and CServiceHash(0, 0)(service) is valid and intentional in such tests.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/instantsend/instantsend.h (3)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/instantsend/db.cpp (6)
Learnt from: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
src/instantsend/signing.h (2)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
src/instantsend/db.h (4)
Learnt from: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.592Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/instantsend/signing.cpp (7)
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
Learnt from: kwvg
PR: #6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The CQuorumManager is fully initialized by LLMQContext, addressing any concerns about the manager’s initialization sequence.
src/instantsend/instantsend.cpp (6)
Learnt from: kwvg
PR: #6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the CInstantSendManager. Previously, this was handled globally; now it's handled as a class member in LLMQContext, but the concurrency control remains consistent.
Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: #6530
File: src/validation.cpp:360-362
Timestamp: 2025-01-14T08:37:16.955Z
Learning: The UpdateTransactionsFromBlock() method in txmempool.cpp takes parameters in the order: vHashUpdate, ancestor_size_limit, ancestor_count_limit. The size limit comes before the count limit.
Learnt from: knst
PR: #6691
File: src/test/llmq_params_tests.cpp:148-151
Timestamp: 2025-07-15T14:53:04.819Z
Learning: In the Dash Core LLMQ implementation, signingActiveQuorumCount is never 0 in the actual parameters defined in params.h, making division by zero scenarios unrealistic in the max_cycles() function.
🪛 GitHub Actions: Clang Diff Format Check
src/instantsend/signing.h
[error] 69-78: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
[error] 104-110: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
src/instantsend/instantsend.cpp
[error] 368-377: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
[error] 706-715: Clang format differences found. Code formatting does not match the required style. Please run clang-format to fix.
🔇 Additional comments (28)
src/dsnotificationinterface.cpp (1)
109-109: LGTM!The removal of the
m_peermanparameter from theTransactionAddedToMempoolcall is consistent with the interface changes inCInstantSendManager.src/instantsend/lock.cpp (4)
9-9: Good addition for hasher support.The inclusion of
util/hasher.his necessary for usingSaltedOutpointHasherin the unordered_set implementation.
12-12: Correct header update.Replacing
<set>with<unordered_set>aligns with the container type change.
14-14: Good use of constexpr for compile-time constant.Converting
ISLOCK_REQUESTID_PREFIXtoconstexpr std::string_viewenables compile-time evaluation and is consistent with similar changes throughout the codebase.
36-37: Efficient uniqueness check implementation.The switch from
std::settostd::unordered_set<COutPoint, SaltedOutpointHasher>provides O(1) average case performance for uniqueness checking, which is more efficient than the O(log n) performance ofstd::set.src/instantsend/db.h (2)
36-36: Good thread safety annotations.Adding
GUARDED_BY(cs_db)annotations to member variables improves code documentation and helps static analysis tools detect potential threading issues.Also applies to: 38-38
54-55: Efficient parameter passing for shared pointers.Changing from
InstantSendLockPtr(by value) toconst InstantSendLockPtr&avoids unnecessary reference counting operations on the shared_ptr, which is a good optimization for frequently called methods.Also applies to: 92-92
src/instantsend/instantsend.h (5)
13-13: Correct include ordering.Moving
util/threadinterrupt.hafterthreadsafety.hand addinginstantsend/signing.hmaintains proper include ordering and supports the new interface inheritance.Also applies to: 17-17
47-47: Good use of final and interface inheritance.Marking
CInstantSendManagerasfinaland inheriting frominstantsend::InstantSendSignerParentimplements the interface abstraction pattern correctly, preventing further inheritance while enabling polymorphic usage through the interface.
71-72: Clear comment formatting.The multi-line comment formatting improves readability.
120-120: Simplified method signatures.Removing the
PeerManager¶meter fromRemoveMempoolConflictsForLockandTransactionAddedToMempoolsimplifies the interface and aligns with the PR objective of avoiding redundant queries.Also applies to: 138-138
132-132: Proper override specifiers.Adding
overrideto virtual methods (IsLocked,GetConflictingLock,TryEmplacePendingLock,IsInstantSendEnabled) ensures compile-time verification of the interface implementation.Also applies to: 134-134, 156-156, 161-161
src/instantsend/signing.cpp (6)
30-30: Consistent use of constexpr.Converting
INPUTLOCK_REQUESTID_PREFIXtoconstexpr std::string_viewis consistent with the similar change inlock.cpp.
33-33: Proper interface abstraction.Changing the constructor parameter from
llmq::CInstantSendManager&toInstantSendSignerParent&correctly implements the dependency inversion principle, allowing for better testability and decoupling.
62-62: Fixed mutex naming typo.Correcting
cs_inputReqeststocs_inputRequeststhroughout the file fixes the typo and ensures consistency with the header file declaration.Also applies to: 86-86, 333-333
108-109: Good practice for unused variables.Prefixing the unused
hashBlockvariable with an underscore (_hashBlock) clearly indicates it's intentionally unused, following common C++ conventions.
91-91: Improved code formatting and readability.The formatting improvements for comments and method signatures enhance code readability without changing functionality.
Also applies to: 179-180, 217-218, 293-294
156-156: Consistent logging format improvements.The logging statement improvements provide better formatting and consistency throughout the file, making debug output more readable.
Also applies to: 203-204, 246-247, 265-265, 273-274, 316-317, 327-328, 334-335, 357-357, 376-377
src/instantsend/db.cpp (3)
12-18: Good use of constexpr std::string_view for compile-time constantsUsing
constexpr std::string_viewinstead ofstatic const std::stringimproves performance by avoiding runtime string construction and reduces memory usage.
54-70: Efficient parameter passing with const InstantSendLockPtr&The change to pass
InstantSendLockPtrby const reference avoids unnecessary reference counting overhead from copying shared pointers, which aligns with the PR's optimization goals.
386-388: Proper cache handling in lock removalGood practice to explicitly disable cache usage when retrieving the lock for removal to ensure we're working with the latest data from the database.
src/instantsend/signing.h (2)
28-37: Excellent interface abstraction with InstantSendSignerParentThe introduction of this abstract interface effectively decouples
InstantSendSignerfrom the concreteCInstantSendManagerimplementation, enabling better testability through mocking and adhering to dependency inversion principles.
53-53: Fixed typo in mutex nameGood catch on correcting the typo from
cs_inputReqeststocs_inputRequests.Also applies to: 60-60
src/instantsend/instantsend.cpp (5)
37-37: Good use of constexpr for compile-time constantConverting
INPUTLOCK_REQUESTID_PREFIXtoconstexpr std::string_viewensures compile-time evaluation and avoids runtime overhead.
154-167: Clean lambda encapsulation for time difference calculationThe lambda function nicely encapsulates the time difference calculation logic, making the code more readable and the scope of the lock clearer.
364-375: Optimized database checks for better performanceMoving the cheaper database checks (
GetInstantSendLockByTxidandGetInstantSendLockByInput) before more expensive operations aligns with the PR's goal of optimizing theProcessInstantSendLock()function for faster bail-out.
431-431: Simplified method signatures by removing PeerManager parameterThe removal of
PeerManager¶meter fromTransactionAddedToMempoolandRemoveMempoolConflictsForLockmethods simplifies the interface. This change appears safe as these methods don't need direct access to the peer manager.Also applies to: 462-462, 694-694
427-428: Verify the removal of redundant transaction queryAccording to the PR objectives, the peer request for a transaction known to the system in
RemoveMempoolConflictsForLock()has been dropped. However, I see thatAskPeersForTransactionis still called here when the transaction is not found. This seems correct - we only ask for the transaction when we don't have it locally.
| @@ -25,12 +25,23 @@ class CQuorumManager; | |||
| } // namespace llmq | |||
|
|
|||
| namespace instantsend { | |||
| class InstantSendSigner : public llmq::CRecoveredSigsListener | |||
| class InstantSendSignerParent | |||
There was a problem hiding this comment.
InstantSendSignerParent is meant to be used outside of instantsend:: but CInstantSendManager to be hidden inside; may be even rename CInstantSendManager to InstantSendImplementation.
please, rename InstantSendSignerParent to InstantSendInterface or InstantSendManager.
There was a problem hiding this comment.
InstantSendSignerParentis meant to be used outside ofinstantsend::butCInstantSendManagerto be hidden inside;
Didn't migrate CInstantSendManager to the instantsend namespace as decoupling of networking capabilities aren't finished yet.
There are no plans to hide CInstantSendManager, at least as far as bitcoin-chainstate is concerned. I think the name is fine as-is, we specifically do not want InstantSendSignerParent to be used by anything other than InstantSendSigner and the existing name conveys that intent correctly.
…k()` Co-Authored-By: Konstantin Akimov <knstqq@gmail.com>
Co-Authored-By: Konstantin Akimov <knstqq@gmail.com>
It's faster to check the InstantSend database than to search for a
transaction and the block that included it, so allow that faster bail
out. Additionally, we'll do some additional things:
* Assign the success of `GetTransaction` to `found_transaction` to mark
the two different processing paths taken when we know or don't know
the transaction associated with the ISLock
* Invert an if-else block to improve reading
* Push down the networking requests to the tail end of the function as
they are independent of the changes made by `ProcessInstantSendLock()`
* The `peerman` passed to `RemoveMempoolConflictsForLock()` will be
sorted by the next commit
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
`RemoveMempoolConflictsForLock` is called in two instances * `ProcessInstantSendLock` when we *have* a valid transaction (tx != nullptr) and want to purge conflicting transactions * `TransactionAddedToMempool` when we have received a transaction and found the corresponding lock (else condition to `islock == nullptr`), and want to purge conflicting transactions Neither case calls for fetching the transaction based on the islock hash since in both cases, we already have the transaction
Additional Information
Dependent on refactor: move InstantSend impl. to
src/instantsend/and split sources for islock, database and masternode logic out of manager source #6742This pull request aims to address reviewer concerns raised during dash#6742, the following changes were made with that in mind
hashBlockby prefixing with underscore, commentcs_inputReqests(nowcs_input_requests, comment), commentconstexprforstd::string_views, commentstd::unordered_set), comment. The construction of the unordered set is inspired by similar usage inpolicy/package.cpp(source).std::shared_ptrcopying (InstantSendLockPtris astd::shared_ptr<InstantSendLock>), commentInstantSendSignerParent. For rationale, see comment.Additionally, optimizations have been made to
ProcessInstantSendLock()The cheaper database checks have been moved earlier in the function to allow for faster bail-out. This is alongside consolidating network requests to the tail of the function and general cleanup for better clarity.
Dropping the peer request for a transaction we know about in
RemoveMempoolConflictsForLock()(introduced in dash#2898) asProcessInstantSendLock()calls it when we know about the transaction and seek to remove conflicts (source)The only other caller,
TransactionAddedToMempool()calls it when we have knowledge of the transaction and want to purge conflicts from the mempool (source).In both cases, we already know the good transaction. Additionally, tests introduced in dash#2898 still pass (source), indicating a lack of regression but this change may require additional scrutiny.
Breaking Changes
None expected.
Checklist