Merge #6815: refactor: follow-up dash#6761, implement review suggestions, ChainLock and InstantSend refactoring

34a90e6 chore: remove unused `IsInvInFilter` from `PeerManager` interface (Kittywhiskers Van Gogh)
f3224ae refactor: consolidate `INPUTLOCK_REQUESTID_PREFIX` usage to `lock.cpp` (Kittywhiskers Van Gogh)
7b4ee6b trivial: document transaction confirmation safety threshold (Kittywhiskers Van Gogh)
25f05c1 refactor: make unknown block clsig flow easier to follow (Kittywhiskers Van Gogh)
9578146 refactor: document `pindex` assumptions in chainlocks code (Kittywhiskers Van Gogh)
4a744c7 refactor: use `std::chrono` for time variables, reduce resolution (Kittywhiskers Van Gogh)
b051c22 refactor: consolidate `CLSIG_REQUESTID_PREFIX` usage to `clsig.cpp` (Kittywhiskers Van Gogh)
024b466 chore: move lock annotations in `chainlock.h` to the next line (Kittywhiskers Van Gogh)
c6e99fb chore: apply most `clang-format` suggestions (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6761

  * Dependency for #6821

  * Assumptions surrounding `pindex` usage have been documented in response to reviewer comments in [dash#6761](#6761)  ([comment](#6761 (comment)), [comment](#6761 (comment))).

  * The internal structures of `CChainLocksHandler` (`txFirstSeenTime`, `seenChainLocks`, `lastCleanupTime`) have their time resolution reduced from milliseconds to seconds while migrating to `std::chrono`.

  * `CInstantSendManager::AskNodesForLockedTx()` was moved as `PeerManagerImpl::AskPeersForTransaction()` in [dash#6425](#6425) and with that, the sole external usage of `PeerManagerImpl::IsInvInFilter()` was moved internally, we can therefore safely remove it from the `PeerManager` interface.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 34a90e6

Tree-SHA512: 28b532cb5b8b5e6d7c1331c7c6c0ecd4d45b186922c279db6d2d3e8974d422ec8b67d75aeadce77986d409a8ed071e85359ee08609e0c2dde657e4520c546817

Author: PastaPastaPasta

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)
@@ -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()
@@ -131,7 +133,7 @@ MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId fro
 
     {
         LOCK(cs);
-        if (!seenChainLocks.emplace(hash, TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now())).second) {
+        if (!seenChainLocks.emplace(hash, GetTime<std::chrono::seconds>()).second) {
             return {};
         }
 
@@ -142,61 +144,59 @@ 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};
         }
         return {};
     }
 
     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",
-                        __func__, clsig.ToString(), pindex->nHeight);
+                          __func__, clsig.ToString(), pindex->nHeight);
                 // 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(
+        [&]() {
+            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;
+    return clsig_inv;
 }
 
 void CChainLocksHandler::AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew)
 {
     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 +220,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 +283,8 @@ void CChainLocksHandler::BlockConnected(const std::shared_ptr<const CBlock>& pbl
     }
 }
 
-void CChainLocksHandler::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> pindexDisconnected)
+void CChainLocksHandler::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock,
+                                           gsl::not_null<const CBlockIndex*> pindexDisconnected)
 {
     if (m_signer) {
         m_signer->EraseFromBlockHashTxidMap(pindexDisconnected->GetBlockHash());
@@ -297,16 +300,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<std::chrono::seconds>().count() - it->second;
+            tx_age = GetTime<std::chrono::seconds>() - it->second;
         }
     }
 
-    return txAge >= WAIT_FOR_ISLOCK_TIMEOUT;
+    return tx_age >= WAIT_FOR_ISLOCK_TIMEOUT;
 }
 
 // WARNING: cs_main and cs should not be held!
@@ -340,17 +343,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;
         }
     }
@@ -369,9 +376,10 @@ 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());
+    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
@@ -431,15 +439,15 @@ void CChainLocksHandler::Cleanup()
         return;
     }
 
-    if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - lastCleanupTime < CLEANUP_INTERVAL) {
+    if (GetTime<std::chrono::seconds>() - lastCleanupTime.load() < CLEANUP_INTERVAL) {
         return;
     }
-    lastCleanupTime = TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now());
+    lastCleanupTime = GetTime<std::chrono::seconds>();
 
     {
         LOCK(cs);
-        for (auto it = seenChainLocks.begin(); it != seenChainLocks.end(); ) {
-            if (TicksSinceEpoch<std::chrono::milliseconds>(SystemClock::now()) - it->second >= CLEANUP_SEEN_TIMEOUT) {
+        for (auto it = seenChainLocks.begin(); it != seenChainLocks.end();) {
+            if (GetTime<std::chrono::seconds>() - it->second >= CLEANUP_SEEN_TIMEOUT) {
                 it = seenChainLocks.erase(it);
             } else {
                 ++it;
@@ -459,15 +467,17 @@ 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) {
-                // tx got confirmed >= 6 times, so we can stop keeping track of it
+            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 > chainlock::TX_CONFIRM_THRESHOLD) {
+                // tx is sufficiently deep, we can stop tracking it
                 it = txFirstSeenTime.erase(it);
             } else {
                 ++it;

src/chainlock/chainlock.h

@@ -57,14 +57,14 @@ 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<uint256, int64_t, StaticSaltedHasher> txFirstSeenTime GUARDED_BY(cs);
+    std::unordered_map<uint256, std::chrono::seconds, StaticSaltedHasher> txFirstSeenTime GUARDED_BY(cs);
 
-    std::map<uint256, int64_t> seenChainLocks GUARDED_BY(cs);
+    std::map<uint256, std::chrono::seconds> seenChainLocks GUARDED_BY(cs);
 
-    std::atomic<int64_t> lastCleanupTime{0};
+    std::atomic<std::chrono::seconds> lastCleanupTime{0s};
 
 public:
     explicit CChainLocksHandler(CChainState& chainstate, CQuorumManager& _qman, CSigningManager& _sigman,
@@ -75,23 +75,32 @@ 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<uint256, StaticSaltedHasher>& tx, const int64_t& time) override
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     [[nodiscard]] MessageProcessingResult ProcessNewChainLock(NodeId from, const chainlock::ChainLockSig& clsig,
                                                               const uint256& hash) override
         EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
-    void AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> pindexNew) EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    void AcceptedBlockHeader(gsl::not_null<const CBlockIndex*> 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<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> pindex) EXCLUSIVE_LOCKS_REQUIRED(!cs);
-    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> 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<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> pindex)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> 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);
@@ -106,7 +115,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);

src/chainlock/clsig.cpp

@@ -6,8 +6,10 @@
 
 #include <tinyformat.h>
 
+#include <string_view>
+
 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

src/chainlock/clsig.h

@@ -13,8 +13,6 @@
 #include <cstdint>
 
 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