Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/chainlock/chainlock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,30 @@ void Chainlocks::AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew
bestChainLockBlockIndex = pindexNew;
}
}

void Chainlocks::QueueCoinbaseChainLock(const chainlock::ChainLockSig& clsig)
{
LOCK(cs);

if (!IsEnabled()) {
return;
}

// Only queue if it's potentially newer than what we have
if (!bestChainLock.IsNull() && clsig.getHeight() <= bestChainLock.getHeight()) {
return;
}
Comment on lines +184 to +186
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same-height chainlock rejection may skip valid fork chainlocks.

The condition clsig.getHeight() <= bestChainLock.getHeight() (line 184) rejects chainlocks at the same height as the current best. During a chain fork, competing blocks at the same height can both have valid chainlocks, and the coinbase chainlock for the correct fork should still be queued and processed. Using <= instead of < may cause the node to skip recovery of a valid chainlock when the incoming coinbase chainlock is at the same height but for a different block hash than bestChainLock.

Consider changing the condition to < or adding a block-hash inequality check.

Proposed fix
-    if (!bestChainLock.IsNull() && clsig.getHeight() <= bestChainLock.getHeight()) {
+    if (!bestChainLock.IsNull() && clsig.getHeight() < bestChainLock.getHeight()) {
         return;
     }

Alternatively, if same-height entries must be filtered, check the block hash as well:

-    if (!bestChainLock.IsNull() && clsig.getHeight() <= bestChainLock.getHeight()) {
+    if (!bestChainLock.IsNull() && 
+        (clsig.getHeight() < bestChainLock.getHeight() ||
+         (clsig.getHeight() == bestChainLock.getHeight() && clsig.getBlockHash() == bestChainLock.getBlockHash()))) {
         return;
     }
📝 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.

Suggested change
if (!bestChainLock.IsNull() && clsig.getHeight() <= bestChainLock.getHeight()) {
return;
}
if (!bestChainLock.IsNull() && clsig.getHeight() < bestChainLock.getHeight()) {
return;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/chainlock/chainlock.cpp` around lines 184 - 186, The current check using
if (!bestChainLock.IsNull() && clsig.getHeight() <= bestChainLock.getHeight())
wrongly rejects incoming chainlocks at the same height even when they target a
different block; change the condition to only reject strictly lower heights or,
if you want to continue rejecting duplicates, reject equal heights only when the
block hash matches: update the test to if (!bestChainLock.IsNull() &&
(clsig.getHeight() < bestChainLock.getHeight() || (clsig.getHeight() ==
bestChainLock.getHeight() && clsig.getBlockHash() ==
bestChainLock.getBlockHash()))) return; so use clsig.getHeight(),
bestChainLock.getHeight(), and clsig.getBlockHash()/bestChainLock.getBlockHash()
to locate and implement the fix.


pendingCoinbaseChainLocks.push_back(clsig);
}

std::vector<chainlock::ChainLockSig> Chainlocks::DrainPendingCoinbaseChainLocks()
{
LOCK(cs);
// O(1) zero-copy swap rather than per-element copy.
std::vector<chainlock::ChainLockSig> drained;
drained.swap(pendingCoinbaseChainLocks);
return drained;
}

} // namespace chainlock
12 changes: 12 additions & 0 deletions src/chainlock/chainlock.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include <sync.h>
#include <uint256.h>

#include <vector>

class CBlockIndex;
class CSporkManager;
class uint256;
Expand Down Expand Up @@ -57,6 +59,9 @@ class Chainlocks

chainlock::ChainLockSig bestChainLockWithKnownBlock GUARDED_BY(cs);

// Queue for coinbase chainlocks pending asynchronous processing by ChainlockHandler
std::vector<chainlock::ChainLockSig> pendingCoinbaseChainLocks GUARDED_BY(cs);

public:
Chainlocks(const CSporkManager& sporkman);

Expand All @@ -82,6 +87,13 @@ class Chainlocks
void AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew) EXCLUSIVE_LOCKS_REQUIRED(!cs);

void ResetChainlock() EXCLUSIVE_LOCKS_REQUIRED(!cs);

// Queue a coinbase chainlock for asynchronous processing by the ChainlockHandler.
// Called during block validation to avoid blocking the main validation flow.
void QueueCoinbaseChainLock(const chainlock::ChainLockSig& clsig) EXCLUSIVE_LOCKS_REQUIRED(!cs);

// Drain pending coinbase chainlocks for processing by ChainlockHandler.
std::vector<chainlock::ChainLockSig> DrainPendingCoinbaseChainLocks() EXCLUSIVE_LOCKS_REQUIRED(!cs);
};

} // namespace chainlock
Expand Down
31 changes: 30 additions & 1 deletion src/chainlock/handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,46 @@ ChainlockHandler::~ChainlockHandler()
scheduler_thread->join();
}

void ChainlockHandler::Start()
void ChainlockHandler::Start(const llmq::CQuorumManager& qman)
{
scheduler->scheduleEvery(
[&]() {
CheckActiveState();
ProcessPendingCoinbaseChainLocks(qman);
EnforceBestChainLock();
Cleanup();
},
std::chrono::seconds{5});
}

void ChainlockHandler::ProcessPendingCoinbaseChainLocks(const llmq::CQuorumManager& qman)
{
AssertLockNotHeld(cs);
AssertLockNotHeld(cs_main);

if (!isEnabled) {
return;
}

auto pending = m_chainlocks.DrainPendingCoinbaseChainLocks();
// Process newest first (LIFO): once a newer chainlock is accepted, older
// ones short-circuit on the cheap height check below.
for (auto it = pending.rbegin(); it != pending.rend(); ++it) {
const auto& clsig = *it;
// Cheap height check before computing a hash; during reindex this
// skips ~all queued entries without paying for SHA256.
if (clsig.getHeight() <= m_chainlocks.GetBestChainLockHeight()) {
continue;
}
const uint256 hash = ::SerializeHash(clsig);
if (WITH_LOCK(cs, return seenChainLocks.count(hash) != 0)) {
continue;
}
// Process as if we discovered it locally (from = -1 means internal/coinbase).
(void)ProcessNewChainLock(-1, clsig, qman, hash);
}
}

void ChainlockHandler::Stop() { scheduler->stop(); }

bool ChainlockHandler::AlreadyHave(const CInv& inv) const
Expand Down
3 changes: 2 additions & 1 deletion src/chainlock/handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class ChainlockHandler final : public CValidationInterface
const CMasternodeSync& mn_sync);
~ChainlockHandler();

void Start();
void Start(const llmq::CQuorumManager& qman);
void Stop();

bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
Expand Down Expand Up @@ -100,6 +100,7 @@ class ChainlockHandler final : public CValidationInterface

private:
void Cleanup() EXCLUSIVE_LOCKS_REQUIRED(!cs);
void ProcessPendingCoinbaseChainLocks(const llmq::CQuorumManager& qman) EXCLUSIVE_LOCKS_REQUIRED(!cs);
};
} // namespace chainlock

Expand Down
2 changes: 1 addition & 1 deletion src/evo/chainhelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ CChainstateHelper::CChainstateHelper(CEvoDB& evodb, CDeterministicMNManager& dmn
llmq::CInstantSendManager& isman, llmq::CQuorumBlockProcessor& qblockman,
llmq::CQuorumSnapshotManager& qsnapman, const ChainstateManager& chainman,
const Consensus::Params& consensus_params, const CMasternodeSync& mn_sync,
const chainlock::Chainlocks& chainlocks, const llmq::CQuorumManager& qman) :
chainlock::Chainlocks& chainlocks, const llmq::CQuorumManager& qman) :
isman{isman},
credit_pool_manager{std::make_unique<CCreditPoolManager>(evodb, chainman)},
m_chainlocks{chainlocks},
Expand Down
4 changes: 2 additions & 2 deletions src/evo/chainhelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class CChainstateHelper

public:
const std::unique_ptr<CCreditPoolManager> credit_pool_manager;
const chainlock::Chainlocks& m_chainlocks;
chainlock::Chainlocks& m_chainlocks;
const std::unique_ptr<CMNHFManager> ehf_manager;
const std::unique_ptr<CMNPaymentsProcessor> mn_payments;
const std::unique_ptr<CSpecialTxProcessor> special_tx;
Expand All @@ -54,7 +54,7 @@ class CChainstateHelper
llmq::CInstantSendManager& isman, llmq::CQuorumBlockProcessor& qblockman,
llmq::CQuorumSnapshotManager& qsnapman, const ChainstateManager& chainman,
const Consensus::Params& consensus_params, const CMasternodeSync& mn_sync,
const chainlock::Chainlocks& chainlocks, const llmq::CQuorumManager& qman);
chainlock::Chainlocks& chainlocks, const llmq::CQuorumManager& qman);
~CChainstateHelper();

/** Passthrough functions to chainlock::Chainlocks */
Expand Down
11 changes: 11 additions & 0 deletions src/evo/specialtxman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,17 @@ bool CSpecialTxProcessor::ProcessSpecialTxsInBlock(const CBlock& block, const CB
return false;
}

// Queue the coinbase chainlock for asynchronous processing if it's valid
if (opt_cbTx->bestCLSignature.IsValid() && !fJustCheck) {
int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(opt_cbTx->bestCLHeightDiff) - 1;
const CBlockIndex* pindexCL = pindex->GetAncestor(curBlockCoinbaseCLHeight);
if (pindexCL) {
uint256 curBlockCoinbaseCLBlockHash = pindexCL->GetBlockHash();
chainlock::ChainLockSig clsig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, opt_cbTx->bestCLSignature);
m_chainlocks.QueueCoinbaseChainLock(clsig);
}
}

int64_t nTime6_3 = GetTimeMicros();
nTimeCbTxCL += nTime6_3 - nTime6_2;
LogPrint(BCLog::BENCHMARK, " - CheckCbTxBestChainlock: %.2fms [%.2fs]\n",
Expand Down
4 changes: 2 additions & 2 deletions src/evo/specialtxman.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ class CSpecialTxProcessor
llmq::CQuorumSnapshotManager& m_qsnapman;
const ChainstateManager& m_chainman;
const Consensus::Params& m_consensus_params;
const chainlock::Chainlocks& m_chainlocks;
chainlock::Chainlocks& m_chainlocks;
const llmq::CQuorumManager& m_qman;

public:
explicit CSpecialTxProcessor(CCreditPoolManager& cpoolman, CDeterministicMNManager& dmnman, CMNHFManager& mnhfman,
llmq::CQuorumBlockProcessor& qblockman, llmq::CQuorumSnapshotManager& qsnapman,
const ChainstateManager& chainman, const Consensus::Params& consensus_params,
const chainlock::Chainlocks& chainlocks, const llmq::CQuorumManager& qman) :
chainlock::Chainlocks& chainlocks, const llmq::CQuorumManager& qman) :
m_cpoolman(cpoolman),
m_dmnman{dmnman},
m_mnhfman{mnhfman},
Expand Down
2 changes: 1 addition & 1 deletion src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2338,7 +2338,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// ********************************************************* Step 10a: schedule Dash-specific tasks

node.peerman->StartHandlers();
node.clhandler->Start();
node.clhandler->Start(*node.llmq_ctx->qman);

node.scheduler->scheduleEvery(std::bind(&CNetFulfilledRequestManager::DoMaintenance, std::ref(*node.netfulfilledman)), std::chrono::minutes{1});
node.scheduler->scheduleEvery(std::bind(&CMasternodeUtils::DoMaintenance, std::ref(*node.connman), std::ref(*node.dmnman), std::ref(*node.mn_sync), node.cj_walletman.get()), std::chrono::minutes{1});
Expand Down
2 changes: 1 addition & 1 deletion src/test/llmq_chainlock_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include <streams.h>
#include <util/strencodings.h>

#include <chainlock/chainlock.h>
#include <chainlock/clsig.h>

#include <boost/test/unit_test.hpp>

Expand Down
72 changes: 71 additions & 1 deletion test/functional/feature_llmq_chainlocks.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

from test_framework.messages import CBlock, CCbTx
from test_framework.test_framework import DashTestFramework
from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync
from test_framework.util import assert_equal, assert_greater_than, assert_raises_rpc_error, force_finish_mnsync

import time

Expand Down Expand Up @@ -254,6 +254,9 @@ def test_cb(self):
self.log.info("Test bestCLHeightDiff restrictions")
self.test_bestCLHeightDiff()

self.log.info("Test coinbase chainlock recovery")
self.test_coinbase_chainlock_recovery()

def create_chained_txs(self, node, amount):
txid = node.sendtoaddress(node.getnewaddress(), amount)
tx = node.getrawtransaction(txid, 1)
Expand Down Expand Up @@ -357,6 +360,73 @@ def test_bestCLHeightDiff(self):
self.reconnect_isolated_node(1, 0)
self.sync_all()

def test_coinbase_chainlock_recovery(self):
"""
Test that nodes can learn about chainlocks from coinbase transactions
when they miss the P2P broadcast.

This verifies the async chainlock queueing and processing mechanism.
"""
self.log.info("Testing coinbase chainlock recovery from submitted blocks...")

# Isolate node4 before creating a chainlock
self.isolate_node(4)

# Mine one block on nodes 0-3 and wait for it to be chainlocked
cl_block_hash = self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_blocks(self.nodes[0:4]))[0]
self.wait_for_chainlocked_block(self.nodes[0], cl_block_hash, timeout=15)
cl_height = self.nodes[0].getblockcount()

# Mine another block - its coinbase should contain the chainlock
new_block_hash = self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_blocks(self.nodes[0:4]))[0]

# Verify the new block's coinbase contains the chainlock for cl_block_hash
cbtx = self.nodes[0].getblock(new_block_hash, 2)["cbTx"]
# CbTx should include chainlock fields
assert_greater_than(int(cbtx["version"]), 2)
# CbTx should reference immediately previous block
assert_equal(int(cbtx["bestCLHeightDiff"]), 0)

# Verify the chainlock in coinbase matches our saved block
cb_cl_height = int(cbtx["height"]) - int(cbtx["bestCLHeightDiff"]) - 1
assert_equal(cb_cl_height, cl_height)
cb_cl_block_hash = self.nodes[0].getblockhash(cb_cl_height)
assert_equal(cb_cl_block_hash, cl_block_hash)

# Now submit both blocks to isolated node4 via submitblock (NOT via P2P)
# This way node4 gets the blocks but NOT the chainlock P2P message
cl_block_hex = self.nodes[0].getblock(cl_block_hash, 0)
self.nodes[4].submitblock(cl_block_hex)

new_block_hex = self.nodes[0].getblock(new_block_hash, 0)
result = self.nodes[4].submitblock(new_block_hex)
assert_equal(result, None)
assert_equal(self.nodes[4].getbestblockhash(), new_block_hash)

# Verify node4 has the blocks but NOT the chainlock (missed P2P message)
node4_block = self.nodes[4].getblock(cl_block_hash)
assert not node4_block["chainlock"], "Node4 should not have chainlock yet (no P2P)"

# At this point:
# - Node4 has both blocks
# - Node4 has NOT received chainlock via P2P
# - Node4 HAS seen the chainlock in the coinbase of new_block_hash
# - The chainlock should be queued for async processing

# Trigger scheduler to process pending coinbase chainlocks
# The scheduler runs every 5 seconds, so advancing by 6 seconds ensures it runs
self.log.info("Triggering async chainlock processing from coinbase...")
self.nodes[4].mockscheduler(6)

# Verify node4 learned about the chainlock from the coinbase
self.wait_for_chainlocked_block(self.nodes[4], cl_block_hash, timeout=5)
Comment on lines +416 to +422
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: mockscheduler does not advance ChainlockHandler's private scheduler

This test says mockscheduler(6) triggers async coinbase ChainLock processing, but the RPC advances NodeContext::scheduler only. ChainlockHandler owns a separate private CScheduler and thread, so ProcessPendingCoinbaseChainLocks() only runs when that real 5-second scheduler ticks. The test can therefore pass because the real thread happened to run within the wait, fail if it does not, or even fail the earlier 'should not have chainlock yet' assertion if the private scheduler runs before that check; it needs either a test hook/RPC for the ChainlockHandler scheduler or expectations that explicitly account for the real private scheduler interval.

source: ['claude-general', 'codex-general']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `test/functional/feature_llmq_chainlocks.py`:
- [SUGGESTION] lines 416-422: mockscheduler does not advance ChainlockHandler's private scheduler
  This test says mockscheduler(6) triggers async coinbase ChainLock processing, but the RPC advances NodeContext::scheduler only. ChainlockHandler owns a separate private CScheduler and thread, so ProcessPendingCoinbaseChainLocks() only runs when that real 5-second scheduler ticks. The test can therefore pass because the real thread happened to run within the wait, fail if it does not, or even fail the earlier 'should not have chainlock yet' assertion if the private scheduler runs before that check; it needs either a test hook/RPC for the ChainlockHandler scheduler or expectations that explicitly account for the real private scheduler interval.


self.log.info("Node successfully recovered chainlock from coinbase (not P2P)")

# Reconnect and verify everything is consistent
self.reconnect_isolated_node(4, 0)
self.sync_blocks()


if __name__ == '__main__':
LLMQChainLocksTest().main()
Loading