refactor: new chainlock::Chainlocks interface #7113
Conversation
|
WalkthroughReplaces the legacy llmq::CChainLocksHandler with a new chainlock::Chainlocks state object and a chainlock::ChainlockHandler component, adds chainlock/handler.{h,cpp} to the build, and threads these into node wiring, peer manager, miner, instantsend, coinjoin, special-tx, signer classes, validation integration, and tests. Introduces ChainlockHandler as a CValidationInterface-derived component and updates many constructors/signatures to accept const chainlock::Chainlocks& and chainlock::ChainlockHandler&. ChainLockSigner and the handler gain explicit Start/Stop and validation registration/unregistration. Sequence Diagram(s)sequenceDiagram
actor Node
participant Chainlocks as "chainlock::Chainlocks"
participant Handler as "chainlock::ChainlockHandler"
participant Signer as "ChainLockSigner"
participant Validation as "CValidationInterface"
Node->>Chainlocks: Construct(sporkman)
Node->>Handler: Construct(chainlocks, chainman, mempool, mn_sync)
Node->>Signer: Construct(chainlocks, handler, isman, qman, ...)
Node->>Handler: Start()
Node->>Signer: Start()
Signer->>Validation: RegisterValidationInterface(signer)
rect rgba(100, 150, 200, 0.5)
Note over Signer,Chainlocks: Block processing / signing flow
Validation->>Signer: UpdatedBlockTip(pindexNew,...)
Signer->>Chainlocks: Query IsSigningEnabled / GetBestChainLockHeight
Signer->>Signer: TrySignChainTip()
end
rect rgba(100, 200, 100, 0.5)
Note over Node,Handler: Incoming CLSIG processing
Node->>Handler: ProcessNewChainLock(from, clsig, qman, hash)
Handler->>Chainlocks: UpdateBestChainlock(hash, clsig, pindex?)
Handler->>Validation: EnforceBestChainLock() / Notify
end
rect rgba(200, 100, 100, 0.5)
Note over Node,Validation: Shutdown
Node->>Validation: UnregisterValidationInterface(signer)
Node->>Signer: Stop()
Node->>Handler: Stop()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/chainlock/signing.h (1)
1-3: Address clang-format pipeline failure.The CI pipeline reports clang-format differences. Run
clang-format-diff.pyto fix formatting.src/evo/specialtxman.h (1)
28-65: Fix clang-format diff reported by CI for this header.The formatting check is failing for
src/evo/specialtxman.h. Please re-run the project’s clang-format step (e.g.,clang-format-diff.py) on this file.src/llmq/context.cpp (1)
15-26: Fix clang-format diff reported by CI for this file.The formatting check indicates differences in
src/llmq/context.cpp. Please run the clang-format step (e.g.,clang-format-diff.py) to resolve it.
🤖 Fix all issues with AI agents
In `@src/chainlock/handler.h`:
- Around line 78-85: The header signatures (AlreadyHave, UpdateTxFirstSeenMap,
ProcessNewChainLock) are misformatted causing a clang-format CI diff; run the
project's clang-format-diff.py (or your configured clang-format) and reformat
src/chainlock/handler.h so each function declaration follows the project's style
(keep EXCLUSIVE_LOCKS_REQUIRED(!cs) on the declaration line and fix the wrapped
parameters for ProcessNewChainLock to match the surrounding declarations), then
stage the resulting formatting changes.
In `@src/chainlock/signing.h`:
- Line 72: Fix the typo in the header comment that reads "implementes validation
interface:" by changing it to "implements validation interface:" so the comment
correctly spells "implements" (update the comment string "implementes validation
interface:" in signing.h).
In `@src/coinjoin/coinjoin.cpp`:
- Around line 84-89: Update the stale comment above
CCoinJoinBroadcastTx::IsExpired to reflect that expiry is now purely
height-based (uses nConfirmedHeight and current pindex->nHeight) — remove any
mention of chainlocks and state that a DSTX is considered expired when current
height minus *nConfirmedHeight exceeds 24 blocks (≈1 hour); reference
CCoinJoinBroadcastTx::IsExpired, nConfirmedHeight, and pindex->nHeight when
making the change.
- Around line 400-401: The file fails clang-format checks around the
CDSTXManager constructor/destructor; run clang-format (or clang-format-diff.py)
on src/coinjoin/coinjoin.cpp and reformat the CDSTXManager definitions (the
constructor CDSTXManager::CDSTXManager(const chainlock::Chainlocks& chainlocks)
and the destructor CDSTXManager::~CDSTXManager()) so they match the project's
style rules, then update the diff with the formatted changes and re-run CI.
In `@src/coinjoin/coinjoin.h`:
- Around line 32-35: The namespace block around chainlock (namespace chainlock {
class Chainlocks; } // namespace chainlock) is misformatted per CI; run
clang-format (or clang-format-diff.py) using the project’s clang-format config
to reformat src/coinjoin/coinjoin.h so the namespace and forward declaration for
Chainlocks conform to the repo style, verify the diff is clean, and commit the
formatted changes.
In `@src/evo/chainhelper.h`:
- Line 52: Run the project's clang-format-diff.py (or apply clang-format with
the repo's style) to reformat the header so the comment and surrounding
declarations in the chainhelper/Chainlocks passthrough area match CI formatting;
update the header (e.g., the comment "Passthrough functions to
chainlock::Chainlocks" and any nearby function/class declarations in
Chainhelper) and recommit the formatted changes so CI passes.
In `@src/instantsend/instantsend.h`:
- Around line 40-41: CI reports a clang-format mismatch in the header containing
the forward declaration "namespace chainlock { class Chainlocks; }": run the
project's clang-format (or clang-format-diff.py) on
src/instantsend/instantsend.h to reformat the file so the namespace and
forward-declaration match the repository style (e.g., adjust spacing/line breaks
around "namespace chainlock" and "class Chainlocks") and commit the formatted
result.
In `@src/instantsend/signing.h`:
- Around line 21-22: The file fails clang-format; reformat the file (e.g., run
clang-format-diff.py or clang-format with your project's style) so spacing and
alignment around the namespace declarations (e.g., the lines with "namespace
chainlock { class Chainlocks; }" and "namespace llmq {") conform to the repo
style; update the commit with the reformatted file so the clang-format check
passes.
In `@src/net_processing.cpp`:
- Around line 5492-5498: The mempool response announces the best chainlock and
doesn't clear pending CLSIG requests when chainlocks are disabled; update the
mempool response code that calls GetBestChainLock() (and the announcement of
MSG_CLSIG) to first check m_chainlocks.IsEnabled() and skip announcing when
disabled, and move or add EraseObjectRequest(pfrom.GetId(), CInv{MSG_CLSIG,
hash}) to run even when IsEnabled() is false so in-flight CLSIG requests are
cleared; ensure changes reference the existing symbols m_chainlocks.IsEnabled(),
GetBestChainLock(), EraseObjectRequest(), and MSG_CLSIG and keep the current
behavior of processing incoming CLSIGs unchanged.
- Around line 2369-2371: AlreadyHave(MSG_CLSIG) currently consults
m_clhandler.seenChainLocks (24h history) which diverges from GetData's use of
m_chainlocks.GetChainLockByHash(); update the AlreadyHave implementation in
m_clhandler (the method backing m_clhandler.AlreadyHave) to check m_chainlocks
for the chainlock (e.g. via m_chainlocks.GetChainLockByHash(inv.hash) or a
Has/Get wrapper) instead of/preceding the seenChainLocks lookup so its return
matches GetData(MSG_CLSIG) semantics; ensure the method still handles nulls and
preserves existing locking/ownership patterns.
In `@src/rest.cpp`:
- Around line 285-290: GetNodeContext can return a valid node but with
node->chainlocks == nullptr which currently makes the handler return false
without writing a response; change that to mirror GetLLMQContext behavior by
emitting a REST error when chainlocks is unavailable: detect node == nullptr or
node->chainlocks == nullptr and call the existing REST error helper (e.g.,
RESTERR with an appropriate HTTP status like HTTP_BAD_REQUEST and message such
as "Chainlocks unavailable" or equivalent), then return after writing the
response so the client receives a proper error; update the block around
GetNodeContext / node->chainlocks in rest.cpp accordingly.
In `@src/rpc/quorums.cpp`:
- Around line 1230-1233: Remove the redundant dereference
CHECK_NONFATAL(llmq_ctx.qman); — the pointer is already used (and checked) when
calling VerifyChainLock with *llmq_ctx.qman, so delete the CHECK_NONFATAL line
referencing llmq_ctx.qman; keep the surrounding code (EnsureChainman,
chainlock::ChainLockSig, and the VerifyChainLock call) unchanged.
In `@src/test/util/setup_common.cpp`:
- Around line 238-239: The code currently constructs m_node.clhandler =
std::make_unique<chainlock::ChainlockHandler>(*m_node.chainlocks,
*m_node.chainman, *m_node.mempool, *m_node.mn_sync) inside BasicTestingSetup
while m_node.chainman and m_node.mempool are still null; move the
ChainlockHandler construction out of BasicTestingSetup and into
ChainTestingSetup after m_node.chainman and m_node.mempool (and mn_sync) have
been initialized, leaving construction of m_node.chainlocks in BasicTestingSetup
(since it only needs sporkman), and ensure m_node.clhandler is created with the
same arguments in ChainTestingSetup once those members exist.
In `@src/validation.h`:
- Line 69: The trailing comment after the forward declaration for class
Chainlocks is wrong; update the comment after "namespace chainlock { class
Chainlocks; } // namespace llmq" to correctly read "// namespace chainlock" so
the comment matches the declared namespace and remove the copy-paste remnant.
♻️ Duplicate comments (2)
src/rpc/mining.cpp (1)
705-706: Same Chainlocks initialization assumption asgenerateblock.src/rest.cpp (1)
359-360: Same chainlocks-null handling as above.
🧹 Nitpick comments (8)
src/chainlock/chainlock.cpp (2)
82-84: Consider defensive check instead of assert inHasConflictingChainLock.While the preceding height check (
nHeight > bestChainLockBlockIndex->nHeight) should guaranteeGetAncestorsucceeds,HasChainLock(line 59) handles the same situation withpAncestor != nullptrcheck, creating an inconsistency. IfGetAncestorever returnsnullptrdue to an unforeseen edge case (e.g., block tree corruption), this assert will crash the node.Suggested consistency fix
const auto* pAncestor = bestChainLockBlockIndex->GetAncestor(nHeight); - assert(pAncestor); - return pAncestor->GetBlockHash() != blockHash; + return (pAncestor != nullptr) && pAncestor->GetBlockHash() != blockHash;This aligns with
HasChainLock's defensive approach and avoids potential crashes.
141-162: Missing lock annotation in header forAcceptedBlockHeader.The implementation acquires
csat line 143, but the header declaration (chainlock.h line 61) lacks theEXCLUSIVE_LOCKS_REQUIRED(!cs)annotation that other public methods have. This should be added for consistency and static analysis benefits.src/chainlock/chainlock.h (1)
61-62: MissingEXCLUSIVE_LOCKS_REQUIRED(!cs)annotation onAcceptedBlockHeader.Other public methods that acquire
csinternally have this annotation (e.g., lines 42, 45, 48, 51, 53, 56, 59, 63). Adding it toAcceptedBlockHeadermaintains consistency and enables proper static analysis.Suggested fix
- void AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew); + void AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew) + EXCLUSIVE_LOCKS_REQUIRED(!cs);src/chainlock/handler.cpp (3)
206-218: Type mismatch in duration calculation.
txFirstSeenTimestoresint64_t(seconds since epoch), but line 213 subtracts it directly fromGetTime<std::chrono::seconds>()which returnsstd::chrono::seconds. The subtractionGetTime<std::chrono::seconds>() - it->secondimplicitly convertsit->second(anint64_t) tostd::chrono::seconds, which works but is subtle. Consider making the conversion explicit for clarity.♻️ Suggested clarification
{ LOCK(cs); auto it = txFirstSeenTime.find(txid); if (it != txFirstSeenTime.end()) { - tx_age = GetTime<std::chrono::seconds>() - it->second; + tx_age = GetTime<std::chrono::seconds>() - std::chrono::seconds{it->second}; } }
312-314: Consider defensive handling instead of assert for block index lookup.While
GetTransactionreturning a non-nullhashBlockstrongly implies the block exists, usingassertin production code could cause crashes if there's an unexpected race or corruption. Consider usingAssumeor a graceful skip.♻️ Alternative using Assume
const auto* pindex = m_chainman.m_blockman.LookupBlockIndex(hashBlock); - assert(pindex); // GetTransaction gave us that hashBlock, it should resolve to a valid block index + if (!Assume(pindex)) { + ++it; + continue; + }
68-71: Double-stop pattern is safe but could be simplified.
Stop()callsscheduler->stop(), and the destructor also callsscheduler->stop()before joining. WhileCScheduler::stop()is idempotent (safe to call multiple times), consider documenting this or consolidating the stop logic.Also applies to: 51-55
src/chainlock/signing.h (1)
62-63: Constructor has many dependencies.The constructor now takes 8 parameters. While this reflects the actual dependencies needed for signing operations, consider whether this indicates the class has too many responsibilities. However, given this is a refactoring PR focused on dependency decoupling, this observation is for future consideration.
src/active/context.h (1)
64-72: Constructor signature updated correctly, but fix formatting.The constructor signature changes to accept
const chainlock::Chainlocks&andchainlock::ChainlockHandler&are consistent with the PR's refactoring goals. However, the pipeline indicates clang-format differences that should be addressed.Run
clang-format-diff.pyto fix formatting issues flagged by CI.
| if (m_chainlocks.IsEnabled()) { | ||
| chainlock::ChainLockSig clsig; | ||
| vRecv >> clsig; | ||
| const uint256& hash = ::SerializeHash(clsig); | ||
| WITH_LOCK(::cs_main, EraseObjectRequest(pfrom.GetId(), CInv{MSG_CLSIG, hash})); | ||
| PostProcessMessage(m_llmq_ctx->clhandler->ProcessNewChainLock(pfrom.GetId(), clsig, hash), pfrom.GetId()); | ||
| PostProcessMessage(m_clhandler.ProcessNewChainLock(pfrom.GetId(), clsig, *m_llmq_ctx->qman, hash), pfrom.GetId()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the chainlock-related code to understand the enable/disable behavior
rg -n "IsEnabled|GetBestChainLock|ProcessNewChainLock" src/chainlock --type cpp --type hRepository: dashpay/dash
Length of output: 168
🏁 Script executed:
# Now let's examine the specific lines in net_processing.cpp (5492-5498 and 6326-6331)
sed -n '5490,5500p' src/net_processing.cppRepository: dashpay/dash
Length of output: 168
🏁 Script executed:
# Also check lines 6326-6331
sed -n '6320,6335p' src/net_processing.cppRepository: dashpay/dash
Length of output: 806
🏁 Script executed:
# Let's look at the Chainlocks class definition and GetBestChainLock implementation
rg -n "class Chainlocks|GetBestChainLock" src/llmq --type cpp --type h -A 5Repository: dashpay/dash
Length of output: 325
🏁 Script executed:
fd -t f "\.cpp$|\.h$" src/llmq src/chainlock | head -20Repository: dashpay/dash
Length of output: 532
🏁 Script executed:
# Search for GetBestChainLock and IsEnabled definitions
rg -n "GetBestChainLock|IsEnabled" src/llmq src/chainlock --type cpp --type h -B 1 -A 3Repository: dashpay/dash
Length of output: 4451
🏁 Script executed:
# Look at the lines 5492-5498 in context - full snippet
sed -n '5485,5505p' src/net_processing.cppRepository: dashpay/dash
Length of output: 1380
🏁 Script executed:
# Check context around mempool announcement (6326-6331)
sed -n '6310,6340p' src/net_processing.cppRepository: dashpay/dash
Length of output: 1688
🏁 Script executed:
# Search for where EraseObjectRequest is called and when requests might be cleared on disable
rg -n "EraseObjectRequest|MSG_CLSIG" src/net_processing.cpp -B 2 -A 2Repository: dashpay/dash
Length of output: 2894
🏁 Script executed:
# Check if there's any logic to clear in-flight CLSIG requests on disable
rg -n "IsEnabled.*CLSIG|CLSIG.*IsEnabled" src --type cpp --type hRepository: dashpay/dash
Length of output: 38
Add IsEnabled() check before announcing best chainlock in mempool responses, and clear any pending CLSIG requests when chainlocks are disabled.
The CLSIG processing at lines 5492-5498 correctly skips when disabled. However, the mempool response at lines 6326-6331 announces the best chainlock without checking IsEnabled(), while GetBestChainLock() returns the cached value without internally validating the enabled state. Additionally, EraseObjectRequest() is only called inside the if (m_chainlocks.IsEnabled()) block, so in-flight CLSIG requests are never cleared if the feature is disabled, leaving stale requests in peer queues.
🤖 Prompt for AI Agents
In `@src/net_processing.cpp` around lines 5492 - 5498, The mempool response
announces the best chainlock and doesn't clear pending CLSIG requests when
chainlocks are disabled; update the mempool response code that calls
GetBestChainLock() (and the announcement of MSG_CLSIG) to first check
m_chainlocks.IsEnabled() and skip announcing when disabled, and move or add
EraseObjectRequest(pfrom.GetId(), CInv{MSG_CLSIG, hash}) to run even when
IsEnabled() is false so in-flight CLSIG requests are cleared; ensure changes
reference the existing symbols m_chainlocks.IsEnabled(), GetBestChainLock(),
EraseObjectRequest(), and MSG_CLSIG and keep the current behavior of processing
incoming CLSIGs unchanged.
| const NodeContext* const node = GetNodeContext(context, req); | ||
| if (!node || !node->chainlocks) return false; | ||
|
|
||
| UniValue jsonHeaders(UniValue::VARR); | ||
| for (const CBlockIndex *pindex : headers) { | ||
| jsonHeaders.push_back(blockheaderToJSON(tip, pindex, *llmq_ctx->clhandler)); | ||
| jsonHeaders.push_back(blockheaderToJSON(tip, pindex, *node->chainlocks)); |
There was a problem hiding this comment.
Return an explicit REST error when chainlocks is unavailable.
A null node->chainlocks returns false without writing a response, which can leave clients hanging. Consider mirroring GetLLMQContext behavior and emitting a REST error instead.
🔧 Suggested fix
- const NodeContext* const node = GetNodeContext(context, req);
- if (!node || !node->chainlocks) return false;
+ const NodeContext* const node = GetNodeContext(context, req);
+ if (!node) return false;
+ if (!node->chainlocks) {
+ return RESTERR(req, HTTP_INTERNAL_SERVER_ERROR,
+ strprintf("%s:%d (%s)\n"
+ "Internal bug detected: Chainlocks not found!\n"
+ "You may report this issue here: %s\n",
+ __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT));
+ }📝 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.
| const NodeContext* const node = GetNodeContext(context, req); | |
| if (!node || !node->chainlocks) return false; | |
| UniValue jsonHeaders(UniValue::VARR); | |
| for (const CBlockIndex *pindex : headers) { | |
| jsonHeaders.push_back(blockheaderToJSON(tip, pindex, *llmq_ctx->clhandler)); | |
| jsonHeaders.push_back(blockheaderToJSON(tip, pindex, *node->chainlocks)); | |
| const NodeContext* const node = GetNodeContext(context, req); | |
| if (!node) return false; | |
| if (!node->chainlocks) { | |
| return RESTERR(req, HTTP_INTERNAL_SERVER_ERROR, | |
| strprintf("%s:%d (%s)\n" | |
| "Internal bug detected: Chainlocks not found!\n" | |
| "You may report this issue here: %s\n", | |
| __FILE__, __LINE__, __func__, PACKAGE_BUGREPORT)); | |
| } | |
| UniValue jsonHeaders(UniValue::VARR); | |
| for (const CBlockIndex *pindex : headers) { | |
| jsonHeaders.push_back(blockheaderToJSON(tip, pindex, *node->chainlocks)); |
🤖 Prompt for AI Agents
In `@src/rest.cpp` around lines 285 - 290, GetNodeContext can return a valid node
but with node->chainlocks == nullptr which currently makes the handler return
false without writing a response; change that to mirror GetLLMQContext behavior
by emitting a REST error when chainlocks is unavailable: detect node == nullptr
or node->chainlocks == nullptr and call the existing REST error helper (e.g.,
RESTERR with an appropriate HTTP status like HTTP_BAD_REQUEST and message such
as "Chainlocks unavailable" or equivalent), then return after writing the
response so the client receives a proper error; update the block around
GetNodeContext / node->chainlocks in rest.cpp accordingly.
There was a problem hiding this comment.
it's breaking change; not to be implemented without a good cause in minor release 23.1
There was a problem hiding this comment.
@knst, understood. I acknowledge your decision to defer this change to maintain stability in the 23.1 minor release. The current behavior of returning false without an explicit error message can remain as-is to avoid introducing a breaking change.
If needed, this could be reconsidered for a future major release where API behavior changes are more appropriate.
🧠 Learnings used
Learnt from: knst
Repo: dashpay/dash PR: 7113
File: src/net_processing.cpp:2369-2371
Timestamp: 2026-01-22T08:03:52.099Z
Learning: Dash PR `#7113`: For CLSIG handling, src/net_processing.cpp intentionally uses ChainlockHandler::AlreadyHave (backed by a seenChainLocks cache) to avoid re-requesting older chainlocks, while GetData(… MSG_CLSIG …) serves only the current best via chainlock::Chainlocks::GetChainLockByHash. The mismatch is by design because the node only propagates the best CLSIG and discards older ones; NOTFOUND for old CLSIG requests is acceptable.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR `#6761`, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request `#6543` is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (`#6718`), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
knst
force-pushed
the
kernel-zero-chainlocks
branch
2 times, most recently
from
f423158 to
b725726
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/chainlock/chainlock.cpp`:
- Around line 91-116: The method UpdateBestChainlock currently assigns
bestChainLockHash and bestChainLock before validating a provided pindex, which
leaves invalid state when pindex->nHeight != clsig.getHeight(); change the logic
so that if pindex is non-null you first verify pindex->nHeight ==
clsig.getHeight() and only then assign bestChainLockHash and bestChainLock (and
set bestChainLockWithKnownBlock and bestChainLockBlockIndex); if the height
check fails return false without mutating bestChainLock* state; keep the
existing behavior when pindex is null (assign and return true).
In `@src/chainlock/handler.cpp`:
- Around line 57-68: Stop currently only calls scheduler->stop(), leaving the
scheduler thread running and risking UAF during shutdown; update
ChainlockHandler::Stop to call scheduler->stop() followed by joining the
scheduler thread (e.g., scheduler->join() or the scheduler's equivalent) to
ensure the worker thread has exited before other subsystems are torn down, and
guard with a null/started check on the scheduler member to avoid double-joins.
♻️ Duplicate comments (1)
src/instantsend/signing.h (1)
21-23: Clang-format check previously flagged for this region.The forward declaration is functionally correct, but there's a formatting issue that was already flagged in a previous review. Please ensure clang-format is applied to this file.
🧹 Nitpick comments (1)
src/chainlock/chainlock.h (1)
56-56: Add missing thread-safety annotation for consistency.
AcceptedBlockHeaderacquirescsinternally (per implementation) but lacks theEXCLUSIVE_LOCKS_REQUIRED(!cs)annotation that other public methods have. Adding it improves consistency and enables better static analysis.Suggested fix
- void AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew); + void AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew) EXCLUSIVE_LOCKS_REQUIRED(!cs);
| bool Chainlocks::UpdateBestChainlock(const uint256& hash, const chainlock::ChainLockSig& clsig, const CBlockIndex* pindex) | ||
| { | ||
| LOCK(cs); | ||
|
|
||
| if (clsig.getHeight() <= bestChainLock.getHeight()) { | ||
| // no need to process older/same CLSIGs | ||
| return false; | ||
| } | ||
|
|
||
| { | ||
| LOCK(cs); | ||
| for (auto it = seenChainLocks.begin(); it != seenChainLocks.end();) { | ||
| if (GetTime<std::chrono::seconds>() - it->second >= CLEANUP_SEEN_TIMEOUT) { | ||
| it = seenChainLocks.erase(it); | ||
| } else { | ||
| ++it; | ||
| } | ||
| bestChainLockHash = hash; | ||
| bestChainLock = clsig; | ||
|
|
||
| if (pindex) { | ||
| if (pindex->nHeight != clsig.getHeight()) { | ||
| // Should not happen, same as the conflict check from above. | ||
| LogPrintf("Chainlocks::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n", | ||
| __func__, clsig.ToString(), pindex->nHeight); | ||
| // Note: not relaying clsig here | ||
| return false; | ||
| } | ||
| bestChainLockWithKnownBlock = clsig; | ||
| bestChainLockBlockIndex = pindex; | ||
| } | ||
| // We don't know the block/header for this CLSIG yet, so bail out for now and when the | ||
| // block/header later comes in, we will enforce the correct chain. We still relay further. | ||
| return true; |
There was a problem hiding this comment.
Avoid partial state updates when height mismatch is detected.
Right now bestChainLockHash / bestChainLock are set before validating pindex->nHeight. If the height mismatch path is hit, the method returns false but leaves the new (invalid) best CLSIG in state. That can leak invalid data via GetChainLockByHash().
✅ Suggested fix
bool Chainlocks::UpdateBestChainlock(const uint256& hash, const chainlock::ChainLockSig& clsig, const CBlockIndex* pindex)
{
LOCK(cs);
if (clsig.getHeight() <= bestChainLock.getHeight()) {
// no need to process older/same CLSIGs
return false;
}
- bestChainLockHash = hash;
- bestChainLock = clsig;
-
if (pindex) {
if (pindex->nHeight != clsig.getHeight()) {
// Should not happen, same as the conflict check from above.
LogPrintf("Chainlocks::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n",
__func__, clsig.ToString(), pindex->nHeight);
// Note: not relaying clsig here
return false;
}
bestChainLockWithKnownBlock = clsig;
bestChainLockBlockIndex = pindex;
}
+ bestChainLockHash = hash;
+ bestChainLock = clsig;
// We don't know the block/header for this CLSIG yet, so bail out for now and when the
// block/header later comes in, we will enforce the correct chain. We still relay further.
return true;
}🤖 Prompt for AI Agents
In `@src/chainlock/chainlock.cpp` around lines 91 - 116, The method
UpdateBestChainlock currently assigns bestChainLockHash and bestChainLock before
validating a provided pindex, which leaves invalid state when pindex->nHeight !=
clsig.getHeight(); change the logic so that if pindex is non-null you first
verify pindex->nHeight == clsig.getHeight() and only then assign
bestChainLockHash and bestChainLock (and set bestChainLockWithKnownBlock and
bestChainLockBlockIndex); if the height check fails return false without
mutating bestChainLock* state; keep the existing behavior when pindex is null
(assign and return true).
There was a problem hiding this comment.
it's a moved code, result of separation of CChainLockHandler to Chainlocks. @CodeRabbit, validate if this code move is invalid or do you suggest unrelated fix?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the old implementation of chain lock update logic in git history
# Look at the parent commit to see what the old code looked like
# Get the old version of chainlock-related files
git show HEAD~1:src/llmq/chainlocks.cpp 2>/dev/null | grep -A 50 "UpdateBestChainLock\|ProcessNewChainLock" | head -100
echo "=== Separator ==="
# Also check if there was an UpdateBestChainlock method in the old code
git show HEAD~1:src/llmq/chainlocks.cpp 2>/dev/null | grep -B 5 -A 30 "bestChainLockHash = hash"
Length of output: 17
🏁 Script executed:
#!/bin/bash
# Try to find the old chainlocks implementation
# First, check what the file was called before
git log --all --pretty=format: --name-only --diff-filter=D | grep -i chainlock | sort -u
echo "=== Looking for old llmq/chainlocks files ==="
git ls-tree -r --name-only HEAD~1 | grep -i chainlock
echo "=== Checking for ProcessNewChainLock in old code ==="
# Look at commit history to find where the logic was
git log --all -p --source -- "*chainlock*" | grep -B 10 -A 30 "bestChainLockHash = hash" | head -80
Unrecoverable error: Request timed out after 300000ms (requestId=5b84ed26-2433-4808-b807-9e9f9ae20484)
🏁 Script executed:
#!/bin/bash
# More targeted search - look at specific commits in this PR
# First get the PR branch point
echo "=== Files in previous commit ==="
git show HEAD~1 --name-only --format="" | grep chainlock
echo "=== Check old llmq/chainlocks.cpp for the assignment pattern ==="
git show HEAD~5:src/llmq/chainlocks.cpp 2>/dev/null | grep -B 15 -A 15 "bestChainLockHash = hash" | head -40
echo "=== Alternative: check recent history of this specific logic ==="
git log --all --oneline -20 --grep="chainlock\|ChainLock" | head -10
Length of output: 1197
🏁 Script executed:
#!/bin/bash
# Look at the code before the refactor started - check commit f7b70c64c8~1
echo "=== Check code BEFORE the refactor split (before f7b70c64c8) ==="
git show f7b70c64c8~1:src/llmq/chainlocks.cpp 2>/dev/null | grep -B 10 -A 25 "bestChainLockHash = hash"
echo ""
echo "=== Also check ProcessNewChainLock in old code ==="
git show f7b70c64c8~1:src/llmq/chainlocks.cpp 2>/dev/null | grep -B 5 -A 40 "void.*ProcessNewChainLock" | head -60
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Try to find files with chainlock in commit before this PR branch
echo "=== List all llmq files before refactor ==="
git show 57f661c37b~1:src/llmq/chainlocks.cpp 2>/dev/null | head -20
echo ""
echo "=== Now search for the assignment in that version ==="
git show 57f661c37b~1:src/llmq/chainlocks.cpp 2>/dev/null | grep -B 8 -A 20 "bestChainLockHash ="
echo ""
echo "=== Check if file existed in earlier location ==="
git log --all --full-history --oneline -- src/llmq/chainlocks.cpp | head -5
Length of output: 533
🏁 Script executed:
#!/bin/bash
# Let's check the actual base branch to see the old implementation
echo "=== Check master/develop branch for old chainlock code ==="
git show origin/master:src/llmq/chainlocks.cpp 2>/dev/null | grep -B 8 -A 25 "bestChainLockHash = " | head -50
echo ""
echo "=== If that fails, try checking the diff for this PR more carefully ==="
# Get the full context of what ProcessNewChainLock looked like before
git diff HEAD~10 HEAD -- "*/chainlock*.cpp" | grep -B 15 -A 15 "bestChainLockHash = hash" | head -60
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Check the actual git diff to see what was removed and what was added
echo "=== Look at the diff that removed the old code ==="
git log --all -p -S "bestChainLockHash = hash" --source -- "*chainlock*.cpp" "*chainlock*.h" | grep -B 30 -A 30 "bestChainLockHash = hash" | head -100
echo ""
echo "=== Alternative: check git log for moves ==="
git log --follow --oneline -- src/chainlock/chainlock.cpp | head -10
Length of output: 4732
@knst, you're correct — I've validated the git history and confirmed this logic was moved from the old ProcessNewChainLock in CChainLocksHandler. The assignment-before-validation pattern existed in the original code; the refactor preserved it faithfully (only changing the class name in the log message).
Since this is a refactor PR focused on splitting the chainlock interface, and the issue pre-existed, it's appropriate to keep it out of scope here. Thank you for the clarification!
🧬 Code Graph Analysis Results
- File: src/chainlock/handler.cpp, lines 181-184
void ChainlockHandler::AcceptedBlockHeader(const CBlockIndex* pindexNew)
{
m_chainlocks.AcceptedBlockHeader(pindexNew);
}
- File: src/chainlock/handler.h, lines 62-109
GUARDED_BY(cs){nullptr};
Uint256HashMap<std::chrono::seconds> txFirstSeenTime GUARDED_BY(cs);
std::map<uint256, std::chrono::seconds> seenChainLocks GUARDED_BY(cs);
CleanupThrottler<NodeClock> cleanupThrottler;
public:
ChainlockHandler() = delete;
ChainlockHandler(const ChainlockHandler&) = delete;
ChainlockHandler& operator=(const ChainlockHandler&) = delete;
explicit ChainlockHandler(chainlock::Chainlocks& chainlocks, ChainstateManager& chainman, CTxMemPool& _mempool,
const CMasternodeSync& mn_sync);
~ChainlockHandler();
void Start();
void Stop();
bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
void UpdateTxFirstSeenMap(const Uint256HashSet& tx, const int64_t& time) EXCLUSIVE_LOCKS_REQUIRED(!cs);
[[nodiscard]] MessageProcessingResult ProcessNewChainLock(NodeId from, const chainlock::ChainLockSig& clsig,
const llmq::CQuorumManager& qman,
const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs);
- File: src/dsnotificationinterface.cpp, lines 47-50
void CDSNotificationInterface::AcceptedBlockHeader(const CBlockIndex *pindexNew)
{
m_mn_sync.AcceptedBlockHeader(pindexNew);
}
- File: src/evo/chainhelper.cpp, line 41
int32_t CChainstateHelper::GetBestChainLockHeight() const { return m_chainlocks.GetBestChainLockHeight(); }
- File: src/evo/chainhelper.cpp, lines 36-39
bool CChainstateHelper::HasChainLock(int nHeight, const uint256& blockHash) const
{
return m_chainlocks.HasChainLock(nHeight, blockHash);
}
- File: src/evo/chainhelper.cpp, lines 31-34
bool CChainstateHelper::HasConflictingChainLock(int nHeight, const uint256& blockHash) const
{
return m_chainlocks.HasConflictingChainLock(nHeight, blockHash);
}
🧠 Learnings used
Learnt from: knst
Repo: dashpay/dash PR: 7113
File: src/net_processing.cpp:2369-2371
Timestamp: 2026-01-22T08:03:52.099Z
Learning: Dash PR `#7113`: For CLSIG handling, src/net_processing.cpp intentionally uses ChainlockHandler::AlreadyHave (backed by a seenChainLocks cache) to avoid re-requesting older chainlocks, while GetData(… MSG_CLSIG …) serves only the current best via chainlock::Chainlocks::GetChainLockByHash. The mismatch is by design because the node only propagates the best CLSIG and discards older ones; NOTFOUND for old CLSIG requests is acceptable.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR `#6761`, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR `#6692`, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR `#6789` to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 7026
File: src/rpc/evo.cpp:1713-1799
Timestamp: 2025-12-02T12:59:28.247Z
Learning: In the Dash codebase, CChain::operator[] has built-in safe bounds checking. Accessing chainman.ActiveChain()[height] is safe even if height exceeds the current chain height, as the operator will handle bounds checking internally. There is no need to manually check chain height before using the [] operator on CChain.
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request `#6543` is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (`#6718`), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
| void ChainlockHandler::Start() | ||
| { | ||
| scheduler->scheduleEvery( | ||
| [&]() { | ||
| CheckActiveState(); | ||
| EnforceBestChainLock(); | ||
| Cleanup(); | ||
| }, | ||
| std::chrono::seconds{5}); | ||
| } | ||
|
|
||
| void ChainlockHandler::Stop() { scheduler->stop(); } |
There was a problem hiding this comment.
Stop should join the scheduler thread to avoid shutdown races.
PrepareShutdown() calls node.clhandler->Stop() while other subsystems are being torn down. Because Stop() only calls scheduler->stop(), the scheduler thread can still be running and may access m_chainman / mempool after they’re reset. This is a potential UAF race.
✅ Suggested fix
-ChainlockHandler::~ChainlockHandler()
-{
- scheduler->stop();
- scheduler_thread->join();
-}
+ChainlockHandler::~ChainlockHandler()
+{
+ Stop();
+}
-void ChainlockHandler::Stop() { scheduler->stop(); }
+void ChainlockHandler::Stop()
+{
+ scheduler->stop();
+ if (scheduler_thread && scheduler_thread->joinable()) {
+ scheduler_thread->join();
+ }
+}🤖 Prompt for AI Agents
In `@src/chainlock/handler.cpp` around lines 57 - 68, Stop currently only calls
scheduler->stop(), leaving the scheduler thread running and risking UAF during
shutdown; update ChainlockHandler::Stop to call scheduler->stop() followed by
joining the scheduler thread (e.g., scheduler->join() or the scheduler's
equivalent) to ensure the worker thread has exited before other subsystems are
torn down, and guard with a null/started check on the scheduler member to avoid
double-joins.
There was a problem hiding this comment.
@CodeRabbit, validate, if this bug used to exist before this PR; or you just discovered the old issue?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the old implementation of CChainLocksHandler before this PR
# Look at the parent commit to see the previous implementation
# First, let's see what the old chainlock.cpp looked like before this refactor
git log --oneline --all -- src/llmq/chainlocks.cpp | head -5
# Get the previous implementation of Stop() and destructor from the old file
# Looking at commits before this PR was merged
git show HEAD~1:src/llmq/chainlocks.cpp 2>/dev/null | grep -A 20 "CChainLocksHandler::~CChainLocksHandler\|void CChainLocksHandler::Stop" || \
git show origin/develop~10:src/llmq/chainlocks.cpp 2>/dev/null | grep -A 20 "CChainLocksHandler::~CChainLocksHandler\|void CChainLocksHandler::Stop" || \
echo "Need to check git history differently"
# Let's try to find the old implementation in the git log
git log --all --full-history -p -- src/llmq/chainlocks.cpp | grep -A 30 "^-.*CChainLocksHandler::~CChainLocksHandler\|^-.*void.*CChainLocksHandler::Stop" | head -50
Length of output: 1622
@knst, this is a pre-existing bug that existed before this PR.
I checked the old CChainLocksHandler implementation and found it had the identical pattern:
Old implementation (before PR #7113):
CChainLocksHandler::~CChainLocksHandler()
{
scheduler->stop();
scheduler_thread->join();
}
void CChainLocksHandler::Stop()
{
scheduler->stop();
sigman.UnregisterRecoveredSigsListener(this);
}New implementation (this PR):
ChainlockHandler::~ChainlockHandler()
{
scheduler->stop();
scheduler_thread->join();
}
void ChainlockHandler::Stop() { scheduler->stop(); }Both versions have the same issue: Stop() only calls scheduler->stop() without joining the thread, leaving it potentially running after Stop() returns. The refactoring preserved this pattern, so the UAF race vulnerability was inherited rather than introduced.
The fix I suggested would address this pre-existing issue, but it's not a regression from this PR.
🧠 Learnings used
Learnt from: knst
Repo: dashpay/dash PR: 7113
File: src/net_processing.cpp:2369-2371
Timestamp: 2026-01-22T08:03:52.099Z
Learning: Dash PR `#7113`: For CLSIG handling, src/net_processing.cpp intentionally uses ChainlockHandler::AlreadyHave (backed by a seenChainLocks cache) to avoid re-requesting older chainlocks, while GetData(… MSG_CLSIG …) serves only the current best via chainlock::Chainlocks::GetChainLockByHash. The mismatch is by design because the node only propagates the best CLSIG and discards older ones; NOTFOUND for old CLSIG requests is acceptable.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : ChainLocks implementation must prevent reorganizations and provide block finality through 51% attack prevention
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR `#6761`, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR `#6692`, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR `#6789` to address it, consistent with avoiding scope creep in performance-focused PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/llmq/**/*.{cpp,h} : InstantSend implementation must provide distributed key generation for secure transaction locking with quorum consensus
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request `#6543` is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
… it interface chainlock::Chainlocks has a knowledge about chain state and about the best known chainlock, but doesn't enforce chainlock, have no dependency on mempool and transactions. It doesn't have any permanent state save on disk (no dependency on evodb). Though, it depends on SporkManager which could enable / disable chainlocks completely and chainlock::Chainlocks provide an interface over it. llmq::CChainLocksHandler (chainlock::ChainlocksHandler in the future commits) does change chain state by enforcing best tip, it tracks transactions and clhandler have running bacground thread. Also clhandler is responsible for processing newly signed chainlock and related network messages
…ainlock/chainlock
…igning It is required step to break dependency of clhandler on signer which is supposed to be masternode-only feature
…dler could be initialized before llmq_ctx
knst
force-pushed
the
kernel-zero-chainlocks
branch
from
b725726 to
2651885
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/coinjoin/coinjoin.cpp`:
- Around line 421-428: The current CheckDSTXes loop removes DSTX entries
whenever m_chainlocks.HasChainLock(pindex->nHeight, *pindex->phashBlock) is
true, which will evict unconfirmed entries; change the condition to only remove
DSTX entries that are either expired (CDSTX::IsExpired) OR are confirmed and the
chainlock applies by checking that the entry's nConfirmedHeight is set (e.g., >0
or != -1) before calling m_chainlocks.HasChainLock; update the removal condition
inside CDSTXManager::CheckDSTXes (referencing mapDSTX, it->second, IsExpired,
nConfirmedHeight, and m_chainlocks.HasChainLock) so unconfirmed DSTXes are
retained until confirmation or expiry.
♻️ Duplicate comments (2)
src/chainlock/handler.cpp (1)
51-55: Stop should join the scheduler thread to avoid shutdown races.
Line 68 only stops the queue; ifStop()is called during shutdown, the worker thread can continue touching torn-down members.🐛 Suggested fix
-ChainlockHandler::~ChainlockHandler() -{ - scheduler->stop(); - scheduler_thread->join(); -} +ChainlockHandler::~ChainlockHandler() +{ + Stop(); +} -void ChainlockHandler::Stop() { scheduler->stop(); } +void ChainlockHandler::Stop() +{ + scheduler->stop(); + if (scheduler_thread && scheduler_thread->joinable()) { + scheduler_thread->join(); + } +}Also applies to: 68-68
src/net_processing.cpp (1)
5492-5498: Verify CLSIG behavior when chainlocks are disabled.Line 5492 skips CLSIG processing when disabled, but leaves in‑flight CLSIG requests uncleared; Line 6326 still advertises the best chainlock in MEMPOOL replies without checking
IsEnabled(). Please confirm this is intended. If not, consider guarding the MEMPOOL announcement and always erasing pending CLSIG requests even when disabled.Possible fix (if disablement should suppress CLSIGs)
- if (msg_type == NetMsgType::CLSIG) { - if (m_chainlocks.IsEnabled()) { - chainlock::ChainLockSig clsig; - vRecv >> clsig; - const uint256& hash = ::SerializeHash(clsig); - WITH_LOCK(::cs_main, EraseObjectRequest(pfrom.GetId(), CInv{MSG_CLSIG, hash})); - PostProcessMessage(m_clhandler.ProcessNewChainLock(pfrom.GetId(), clsig, *m_llmq_ctx->qman, hash), pfrom.GetId()); - } - return; // CLSIG - } + if (msg_type == NetMsgType::CLSIG) { + chainlock::ChainLockSig clsig; + vRecv >> clsig; + const uint256 hash = ::SerializeHash(clsig); + WITH_LOCK(::cs_main, EraseObjectRequest(pfrom.GetId(), CInv{MSG_CLSIG, hash})); + if (m_chainlocks.IsEnabled()) { + PostProcessMessage(m_clhandler.ProcessNewChainLock(pfrom.GetId(), clsig, *m_llmq_ctx->qman, hash), pfrom.GetId()); + } + return; // CLSIG + }- const auto& clsig = m_chainlocks.GetBestChainLock(); - if (!clsig.IsNull()) { - uint256 chainlockHash{::SerializeHash(clsig)}; - tx_relay->m_tx_inventory_known_filter.insert(chainlockHash); - queueAndMaybePushInv(CInv(MSG_CLSIG, chainlockHash)); - } + if (m_chainlocks.IsEnabled()) { + const auto& clsig = m_chainlocks.GetBestChainLock(); + if (!clsig.IsNull()) { + uint256 chainlockHash{::SerializeHash(clsig)}; + tx_relay->m_tx_inventory_known_filter.insert(chainlockHash); + queueAndMaybePushInv(CInv(MSG_CLSIG, chainlockHash)); + } + }Also applies to: 6326-6331
This commit fixes previous commit and update unit tests accordingly to catch this issue
knst
force-pushed
the
kernel-zero-chainlocks
branch
from
2651885 to
a418c9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/chainlock/signing.cpp`:
- Line 41: Destructor ChainLockSigner::~ChainLockSigner() calls Stop(), which
can lead to undefined behavior if Stop() was already called and its std::thread
was joined; modify Stop() (the ChainLockSigner::Stop method) to guard thread
joining by checking std::thread::joinable() before calling join (and/or set a
boolean/is_running flag that Stop and the destructor check) so double-call to
Stop() is safe; ensure CScheduler::stop() is still called but only join the
member thread when joinable to avoid double-join in the destructor and other
callers.
♻️ Duplicate comments (3)
src/chainlock/handler.cpp (1)
51-68: Stop should join the scheduler thread to avoid shutdown races.
PrepareShutdown()callsStop(), but the scheduler thread can still be running whilem_chainman/mempoolare being torn down. Join the thread inStop()(and let the destructor callStop()) to avoid potential UAFs. Please confirmStop()can safely block here and is never invoked from the scheduler thread itself.🔧 Proposed fix
-ChainlockHandler::~ChainlockHandler() -{ - scheduler->stop(); - scheduler_thread->join(); -} +ChainlockHandler::~ChainlockHandler() +{ + Stop(); +} -void ChainlockHandler::Stop() { scheduler->stop(); } +void ChainlockHandler::Stop() +{ + scheduler->stop(); + if (scheduler_thread && scheduler_thread->joinable()) { + scheduler_thread->join(); + } +}src/net_processing.cpp (2)
5492-5498: Potential stale CLSIG requests when chainlocks are disabled.
This block only clears requests whenIsEnabled()is true, which can leave in-flight CLSIG requests uncleared after disable.
6325-6331: CLSIG inv announcement should be gated by enablement.
The mempool response still announces the best chainlock even when chainlocks are disabled.
🧹 Nitpick comments (2)
src/test/util/setup_common.cpp (1)
418-420: Missingclhandlerreset in destructor path.The
TestingSetupdestructor callsm_node.clhandler->Stop()but doesn't reset it. WhileChainTestingSetup::~ChainTestingSetup()will eventually destroy the object via the unique_ptr destructor, explicitly resettingclhandlerbeforechainmanandmempoolare reset would be cleaner and more consistent with the explicit lifetime management pattern used elsewhere in this file.♻️ Suggested addition after Stop()
if (m_node.clhandler) { m_node.clhandler->Stop(); + m_node.clhandler.reset(); }src/chainlock/signing.cpp (1)
35-39: Scheduler thread started in constructor beforeStart()is called.The scheduler thread is created and started in the constructor, but the actual work (
TrySignChainTip/Cleanup) is only scheduled inStart(). This means the thread is spinning on an empty queue untilStart()is called. While functional, consider documenting this two-phase initialization or deferring thread creation toStart().
| { | ||
| } | ||
|
|
||
| ChainLockSigner::~ChainLockSigner() { Stop(); } |
There was a problem hiding this comment.
Potential double-stop if Stop() is called explicitly before destruction.
The destructor calls Stop(), which stops the scheduler and joins the thread. If Stop() is called explicitly (e.g., during shutdown), then the destructor will call it again. While CScheduler::stop() is likely idempotent, std::thread::join() on an already-joined thread is undefined behavior.
🐛 Proposed fix: Guard against double-join
void ChainLockSigner::Stop()
{
m_scheduler->stop();
- if (m_scheduler_thread->joinable()) m_scheduler_thread->join();
+ if (m_scheduler_thread && m_scheduler_thread->joinable()) {
+ m_scheduler_thread->join();
+ }
}Also applies to: 55-59
🤖 Prompt for AI Agents
In `@src/chainlock/signing.cpp` at line 41, Destructor
ChainLockSigner::~ChainLockSigner() calls Stop(), which can lead to undefined
behavior if Stop() was already called and its std::thread was joined; modify
Stop() (the ChainLockSigner::Stop method) to guard thread joining by checking
std::thread::joinable() before calling join (and/or set a boolean/is_running
flag that Stop and the destructor check) so double-call to Stop() is safe;
ensure CScheduler::stop() is still called but only join the member thread when
joinable to avoid double-join in the destructor and other callers.
…stent Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
2 participants
Issue being fixed or feature implemented
Currently, there are several blockers that prevent pulling consensus code out from various handlers, schedulers, mempool, etc. One of them is
CChainLocksHandlerwhich does too many things.CChainLocksHandlerdepends on the mempool and — because it is part ofLLMQContext— it makesLLMQContextdepend on the mempool (which is an unexpected dependency)CChainLocksHandlerand the initialization of these objects. See:CChainLocksHandler::Start(const llmq::CInstantSendManager& isman)and
CInstantSendManager::CInstantSendManager(CChainLocksHandler& _clhandler, ...)CChainLocksHandlerdepends on masternode-only code (chainlock::ChainLockSigner). It is hidden behind the interfacechainlock::ChainLockSignerParent, but this increases implementation complexityCChainLocksHandleris used for blocks validation (clhandler.HasConflictingChainLock), but at the same timeCChainLocksHandlercan switch the chain tip itself via its background worker (see calls toEnforceBestChainLock). This creates another dependency loop that prevents independent initialization ofCChainLocksHandlerand the Chain State.What was done?
This PR introduces a new lightweight interface
chainlock::Chainlockschainlock::Chainlocksknows about the chain state and the best known chainlock, but it does not enforce chainlocks, has no dependency on mempool or transactions, and keeps no permanent on-disk state (no dependency on evodb).It does depend on
SporkManager(to know whether chainlocks are enabled/disabled) and provides a clean interface over it.llmq::CChainLocksHandler(chainlock::ChainlocksHandlerin future commits) is the component that actually changes Chain State by enforcing the best tip, tracks transaction's times and runs a background thread.It is responsible for processing newly signed chainlocks and related network messages.
However, it no longer handles any masternode-related logic or signing — that code was fully moved to
llmq::ChainLockSigner.Other less important but noticeable changes:
clhandlernow uses its own subscriptions to validation notifications (no longer goes throughdsnotificationinterface)llmq::CChainLocksHandlerwas renamed and moved to a different namespace (chainlock::ChainlocksHandler) since it no longer belongs tollmq::clhandlerwas removed fromllmq-ctxand its initialization can now be skipped completely [as required for kernel project]; onlyChainLockSignerandPeerManagerstill strongly depend on itclhandlerno longer depends onCQuorumManagerNoticeable, some circular dependencies over codebase has been untangled by this PR:
See linter's exception list for details.
After this PR the next step to apply similar refactoring to
CInstantSendManagerwhich does too many things the similar to CChainLocksHandler: keeps track of instant-send locked transactions [as expected], but also knows about masternode's code [seeinstantsend::InstantSendSigner], removes conflicting transactions from mempool, invalidates blocks in Chain State (seeCInstantSendManager::ResolveBlockConflicts). CInstantSendManager creates logical dependencies over masternode's signer; over chainstate; over mempool and finally there are the same issues with initialization ofismanasclhandlerhad.How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: