refactor: follow-up dash#6761, implement review suggestions, ChainLock and InstantSend refactoring #6815
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughTime handling in chainlock code is migrated from raw integer ticks to std::chrono durations and seconds-based timestamps; cleanup, mining-safety, and related timing checks are updated accordingly and a TX_CONFIRM_THRESHOLD constant is introduced. CLSIG request ID generation is moved to GenSigRequestId(height) and the prior string-constant approach is replaced by an internal constexpr. InstantSend input-lock IDs are centralized via templated GenInputLockRequestId with a string_view prefix. ChainLock and InstantSend signing logic gained additional gating/null-safety and some constructor dependency changes. net_processing removed the NodeId-based IsInvInFilter overload. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 4
🧹 Nitpick comments (1)
src/chainlock/chainlock.h (1)
61-63: Nit: Prefer consistent initialization syntax for pointer membersStyle-only: prefer “= nullptr” for pointer members to match the rest of the codebase and improve readability. Also likely satisfies clang-format’s expectations.
Apply this diff:
- const CBlockIndex* bestChainLockBlockIndex GUARDED_BY(cs){nullptr}; - const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs){nullptr}; + const CBlockIndex* bestChainLockBlockIndex GUARDED_BY(cs) = nullptr; + const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs) = nullptr;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
-
src/chainlock/chainlock.cpp(11 hunks) -
src/chainlock/chainlock.h(3 hunks) -
src/chainlock/clsig.cpp(2 hunks) -
src/chainlock/clsig.h(1 hunks) -
src/chainlock/signing.cpp(8 hunks) -
src/chainlock/signing.h(1 hunks) -
src/instantsend/instantsend.cpp(13 hunks) -
src/instantsend/instantsend.h(2 hunks) -
src/instantsend/lock.cpp(2 hunks) -
src/instantsend/lock.h(1 hunks) -
src/instantsend/signing.cpp(3 hunks) -
src/net_processing.cpp(9 hunks) -
src/net_processing.h(2 hunks) -
src/protocol.h(2 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- src/chainlock/chainlock.cpp
- src/instantsend/instantsend.h
- src/net_processing.cpp
- src/instantsend/instantsend.cpp
🧰 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/instantsend/lock.hsrc/chainlock/signing.hsrc/instantsend/lock.cppsrc/instantsend/signing.cppsrc/chainlock/clsig.hsrc/net_processing.hsrc/protocol.hsrc/chainlock/clsig.cppsrc/chainlock/signing.cppsrc/instantsend/instantsend.cppsrc/chainlock/chainlock.cppsrc/instantsend/instantsend.hsrc/net_processing.cppsrc/chainlock/chainlock.h
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
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.
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/chainlock/signing.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
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.
Applied to files:
src/chainlock/chainlock.h
🧬 Code Graph Analysis (9)
src/instantsend/lock.h (1)
src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/instantsend/signing.cpp (1)
src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/chainlock/clsig.h (1)
src/chainlock/clsig.cpp (2)
GenSigRequestId(27-30)GenSigRequestId(27-27)
src/net_processing.h (1)
src/net_processing.cpp (23)
RelayInv(2346-2357)RelayInv(2346-2346)RelayInv(2359-2366)RelayInv(2359-2359)inv(630-630)inv(631-631)inv(653-653)inv(659-659)inv(937-937)inv(2316-2316)PostProcessMessage(3500-3525)PostProcessMessage(3500-3500)ret(662-662)ret(663-663)node(614-614)node(615-615)node(640-640)node(767-767)node(864-868)node(879-879)node(890-890)node(901-901)node(909-909)
src/chainlock/signing.cpp (2)
src/chainlock/signing.h (1)
TX_CONFIRM_THRESHOLD(24-24)src/chainlock/clsig.cpp (2)
GenSigRequestId(27-30)GenSigRequestId(27-27)
src/instantsend/instantsend.cpp (2)
src/net_processing.cpp (16)
ret(662-662)ret(663-663)ProcessMessage(3527-5311)ProcessMessage(3527-3532)pfrom(616-616)pfrom(637-638)pfrom(731-733)pfrom(739-739)pfrom(747-747)pfrom(750-750)pfrom(752-752)pfrom(754-754)pfrom(847-847)pfrom(1029-1029)from(1032-1032)node_id(728-728)src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/chainlock/chainlock.cpp (4)
src/chainlock/chainlock.h (2)
cs(61-124)cs(62-121)src/chainlock/clsig.cpp (2)
GenSigRequestId(27-30)GenSigRequestId(27-27)src/instantsend/signing.cpp (1)
GetTransaction(21-22)src/chainlock/signing.h (1)
TX_CONFIRM_THRESHOLD(24-24)
src/instantsend/instantsend.h (1)
src/protocol.h (2)
MessageProcessingResult(622-624)MessageProcessingResult(625-628)
src/chainlock/chainlock.h (5)
src/llmq/signing.h (1)
lastCleanupTime(173-173)src/chainlock/chainlock.cpp (20)
AlreadyHave(95-99)AlreadyHave(95-95)GetChainLockByHash(101-112)GetChainLockByHash(101-101)GetBestChainLock(114-118)GetBestChainLock(114-114)UpdateTxFirstSeenMap(120-127)UpdateTxFirstSeenMap(120-120)ProcessNewChainLock(129-192)ProcessNewChainLock(129-130)AcceptedBlockHeader(194-215)AcceptedBlockHeader(194-194)UpdatedBlockTip(217-236)UpdatedBlockTip(217-217)CheckActiveState(238-252)CheckActiveState(238-238)EnforceBestChainLock(318-375)EnforceBestChainLock(318-318)Cleanup(437-490)Cleanup(437-437)src/chainlock/clsig.h (2)
chainlock(15-38)nodiscard(29-31)src/validationinterface.h (2)
chainlock(28-30)llmq(37-39)src/chainlock/signing.h (1)
llmq(14-20)
🪛 GitHub Actions: Clang Diff Format Check
src/chainlock/chainlock.cpp
[error] 165-171: Clang-format differences found in src/chainlock/chainlock.cpp. Run './contrib/devtools/clang-format-diff.py -p1' (or clang-format) to fix formatting. Failing command: git diff -U0 origin/develop --
src/chainlock/chainlock.h
[error] 76-123: Clang-format differences found in src/chainlock/chainlock.h. Run 'clang-format' to fix formatting. Failing command: git diff -U0 origin/develop --
⏰ 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). (1)
- GitHub Check: linux64_multiprocess-test / Test source
🔇 Additional comments (18)
src/chainlock/clsig.h (1)
39-41: LGTM! Clean API refactor for centralized CLSIG request ID generation.This change properly encapsulates the CLSIG request ID generation by removing the exported constant and replacing it with a centralized function. The approach aligns well with the similar refactoring done for InstantSend input locks.
src/chainlock/signing.h (1)
23-24: LGTM! Well-documented compile-time constant for transaction confirmation threshold.The addition of
TX_CONFIRM_THRESHOLDwith clear documentation improves code maintainability by replacing magic numbers throughout the codebase. The value of 5 blocks is appropriate for the safety window.src/instantsend/lock.h (1)
43-44: LGTM! Template design enables centralized input lock ID generation.The template declaration provides a clean, type-safe interface for generating input lock request IDs. The generic design allows for future extensibility while maintaining type safety through explicit instantiations in the implementation file.
src/instantsend/lock.cpp (2)
14-14: LGTM! Consistent prefix naming convention.The
INPUTLOCK_REQUESTID_PREFIXfollows the same naming pattern established byISLOCK_REQUESTID_PREFIXand uses the modernstd::string_viewtype for efficiency.
41-47: LGTM! Proper template implementation with explicit instantiations.The template implementation correctly uses
SerializeHashwith the prefix-value pair, maintaining consistency with the existing ISLOCK approach. The explicit instantiations forCOutPointandCTxInensure proper linkage while preventing template bloat.src/instantsend/signing.cpp (3)
114-114: LGTM! Centralized ID generation usage.The code correctly uses the new
GenInputLockRequestIdfunction instead of direct serialization, improving maintainability and consistency across the codebase.
298-298: LGTM! Consistent ID generation in signing flow.The usage of
GenInputLockRequestId(in)aligns with the centralized approach and correctly generates IDs for the entireCTxInobject rather than just theprevout.
347-347: LGTM! Proper ID generation for InstantSend lock creation.The code consistently uses
GenInputLockRequestId(in)when checking for recovered signatures, maintaining the same ID generation approach used throughout the signing process.src/protocol.h (1)
20-20: LGTM: variant-based filter and richer MessageProcessingResult payload
- include is correct.
- m_is_masternode, m_inv_filter, and m_request_tx additions align with the new PostProcessMessage flow.
- The constructor with the is_masternode flag is fine and preserves default initialization.
Also applies to: 601-614, 621-629
src/chainlock/chainlock.h (2)
64-67: Chrono migration: verify callers supply seconds (or convert appropriately)txFirstSeenTime and seenChainLocks now store std::chrono::seconds. Ensure all write sites (e.g., UpdateTxFirstSeenMap, ProcessNewChainLock cleanup timers) pass seconds or values convertible to seconds. Header still declares UpdateTxFirstSeenMap taking int64_t; that’s acceptable if the implementation constructs seconds from it, but consistency with seconds at the API boundary would be cleaner in a follow-up.
Would you like me to generate a follow-up patch to change UpdateTxFirstSeenMap to accept std::chrono::seconds across the interface (if it doesn’t break overrides)?
76-123: No clang-format issues detected inchainlock.h
Running clang-format onsrc/chainlock/chainlock.hproduced no changes—no formatting fixes are needed.src/net_processing.h (1)
95-97: LGTM: const-correct RelayInv overloadsSwitching RelayInv to take const CInv& is correct and aligns with the implementations in net_processing.cpp.
src/chainlock/signing.cpp (6)
18-27: LGTM: constructor now stores managers (shareman/spork/mn_sync)Wiring m_shareman, m_sporkman, and m_mn_sync via the ctor is correct and aligns with the broader refactor.
59-61: LGTM: additional null-check for pindexGuarding against null pindex and missing pprev prevents deref and aligns with prior review feedback about null safety.
98-103: LGTM: TX_CONFIRM_THRESHOLD applied; message aligned with logicThe loop now cleanly limits checks to tip + previous TX_CONFIRM_THRESHOLD blocks; the log message reflects the constant.
129-144: LGTM: centralized CLSIG request ID generationSwitching to GenSigRequestId(pindex->nHeight) removes duplication and enforces a single request ID scheme.
195-198: LGTM: GetBlockTxs null-check for LookupBlockIndexReturning early if pindex is missing addresses the null lookup issue noted in prior reviews.
251-263: LGTM: Cleanup handles missing pindex and conflicting/best CLSIGsErasing entries when the index is gone and pruning on ChainLock state prevents stale growth and matches the concurrency/safety expectations.
|
Portions of this PR to be spun off, marking as draft. |
kwvg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/chainlock/clsig.cpp (1)
9-10: Good fix: include <string_view> addedResolves prior compile fragility from relying on indirect includes.
🧹 Nitpick comments (4)
src/chainlock/clsig.cpp (1)
29-32: GenSigRequestId implementation looks correct; minor signature nitSerializeHash over the (prefix, height) pair provides clear domain separation and is consistent with our string_view serialization support. Two small nits:
- Prefer pass-by-value without const for primitives to match local style (e.g., constructor takes int32_t).
- Consider marking the function [[nodiscard]] in the header so accidental discards are flagged.
Apply this minimal diff locally to align the signature style:
-uint256 GenSigRequestId(const int32_t nHeight) +uint256 GenSigRequestId(int32_t nHeight)And in the header (not shown here), consider:
[[nodiscard]] uint256 GenSigRequestId(int32_t nHeight);src/chainlock/signing.cpp (3)
98-102: Threshold window logic LGTM; nit: cast constant to match printf formatSemantics are correct (tip + previous TX_CONFIRM_THRESHOLD blocks). Minor type-safety nit to avoid potential format warnings on some platforms.
Apply this diff:
- LogPrint(BCLog::CHAINLOCKS, "%s -- tip and previous %d blocks all safe\n", __func__, TX_CONFIRM_THRESHOLD); + LogPrint(BCLog::CHAINLOCKS, "%s -- tip and previous %d blocks all safe\n", + __func__, static_cast<int>(TX_CONFIRM_THRESHOLD));
118-121: Minor style cleanup: remove inline “/ Continued /”The comment is unnecessary in the LogPrint call and can be removed to reduce noise.
Apply this diff:
- LogPrint(BCLog::CHAINLOCKS, /* Continued */ - "%s -- not signing block %s due to TX %s not being islocked and not old enough.\n", + LogPrint(BCLog::CHAINLOCKS, + "%s -- not signing block %s due to TX %s not being islocked and not old enough.\n", __func__, pindexWalk->GetBlockHash().ToString(), txid.ToString());
195-198: Good null-check; consider reducing cs_main hold time around disk I/OEarly return on missing index avoids a null deref. Optionally, capture pindex (and any needed data like nTime/pos) under cs_main, release cs_main, then perform ReadBlockFromDisk and set-building outside the lock to reduce contention on cs_main in I/O-heavy paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/chainlock/chainlock.cpp(11 hunks)src/chainlock/chainlock.h(3 hunks)src/chainlock/clsig.cpp(2 hunks)src/chainlock/clsig.h(1 hunks)src/chainlock/signing.cpp(8 hunks)src/chainlock/signing.h(1 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/lock.cpp(2 hunks)src/instantsend/lock.h(1 hunks)src/instantsend/signing.cpp(3 hunks)src/net_processing.cpp(0 hunks)src/net_processing.h(0 hunks)
💤 Files with no reviewable changes (2)
- src/net_processing.h
- src/net_processing.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- src/instantsend/lock.h
- src/chainlock/signing.h
- src/chainlock/clsig.h
- src/instantsend/instantsend.cpp
- src/instantsend/signing.cpp
- src/chainlock/chainlock.cpp
- src/chainlock/chainlock.h
🧰 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/instantsend/lock.cppsrc/chainlock/clsig.cppsrc/chainlock/signing.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
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.
📚 Learning: 2025-08-13T16:10:51.336Z
Learnt from: kwvg
PR: dashpay/dash#6815
File: src/chainlock/clsig.cpp:27-30
Timestamp: 2025-08-13T16:10:51.336Z
Learning: The Dash codebase has explicit serialization support for std::string_view through template specializations in src/serialize.h (lines 976-987), allowing SerializeHash and other serialization functions to work directly with string_view without conversion to std::string.
Applied to files:
src/chainlock/clsig.cpp
📚 Learning: 2025-05-10T00:54:30.887Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.887Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
Applied to files:
src/chainlock/clsig.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/chainlock/clsig.cppsrc/chainlock/signing.cpp
🧬 Code Graph Analysis (1)
src/chainlock/signing.cpp (2)
src/chainlock/signing.h (1)
TX_CONFIRM_THRESHOLD(24-24)src/chainlock/clsig.cpp (2)
GenSigRequestId(29-32)GenSigRequestId(29-29)
🔇 Additional comments (7)
src/chainlock/clsig.cpp (1)
12-12: Solid choice: internalize the CLSIG prefix as static constexpr string_viewInternal linkage avoids ODR/exported symbol issues; using string_view is fine here since Dash’s serializer supports it.
src/chainlock/signing.cpp (5)
18-20: Constructor wiring looks correct; confirm referenced lifetimesStoring references to shareman/sporkman/mn_sync is fine; just confirm their lifetimes are guaranteed to outlive ChainLockSigner instances to avoid dangling refs.
59-60: Good: added null/genesis guard for pindexPrevents dereferencing a null index and avoids attempting to sign the genesis tip.
88-90: Logging improvement is clear and actionableThe added context (hash and height) is useful for diagnosing signing attempts.
129-129: Good: centralizes request-id generation with GenSigRequestIdLeaning on the new API removes duplication and keeps CLSIG request-id semantics in one place.
251-255: Cleanup: missing-index guard added — addresses prior NPE riskThis directly addresses the previously acknowledged null-check gap in Cleanup (per earlier review on #6761). Thanks for closing the loop.
src/instantsend/lock.cpp (1)
14-15: Confirmed: request-id prefixes are "inlock" / "islock" and are used consistently — no functional change required
- Definitions (values confirmed):
- src/instantsend/lock.cpp: lines 14-15
- static constexpr std::string_view INPUTLOCK_REQUESTID_PREFIX{"inlock"};
- static constexpr std::string_view ISLOCK_REQUESTID_PREFIX{"islock"};
- Usage (serialization confirmed in code + tests):
- src/instantsend/lock.cpp: InstantSendLock::GetRequestId() — hw << ISLOCK_REQUESTID_PREFIX;
- src/instantsend/lock.cpp: GenInputLockRequestId(...) — ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
- src/test/evo_islock_tests.cpp: test computes expected hash with hw << std::string_view("islock") (verifies serialization)
- Namespace/scope note:
- The constants are declared static at file scope (internal linkage), so they do not pollute the global symbol namespace. Moving them into the instantsend namespace would be purely stylistic/readability — not required for correctness.
Outcome: no changes required; the prefixes and their serialization are consistent with existing code and tests.
| #include <util/hasher.h> | ||
|
|
||
| #include <string> | ||
| #include <string_view> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add missing for std::make_pair and confirm string_view serialization support
- std::make_pair is used on Line 44 but isn’t included here; relying on transitive includes is brittle.
- Also, since we now hash std::string_view, please confirm the serialization framework supports std::string_view for both CHashWriter << and SerializeHash with std::pair<string_view, T>. If not, we’ll need to convert to a span or a string at the call site.
Apply this small include fix:
#include <util/hasher.h>
-#include <string_view>
+#include <string_view>
+#include <utility>
#include <unordered_set>Run this script to verify serializer/operator coverage for std::string_view:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for serializer or operator<< support for std::string_view..."
rg -n -C2 -P 'Serialize\([^,]+,\s*std::string_view\b' src || true
rg -n -C2 -P 'operator<<\s*\(.*CHashWriter[^)]*,\s*std::string_view\b' src || true
echo "Other usages of CHashWriter << std::string_view (to gauge support footprint):"
rg -n -C2 -P '<<\s*std::string_view\b' src || trueLength of output: 811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/instantsend/lock.cpp (1-120) ==="
nl -ba src/instantsend/lock.cpp | sed -n '1,120p' || true
echo
echo "=== grep for make_pair / std::make_pair ==="
rg -n --hidden --no-ignore-vcs '\b(std::make_pair|make_pair)\b' || true
echo
echo "=== src/serialize.h (940-1000) ==="
nl -ba src/serialize.h | sed -n '940,1000p' || true
echo
echo "=== occurrences of string_view in src ==="
rg -n --hidden --no-ignore-vcs --type-add 'cc:*.h' -S 'std::string_view' src || true
echo
echo "=== src/test/evo_islock_tests.cpp (1-60) ==="
nl -ba src/test/evo_islock_tests.cpp | sed -n '1,60p' || trueLength of output: 54734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/instantsend/lock.cpp (1-160) ==="
sed -n '1,160p' src/instantsend/lock.cpp || true
echo
echo "=== Lines 36-52 in src/instantsend/lock.cpp ==="
sed -n '36,52p' src/instantsend/lock.cpp || true
echo
echo "=== #includes in src/instantsend/lock.cpp ==="
rg -n '^#include' src/instantsend/lock.cpp || true
echo
echo "=== Is <utility> included anywhere in repo? ==="
rg -n '^#include\s*<utility>' src || true
echo
echo "=== serialize.h (940-1000) ==="
sed -n '940,1000p' src/serialize.h || true
echo
echo "=== Lines in src/serialize.h that mention string_view ==="
rg -n 'string_view' src/serialize.h || true
echo
echo "=== Any operator<< overloads or Serialize specializations involving std::string_view or CHashWriter? ==="
rg -n -C2 -P 'operator<<\s*\(.*CHashWriter|CHashWriter\s*&\s*operator<<|Serialize\([^,]+,\s*std::string_view' src || true
echo
echo "=== Any usages of CHashWriter << std::string_view in repo (context) ==="
rg -n -C3 -P 'CHashWriter[^;]*<<.*std::string_view|<<\s*std::string_view' src || trueLength of output: 11591
Add missing to src/instantsend/lock.cpp; string_view serialization confirmed
- Issue: src/instantsend/lock.cpp uses std::make_pair in GenInputLockRequestId but does not include . Add the include to avoid relying on transitive includes.
- Serialization check: src/serialize.h contains Serialize/Unserialize overloads for std::basic_string_view (around lines ~973–984), and CHashWriter::operator<< delegates to ::Serialize (hash.h), so hashing/SerializeHash of std::string_view is supported. Tests exercise this (src/test/evo_islock_tests.cpp: hw << std::string_view("islock")).
Apply this change:
#include <util/hasher.h>
#include <string_view>
+#include <utility>
#include <unordered_set>File to edit:
- src/instantsend/lock.cpp — add #include near the other standard includes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <string_view> | |
| #include <util/hasher.h> | |
| #include <string_view> | |
| #include <utility> | |
| #include <unordered_set> |
🤖 Prompt for AI Agents
In src/instantsend/lock.cpp around line 11, the code uses std::make_pair in
GenInputLockRequestId but does not include the <utility> header; add the line
#include <utility> alongside the other standard includes near the top of the
file to avoid relying on transitive includes and ensure the build is robust (no
other code changes required; string_view serialization is already supported).
| template <typename T> | ||
| uint256 GenInputLockRequestId(const T& val) | ||
| { | ||
| return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val)); | ||
| } | ||
| template uint256 GenInputLockRequestId(const COutPoint& val); | ||
| template uint256 GenInputLockRequestId(const CTxIn& val); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Template defined in .cpp: ensure extern templates exist in the header and verify no unintended instantiations occur
Defining the function template in a .cpp and providing explicit instantiations is fine, but only if:
- The header (instantsend/lock.h) declares the template and provides extern template declarations for the explicitly instantiated types (COutPoint, CTxIn), preventing implicit instantiation in other TUs.
- All call sites only use the explicitly-instantiated types. Any new T will fail to link/compile.
Optionally, consider replacing the template with two overloads for the supported types to make the constraint explicit and avoid the extern template pattern.
Proposed optional refactor within this hunk (keeps the implementation local and avoids template machinery):
-template <typename T>
-uint256 GenInputLockRequestId(const T& val)
-{
- return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
-}
-template uint256 GenInputLockRequestId(const COutPoint& val);
-template uint256 GenInputLockRequestId(const CTxIn& val);
+uint256 GenInputLockRequestId(const COutPoint& val)
+{
+ return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
+}
+
+uint256 GenInputLockRequestId(const CTxIn& val)
+{
+ return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
+}Otherwise, please ensure the header has the matching extern template declarations:
// in src/instantsend/lock.h
namespace instantsend {
template <typename T> uint256 GenInputLockRequestId(const T& val);
extern template uint256 GenInputLockRequestId(const COutPoint& val);
extern template uint256 GenInputLockRequestId(const CTxIn& val);
} // namespace instantsendAnd run these checks:
Also, please confirm hash equivalence with the previous implementation (if it was hw << prefix << val). SerializeHash(std::pair(prefix, val)) should produce identical bytes, but a quick cross-check would be prudent given consensus sensitivity.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Ensure extern templates exist in the header:"
rg -n -C2 -P 'extern\s+template\s+uint256\s+GenInputLockRequestId\s*\(' src/instantsend/lock.h || true
echo "Verify only supported types are used at call sites:"
rg -n -C3 -P '\bGenInputLockRequestId\s*\(' src | sed -E 's/^[^:]+://' || trueLength of output: 2569
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/instantsend/lock.h ==="
sed -n '1,240p' src/instantsend/lock.h || true
echo
echo "=== src/instantsend/lock.cpp ==="
sed -n '1,240p' src/instantsend/lock.cpp || true
echo
echo "=== src/instantsend/instantsend.cpp ==="
sed -n '1,240p' src/instantsend/instantsend.cpp || true
echo
echo "=== src/instantsend/signing.cpp ==="
sed -n '1,420p' src/instantsend/signing.cpp || trueLength of output: 26216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rg for string_view occurrences ==="
rg -n --hidden -S "string_view" || true
echo
echo "=== rg for SerializeHash usages/definition ==="
rg -n --hidden -S "SerializeHash\(" || true
rg -n --hidden -S "uint256\s+SerializeHash" || true
echo
echo "=== show src/serialize.h (if present) ==="
sed -n '1,240p' src/serialize.h || true
echo
echo "=== show src/hash.h and src/util/hasher.h ==="
sed -n '1,240p' src/hash.h || true
echo
sed -n '1,240p' src/util/hasher.h || true
echo
echo "=== search for serialization of std::string/std::string_view or pair<> ==="
rg -n --hidden -S "template.*Serialize|SERIALIZE_METHODS|std::pair|pair<" || trueLength of output: 114408
Action required: add extern-template declarations for GenInputLockRequestId (or replace with explicit overloads)
Short summary: I verified that src/instantsend/lock.h only declares the template, src/instantsend/lock.cpp defines it and explicitly instantiates for COutPoint and CTxIn, and there are multiple call sites using those two types (e.g. src/instantsend/instantsend.cpp, src/instantsend/signing.cpp). The header currently lacks extern template declarations — add them (or switch to explicit overloads) to avoid unintended implicit instantiations across translation units.
Files to update:
- src/instantsend/lock.h — add forward decl for CTxIn (or include primitives/transaction.h) and add extern template declarations for the two instantiations.
- src/instantsend/lock.cpp — already defines the template and contains explicit instantiations; keep as-is if you add extern templates. Alternatively, replace the template with two overloads (implementation-local).
Suggested edits:
Add extern templates to the header (preferred minimal change)
+// forward-declare if not including primitives/transaction.h
+class CTxIn;
class COutPoint;
namespace instantsend {
template <typename T>
uint256 GenInputLockRequestId(const T& val);
+extern template uint256 GenInputLockRequestId(const COutPoint& val);
+extern template uint256 GenInputLockRequestId(const CTxIn& val);
...
}Optional: replace the template with explicit overloads (keeps implementation local)
-template <typename T>
-uint256 GenInputLockRequestId(const T& val)
-{
- return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
-}
-template uint256 GenInputLockRequestId(const COutPoint& val);
-template uint256 GenInputLockRequestId(const CTxIn& val);
+uint256 GenInputLockRequestId(const COutPoint& val)
+{
+ return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
+}
+
+uint256 GenInputLockRequestId(const CTxIn& val)
+{
+ return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val));
+}Note: this code uses SerializeHash(std::make_pair(prefix, val)) (consistent with other places in the repo). Given consensus sensitivity, double-check that this produces the same bytes as the previous hw << prefix << val approach (serialize.h provides string_view serialization and similar pair-based patterns are used elsewhere, e.g. chainlock/clsig).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <typename T> | |
| uint256 GenInputLockRequestId(const T& val) | |
| { | |
| return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val)); | |
| } | |
| template uint256 GenInputLockRequestId(const COutPoint& val); | |
| template uint256 GenInputLockRequestId(const CTxIn& val); | |
| template <typename T> | |
| uint256 GenInputLockRequestId(const T& val) | |
| { | |
| return ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, val)); | |
| } | |
| template uint256 GenInputLockRequestId(const COutPoint& val); | |
| template uint256 GenInputLockRequestId(const CTxIn& val); |
🤖 Prompt for AI Agents
In src/instantsend/lock.h (around where the template is declared, corresponding
to src/instantsend/lock.cpp lines 41-47), add forward declaration or include for
CTxIn (e.g. include "primitives/transaction.h" or forward-declare CTxIn) and add
extern template declarations for the two explicit instantiations: extern
template uint256 GenInputLockRequestId(const COutPoint&); and extern template
uint256 GenInputLockRequestId(const CTxIn&); keep the definition and explicit
instantiations in src/instantsend/lock.cpp unchanged; alternatively, if you
prefer to avoid extern templates, replace the template with two explicit
overloads implemented in lock.cpp to keep instantiation local.
dash#6425 moved AskNodesForLockedTx to PeerManager as AskPeersForTransaction, which removed the only usage of IsInvInFilter. We can safely remove it from the PeerManager interface as its usage is now purely internal.
There was a problem hiding this comment.
Actionable comments posted: 0> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limits.
🔭 Outside diff range comments (1)
src/chainlock/signing.cpp (1)
44-55: Spork gating is inconsistent with AreChainLocksEnabled/IsEnabled; remove or unifyTrySignChainTip first checks
!m_clhandler.IsEnabled()(which already derives fromAreChainLocksEnabled(spork_manager)), then additionally checksm_sporkman.GetSporkValue(SPORK_19_CHAINLOCKS_ENABLED) != 0and returns with a comment “not enabled.” This duplicates and can contradict the canonical gating logic and may suppress signing when ChainLocks are actually enabled.Recommend relying solely on
m_clhandler.IsEnabled()or usingsporkman.IsSporkActive(SPORK_19_CHAINLOCKS_ENABLED)for consistency with the rest of the codebase.Apply this diff to remove the redundant and potentially inverted check:
- if (m_sporkman.GetSporkValue(SPORK_19_CHAINLOCKS_ENABLED) != 0) { - // ChainLocks signing not enabled - return; - }
♻️ Duplicate comments (2)
src/chainlock/signing.cpp (1)
251-263: Cleanup() null-check fix implemented as discussed in PR #6761The added
pindexnull-check and safe erasure path resolve the follow-up noted previously. Looks good.src/chainlock/chainlock.cpp (1)
163-177: Race reduction: update “best” and “known block” under the same lockBringing the
pindexcheck and updates forbestChainLockWithKnownBlockandbestChainLockBlockIndexinto the same critical section addresses prior concerns about temporary inconsistency. Good change.
🧹 Nitpick comments (6)
src/chainlock/signing.cpp (1)
118-121: Early exit on unsafe TXs is correct; nit: consolidate the continued logBehavior is correct. Optional: fold the multi-line LogPrint format arguments into a single call for readability.
src/chainlock/chainlock.cpp (2)
442-446: Cleanup interval now uses durations with atomic secondsUsing
lastCleanupTime.load()and assigning asecondsvalue is correct. Minor nit: consider.store(...)for symmetry, though assignment works for atomics.
468-470: Lock ordering calloutLOCK(::cs_main) followed by LOCK2(mempool.cs, cs) matches common ordering. Keep this consistent elsewhere to avoid deadlocks.
Would you like me to scan the repo to verify no inverse ordering exists for these three locks?
src/instantsend/lock.h (1)
16-16: Make instantiation intent explicit (add externs) and forward-declare CTxInYou’re declaring the template in the header and (per lock.cpp) providing explicit instantiations for COutPoint and CTxIn in the .cpp. To avoid accidental implicit instantiation in other TUs and to document the supported specializations, add extern template declarations in the header. Also forward-declare CTxIn here to keep the header self-contained.
Apply this diff:
class COutPoint; +class CTxIn; namespace instantsend { @@ template <typename T> uint256 GenInputLockRequestId(const T& val); + +// Explicitly instantiated in lock.cpp +extern template uint256 GenInputLockRequestId(const COutPoint& val); +extern template uint256 GenInputLockRequestId(const CTxIn& val);If you intend to allow more types in the future, consider moving the (small) template definition into the header instead and dropping explicit instantiations.
Also applies to: 43-45
src/instantsend/signing.cpp (2)
302-303: Use the llmqType parameter consistentlyInside TrySignInputLocks you accept llmqType as a parameter but call GetVoteForId with params.llmqTypeDIP0024InstantSend. Prefer the passed-in llmqType for consistency and testability.
Apply this diff:
- if (m_sigman.GetVoteForId(params.llmqTypeDIP0024InstantSend, id, otherTxHash)) { + if (m_sigman.GetVoteForId(llmqType, id, otherTxHash)) {
114-118: Standardize Input-Lock ID Basis to COutPointWe’ve confirmed that in
src/instantsend/signing.cpptwo ID-generation sites still use the fullCTxIn, while recovery lookup (and logging) usesin.prevout. To avoid mismatches, update both to use only the outpoint:• In TrySignInputLocks (around line 298):
- auto id = GenInputLockRequestId(in); + auto id = GenInputLockRequestId(in.prevout);• In TrySignInstantSendLock (around line 347):
- auto id = GenInputLockRequestId(in); + auto id = GenInputLockRequestId(in.prevout);Optionally, review other callers (e.g. the
GetIdsFromLockable(tx.vin)template ininstantsend.cpp) to ensure they, too, generate request IDs fromCOutPointif they must match recovered-sig IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
-
src/chainlock/chainlock.cpp(11 hunks) -
src/chainlock/signing.cpp(8 hunks) -
src/chainlock/signing.h(1 hunks) -
src/instantsend/instantsend.cpp(1 hunks) -
src/instantsend/lock.cpp(2 hunks) -
src/instantsend/lock.h(1 hunks) -
src/instantsend/signing.cpp(3 hunks) -
src/net_processing.cpp(0 hunks) -
src/net_processing.h(0 hunks)
💤 Files with no reviewable changes (2)
- src/net_processing.h
- src/net_processing.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/instantsend/instantsend.cpp
- src/chainlock/signing.h
- src/instantsend/lock.cpp
🧰 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/instantsend/lock.hsrc/chainlock/signing.cppsrc/instantsend/signing.cppsrc/chainlock/chainlock.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
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.
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/chainlock/signing.cppsrc/chainlock/chainlock.cpp
📚 Learning: 2025-02-06T14:34:30.466Z
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.
Applied to files:
src/chainlock/chainlock.cpp
🧬 Code Graph Analysis (4)
src/instantsend/lock.h (1)
src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/chainlock/signing.cpp (2)
src/chainlock/signing.h (1)
TX_CONFIRM_THRESHOLD(24-24)src/chainlock/clsig.cpp (2)
GenSigRequestId(29-32)GenSigRequestId(29-29)
src/instantsend/signing.cpp (1)
src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/chainlock/chainlock.cpp (4)
src/chainlock/chainlock.h (2)
cs(61-124)cs(62-121)src/chainlock/clsig.cpp (2)
GenSigRequestId(29-32)GenSigRequestId(29-29)src/instantsend/signing.cpp (1)
GetTransaction(21-22)src/chainlock/signing.h (1)
TX_CONFIRM_THRESHOLD(24-24)
🪛 GitHub Actions: Clang Diff Format Check
src/chainlock/chainlock.cpp
[error] 163-170: Clang-format differences detected for src/chainlock/chainlock.cpp. The failing step ran: 'git diff -U0 origin/develop --
⏰ 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)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (22)
src/chainlock/signing.cpp (6)
18-26: Constructor dependency injection looks good; confirm lifetimesStoring references to shareman, sporkman, and mn_sync is appropriate. Please confirm these outlive ChainLockSigner instances under all lifecycle paths.
59-61: Null-safety for chain tip traversal is correctSkipping when
pindexis null or when there is no previous block avoids edge-case dereferences (e.g., genesis). Good addition.
88-90: Log clarity improvement looks fineExpanded log includes hash and height; matches surrounding logging style.
98-103: Use of TX_CONFIRM_THRESHOLD improves readabilityReplacing the magic number with the named constant makes the intent explicit and keeps this logic tied to a single definition.
129-131: Switch to GenSigRequestId(height) is correct and consistentUsing GenSigRequestId standardizes request ID derivation across CLSIG flows.
191-198: Good: avoid deref when LookupBlockIndex() returns nullptrReturning early when the block index is missing prevents null dereference and matches the “freshly started” path.
src/chainlock/chainlock.cpp (16)
35-39: Migration to chrono literals for intervals is a good moveReplacing raw integers with
30s,24h, and10minimproves type-safety. Ensure chrono literals are visible (util/time.h typically brings them in).
74-85: Scheduler loop composition is soundPeriodic CheckActiveState, Enforce, Cleanup, and retry TrySignChainTip is appropriate. Frequency of 5s aligns with prior behavior.
136-137: seenChainLocks timestamp now uses seconds; goodStoring
GetTime<std::chrono::seconds>()as the value gives consistent units with cleanup logic.
147-149: Expanded logging for invalid CLSIGs looks usefulIncluding
ToUnderlying(ret)andpeeraids debugging.
156-157: Precompute clsig_inv near usageLocalizing
clsig_invcreation improves readability and ensures the same INV is returned across branches.
163-170: Clang-format failure in CI; please run the fixerThe pipeline reports a clang-format diff around these lines. Please run the repo’s formatting script and push the result.
Apply:
- contrib/devtools/clang-format-diff.py on the touched hunk or clang-format the file.
180-186: Immediate re-enforcement scheduling is appropriateScheduling
EnforceBestChainLock()from now avoids reentrancy while promptly acting on new CLSIGs.
187-191: Post-processing logs and return value look goodReturning
clsig_inveven when the block was known ensures propagation; logging is concise.
198-200: Helpful late-header logClear message for the late header case; guards against height mismatch with a log and return.
223-234: Atomic gating of TrySignChainTip scheduling prevents concurrent runsThe
compare_exchange_strongpattern and resetting the flag at the end of the task is correct.
286-292: Erasing block->txid map on disconnect is correctKeeps signer’s ephemeral view consistent on reorgs.
303-313: IsTxSafeForMining duration logic is correctComputing
tx_agein seconds and comparing toWAIT_FOR_ISLOCK_TIMEOUTaligns with the new model.
346-348: Enforce log includes both block and CLSIG; good for traceabilityThis will help when diagnosing enforcement behavior.
351-361: Activation pre-check avoids unnecessary ActivateBestChainUsing an ancestor comparison with cs_main held is a clean way to decide if activation is needed; the post-activation re-check is also correct.
449-456: Aging out seenChainLocks by 24h is fineDuration-based comparison is clear and efficient; iteration/erase pattern is correct.
471-489: Depth-based pruning of txFirstSeenTime aligns with TX_CONFIRM_THRESHOLD
- Asserts that
hashBlockresolves to a valid index, which should hold given GetTransaction’s contract.- Uses active-chain check before applying depth threshold.
- Erases tracking once “sufficiently deep,” avoiding unbounded growth.
Looks good.
…er functions a02e843 refactor: privatise `RequestObject` and `EraseObjectRequest` (Kittywhiskers Van Gogh) f95b50a refactor: privatise `RelayInvFiltered` and `AskPeersForTransaction` (Kittywhiskers Van Gogh) c93ade5 refactor: reduce `PeerManager` usage in InstantSend worker functions (Kittywhiskers Van Gogh) 9b2c916 refactor: drop `is_masternode` from `PeerManager` interface functions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6815 * Depends on #6820 * While this pull request doesn't _entirely_ decouple `PeerManager` from `CInstantSendManager` as the worker thread isn't _reactive_ (i.e. triggered by `ProcessMessage()`), we can reduce its usage by folding all expected network activity into a `MessageProcessingResult` batch that can be consumed by `PostProcessMessage`. To achieve this, `MessageProcessingResult` has been extended to allow * Requesting peers for a transaction (by setting `m_request_tx`) * Relaying inventories to interested peer based on filter match (by setting `m_inv_filter`) This has the additional effect of removing external uses of `RelayInvFiltered()` and `AskPeersForTransaction()`, which can now be safely removed from the `PeerManager` interface as it is now brokered through `MessageProcessingResult`. * Note that while the old `RelayInvFiltered()` allowed specifying _any_ minimum protocol version, `PostProcessMessage()` will assume that `minProtoVersion` is `ISDLOCK_PROTO_VERSION`. This is acceptable as `RelayInvFiltered()` isn't used anywhere else and deterministic InstantSend locks were introduced in v0.18 (see [dash#4381](#4381)). If there is any foreseeable use needing to relay messages to even older versions of Dash Core, `m_inv_filter` can be modified to accommodate that. ## 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 **(note: N/A)** - [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: knst: utACK a02e843 UdjinM6: utACK a02e843 Tree-SHA512: 6a83277781faf996cbb5187d352971c3508191dbf84cd801e062e2cde2db09cf64696bf27b9790f2b278f53adb46c042e04ac5c4dbdebdbc1faba2b262e39930
4 participants
Additional Information
Depends on refactor: move ChainLocks impl. to
src/chainlock/and split masternode logic out of manager source #6761Dependency for refactor: reduce
PeerManagerusage in InstantSend worker functions #6821Assumptions surrounding
pindexusage have been documented in response to reviewer comments in dash#6761 (comment, comment).The internal structures of
CChainLocksHandler(txFirstSeenTime,seenChainLocks,lastCleanupTime) have their time resolution reduced from milliseconds to seconds while migrating tostd::chrono.CInstantSendManager::AskNodesForLockedTx()was moved asPeerManagerImpl::AskPeersForTransaction()in dash#6425 and with that, the sole external usage ofPeerManagerImpl::IsInvInFilter()was moved internally, we can therefore safely remove it from thePeerManagerinterface.Breaking Changes
None expected.
Checklist