From c6e99fb3552fbf40d544865ca9a53936b0b96dfa Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 12 Aug 2025 09:30:03 +0000 Subject: [PATCH 1/9] chore: apply most `clang-format` suggestions --- src/chainlock/chainlock.cpp | 86 +++++++++++++++++++++---------------- src/chainlock/chainlock.h | 4 +- src/chainlock/signing.cpp | 16 ++++--- 3 files changed, 61 insertions(+), 45 deletions(-) diff --git a/src/chainlock/chainlock.cpp b/src/chainlock/chainlock.cpp index ea1d179dbd312..c4acbee89a885 100644 --- a/src/chainlock/chainlock.cpp +++ b/src/chainlock/chainlock.cpp @@ -71,15 +71,17 @@ void CChainLocksHandler::Start(const llmq::CInstantSendManager& isman) if (m_signer) { m_signer->Start(); } - scheduler->scheduleEvery([&]() { - CheckActiveState(); - EnforceBestChainLock(); - Cleanup(); - // regularly retry signing the current chaintip as it might have failed before due to missing islocks - if (m_signer) { - m_signer->TrySignChainTip(isman); - } - }, std::chrono::seconds{5}); + scheduler->scheduleEvery( + [&]() { + CheckActiveState(); + EnforceBestChainLock(); + Cleanup(); + // regularly retry signing the current chaintip as it might have failed before due to missing islocks + if (m_signer) { + m_signer->TrySignChainTip(isman); + } + }, + std::chrono::seconds{5}); } void CChainLocksHandler::Stop() @@ -142,7 +144,8 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro } if (const auto ret = VerifyChainLock(clsig); ret != VerifyRecSigStatus::Valid) { - LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- invalid CLSIG (%s), status=%d peer=%d\n", __func__, clsig.ToString(), ToUnderlying(ret), from); + LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- invalid CLSIG (%s), status=%d peer=%d\n", __func__, + clsig.ToString(), ToUnderlying(ret), from); if (from != -1) { return MisbehavingError{10}; } @@ -157,11 +160,10 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro bestChainLock = clsig; if (pindex != nullptr) { - if (pindex->nHeight != clsig.getHeight()) { // Should not happen, same as the conflict check from above. LogPrintf("CChainLocksHandler::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n", - __func__, clsig.ToString(), pindex->nHeight); + __func__, clsig.ToString(), pindex->nHeight); // Note: not relaying clsig here return {}; } @@ -181,13 +183,15 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro return clsigInv; } - scheduler->scheduleFromNow([&]() { - CheckActiveState(); - EnforceBestChainLock(); - }, std::chrono::seconds{0}); + scheduler->scheduleFromNow( + [&]() { + CheckActiveState(); + EnforceBestChainLock(); + }, + std::chrono::seconds{0}); - LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- processed new CLSIG (%s), peer=%d\n", - __func__, clsig.ToString(), from); + LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- processed new CLSIG (%s), peer=%d\n", __func__, + clsig.ToString(), from); return clsigInv; } @@ -196,7 +200,8 @@ void CChainLocksHandler::AcceptedBlockHeader(gsl::not_null p LOCK(cs); if (pindexNew->GetBlockHash() == bestChainLock.getBlockHash()) { - LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- block header %s came in late, updating and enforcing\n", __func__, pindexNew->GetBlockHash().ToString()); + LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- block header %s came in late, updating and enforcing\n", + __func__, pindexNew->GetBlockHash().ToString()); if (bestChainLock.getHeight() != pindexNew->nHeight) { // Should not happen, same as the conflict check from ProcessNewChainLock. @@ -220,15 +225,17 @@ void CChainLocksHandler::UpdatedBlockTip(const llmq::CInstantSendManager& isman) // EnforceBestChainLock switching chains. // atomic[If tryLockChainTipScheduled is false, do (set it to true] and schedule signing). if (bool expected = false; tryLockChainTipScheduled.compare_exchange_strong(expected, true)) { - scheduler->scheduleFromNow([&]() { - CheckActiveState(); - EnforceBestChainLock(); - Cleanup(); - if (m_signer) { - m_signer->TrySignChainTip(isman); - } - tryLockChainTipScheduled = false; - }, std::chrono::seconds{0}); + scheduler->scheduleFromNow( + [&]() { + CheckActiveState(); + EnforceBestChainLock(); + Cleanup(); + if (m_signer) { + m_signer->TrySignChainTip(isman); + } + tryLockChainTipScheduled = false; + }, + std::chrono::seconds{0}); } } @@ -281,7 +288,8 @@ void CChainLocksHandler::BlockConnected(const std::shared_ptr& pbl } } -void CChainLocksHandler::BlockDisconnected(const std::shared_ptr& pblock, gsl::not_null pindexDisconnected) +void CChainLocksHandler::BlockDisconnected(const std::shared_ptr& pblock, + gsl::not_null pindexDisconnected) { if (m_signer) { m_signer->EraseFromBlockHashTxidMap(pindexDisconnected->GetBlockHash()); @@ -340,17 +348,21 @@ void CChainLocksHandler::EnforceBestChainLock() // Go backwards through the chain referenced by clsig until we find a block that is part of the main chain. // For each of these blocks, check if there are children that are NOT part of the chain referenced by clsig // and mark all of them as conflicting. - LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- enforcing block %s via CLSIG (%s)\n", __func__, pindex->GetBlockHash().ToString(), clsig->ToString()); + LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- enforcing block %s via CLSIG (%s)\n", __func__, + pindex->GetBlockHash().ToString(), clsig->ToString()); m_chainstate.EnforceBlock(dummy_state, pindex); - if (/*activateNeeded =*/ WITH_LOCK(::cs_main, return m_chainstate.m_chain.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight)) != currentBestChainLockBlockIndex) { + if (/*activateNeeded =*/WITH_LOCK(::cs_main, return m_chainstate.m_chain.Tip()->GetAncestor( + currentBestChainLockBlockIndex->nHeight)) != + currentBestChainLockBlockIndex) { if (!m_chainstate.ActivateBestChain(dummy_state)) { LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, dummy_state.ToString()); return; } LOCK(::cs_main); - if (m_chainstate.m_chain.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex) { + if (m_chainstate.m_chain.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != + currentBestChainLockBlockIndex) { return; } } @@ -371,7 +383,8 @@ VerifyRecSigStatus CChainLocksHandler::VerifyChainLock(const chainlock::ChainLoc const auto llmqType = Params().GetConsensus().llmqTypeChainLocks; const uint256 nRequestId = ::SerializeHash(std::make_pair(chainlock::CLSIG_REQUESTID_PREFIX, clsig.getHeight())); - return llmq::VerifyRecoveredSig(llmqType, m_chainstate.m_chain, qman, clsig.getHeight(), nRequestId, clsig.getBlockHash(), clsig.getSig()); + return llmq::VerifyRecoveredSig(llmqType, m_chainstate.m_chain, qman, clsig.getHeight(), nRequestId, + clsig.getBlockHash(), clsig.getSig()); } bool CChainLocksHandler::HasChainLock(int nHeight, const uint256& blockHash) const @@ -438,7 +451,7 @@ void CChainLocksHandler::Cleanup() { LOCK(cs); - for (auto it = seenChainLocks.begin(); it != seenChainLocks.end(); ) { + for (auto it = seenChainLocks.begin(); it != seenChainLocks.end();) { if (TicksSinceEpoch(SystemClock::now()) - it->second >= CLEANUP_SEEN_TIMEOUT) { it = seenChainLocks.erase(it); } else { @@ -459,14 +472,15 @@ void CChainLocksHandler::Cleanup() LOCK(::cs_main); LOCK2(mempool.cs, cs); // need mempool.cs due to GetTransaction calls - for (auto it = txFirstSeenTime.begin(); it != txFirstSeenTime.end(); ) { + for (auto it = txFirstSeenTime.begin(); it != txFirstSeenTime.end();) { uint256 hashBlock; if (auto tx = GetTransaction(nullptr, &mempool, it->first, Params().GetConsensus(), hashBlock); !tx) { // tx has vanished, probably due to conflicts it = txFirstSeenTime.erase(it); } else if (!hashBlock.IsNull()) { const auto* pindex = m_chainstate.m_blockman.LookupBlockIndex(hashBlock); - if (m_chainstate.m_chain.Tip()->GetAncestor(pindex->nHeight) == pindex && m_chainstate.m_chain.Height() - pindex->nHeight >= 6) { + if (m_chainstate.m_chain.Tip()->GetAncestor(pindex->nHeight) == pindex && + m_chainstate.m_chain.Height() - pindex->nHeight >= 6) { // tx got confirmed >= 6 times, so we can stop keeping track of it it = txFirstSeenTime.erase(it); } else { diff --git a/src/chainlock/chainlock.h b/src/chainlock/chainlock.h index 69ba8130ae75b..b8b84b98e8c82 100644 --- a/src/chainlock/chainlock.h +++ b/src/chainlock/chainlock.h @@ -58,8 +58,8 @@ class CChainLocksHandler final : public chainlock::ChainLockSignerParent chainlock::ChainLockSig bestChainLock GUARDED_BY(cs); chainlock::ChainLockSig bestChainLockWithKnownBlock GUARDED_BY(cs); - 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}; std::unordered_map txFirstSeenTime GUARDED_BY(cs); diff --git a/src/chainlock/signing.cpp b/src/chainlock/signing.cpp index 0ef9aa550dfb2..89700d447795f 100644 --- a/src/chainlock/signing.cpp +++ b/src/chainlock/signing.cpp @@ -15,9 +15,9 @@ using node::ReadBlockFromDisk; namespace chainlock { -ChainLockSigner::ChainLockSigner(CChainState& chainstate, ChainLockSignerParent& clhandler, llmq::CSigningManager& sigman, - llmq::CSigSharesManager& shareman, CSporkManager& sporkman, - const CMasternodeSync& mn_sync) : +ChainLockSigner::ChainLockSigner(CChainState& chainstate, ChainLockSignerParent& clhandler, + llmq::CSigningManager& sigman, llmq::CSigSharesManager& shareman, + CSporkManager& sporkman, const CMasternodeSync& mn_sync) : m_chainstate{chainstate}, m_clhandler{clhandler}, m_sigman{sigman}, @@ -85,7 +85,8 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman) return; } - LogPrint(BCLog::CHAINLOCKS, "%s -- trying to sign %s, height=%d\n", __func__, pindex->GetBlockHash().ToString(), pindex->nHeight); + LogPrint(BCLog::CHAINLOCKS, "%s -- trying to sign %s, height=%d\n", __func__, pindex->GetBlockHash().ToString(), + pindex->nHeight); // When the new IX system is activated, we only try to ChainLock blocks which include safe transactions. A TX is // considered safe when it is islocked or at least known since 10 minutes (from mempool or block). These checks are @@ -114,8 +115,9 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman) for (const auto& txid : *txids) { if (!m_clhandler.IsTxSafeForMining(txid) && !isman.IsLocked(txid)) { - 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()); + LogPrint(BCLog::CHAINLOCKS, /* Continued */ + "%s -- not signing block %s due to TX %s not being islocked and not old enough.\n", + __func__, pindexWalk->GetBlockHash().ToString(), txid.ToString()); return; } } @@ -243,7 +245,7 @@ std::vector>> Ch AssertLockNotHeld(cs_signer); std::vector>> removed; LOCK2(::cs_main, cs_signer); - for (auto it = blockTxs.begin(); it != blockTxs.end(); ) { + for (auto it = blockTxs.begin(); it != blockTxs.end();) { const auto* pindex = m_chainstate.m_blockman.LookupBlockIndex(it->first); if (m_clhandler.HasChainLock(pindex->nHeight, pindex->GetBlockHash())) { removed.push_back(it->second); From 024b46617e2e6fc80eeee05356eb3ab74454eeae Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 11 Aug 2025 09:39:29 +0000 Subject: [PATCH 2/9] chore: move lock annotations in `chainlock.h` to the next line --- src/chainlock/chainlock.h | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/chainlock/chainlock.h b/src/chainlock/chainlock.h index b8b84b98e8c82..7e87a143115cf 100644 --- a/src/chainlock/chainlock.h +++ b/src/chainlock/chainlock.h @@ -76,9 +76,12 @@ class CChainLocksHandler final : public chainlock::ChainLockSignerParent void Start(const llmq::CInstantSendManager& isman); void Stop(); - bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs); - bool GetChainLockByHash(const uint256& hash, chainlock::ChainLockSig& ret) const EXCLUSIVE_LOCKS_REQUIRED(!cs); - chainlock::ChainLockSig GetBestChainLock() const EXCLUSIVE_LOCKS_REQUIRED(!cs); + bool AlreadyHave(const CInv& inv) const + EXCLUSIVE_LOCKS_REQUIRED(!cs); + bool GetChainLockByHash(const uint256& hash, chainlock::ChainLockSig& ret) const + EXCLUSIVE_LOCKS_REQUIRED(!cs); + chainlock::ChainLockSig GetBestChainLock() const + EXCLUSIVE_LOCKS_REQUIRED(!cs); void UpdateTxFirstSeenMap(const std::unordered_set& tx, const int64_t& time) override EXCLUSIVE_LOCKS_REQUIRED(!cs); @@ -86,13 +89,19 @@ class CChainLocksHandler final : public chainlock::ChainLockSignerParent const uint256& hash) override EXCLUSIVE_LOCKS_REQUIRED(!cs); - void AcceptedBlockHeader(gsl::not_null pindexNew) EXCLUSIVE_LOCKS_REQUIRED(!cs); + void AcceptedBlockHeader(gsl::not_null pindexNew) + EXCLUSIVE_LOCKS_REQUIRED(!cs); void UpdatedBlockTip(const llmq::CInstantSendManager& isman); - void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) EXCLUSIVE_LOCKS_REQUIRED(!cs); - void BlockConnected(const std::shared_ptr& pblock, gsl::not_null pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs); - void BlockDisconnected(const std::shared_ptr& pblock, gsl::not_null pindexDisconnected) EXCLUSIVE_LOCKS_REQUIRED(!cs); - void CheckActiveState() EXCLUSIVE_LOCKS_REQUIRED(!cs); - void EnforceBestChainLock() EXCLUSIVE_LOCKS_REQUIRED(!cs); + void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + void BlockConnected(const std::shared_ptr& pblock, gsl::not_null pindex) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + void BlockDisconnected(const std::shared_ptr& pblock, gsl::not_null pindexDisconnected) + EXCLUSIVE_LOCKS_REQUIRED(!cs); + void CheckActiveState() + EXCLUSIVE_LOCKS_REQUIRED(!cs); + void EnforceBestChainLock() + EXCLUSIVE_LOCKS_REQUIRED(!cs); bool HasChainLock(int nHeight, const uint256& blockHash) const override EXCLUSIVE_LOCKS_REQUIRED(!cs); @@ -107,7 +116,8 @@ class CChainLocksHandler final : public chainlock::ChainLockSignerParent [[nodiscard]] bool IsEnabled() const override { return isEnabled; } private: - void Cleanup() EXCLUSIVE_LOCKS_REQUIRED(!cs); + void Cleanup() + EXCLUSIVE_LOCKS_REQUIRED(!cs); }; bool AreChainLocksEnabled(const CSporkManager& sporkman); From b051c22c9ea1a48b8a00b49ad7ff6675d8949672 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 15 Aug 2025 10:25:55 +0000 Subject: [PATCH 3/9] refactor: consolidate `CLSIG_REQUESTID_PREFIX` usage to `clsig.cpp` --- src/chainlock/chainlock.cpp | 2 +- src/chainlock/clsig.cpp | 9 ++++++++- src/chainlock/clsig.h | 5 +++-- src/chainlock/signing.cpp | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/chainlock/chainlock.cpp b/src/chainlock/chainlock.cpp index c4acbee89a885..b53a73bcc217e 100644 --- a/src/chainlock/chainlock.cpp +++ b/src/chainlock/chainlock.cpp @@ -381,7 +381,7 @@ void CChainLocksHandler::EnforceBestChainLock() VerifyRecSigStatus CChainLocksHandler::VerifyChainLock(const chainlock::ChainLockSig& clsig) const { const auto llmqType = Params().GetConsensus().llmqTypeChainLocks; - const uint256 nRequestId = ::SerializeHash(std::make_pair(chainlock::CLSIG_REQUESTID_PREFIX, clsig.getHeight())); + const uint256 nRequestId = chainlock::GenSigRequestId(clsig.getHeight()); return llmq::VerifyRecoveredSig(llmqType, m_chainstate.m_chain, qman, clsig.getHeight(), nRequestId, clsig.getBlockHash(), clsig.getSig()); diff --git a/src/chainlock/clsig.cpp b/src/chainlock/clsig.cpp index 163c8bad52715..e781a2eb10ac3 100644 --- a/src/chainlock/clsig.cpp +++ b/src/chainlock/clsig.cpp @@ -6,8 +6,10 @@ #include +#include + namespace chainlock { -const std::string CLSIG_REQUESTID_PREFIX = "clsig"; +static constexpr std::string_view CLSIG_REQUESTID_PREFIX{"clsig"}; ChainLockSig::ChainLockSig() = default; ChainLockSig::~ChainLockSig() = default; @@ -23,4 +25,9 @@ std::string ChainLockSig::ToString() const { return strprintf("ChainLockSig(nHeight=%d, blockHash=%s)", nHeight, blockHash.ToString()); } + +uint256 GenSigRequestId(const int32_t nHeight) +{ + return ::SerializeHash(std::make_pair(CLSIG_REQUESTID_PREFIX, nHeight)); +} } // namespace chainlock diff --git a/src/chainlock/clsig.h b/src/chainlock/clsig.h index 9efd399c27899..6cd532ce62488 100644 --- a/src/chainlock/clsig.h +++ b/src/chainlock/clsig.h @@ -13,8 +13,6 @@ #include namespace chainlock { -extern const std::string CLSIG_REQUESTID_PREFIX; - struct ChainLockSig { private: int32_t nHeight{-1}; @@ -38,6 +36,9 @@ struct ChainLockSig { READWRITE(obj.nHeight, obj.blockHash, obj.sig); } }; + +//! Generate clsig request ID with block height +uint256 GenSigRequestId(const int32_t nHeight); } // namespace chainlock #endif // BITCOIN_CHAINLOCK_CLSIG_H diff --git a/src/chainlock/signing.cpp b/src/chainlock/signing.cpp index 89700d447795f..3d6d0ba738a27 100644 --- a/src/chainlock/signing.cpp +++ b/src/chainlock/signing.cpp @@ -126,7 +126,7 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman) } } - uint256 requestId = ::SerializeHash(std::make_pair(CLSIG_REQUESTID_PREFIX, pindex->nHeight)); + uint256 requestId = GenSigRequestId(pindex->nHeight); uint256 msgHash = pindex->GetBlockHash(); if (m_clhandler.GetBestChainLockHeight() >= pindex->nHeight) { From 4a744c7a55baa4e13ed1452a73881c137ce64b10 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 11 Aug 2025 11:23:21 +0000 Subject: [PATCH 4/9] refactor: use `std::chrono` for time variables, reduce resolution --- src/chainlock/chainlock.cpp | 20 ++++++++++---------- src/chainlock/chainlock.h | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/chainlock/chainlock.cpp b/src/chainlock/chainlock.cpp index b53a73bcc217e..d83a055dd2e34 100644 --- a/src/chainlock/chainlock.cpp +++ b/src/chainlock/chainlock.cpp @@ -32,10 +32,10 @@ using node::GetTransaction; namespace llmq { namespace { -static constexpr int64_t CLEANUP_INTERVAL{1000 * 30}; -static constexpr int64_t CLEANUP_SEEN_TIMEOUT{24 * 60 * 60 * 1000}; +static constexpr auto CLEANUP_INTERVAL{30s}; +static constexpr auto CLEANUP_SEEN_TIMEOUT{24h}; //! How long to wait for islocks until we consider a block with non-islocked TXs to be safe to sign -static constexpr int64_t WAIT_FOR_ISLOCK_TIMEOUT{10 * 60}; +static constexpr auto WAIT_FOR_ISLOCK_TIMEOUT{10min}; } // anonymous namespace bool AreChainLocksEnabled(const CSporkManager& sporkman) @@ -133,7 +133,7 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro { LOCK(cs); - if (!seenChainLocks.emplace(hash, TicksSinceEpoch(SystemClock::now())).second) { + if (!seenChainLocks.emplace(hash, GetTime()).second) { return {}; } @@ -305,16 +305,16 @@ int32_t CChainLocksHandler::GetBestChainLockHeight() const bool CChainLocksHandler::IsTxSafeForMining(const uint256& txid) const { - int64_t txAge = 0; + auto tx_age{0s}; { LOCK(cs); auto it = txFirstSeenTime.find(txid); if (it != txFirstSeenTime.end()) { - txAge = GetTime().count() - it->second; + tx_age = GetTime() - it->second; } } - return txAge >= WAIT_FOR_ISLOCK_TIMEOUT; + return tx_age >= WAIT_FOR_ISLOCK_TIMEOUT; } // WARNING: cs_main and cs should not be held! @@ -444,15 +444,15 @@ void CChainLocksHandler::Cleanup() return; } - if (TicksSinceEpoch(SystemClock::now()) - lastCleanupTime < CLEANUP_INTERVAL) { + if (GetTime() - lastCleanupTime.load() < CLEANUP_INTERVAL) { return; } - lastCleanupTime = TicksSinceEpoch(SystemClock::now()); + lastCleanupTime = GetTime(); { LOCK(cs); for (auto it = seenChainLocks.begin(); it != seenChainLocks.end();) { - if (TicksSinceEpoch(SystemClock::now()) - it->second >= CLEANUP_SEEN_TIMEOUT) { + if (GetTime() - it->second >= CLEANUP_SEEN_TIMEOUT) { it = seenChainLocks.erase(it); } else { ++it; diff --git a/src/chainlock/chainlock.h b/src/chainlock/chainlock.h index 7e87a143115cf..391463f0a76df 100644 --- a/src/chainlock/chainlock.h +++ b/src/chainlock/chainlock.h @@ -61,11 +61,11 @@ class CChainLocksHandler final : public chainlock::ChainLockSignerParent const CBlockIndex* bestChainLockBlockIndex GUARDED_BY(cs){nullptr}; const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs){nullptr}; - std::unordered_map txFirstSeenTime GUARDED_BY(cs); + std::unordered_map txFirstSeenTime GUARDED_BY(cs); - std::map seenChainLocks GUARDED_BY(cs); + std::map seenChainLocks GUARDED_BY(cs); - std::atomic lastCleanupTime{0}; + std::atomic lastCleanupTime{0s}; public: explicit CChainLocksHandler(CChainState& chainstate, CQuorumManager& _qman, CSigningManager& _sigman, From 95781465a51d11b894bd22d9d535897917311d88 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 11 Aug 2025 12:13:12 +0000 Subject: [PATCH 5/9] refactor: document `pindex` assumptions in chainlocks code --- src/chainlock/chainlock.cpp | 1 + src/chainlock/signing.cpp | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/chainlock/chainlock.cpp b/src/chainlock/chainlock.cpp index d83a055dd2e34..8d7beb9e70ee7 100644 --- a/src/chainlock/chainlock.cpp +++ b/src/chainlock/chainlock.cpp @@ -479,6 +479,7 @@ void CChainLocksHandler::Cleanup() it = txFirstSeenTime.erase(it); } else if (!hashBlock.IsNull()) { const auto* pindex = m_chainstate.m_blockman.LookupBlockIndex(hashBlock); + assert(pindex); // GetTransaction gave us that hashBlock, it should resolve to a valid block index if (m_chainstate.m_chain.Tip()->GetAncestor(pindex->nHeight) == pindex && m_chainstate.m_chain.Height() - pindex->nHeight >= 6) { // tx got confirmed >= 6 times, so we can stop keeping track of it diff --git a/src/chainlock/signing.cpp b/src/chainlock/signing.cpp index 3d6d0ba738a27..53c254cf0640d 100644 --- a/src/chainlock/signing.cpp +++ b/src/chainlock/signing.cpp @@ -56,7 +56,7 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman) const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Tip()); - if (pindex->pprev == nullptr) { + if (!pindex || !pindex->pprev) { return; } @@ -192,6 +192,9 @@ ChainLockSigner::BlockTxs::mapped_type ChainLockSigner::GetBlockTxs(const uint25 { LOCK(::cs_main); const auto* pindex = m_chainstate.m_blockman.LookupBlockIndex(blockHash); + if (!pindex) { + return nullptr; + } CBlock block; if (!ReadBlockFromDisk(block, pindex, Params().GetConsensus())) { return nullptr; @@ -247,7 +250,9 @@ std::vector>> Ch LOCK2(::cs_main, cs_signer); for (auto it = blockTxs.begin(); it != blockTxs.end();) { const auto* pindex = m_chainstate.m_blockman.LookupBlockIndex(it->first); - if (m_clhandler.HasChainLock(pindex->nHeight, pindex->GetBlockHash())) { + if (!pindex) { + it = blockTxs.erase(it); + } else if (m_clhandler.HasChainLock(pindex->nHeight, pindex->GetBlockHash())) { removed.push_back(it->second); it = blockTxs.erase(it); } else if (m_clhandler.HasConflictingChainLock(pindex->nHeight, pindex->GetBlockHash())) { From 25f05c16dd49b139179555ce661f00934ab1b630 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 19 Aug 2025 05:26:59 +0000 Subject: [PATCH 6/9] refactor: make unknown block clsig flow easier to follow --- src/chainlock/chainlock.cpp | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/chainlock/chainlock.cpp b/src/chainlock/chainlock.cpp index 8d7beb9e70ee7..1ef02d3f2ccfc 100644 --- a/src/chainlock/chainlock.cpp +++ b/src/chainlock/chainlock.cpp @@ -153,13 +153,14 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro } const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(clsig.getBlockHash())); + const CInv clsig_inv(MSG_CLSIG, hash); { LOCK(cs); bestChainLockHash = hash; bestChainLock = clsig; - if (pindex != nullptr) { + if (pindex) { if (pindex->nHeight != clsig.getHeight()) { // Should not happen, same as the conflict check from above. LogPrintf("CChainLocksHandler::%s -- height of CLSIG (%s) does not match the specified block's height (%d)\n", @@ -167,20 +168,13 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro // Note: not relaying clsig here return {}; } - bestChainLockWithKnownBlock = bestChainLock; bestChainLockBlockIndex = pindex; + } else { + // 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 clsig_inv; } - // else if (pindex == nullptr) - // Note: make sure to still relay clsig further. - } - - CInv clsigInv(MSG_CLSIG, hash); - - if (pindex == nullptr) { - // we don't know the block/header for this CLSIG yet, so bail out for now - // when the block or the header later comes in, we will enforce the correct chain - return clsigInv; } scheduler->scheduleFromNow( @@ -192,7 +186,8 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro LogPrint(BCLog::CHAINLOCKS, "CChainLocksHandler::%s -- processed new CLSIG (%s), peer=%d\n", __func__, clsig.ToString(), from); - return clsigInv; + + return clsig_inv; } void CChainLocksHandler::AcceptedBlockHeader(gsl::not_null pindexNew) From 7b4ee6b53ba23626d3af1dda58ae08e2fd52fc58 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 15 Aug 2025 10:27:13 +0000 Subject: [PATCH 7/9] trivial: document transaction confirmation safety threshold --- src/chainlock/chainlock.cpp | 4 ++-- src/chainlock/signing.cpp | 6 +++--- src/chainlock/signing.h | 3 +++ 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/chainlock/chainlock.cpp b/src/chainlock/chainlock.cpp index 1ef02d3f2ccfc..15e235dea0938 100644 --- a/src/chainlock/chainlock.cpp +++ b/src/chainlock/chainlock.cpp @@ -476,8 +476,8 @@ void CChainLocksHandler::Cleanup() const auto* pindex = m_chainstate.m_blockman.LookupBlockIndex(hashBlock); assert(pindex); // GetTransaction gave us that hashBlock, it should resolve to a valid block index if (m_chainstate.m_chain.Tip()->GetAncestor(pindex->nHeight) == pindex && - m_chainstate.m_chain.Height() - pindex->nHeight >= 6) { - // tx got confirmed >= 6 times, so we can stop keeping track of it + m_chainstate.m_chain.Height() - pindex->nHeight > chainlock::TX_CONFIRM_THRESHOLD) { + // tx is sufficiently deep, we can stop tracking it it = txFirstSeenTime.erase(it); } else { ++it; diff --git a/src/chainlock/signing.cpp b/src/chainlock/signing.cpp index 53c254cf0640d..ef5a3513f9901 100644 --- a/src/chainlock/signing.cpp +++ b/src/chainlock/signing.cpp @@ -95,10 +95,10 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman) if (isman.IsInstantSendEnabled() && isman.RejectConflictingBlocks()) { const auto* pindexWalk = pindex; while (pindexWalk != nullptr) { - if (pindex->nHeight - pindexWalk->nHeight > 5) { - // no need to check further down, 6 confs is safe to assume that TXs below this height won't be + if (pindex->nHeight - pindexWalk->nHeight > TX_CONFIRM_THRESHOLD) { + // no need to check further down, safe to assume that TXs below this height won't be // islocked anymore if they aren't already - LogPrint(BCLog::CHAINLOCKS, "%s -- tip and previous 5 blocks all safe\n", __func__); + LogPrint(BCLog::CHAINLOCKS, "%s -- tip and previous %d blocks all safe\n", __func__, TX_CONFIRM_THRESHOLD); break; } if (m_clhandler.HasChainLock(pindexWalk->nHeight, pindexWalk->GetBlockHash())) { diff --git a/src/chainlock/signing.h b/src/chainlock/signing.h index a07df18265cd8..6f301334f15c0 100644 --- a/src/chainlock/signing.h +++ b/src/chainlock/signing.h @@ -20,6 +20,9 @@ class CSigSharesManager; } // namespace llmq namespace chainlock { +//! Depth of block including transactions before it's considered safe +static constexpr int32_t TX_CONFIRM_THRESHOLD{5}; + class ChainLockSignerParent { public: From f3224aec1a5a96a83281b121fe4c1d0d24968e66 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Fri, 15 Aug 2025 10:27:40 +0000 Subject: [PATCH 8/9] refactor: consolidate `INPUTLOCK_REQUESTID_PREFIX` usage to `lock.cpp` --- src/instantsend/instantsend.cpp | 4 +--- src/instantsend/lock.cpp | 11 ++++++++++- src/instantsend/lock.h | 3 +++ src/instantsend/signing.cpp | 9 +++------ 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index ea182b3dfb604..3206f59b2c5ab 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -34,8 +34,6 @@ using node::fReindex; using node::GetTransaction; namespace llmq { -static constexpr std::string_view INPUTLOCK_REQUESTID_PREFIX{"inlock"}; - namespace { template requires std::same_as || std::same_as @@ -45,7 +43,7 @@ std::unordered_set GetIdsFromLockable(const std::ve if (vec.empty()) return ret; ret.reserve(vec.size()); for (const auto& in : vec) { - ret.emplace(::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in))); + ret.emplace(instantsend::GenInputLockRequestId(in)); } return ret; } diff --git a/src/instantsend/lock.cpp b/src/instantsend/lock.cpp index dc627ed9a0a1f..10ef0afc3c2d9 100644 --- a/src/instantsend/lock.cpp +++ b/src/instantsend/lock.cpp @@ -8,9 +8,10 @@ #include #include -#include +#include #include +static constexpr std::string_view INPUTLOCK_REQUESTID_PREFIX{"inlock"}; static constexpr std::string_view ISLOCK_REQUESTID_PREFIX{"islock"}; namespace instantsend { @@ -36,4 +37,12 @@ bool InstantSendLock::TriviallyValid() const const std::unordered_set inputs_set{inputs.begin(), inputs.end()}; return inputs_set.size() == inputs.size(); } + +template +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); } // namespace instantsend diff --git a/src/instantsend/lock.h b/src/instantsend/lock.h index 5223ce5f9a767..2786ba1ff7cf9 100644 --- a/src/instantsend/lock.h +++ b/src/instantsend/lock.h @@ -40,6 +40,9 @@ struct InstantSendLock { bool TriviallyValid() const; }; +template +uint256 GenInputLockRequestId(const T& val); + using InstantSendLockPtr = std::shared_ptr; } // namespace instantsend diff --git a/src/instantsend/signing.cpp b/src/instantsend/signing.cpp index 46ea06702de64..e76730ec98be8 100644 --- a/src/instantsend/signing.cpp +++ b/src/instantsend/signing.cpp @@ -27,8 +27,6 @@ using node::fReindex; using node::GetTransaction; namespace instantsend { -static constexpr std::string_view INPUTLOCK_REQUESTID_PREFIX{"inlock"}; - InstantSendSigner::InstantSendSigner(CChainState& chainstate, llmq::CChainLocksHandler& clhandler, InstantSendSignerParent& isman, llmq::CSigningManager& sigman, llmq::CSigSharesManager& shareman, llmq::CQuorumManager& qman, @@ -113,8 +111,7 @@ void InstantSendSigner::HandleNewInputLockRecoveredSig(const llmq::CRecoveredSig if (LogAcceptDebug(BCLog::INSTANTSEND)) { for (const auto& in : tx->vin) { - auto id = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in.prevout)); - if (id == recoveredSig.getId()) { + if (GenInputLockRequestId(in.prevout) == recoveredSig.getId()) { LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: got recovered sig for input %s\n", __func__, txid.ToString(), in.prevout.ToStringShort()); break; @@ -298,7 +295,7 @@ bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroact size_t alreadyVotedCount = 0; for (const auto& in : tx.vin) { - auto id = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in.prevout)); + auto id = GenInputLockRequestId(in); ids.emplace_back(id); uint256 otherTxHash; @@ -347,7 +344,7 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx) const auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend; for (const auto& in : tx.vin) { - auto id = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in.prevout)); + auto id = GenInputLockRequestId(in); if (!m_sigman.HasRecoveredSig(llmqType, id, tx.GetHash())) { return; } From 34a90e6c3fe57073f9ab2c1d180a27032311fa05 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 12 Aug 2025 06:49:52 +0000 Subject: [PATCH 9/9] chore: remove unused `IsInvInFilter` from `PeerManager` interface 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. --- src/net_processing.cpp | 9 --------- src/net_processing.h | 3 --- 2 files changed, 12 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 02b4d347d4889..70f0ba4ea4967 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -645,7 +645,6 @@ class PeerManagerImpl final : public PeerManager void RequestObject(NodeId nodeid, const CInv& inv, std::chrono::microseconds current_time, bool is_masternode, bool fForce = false) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); size_t GetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - bool IsInvInFilter(NodeId nodeid, const uint256& hash) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void AskPeersForTransaction(const uint256& txid, bool is_masternode) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); private: void _RelayTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -2317,14 +2316,6 @@ void PeerManagerImpl::AskPeersForTransaction(const uint256& txid, bool is_master } } -bool PeerManagerImpl::IsInvInFilter(NodeId nodeid, const uint256& hash) const -{ - PeerRef peer = GetPeerRef(nodeid); - if (peer == nullptr) - return false; - return IsInvInFilter(*peer, hash); -} - bool PeerManagerImpl::IsInvInFilter(const Peer& peer, const uint256& hash) const { if (auto tx_relay = peer.GetTxRelay(); tx_relay != nullptr) { diff --git a/src/net_processing.h b/src/net_processing.h index be19716117bdd..6059e31021f47 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -85,9 +85,6 @@ class PeerManager : public CValidationInterface, public NetEventsInterface /** Send ping message to all peers */ virtual void SendPings() = 0; - /** Is an inventory in the known inventory filter. Used by InstantSend. */ - virtual bool IsInvInFilter(NodeId nodeid, const uint256& hash) const = 0; - /** Ask a number of our peers, which have a transaction in their inventory, for the transaction. */ virtual void AskPeersForTransaction(const uint256& txid, bool is_masternode) = 0;