From 63371e06c12b87529e9710c4fd488c70806fb708 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 21 Jul 2025 09:02:56 +0000 Subject: [PATCH 01/10] trivial: move forward declaration above `using` declarations --- src/instantsend/instantsend.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index b46b6ceee3d4..71d17a852e7b 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -23,15 +23,14 @@ #include -using node::fImporting; -using node::fReindex; - // Forward declaration to break dependency over node/transaction.h -namespace node -{ +namespace node { CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock); } // namespace node + +using node::fImporting; +using node::fReindex; using node::GetTransaction; namespace llmq { From cf47f144dc7259b76b2489f46d197221fbd07158 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 21 Jul 2025 09:10:56 +0000 Subject: [PATCH 02/10] chore: apply most `clang-format` suggestions --- src/instantsend/db.cpp | 12 ++++-- src/instantsend/db.h | 4 +- src/instantsend/instantsend.cpp | 70 ++++++++++++++++++--------------- src/instantsend/instantsend.h | 6 +-- src/instantsend/signing.cpp | 47 +++++++++++----------- src/instantsend/signing.h | 4 +- 6 files changed, 77 insertions(+), 66 deletions(-) diff --git a/src/instantsend/db.cpp b/src/instantsend/db.cpp index dbd3596ee757..51279f3e77ef 100644 --- a/src/instantsend/db.cpp +++ b/src/instantsend/db.cpp @@ -19,7 +19,8 @@ static const std::string_view DB_VERSION = "is_v"; namespace instantsend { namespace { -static std::tuple BuildInversedISLockKey(std::string_view k, int nHeight, const uint256& islockHash) +static std::tuple BuildInversedISLockKey(std::string_view k, int nHeight, + const uint256& islockHash) { return std::make_tuple(std::string{k}, htobe32_internal(std::numeric_limits::max() - nHeight), islockHash); } @@ -221,7 +222,8 @@ void CInstantSendDb::WriteBlockInstantSendLocks(const gsl::not_nullWriteBatch(batch); } -void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_null>& pblock, gsl::not_null pindexDisconnected) +void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_null>& pblock, + gsl::not_null pindexDisconnected) { LOCK(cs_db); CDBBatch batch(*db); @@ -241,7 +243,8 @@ void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_nullExists(std::make_tuple(DB_ARCHIVED_BY_HASH, islockHash)); + return GetInstantSendLockByHashInternal(islockHash) != nullptr || + db->Exists(std::make_tuple(DB_ARCHIVED_BY_HASH, islockHash)); } size_t CInstantSendDb::GetInstantSendLockCount() const @@ -354,7 +357,8 @@ std::vector CInstantSendDb::GetInstantSendLocksByParent(const uint256& return result; } -std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid, int nHeight) +std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid, + int nHeight) { LOCK(cs_db); std::vector result; diff --git a/src/instantsend/db.h b/src/instantsend/db.h index 178171d7ea2d..8ef333955ed3 100644 --- a/src/instantsend/db.h +++ b/src/instantsend/db.h @@ -33,9 +33,9 @@ class CInstantSendDb static constexpr int CURRENT_VERSION{1}; - int best_confirmed_height GUARDED_BY(cs_db) {0}; + int best_confirmed_height GUARDED_BY(cs_db){0}; - std::unique_ptr db GUARDED_BY(cs_db) {nullptr}; + std::unique_ptr db GUARDED_BY(cs_db){nullptr}; mutable unordered_lru_cache islockCache GUARDED_BY(cs_db); mutable unordered_lru_cache txidCache GUARDED_BY(cs_db); diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 71d17a852e7b..50d7a66c225f 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -142,16 +142,16 @@ PeerMsgRet CInstantSendManager::ProcessMessageInstantSendLock(const CNode& pfrom return tl::unexpected{100}; } - if (WITH_LOCK(cs_pendingLocks, return pendingInstantSendLocks.count(hash) || pendingNoTxInstantSendLocks.count(hash)) - || db.KnownInstantSendLock(hash)) { + if (WITH_LOCK(cs_pendingLocks, return pendingInstantSendLocks.count(hash) || pendingNoTxInstantSendLocks.count(hash)) || + db.KnownInstantSendLock(hash)) { return {}; } LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: received islock, peer=%d\n", __func__, - islock->txid.ToString(), hash.ToString(), pfrom.GetId()); + islock->txid.ToString(), hash.ToString(), pfrom.GetId()); if (ShouldReportISLockTiming()) { - auto time_diff = [&] () -> int64_t { + auto time_diff = [&]() -> int64_t { LOCK(cs_timingsTxSeen); if (auto it = timingsTxSeen.find(islock->txid); it != timingsTxSeen.end()) { // This is the normal case where we received the TX before the islock @@ -164,7 +164,7 @@ PeerMsgRet CInstantSendManager::ProcessMessageInstantSendLock(const CNode& pfrom }(); ::g_stats_client->timing("islock_ms", time_diff); LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock took %dms\n", __func__, - islock->txid.ToString(), time_diff); + islock->txid.ToString(), time_diff); } LOCK(cs_pendingLocks); @@ -209,7 +209,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks(PeerManager& peerman) return false; } - //TODO Investigate if leaving this is ok + // TODO Investigate if leaving this is ok auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend; const auto& llmq_params_opt = Params().GetLLMQ(llmqType); assert(llmq_params_opt); @@ -222,7 +222,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks(PeerManager& peerman) LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- doing verification on old active set\n", __func__); // filter out valid IS locks from "pend" - for (auto it = pend.begin(); it != pend.end(); ) { + for (auto it = pend.begin(); it != pend.end();) { if (!badISLocks.count(it->first)) { it = pend.erase(it); } else { @@ -238,7 +238,8 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks(PeerManager& peerman) std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks( const Consensus::LLMQParams& llmq_params, PeerManager& peerman, int signOffset, - const std::unordered_map, StaticSaltedHasher>& pend, bool ban) + const std::unordered_map, StaticSaltedHasher>& pend, + bool ban) { CBLSBatchVerifier batchVerifier(false, true, 8); std::unordered_map recSigs; @@ -295,7 +296,8 @@ std::unordered_set CInstantSendManager::ProcessPend // avoids unnecessary double-verification of the signature. We however only do this when verification here // turns out to be good (which is checked further down) if (!sigman.HasRecoveredSigForId(llmq_params.type, id)) { - recSigs.try_emplace(hash, CRecoveredSig(llmq_params.type, quorum->qc->quorumHash, id, islock->txid, islock->sig)); + recSigs.try_emplace(hash, + CRecoveredSig(llmq_params.type, quorum->qc->quorumHash, id, islock->txid, islock->sig)); } } @@ -322,8 +324,8 @@ std::unordered_set CInstantSendManager::ProcessPend const auto& islock = p.second.second; if (batchVerifier.badMessages.count(hash)) { - LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: invalid sig in islock, peer=%d\n", __func__, - islock->txid.ToString(), hash.ToString(), nodeId); + LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: invalid sig in islock, peer=%d\n", + __func__, islock->txid.ToString(), hash.ToString(), nodeId); badISLocks.emplace(hash); continue; } @@ -349,8 +351,8 @@ std::unordered_set CInstantSendManager::ProcessPend void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerman, const uint256& hash, const instantsend::InstantSendLockPtr& islock) { - LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: processing islock, peer=%d\n", __func__, - islock->txid.ToString(), hash.ToString(), from); + LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: processing islock, peer=%d\n", + __func__, islock->txid.ToString(), hash.ToString(), from); if (m_signer) { m_signer->ClearLockFromQueue(islock); } @@ -418,7 +420,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm if (tx != nullptr) { RemoveMempoolConflictsForLock(peerman, hash, *islock); LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- notify about lock %s for tx %s\n", __func__, - hash.ToString(), tx->GetHash().ToString()); + hash.ToString(), tx->GetHash().ToString()); GetMainSignals().NotifyTransactionLock(tx, islock); // bump mempool counter to make sure newly locked txes are picked up by getblocktemplate mempool.AddTransactionsUpdated(1); @@ -474,7 +476,8 @@ void CInstantSendManager::TransactionRemovedFromMempool(const CTransactionRef& t return; } - LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- transaction %s was removed from mempool\n", __func__, tx->GetHash().ToString()); + LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- transaction %s was removed from mempool\n", __func__, + tx->GetHash().ToString()); RemoveConflictingLock(::SerializeHash(*islock), *islock); } @@ -508,7 +511,8 @@ void CInstantSendManager::BlockConnected(const std::shared_ptr& pb db.WriteBlockInstantSendLocks(pblock, pindex); } -void CInstantSendManager::BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) +void CInstantSendManager::BlockDisconnected(const std::shared_ptr& pblock, + const CBlockIndex* pindexDisconnected) { db.RemoveBlockInstantSendLocks(pblock, pindexDisconnected); } @@ -523,7 +527,7 @@ void CInstantSendManager::AddNonLockedTx(const CTransactionRef& tx, const CBlock if (did_insert) { nonLockedTxInfo.tx = tx; - for (const auto &in: tx->vin) { + for (const auto& in : tx->vin) { nonLockedTxs[in.prevout.hash].children.emplace(tx->GetHash()); nonLockedTxsByOutpoints.emplace(in.prevout, tx->GetHash()); } @@ -591,8 +595,8 @@ void CInstantSendManager::RemoveNonLockedTx(const uint256& txid, bool retryChild nonLockedTxs.erase(it); - LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, retryChildren=%d, retryChildrenCount=%d\n", __func__, - txid.ToString(), retryChildren, retryChildrenCount); + LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, retryChildren=%d, retryChildrenCount=%d\n", + __func__, txid.ToString(), retryChildren, retryChildrenCount); } void CInstantSendManager::RemoveConflictedTx(const CTransaction& tx) @@ -614,7 +618,8 @@ void CInstantSendManager::TruncateRecoveredSigsForInputs(const instantsend::Inst } } -void CInstantSendManager::TryEmplacePendingLock(const uint256& hash, const NodeId id, const instantsend::InstantSendLockPtr& islock) +void CInstantSendManager::TryEmplacePendingLock(const uint256& hash, const NodeId id, + const instantsend::InstantSendLockPtr& islock) { if (db.KnownInstantSendLock(hash)) return; LOCK(cs_pendingLocks); @@ -673,7 +678,7 @@ void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex) std::vector toRemove; { LOCK(cs_nonLocked); - for (const auto& p: nonLockedTxs) { + for (const auto& p : nonLockedTxs) { const auto* pindexMined = p.second.pindexMined; if (pindexMined && pindex->GetAncestor(pindexMined->nHeight) == pindexMined) { @@ -704,7 +709,7 @@ void CInstantSendManager::RemoveMempoolConflictsForLock(PeerManager& peerman, co toDelete.emplace(it->second->GetHash(), mempool.get(it->second->GetHash())); LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: mempool TX %s with input %s conflicts with islock\n", __func__, - islock.txid.ToString(), hash.ToString(), it->second->GetHash().ToString(), in.ToStringShort()); + islock.txid.ToString(), hash.ToString(), it->second->GetHash().ToString(), in.ToStringShort()); } } @@ -761,16 +766,17 @@ void CInstantSendManager::ResolveBlockConflicts(const uint256& islockHash, const // If a conflict was mined into a ChainLocked block, then we have no other choice and must prune the ISLOCK and all // chained ISLOCKs that build on top of this one. The probability of this is practically zero and can only happen - // when large parts of the masternode network are controlled by an attacker. In this case we must still find consensus - // and its better to sacrifice individual ISLOCKs then to sacrifice whole ChainLocks. + // when large parts of the masternode network are controlled by an attacker. In this case we must still find + // consensus and its better to sacrifice individual ISLOCKs then to sacrifice whole ChainLocks. if (hasChainLockedConflict) { - LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: at least one conflicted TX already got a ChainLock\n", __func__, - islock.txid.ToString(), islockHash.ToString()); + LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: at least one conflicted TX already got a ChainLock\n", + __func__, islock.txid.ToString(), islockHash.ToString()); RemoveConflictingLock(islockHash, islock); return; } - bool isLockedTxKnown = WITH_LOCK(cs_pendingLocks, return pendingNoTxInstantSendLocks.find(islockHash) == pendingNoTxInstantSendLocks.end()); + bool isLockedTxKnown = WITH_LOCK(cs_pendingLocks, return pendingNoTxInstantSendLocks.find(islockHash) == + pendingNoTxInstantSendLocks.end()); bool activateBestChain = false; for (const auto& p : conflicts) { @@ -828,8 +834,9 @@ bool CInstantSendManager::AlreadyHave(const CInv& inv) const return true; } - return WITH_LOCK(cs_pendingLocks, return pendingInstantSendLocks.count(inv.hash) != 0 || pendingNoTxInstantSendLocks.count(inv.hash) != 0) - || db.KnownInstantSendLock(inv.hash); + return WITH_LOCK(cs_pendingLocks, return pendingInstantSendLocks.count(inv.hash) != 0 || + pendingNoTxInstantSendLocks.count(inv.hash) != 0) || + db.KnownInstantSendLock(inv.hash); } bool CInstantSendManager::GetInstantSendLockByHash(const uint256& hash, instantsend::InstantSendLock& ret) const @@ -885,8 +892,8 @@ bool CInstantSendManager::IsWaitingForTx(const uint256& txHash) const auto it = pendingNoTxInstantSendLocks.begin(); while (it != pendingNoTxInstantSendLocks.end()) { if (it->second.second->txid == txHash) { - LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__, - txHash.ToString(), it->first.ToString()); + LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__, txHash.ToString(), + it->first.ToString()); return true; } ++it; @@ -967,5 +974,4 @@ bool CInstantSendManager::RejectConflictingBlocks() const } return true; } - } // namespace llmq diff --git a/src/instantsend/instantsend.h b/src/instantsend/instantsend.h index 1e4f3c4e764f..3130dfa7a08a 100644 --- a/src/instantsend/instantsend.h +++ b/src/instantsend/instantsend.h @@ -9,8 +9,8 @@ #include #include #include -#include #include +#include #include #include @@ -67,8 +67,8 @@ class CInstantSendManager // Tried to verify but there is no tx yet std::unordered_map, StaticSaltedHasher> pendingNoTxInstantSendLocks GUARDED_BY(cs_pendingLocks); - // TXs which are neither IS locked nor ChainLocked. We use this to determine for which TXs we need to retry IS locking - // of child TXs + // TXs which are neither IS locked nor ChainLocked. We use this to determine for which TXs we need to retry IS + // locking of child TXs struct NonLockedTxInfo { const CBlockIndex* pindexMined; CTransactionRef tx; diff --git a/src/instantsend/signing.cpp b/src/instantsend/signing.cpp index 63bbcd13677a..b5655d239cb7 100644 --- a/src/instantsend/signing.cpp +++ b/src/instantsend/signing.cpp @@ -89,7 +89,7 @@ MessageProcessingResult InstantSendSigner::HandleNewRecoveredSig(const llmq::CRe } if (!txid.IsNull()) { HandleNewInputLockRecoveredSig(recoveredSig, txid); - } else if (/*isInstantSendLock=*/ WITH_LOCK(cs_creating, return creatingInstantSendLocks.count(recoveredSig.getId()))) { + } else if (/*isInstantSendLock=*/WITH_LOCK(cs_creating, return creatingInstantSendLocks.count(recoveredSig.getId()))) { HandleNewInstantSendLockRecoveredSig(recoveredSig); } return {}; @@ -154,8 +154,7 @@ void InstantSendSigner::ProcessPendingRetryLockTxs(const std::vectorGetHash().ToString()); + LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: retrying to lock\n", __func__, tx->GetHash().ToString()); } ProcessTx(*tx, false, Params().GetConsensus()); @@ -178,7 +177,8 @@ bool InstantSendSigner::CheckCanLock(const CTransaction& tx, bool printDebug, co [&](const auto& in) { return CheckCanLock(in.prevout, printDebug, tx.GetHash(), params); }); } -bool InstantSendSigner::CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash, const Consensus::Params& params) const +bool InstantSendSigner::CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash, + const Consensus::Params& params) const { int nInstantSendConfirmationsRequired = params.nInstantSendConfirmationsRequired; @@ -201,8 +201,8 @@ bool InstantSendSigner::CheckCanLock(const COutPoint& outpoint, bool printDebug, // this relies on enabled txindex and won't work if we ever try to remove the requirement for txindex for masternodes if (!tx) { if (printDebug) { - LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to find parent TX %s\n", __func__, - txHash.ToString(), outpoint.hash.ToString()); + LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to find parent TX %s\n", __func__, txHash.ToString(), + outpoint.hash.ToString()); } return false; } @@ -215,7 +215,8 @@ bool InstantSendSigner::CheckCanLock(const COutPoint& outpoint, bool printDebug, nTxAge = m_chainstate.m_chain.Height() - pindexMined->nHeight + 1; } - if (nTxAge < nInstantSendConfirmationsRequired && !m_clhandler.HasChainLock(pindexMined->nHeight, pindexMined->GetBlockHash())) { + if (nTxAge < nInstantSendConfirmationsRequired && + !m_clhandler.HasChainLock(pindexMined->nHeight, pindexMined->GetBlockHash())) { if (printDebug) { LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: outpoint %s too new and not ChainLocked. nTxAge=%d, nInstantSendConfirmationsRequired=%d\n", __func__, txHash.ToString(), outpoint.ToStringShort(), nTxAge, nInstantSendConfirmationsRequired); @@ -243,8 +244,8 @@ void InstantSendSigner::HandleNewInstantSendLockRecoveredSig(const llmq::CRecove } if (islock->txid != recoveredSig.getMsgHash()) { - LogPrintf("%s -- txid=%s: islock conflicts with %s, dropping own version\n", __func__, - islock->txid.ToString(), recoveredSig.getMsgHash().ToString()); + LogPrintf("%s -- txid=%s: islock conflicts with %s, dropping own version\n", __func__, islock->txid.ToString(), + recoveredSig.getMsgHash().ToString()); return; } @@ -263,16 +264,15 @@ void InstantSendSigner::ProcessTx(const CTransaction& tx, bool fRetroactive, con } if (!CheckCanLock(tx, true, params)) { - LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: CheckCanLock returned false\n", __func__, - tx.GetHash().ToString()); + LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: CheckCanLock returned false\n", __func__, tx.GetHash().ToString()); return; } auto conflictingLock = m_isman.GetConflictingLock(tx); if (conflictingLock != nullptr) { auto conflictingLockHash = ::SerializeHash(*conflictingLock); - LogPrintf("%s -- txid=%s: conflicts with islock %s, txid=%s\n", __func__, - tx.GetHash().ToString(), conflictingLockHash.ToString(), conflictingLock->txid.ToString()); + LogPrintf("%s -- txid=%s: conflicts with islock %s, txid=%s\n", __func__, tx.GetHash().ToString(), + conflictingLockHash.ToString(), conflictingLock->txid.ToString()); return; } @@ -291,7 +291,8 @@ void InstantSendSigner::ProcessTx(const CTransaction& tx, bool fRetroactive, con TrySignInstantSendLock(tx); } -bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroactive, Consensus::LLMQType llmqType, const Consensus::Params& params) +bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroactive, Consensus::LLMQType llmqType, + const Consensus::Params& params) { std::vector ids; ids.reserve(tx.vin.size()); @@ -313,8 +314,8 @@ bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroact // don't even try the actual signing if any input is conflicting if (m_sigman.IsConflicting(params.llmqTypeDIP0024InstantSend, id, tx.GetHash())) { - LogPrintf("%s -- txid=%s: m_sigman.IsConflicting returned true. id=%s\n", __func__, - tx.GetHash().ToString(), id.ToString()); + LogPrintf("%s -- txid=%s: m_sigman.IsConflicting returned true. id=%s\n", __func__, tx.GetHash().ToString(), + id.ToString()); return false; } } @@ -324,15 +325,15 @@ bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroact return true; } - LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: trying to vote on %d inputs\n", __func__, - tx.GetHash().ToString(), tx.vin.size()); + LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: trying to vote on %d inputs\n", __func__, tx.GetHash().ToString(), + tx.vin.size()); for (const auto i : irange::range(tx.vin.size())) { const auto& in = tx.vin[i]; auto& id = ids[i]; WITH_LOCK(cs_inputReqests, inputRequestIds.emplace(id)); - LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: trying to vote on input %s with id %s. fRetroactive=%d\n", __func__, - tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString(), fRetroactive); + LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: trying to vote on input %s with id %s. fRetroactive=%d\n", + __func__, tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString(), fRetroactive); if (m_sigman.AsyncSignIfMember(llmqType, m_shareman, id, tx.GetHash(), {}, fRetroactive)) { LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: voted on input %s with id %s\n", __func__, tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString()); @@ -354,7 +355,7 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx) } LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: got all recovered sigs, creating InstantSendLock\n", __func__, - tx.GetHash().ToString()); + tx.GetHash().ToString()); InstantSendLock islock; islock.txid = tx.GetHash(); @@ -373,8 +374,8 @@ void InstantSendSigner::TrySignInstantSendLock(const CTransaction& tx) const auto quorum = llmq::SelectQuorumForSigning(llmq_params_opt.value(), m_chainstate.m_chain, m_qman, id); if (!quorum) { - LogPrint(BCLog::INSTANTSEND, "%s -- failed to select quorum. islock id=%s, txid=%s\n", - __func__, id.ToString(), tx.GetHash().ToString()); + LogPrint(BCLog::INSTANTSEND, "%s -- failed to select quorum. islock id=%s, txid=%s\n", __func__, id.ToString(), + tx.GetHash().ToString()); return; } diff --git a/src/instantsend/signing.h b/src/instantsend/signing.h index 1256fcdefe08..873f90385936 100644 --- a/src/instantsend/signing.h +++ b/src/instantsend/signing.h @@ -50,8 +50,8 @@ class InstantSendSigner : public llmq::CRecoveredSigsListener /** * These are the islocks that are currently in the middle of being created. Entries are created when we observed - * recovered signatures for all inputs of a TX. At the same time, we initiate signing of our sigshare for the islock. - * When the recovered sig for the islock later arrives, we can finish the islock and propagate it. + * recovered signatures for all inputs of a TX. At the same time, we initiate signing of our sigshare for the + * islock. When the recovered sig for the islock later arrives, we can finish the islock and propagate it. */ std::unordered_map creatingInstantSendLocks GUARDED_BY(cs_creating); // maps from txid to the in-progress islock From 31065d11ac57eb21346aaa9eb209c39b0a711fde Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 21 Jul 2025 10:59:16 +0000 Subject: [PATCH 03/10] chore: mark `hashBlock` as unused in `HandleNewInputLockRecoveredSig()` --- src/instantsend/signing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/instantsend/signing.cpp b/src/instantsend/signing.cpp index b5655d239cb7..7a9f77296d31 100644 --- a/src/instantsend/signing.cpp +++ b/src/instantsend/signing.cpp @@ -106,8 +106,8 @@ void InstantSendSigner::HandleNewInputLockRecoveredSig(const llmq::CRecoveredSig g_txindex->BlockUntilSyncedToCurrentChain(); } - uint256 hashBlock{}; - const auto tx = GetTransaction(nullptr, &m_mempool, txid, Params().GetConsensus(), hashBlock); + uint256 _hashBlock{}; + const auto tx = GetTransaction(nullptr, &m_mempool, txid, Params().GetConsensus(), _hashBlock); if (!tx) { return; } From 710c50408d915aff3e0af12655bfd0130444dc4d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 24 Jul 2025 21:28:19 +0000 Subject: [PATCH 04/10] refactor: s/cs_inputReqests/cs_input_requests/g --- src/instantsend/signing.cpp | 6 +++--- src/instantsend/signing.h | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/instantsend/signing.cpp b/src/instantsend/signing.cpp index 7a9f77296d31..9910e2699e6e 100644 --- a/src/instantsend/signing.cpp +++ b/src/instantsend/signing.cpp @@ -60,7 +60,7 @@ void InstantSendSigner::Stop() void InstantSendSigner::ClearInputsFromQueue(const std::unordered_set& ids) { - LOCK(cs_inputReqests); + LOCK(cs_input_requests); for (const auto& id : ids) { inputRequestIds.erase(id); } @@ -84,7 +84,7 @@ MessageProcessingResult InstantSendSigner::HandleNewRecoveredSig(const llmq::CRe } uint256 txid; - if (LOCK(cs_inputReqests); inputRequestIds.count(recoveredSig.getId())) { + if (LOCK(cs_input_requests); inputRequestIds.count(recoveredSig.getId())) { txid = recoveredSig.getMsgHash(); } if (!txid.IsNull()) { @@ -331,7 +331,7 @@ bool InstantSendSigner::TrySignInputLocks(const CTransaction& tx, bool fRetroact for (const auto i : irange::range(tx.vin.size())) { const auto& in = tx.vin[i]; auto& id = ids[i]; - WITH_LOCK(cs_inputReqests, inputRequestIds.emplace(id)); + WITH_LOCK(cs_input_requests, inputRequestIds.emplace(id)); LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: trying to vote on input %s with id %s. fRetroactive=%d\n", __func__, tx.GetHash().ToString(), in.prevout.ToStringShort(), id.ToString(), fRetroactive); if (m_sigman.AsyncSignIfMember(llmqType, m_shareman, id, tx.GetHash(), {}, fRetroactive)) { diff --git a/src/instantsend/signing.h b/src/instantsend/signing.h index 873f90385936..1ce4e6c3cec2 100644 --- a/src/instantsend/signing.h +++ b/src/instantsend/signing.h @@ -39,14 +39,14 @@ class InstantSendSigner : public llmq::CRecoveredSigsListener const CMasternodeSync& m_mn_sync; private: - mutable Mutex cs_inputReqests; + mutable Mutex cs_input_requests; mutable Mutex cs_creating; /** * Request ids of inputs that we signed. Used to determine if a recovered signature belongs to an * in-progress input lock. */ - std::unordered_set inputRequestIds GUARDED_BY(cs_inputReqests); + std::unordered_set inputRequestIds GUARDED_BY(cs_input_requests); /** * These are the islocks that are currently in the middle of being created. Entries are created when we observed @@ -68,18 +68,18 @@ class InstantSendSigner : public llmq::CRecoveredSigsListener void Stop(); void ClearInputsFromQueue(const std::unordered_set& ids) - EXCLUSIVE_LOCKS_REQUIRED(!cs_inputReqests); + EXCLUSIVE_LOCKS_REQUIRED(!cs_input_requests); void ClearLockFromQueue(const InstantSendLockPtr& islock) EXCLUSIVE_LOCKS_REQUIRED(!cs_creating); [[nodiscard]] MessageProcessingResult HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig) override - EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests); + EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_input_requests); void ProcessPendingRetryLockTxs(const std::vector& retryTxs) - EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests); + EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_input_requests); void ProcessTx(const CTransaction& tx, bool fRetroactive, const Consensus::Params& params) - EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests); + EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_input_requests); private: [[nodiscard]] bool CheckCanLock(const CTransaction& tx, bool printDebug, const Consensus::Params& params) const; @@ -95,7 +95,7 @@ class InstantSendSigner : public llmq::CRecoveredSigsListener [[nodiscard]] bool TrySignInputLocks(const CTransaction& tx, bool allowResigning, Consensus::LLMQType llmqType, const Consensus::Params& params) - EXCLUSIVE_LOCKS_REQUIRED(!cs_inputReqests); + EXCLUSIVE_LOCKS_REQUIRED(!cs_input_requests); void TrySignInstantSendLock(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_creating); }; From 0459848c0b140bc22b5d7d927ceca3cc21523a78 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 21 Jul 2025 08:36:31 +0000 Subject: [PATCH 05/10] refactor: `constexpr` static string definitions --- src/instantsend/db.cpp | 14 +++++++------- src/instantsend/instantsend.cpp | 2 +- src/instantsend/lock.cpp | 2 +- src/instantsend/signing.cpp | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/instantsend/db.cpp b/src/instantsend/db.cpp index 51279f3e77ef..91ae89ea6eb8 100644 --- a/src/instantsend/db.cpp +++ b/src/instantsend/db.cpp @@ -9,13 +9,13 @@ #include #include -static const std::string_view DB_ARCHIVED_BY_HASH = "is_a2"; -static const std::string_view DB_ARCHIVED_BY_HEIGHT_AND_HASH = "is_a1"; -static const std::string_view DB_HASH_BY_OUTPOINT = "is_in"; -static const std::string_view DB_HASH_BY_TXID = "is_tx"; -static const std::string_view DB_ISLOCK_BY_HASH = "is_i"; -static const std::string_view DB_MINED_BY_HEIGHT_AND_HASH = "is_m"; -static const std::string_view DB_VERSION = "is_v"; +static constexpr std::string_view DB_ARCHIVED_BY_HASH{"is_a2"}; +static constexpr std::string_view DB_ARCHIVED_BY_HEIGHT_AND_HASH{"is_a1"}; +static constexpr std::string_view DB_HASH_BY_OUTPOINT{"is_in"}; +static constexpr std::string_view DB_HASH_BY_TXID{"is_tx"}; +static constexpr std::string_view DB_ISLOCK_BY_HASH{"is_i"}; +static constexpr std::string_view DB_MINED_BY_HEIGHT_AND_HASH{"is_m"}; +static constexpr std::string_view DB_VERSION{"is_v"}; namespace instantsend { namespace { diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 50d7a66c225f..544b074edcbb 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -34,7 +34,7 @@ using node::fReindex; using node::GetTransaction; namespace llmq { -static const std::string_view INPUTLOCK_REQUESTID_PREFIX = "inlock"; +static constexpr std::string_view INPUTLOCK_REQUESTID_PREFIX{"inlock"}; namespace { template diff --git a/src/instantsend/lock.cpp b/src/instantsend/lock.cpp index 3d093db77806..4cc9e18dcad7 100644 --- a/src/instantsend/lock.cpp +++ b/src/instantsend/lock.cpp @@ -10,7 +10,7 @@ #include #include -static const std::string_view ISLOCK_REQUESTID_PREFIX = "islock"; +static constexpr std::string_view ISLOCK_REQUESTID_PREFIX{"islock"}; namespace instantsend { uint256 InstantSendLock::GetRequestId() const diff --git a/src/instantsend/signing.cpp b/src/instantsend/signing.cpp index 9910e2699e6e..c11e3b5e2129 100644 --- a/src/instantsend/signing.cpp +++ b/src/instantsend/signing.cpp @@ -28,7 +28,7 @@ using node::fReindex; using node::GetTransaction; namespace instantsend { -static const std::string_view INPUTLOCK_REQUESTID_PREFIX = "inlock"; +static constexpr std::string_view INPUTLOCK_REQUESTID_PREFIX{"inlock"}; InstantSendSigner::InstantSendSigner(CChainState& chainstate, llmq::CChainLocksHandler& clhandler, llmq::CInstantSendManager& isman, llmq::CSigningManager& sigman, From 234a16a820fe9a4ee8c90a022d1f1cbdabf6289b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 21 Jul 2025 08:46:09 +0000 Subject: [PATCH 06/10] refactor: use unordered set for faster duplicates checking --- src/instantsend/lock.cpp | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/instantsend/lock.cpp b/src/instantsend/lock.cpp index 4cc9e18dcad7..dc627ed9a0a1 100644 --- a/src/instantsend/lock.cpp +++ b/src/instantsend/lock.cpp @@ -6,9 +6,10 @@ #include #include +#include -#include #include +#include static constexpr std::string_view ISLOCK_REQUESTID_PREFIX{"islock"}; @@ -32,13 +33,7 @@ bool InstantSendLock::TriviallyValid() const } // Check that each input is unique - std::set dups; - for (const auto& o : inputs) { - if (!dups.emplace(o).second) { - return false; - } - } - - return true; + const std::unordered_set inputs_set{inputs.begin(), inputs.end()}; + return inputs_set.size() == inputs.size(); } } // namespace instantsend From cbfd325fdf942fb8a932e55c23c32cf55cf87bc1 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Thu, 24 Jul 2025 21:28:37 +0000 Subject: [PATCH 07/10] refactor: avoid shared_ptr copies in `{WriteNew,Remove}InstantSendLock()` Co-Authored-By: Konstantin Akimov --- src/instantsend/db.cpp | 39 +++++++++++++++------------------ src/instantsend/db.h | 5 +++-- src/instantsend/instantsend.cpp | 2 +- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/src/instantsend/db.cpp b/src/instantsend/db.cpp index 91ae89ea6eb8..1400b10ceed6 100644 --- a/src/instantsend/db.cpp +++ b/src/instantsend/db.cpp @@ -51,44 +51,39 @@ void CInstantSendDb::Upgrade(bool unitTests) } } -void CInstantSendDb::WriteNewInstantSendLock(const uint256& hash, const InstantSendLock& islock) +void CInstantSendDb::WriteNewInstantSendLock(const uint256& hash, const InstantSendLockPtr& islock) { LOCK(cs_db); CDBBatch batch(*db); - batch.Write(std::make_tuple(DB_ISLOCK_BY_HASH, hash), islock); - batch.Write(std::make_tuple(DB_HASH_BY_TXID, islock.txid), hash); - for (const auto& in : islock.inputs) { + batch.Write(std::make_tuple(DB_ISLOCK_BY_HASH, hash), *islock); + batch.Write(std::make_tuple(DB_HASH_BY_TXID, islock->txid), hash); + for (const auto& in : islock->inputs) { batch.Write(std::make_tuple(DB_HASH_BY_OUTPOINT, in), hash); } db->WriteBatch(batch); - islockCache.insert(hash, std::make_shared(islock)); - txidCache.insert(islock.txid, hash); - for (const auto& in : islock.inputs) { + islockCache.insert(hash, islock); + txidCache.insert(islock->txid, hash); + for (const auto& in : islock->inputs) { outpointCache.insert(in, hash); } } -void CInstantSendDb::RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, InstantSendLockPtr islock, bool keep_cache) +void CInstantSendDb::RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, const InstantSendLock& islock, + bool keep_cache) { AssertLockHeld(cs_db); - if (!islock) { - islock = GetInstantSendLockByHashInternal(hash, false); - if (!islock) { - return; - } - } batch.Erase(std::make_tuple(DB_ISLOCK_BY_HASH, hash)); - batch.Erase(std::make_tuple(DB_HASH_BY_TXID, islock->txid)); - for (auto& in : islock->inputs) { + batch.Erase(std::make_tuple(DB_HASH_BY_TXID, islock.txid)); + for (auto& in : islock.inputs) { batch.Erase(std::make_tuple(DB_HASH_BY_OUTPOINT, in)); } if (!keep_cache) { islockCache.erase(hash); - txidCache.erase(islock->txid); - for (const auto& in : islock->inputs) { + txidCache.erase(islock.txid); + for (const auto& in : islock.inputs) { outpointCache.erase(in); } } @@ -152,7 +147,7 @@ std::unordered_map CInstantSend auto& islockHash = std::get<2>(curKey); if (auto islock = GetInstantSendLockByHashInternal(islockHash, false)) { - RemoveInstantSendLock(batch, islockHash, islock); + RemoveInstantSendLock(batch, islockHash, *islock); ret.try_emplace(islockHash, std::move(islock)); } @@ -378,7 +373,7 @@ std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256 continue; } - RemoveInstantSendLock(batch, childIslockHash, childIsLock, false); + RemoveInstantSendLock(batch, childIslockHash, *childIsLock, false); WriteInstantSendLockArchived(batch, childIslockHash, nHeight); result.emplace_back(childIslockHash); @@ -388,7 +383,9 @@ std::vector CInstantSendDb::RemoveChainedInstantSendLocks(const uint256 } } - RemoveInstantSendLock(batch, islockHash, nullptr, false); + if (auto islock = GetInstantSendLockByHashInternal(islockHash, /*use_cache=*/false)) { + RemoveInstantSendLock(batch, islockHash, *islock, false); + } WriteInstantSendLockArchived(batch, islockHash, nHeight); result.emplace_back(islockHash); diff --git a/src/instantsend/db.h b/src/instantsend/db.h index 8ef333955ed3..c11758c3435a 100644 --- a/src/instantsend/db.h +++ b/src/instantsend/db.h @@ -51,7 +51,8 @@ class CInstantSendDb * @param islock The InstantSend Lock object itself * @param keep_cache Should we still keep corresponding entries in the cache or not */ - void RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, InstantSendLockPtr islock, bool keep_cache = true) EXCLUSIVE_LOCKS_REQUIRED(cs_db); + void RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, const InstantSendLock& islock, + bool keep_cache = true) EXCLUSIVE_LOCKS_REQUIRED(cs_db); /** * Marks an InstantSend Lock as archived. * @param batch Object used to batch many calls together @@ -88,7 +89,7 @@ class CInstantSendDb * @param hash The hash of the InstantSend Lock * @param islock The InstantSend Lock object itself */ - void WriteNewInstantSendLock(const uint256& hash, const InstantSendLock& islock) EXCLUSIVE_LOCKS_REQUIRED(!cs_db); + void WriteNewInstantSendLock(const uint256& hash, const InstantSendLockPtr& islock) EXCLUSIVE_LOCKS_REQUIRED(!cs_db); /** * This method updates a DB entry for an InstantSend Lock from being not included in a block to being included in a block * @param hash The hash of the InstantSend Lock diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 544b074edcbb..2f264c6a187d 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -394,7 +394,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm LOCK(cs_pendingLocks); pendingNoTxInstantSendLocks.try_emplace(hash, std::make_pair(from, islock)); } else { - db.WriteNewInstantSendLock(hash, *islock); + db.WriteNewInstantSendLock(hash, islock); if (pindexMined) { db.WriteInstantSendLockMined(hash, pindexMined->nHeight); } From c86d886991a5da0744dcf1bcc6abfc5b53626e46 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Wed, 23 Jul 2025 10:19:47 +0000 Subject: [PATCH 08/10] refactor: abstract away InstantSend parent implementation from signer Co-Authored-By: Konstantin Akimov --- src/instantsend/instantsend.h | 11 ++++++----- src/instantsend/signing.cpp | 3 +-- src/instantsend/signing.h | 17 ++++++++++++++--- test/lint/lint-circular-dependencies.py | 1 - 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/instantsend/instantsend.h b/src/instantsend/instantsend.h index 3130dfa7a08a..5cc34746224a 100644 --- a/src/instantsend/instantsend.h +++ b/src/instantsend/instantsend.h @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -43,7 +44,7 @@ class CQuorumManager; class CSigningManager; class CSigSharesManager; -class CInstantSendManager +class CInstantSendManager final : public instantsend::InstantSendSignerParent { private: instantsend::CInstantSendDb db; @@ -128,9 +129,9 @@ class CInstantSendManager EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry); public: - bool IsLocked(const uint256& txHash) const; + bool IsLocked(const uint256& txHash) const override; bool IsWaitingForTx(const uint256& txHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); - instantsend::InstantSendLockPtr GetConflictingLock(const CTransaction& tx) const; + instantsend::InstantSendLockPtr GetConflictingLock(const CTransaction& tx) const override; PeerMsgRet ProcessMessage(const CNode& pfrom, PeerManager& peerman, std::string_view msg_type, CDataStream& vRecv); @@ -152,12 +153,12 @@ class CInstantSendManager EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry); void RemoveConflictingLock(const uint256& islockHash, const instantsend::InstantSendLock& islock); - void TryEmplacePendingLock(const uint256& hash, const NodeId id, const instantsend::InstantSendLockPtr& islock) + void TryEmplacePendingLock(const uint256& hash, const NodeId id, const instantsend::InstantSendLockPtr& islock) override EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); size_t GetInstantSendLockCount() const; - bool IsInstantSendEnabled() const; + bool IsInstantSendEnabled() const override; /** * If true, MN should sign all transactions, if false, MN should not sign * transactions in mempool, but should sign txes included in a block. This diff --git a/src/instantsend/signing.cpp b/src/instantsend/signing.cpp index c11e3b5e2129..7fb3c7d8ecff 100644 --- a/src/instantsend/signing.cpp +++ b/src/instantsend/signing.cpp @@ -11,7 +11,6 @@ #include #include -#include #include #include #include @@ -31,7 +30,7 @@ namespace instantsend { static constexpr std::string_view INPUTLOCK_REQUESTID_PREFIX{"inlock"}; InstantSendSigner::InstantSendSigner(CChainState& chainstate, llmq::CChainLocksHandler& clhandler, - llmq::CInstantSendManager& isman, llmq::CSigningManager& sigman, + InstantSendSignerParent& isman, llmq::CSigningManager& sigman, llmq::CSigSharesManager& shareman, llmq::CQuorumManager& qman, CSporkManager& sporkman, CTxMemPool& mempool, const CMasternodeSync& mn_sync) : m_chainstate{chainstate}, diff --git a/src/instantsend/signing.h b/src/instantsend/signing.h index 1ce4e6c3cec2..a0e02451469f 100644 --- a/src/instantsend/signing.h +++ b/src/instantsend/signing.h @@ -25,12 +25,23 @@ class CQuorumManager; } // namespace llmq namespace instantsend { -class InstantSendSigner : public llmq::CRecoveredSigsListener +class InstantSendSignerParent +{ +public: + virtual ~InstantSendSignerParent() = default; + + virtual bool IsInstantSendEnabled() const = 0; + virtual bool IsLocked(const uint256& txHash) const = 0; + virtual InstantSendLockPtr GetConflictingLock(const CTransaction& tx) const = 0; + virtual void TryEmplacePendingLock(const uint256& hash, const NodeId id, const InstantSendLockPtr& islock) = 0; +}; + +class InstantSendSigner final : public llmq::CRecoveredSigsListener { private: CChainState& m_chainstate; llmq::CChainLocksHandler& m_clhandler; - llmq::CInstantSendManager& m_isman; + InstantSendSignerParent& m_isman; llmq::CSigningManager& m_sigman; llmq::CSigSharesManager& m_shareman; llmq::CQuorumManager& m_qman; @@ -59,7 +70,7 @@ class InstantSendSigner : public llmq::CRecoveredSigsListener public: explicit InstantSendSigner(CChainState& chainstate, llmq::CChainLocksHandler& clhandler, - llmq::CInstantSendManager& isman, llmq::CSigningManager& sigman, + InstantSendSignerParent& isman, llmq::CSigningManager& sigman, llmq::CSigSharesManager& shareman, llmq::CQuorumManager& qman, CSporkManager& sporkman, CTxMemPool& mempool, const CMasternodeSync& mn_sync); ~InstantSendSigner(); diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py index e9c0efc438e5..c89eed880066 100755 --- a/test/lint/lint-circular-dependencies.py +++ b/test/lint/lint-circular-dependencies.py @@ -44,7 +44,6 @@ "governance/governance -> governance/object -> governance/governance", "governance/governance -> masternode/sync -> governance/governance", "governance/governance -> net_processing -> governance/governance", - "instantsend/instantsend -> instantsend/signing -> instantsend/instantsend", "instantsend/instantsend -> llmq/chainlocks -> instantsend/instantsend", "instantsend/instantsend -> net_processing -> instantsend/instantsend", "instantsend/instantsend -> net_processing -> llmq/context -> instantsend/instantsend", From dd540113dff3638910879654716c48925778e89f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 21 Jul 2025 12:58:54 +0000 Subject: [PATCH 09/10] refactor: move inexpensive checks earlier in `ProcessInstantSendLock()` It's faster to check the InstantSend database than to search for a transaction and the block that included it, so allow that faster bail out. Additionally, we'll do some additional things: * Assign the success of `GetTransaction` to `found_transaction` to mark the two different processing paths taken when we know or don't know the transaction associated with the ISLock * Invert an if-else block to improve reading * Push down the networking requests to the tail end of the function as they are independent of the changes made by `ProcessInstantSendLock()` * The `peerman` passed to `RemoveMempoolConflictsForLock()` will be sorted by the next commit Review with `git log -p -n1 --color-moved=dimmed_zebra`. --- src/instantsend/instantsend.cpp | 59 ++++++++++++++++----------------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 2f264c6a187d..3f72a2701c68 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -353,6 +353,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm { LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: processing islock, peer=%d\n", __func__, islock->txid.ToString(), hash.ToString(), from); + if (m_signer) { m_signer->ClearLockFromQueue(islock); } @@ -360,11 +361,24 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm return; } + if (const auto sameTxIsLock = db.GetInstantSendLockByTxid(islock->txid)) { + // can happen, nothing to do + return; + } + for (const auto& in : islock->inputs) { + const auto sameOutpointIsLock = db.GetInstantSendLockByInput(in); + if (sameOutpointIsLock != nullptr) { + LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: conflicting outpoint in islock. input=%s, other islock=%s, peer=%d\n", __func__, + islock->txid.ToString(), hash.ToString(), in.ToStringShort(), ::SerializeHash(*sameOutpointIsLock).ToString(), from); + } + } + uint256 hashBlock{}; const auto tx = GetTransaction(nullptr, &mempool, islock->txid, Params().GetConsensus(), hashBlock); const CBlockIndex* pindexMined{nullptr}; + const bool found_transaction{tx != nullptr}; // we ignore failure here as we must be able to propagate the lock even if we don't have the TX locally - if (tx && !hashBlock.IsNull()) { + if (found_transaction && !hashBlock.IsNull()) { pindexMined = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock)); // Let's see if the TX that was locked by this islock is already mined in a ChainLocked block. If yes, @@ -376,28 +390,15 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm } } - const auto sameTxIsLock = db.GetInstantSendLockByTxid(islock->txid); - if (sameTxIsLock != nullptr) { - // can happen, nothing to do - return; - } - for (const auto& in : islock->inputs) { - const auto sameOutpointIsLock = db.GetInstantSendLockByInput(in); - if (sameOutpointIsLock != nullptr) { - LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: conflicting outpoint in islock. input=%s, other islock=%s, peer=%d\n", __func__, - islock->txid.ToString(), hash.ToString(), in.ToStringShort(), ::SerializeHash(*sameOutpointIsLock).ToString(), from); - } - } - - if (tx == nullptr) { - // put it in a separate pending map and try again later - LOCK(cs_pendingLocks); - pendingNoTxInstantSendLocks.try_emplace(hash, std::make_pair(from, islock)); - } else { + if (found_transaction) { db.WriteNewInstantSendLock(hash, islock); if (pindexMined) { db.WriteInstantSendLockMined(hash, pindexMined->nHeight); } + } else { + // put it in a separate pending map and try again later + LOCK(cs_pendingLocks); + pendingNoTxInstantSendLocks.try_emplace(hash, std::make_pair(from, islock)); } // This will also add children TXs to pendingRetryTxs @@ -405,26 +406,24 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm // We don't need the recovered sigs for the inputs anymore. This prevents unnecessary propagation of these sigs. // We only need the ISLOCK from now on to detect conflicts TruncateRecoveredSigsForInputs(*islock); - - CInv inv(MSG_ISDLOCK, hash); - if (tx != nullptr) { - peerman.RelayInvFiltered(inv, *tx, ISDLOCK_PROTO_VERSION); - } else { - // we don't have the TX yet, so we only filter based on txid. Later when that TX arrives, we will re-announce - // with the TX taken into account. - peerman.RelayInvFiltered(inv, islock->txid, ISDLOCK_PROTO_VERSION); - } - ResolveBlockConflicts(hash, *islock); - if (tx != nullptr) { + if (found_transaction) { RemoveMempoolConflictsForLock(peerman, hash, *islock); LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- notify about lock %s for tx %s\n", __func__, hash.ToString(), tx->GetHash().ToString()); GetMainSignals().NotifyTransactionLock(tx, islock); // bump mempool counter to make sure newly locked txes are picked up by getblocktemplate mempool.AddTransactionsUpdated(1); + } + + CInv inv(MSG_ISDLOCK, hash); + if (found_transaction) { + peerman.RelayInvFiltered(inv, *tx, ISDLOCK_PROTO_VERSION); } else { + // we don't have the TX yet, so we only filter based on txid. Later when that TX arrives, we will re-announce + // with the TX taken into account. + peerman.RelayInvFiltered(inv, islock->txid, ISDLOCK_PROTO_VERSION); peerman.AskPeersForTransaction(islock->txid, /*is_masternode=*/m_signer != nullptr); } } From f488c43ac314fc0fd953282330f4b6be01600a8b Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Mon, 7 Jul 2025 13:04:35 +0000 Subject: [PATCH 10/10] fix: remove unnecessary tx fetching in `RemoveMempoolConflictsForLock` `RemoveMempoolConflictsForLock` is called in two instances * `ProcessInstantSendLock` when we *have* a valid transaction (tx != nullptr) and want to purge conflicting transactions * `TransactionAddedToMempool` when we have received a transaction and found the corresponding lock (else condition to `islock == nullptr`), and want to purge conflicting transactions Neither case calls for fetching the transaction based on the islock hash since in both cases, we already have the transaction --- src/dsnotificationinterface.cpp | 2 +- src/instantsend/instantsend.cpp | 16 ++++++---------- src/instantsend/instantsend.h | 4 ++-- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/dsnotificationinterface.cpp b/src/dsnotificationinterface.cpp index 04c0a742b635..1bc179bf3217 100644 --- a/src/dsnotificationinterface.cpp +++ b/src/dsnotificationinterface.cpp @@ -106,7 +106,7 @@ void CDSNotificationInterface::TransactionAddedToMempool(const CTransactionRef& { assert(m_cj_ctx && m_llmq_ctx); - m_llmq_ctx->isman->TransactionAddedToMempool(m_peerman, ptx); + m_llmq_ctx->isman->TransactionAddedToMempool(ptx); m_llmq_ctx->clhandler->TransactionAddedToMempool(ptx, nAcceptTime); m_cj_ctx->dstxman->TransactionAddedToMempool(ptx); } diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 3f72a2701c68..1e92ea074a41 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -409,7 +409,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm ResolveBlockConflicts(hash, *islock); if (found_transaction) { - RemoveMempoolConflictsForLock(peerman, hash, *islock); + RemoveMempoolConflictsForLock(hash, *islock); LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- notify about lock %s for tx %s\n", __func__, hash.ToString(), tx->GetHash().ToString()); GetMainSignals().NotifyTransactionLock(tx, islock); @@ -428,7 +428,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm } } -void CInstantSendManager::TransactionAddedToMempool(PeerManager& peerman, const CTransactionRef& tx) +void CInstantSendManager::TransactionAddedToMempool(const CTransactionRef& tx) { if (!IsInstantSendEnabled() || !m_mn_sync.IsBlockchainSynced() || tx->vin.empty()) { return; @@ -459,7 +459,7 @@ void CInstantSendManager::TransactionAddedToMempool(PeerManager& peerman, const // TX is not locked, so make sure it is tracked AddNonLockedTx(tx, nullptr); } else { - RemoveMempoolConflictsForLock(peerman, ::SerializeHash(*islock), *islock); + RemoveMempoolConflictsForLock(::SerializeHash(*islock), *islock); } } @@ -691,8 +691,7 @@ void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex) } } -void CInstantSendManager::RemoveMempoolConflictsForLock(PeerManager& peerman, const uint256& hash, - const instantsend::InstantSendLock& islock) +void CInstantSendManager::RemoveMempoolConflictsForLock(const uint256& hash, const instantsend::InstantSendLock& islock) { std::unordered_map toDelete; @@ -717,11 +716,8 @@ void CInstantSendManager::RemoveMempoolConflictsForLock(PeerManager& peerman, co } } - if (!toDelete.empty()) { - for (const auto& p : toDelete) { - RemoveConflictedTx(*p.second); - } - peerman.AskPeersForTransaction(islock.txid, /*is_masternode=*/m_signer != nullptr); + for (const auto& p : toDelete) { + RemoveConflictedTx(*p.second); } } diff --git a/src/instantsend/instantsend.h b/src/instantsend/instantsend.h index 5cc34746224a..645f33005a7a 100644 --- a/src/instantsend/instantsend.h +++ b/src/instantsend/instantsend.h @@ -117,7 +117,7 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry); void TruncateRecoveredSigsForInputs(const instantsend::InstantSendLock& islock); - void RemoveMempoolConflictsForLock(PeerManager& peerman, const uint256& hash, const instantsend::InstantSendLock& islock) + void RemoveMempoolConflictsForLock(const uint256& hash, const instantsend::InstantSendLock& islock) EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry); void ResolveBlockConflicts(const uint256& islockHash, const instantsend::InstantSendLock& islock) EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); @@ -135,7 +135,7 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent PeerMsgRet ProcessMessage(const CNode& pfrom, PeerManager& peerman, std::string_view msg_type, CDataStream& vRecv); - void TransactionAddedToMempool(PeerManager& peerman, const CTransactionRef& tx) + void TransactionAddedToMempool(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); void TransactionRemovedFromMempool(const CTransactionRef& tx); void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex)