Refactor validation signals for ChainLocks, InstantSend and PrivateSend to get rid of SyncTransaction

CMakeLists.txt

@@ -37,28 +37,40 @@ add_definitions(
 )
 
 file(GLOB SOURCE_FILES
+        src/*.cpp
+        src/*.h
         src/bench/*.cpp
         src/bench/*.h
         src/bls/*.cpp
         src/bls/*.h
         src/compat/*.cpp
         src/compat/*.h
-        src/consensus/*.h
         src/consensus/*.cpp
+        src/consensus/*.h
         src/crypto/*.c
-        src/crypto/*.h
         src/crypto/*.cpp
+        src/crypto/*.h
+        src/evo/*.cpp
+        src/evo/*.h
         src/leveldb/db/*.cc
         src/leveldb/db/*.h
         src/leveldb/include/*.h
+        src/llmq/*.cpp
+        src/llmq/*.h
+        src/masternode/*.cpp
+        src/masternode/*.h
         src/policy/*.cpp
         src/policy/*.h
         src/primitives/*.cpp
         src/primitives/*.h
-        src/qt/test/*.cpp
-        src/qt/test/*.h
+        src/privatesend/*.cpp
+        src/privatesend/*.h
         src/qt/*.cpp
         src/qt/*.h
+        src/qt/test/*.cpp
+        src/qt/test/*.h
+        src/rpc/*.cpp
+        src/rpc/*.h
         src/script/*.cpp
         src/script/*.h
         src/secp256k1/include/*.h
@@ -67,19 +79,11 @@ file(GLOB SOURCE_FILES
         src/univalue/include/*.h
         src/univalue/lib/*.cpp
         src/univalue/lib/*.h
-        src/wallet/test/*.cpp
         src/wallet/*.cpp
         src/wallet/*.h
+        src/wallet/test/*.cpp
         src/zmq/*.cpp
         src/zmq/*.h
-        src/*.cpp
-        src/*.h
-        src/evo/*.h
-        src/evo/*.cpp
-        src/llmq/*.h
-        src/llmq/*.cpp
-        src/rpc/*.cpp
-        src/rpc/*.h
         )
 
 add_executable(dash ${SOURCE_FILES})

src/dsnotificationinterface.cpp

@@ -72,17 +72,12 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
     llmq::quorumDKGSessionManager->UpdatedBlockTip(pindexNew, fInitialDownload);
 }
 
-void CDSNotificationInterface::SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex, int posInBlock)
-{
-    llmq::quorumInstantSendManager->SyncTransaction(tx, pindex, posInBlock);
-    llmq::chainLocksHandler->SyncTransaction(tx, pindex, posInBlock);
-    instantsend.SyncTransaction(tx, pindex, posInBlock);
-    CPrivateSend::SyncTransaction(tx, pindex, posInBlock);
-}
-
 void CDSNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx)
 {
-    SyncTransaction(ptx);
+    llmq::quorumInstantSendManager->TransactionAddedToMempool(ptx);
+    llmq::chainLocksHandler->TransactionAddedToMempool(ptx);
+    CPrivateSend::TransactionAddedToMempool(ptx);
+    instantsend.SyncTransaction(ptx);
 }
 
 void CDSNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted)
@@ -95,18 +90,26 @@ void CDSNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock
     // to abandon a transaction and then have it inadvertantly cleared by
     // the notification that the conflicted transaction was evicted.
 
+    llmq::quorumInstantSendManager->BlockConnected(pblock, pindex, vtxConflicted);
+    llmq::chainLocksHandler->BlockConnected(pblock, pindex, vtxConflicted);
+    CPrivateSend::BlockConnected(pblock, pindex, vtxConflicted);
+
     for (const CTransactionRef& ptx : vtxConflicted) {
-        SyncTransaction(ptx);
+        instantsend.SyncTransaction(ptx);
     }
     for (size_t i = 0; i < pblock->vtx.size(); i++) {
-        SyncTransaction(pblock->vtx[i], pindex, i);
+        instantsend.SyncTransaction(pblock->vtx[i], pindex, i);
     }
 }
 
-void CDSNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex)
+void CDSNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)
 {
+    llmq::quorumInstantSendManager->BlockDisconnected(pblock, pindexDisconnected);
+    llmq::chainLocksHandler->BlockDisconnected(pblock, pindexDisconnected);
+    CPrivateSend::BlockDisconnected(pblock, pindexDisconnected);
+
     for (const CTransactionRef& ptx : pblock->vtx) {
-        SyncTransaction(ptx, pindex, -1);
+        instantsend.SyncTransaction(ptx, pindexDisconnected->pprev, -1);
     }
 }
 

src/dsnotificationinterface.h

@@ -23,14 +23,12 @@ class CDSNotificationInterface : public CValidationInterface
     void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
     void TransactionAddedToMempool(const CTransactionRef& tx) override;
     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted) override;
-    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex) override;
+    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected) override;
     void NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff) override;
     void NotifyChainLock(const CBlockIndex* pindex, const llmq::CChainLockSig& clsig) override;
 
 private:
     CConnman& connman;
-    /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected */
-    void SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex = nullptr, int posInBlock = 0);
 };
 
 #endif // BITCOIN_DSNOTIFICATIONINTERFACE_H

src/llmq/quorums_chainlocks.cpp

@@ -345,38 +345,58 @@ void CChainLocksHandler::TrySignChainTip()
     quorumSigningManager->AsyncSignIfMember(Params().GetConsensus().llmqChainLocks, requestId, msgHash);
 }
 
-void CChainLocksHandler::SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex, int posInBlock)
+void CChainLocksHandler::TransactionAddedToMempool(const CTransactionRef& tx)
 {
-    if (!masternodeSync.IsBlockchainSynced()) {
+    if (tx->IsCoinBase() || tx->vin.empty()) {
         return;
     }
 
-    bool handleTx = true;
-    if (tx->IsCoinBase() || tx->vin.empty()) {
-        handleTx = false;
+    if (!masternodeSync.IsBlockchainSynced()) {
+        return;
     }
 
     LOCK(cs);
+    int64_t curTime = GetAdjustedTime();
+    txFirstSeenTime.emplace(tx->GetHash(), curTime);
+}
 
-    if (handleTx) {
-        int64_t curTime = GetAdjustedTime();
-        txFirstSeenTime.emplace(tx->GetHash(), curTime);
+void CChainLocksHandler::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted)
+{
+    if (!masternodeSync.IsBlockchainSynced()) {
+        return;
     }
 
-    // We listen for SyncTransaction so that we can collect all TX ids of all included TXs of newly received blocks
+    // We listen for BlockConnected so that we can collect all TX ids of all included TXs of newly received blocks
     // We need this information later when we try to sign a new tip, so that we can determine if all included TXs are
     // safe.
-    if (pindex != nullptr && posInBlock != -1) {
-        auto it = blockTxs.find(pindex->GetBlockHash());
-        if (it == blockTxs.end()) {
-            // we want this to be run even if handleTx == false, so that the coinbase TX triggers creation of an empty entry
-            it = blockTxs.emplace(pindex->GetBlockHash(), std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>()).first;
-        }
-        if (handleTx) {
-            auto& txs = *it->second;
-            txs.emplace(tx->GetHash());
+
+    LOCK(cs);
+
+    auto it = blockTxs.find(pindex->GetBlockHash());
+    if (it == blockTxs.end()) {
+        // we must create this entry even if there are no lockable transactions in the block, so that TrySignChainTip
+        // later knows about this block
+        it = blockTxs.emplace(pindex->GetBlockHash(), std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>()).first;
+    }
+    auto& txids = *it->second;
+
+    int64_t curTime = GetAdjustedTime();
+
+    for (const auto& tx : pblock->vtx) {
+        if (tx->IsCoinBase() || tx->vin.empty()) {
+            continue;
         }
+
+        txids.emplace(tx->GetHash());
+        txFirstSeenTime.emplace(tx->GetHash(), curTime);
     }
+
+}
+
+void CChainLocksHandler::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)
+{
+    LOCK(cs);
+    blockTxs.erase(pindexDisconnected->GetBlockHash());
 }
 
 CChainLocksHandler::BlockTxs::mapped_type CChainLocksHandler::GetBlockTxs(const uint256& blockHash)

src/llmq/quorums_chainlocks.h

@@ -90,7 +90,9 @@ class CChainLocksHandler : public CRecoveredSigsListener
     void ProcessNewChainLock(NodeId from, const CChainLockSig& clsig, const uint256& hash);
     void AcceptedBlockHeader(const CBlockIndex* pindexNew);
     void UpdatedBlockTip(const CBlockIndex* pindexNew);
-    void SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex = nullptr, int posInBlock = 0);
+    void TransactionAddedToMempool(const CTransactionRef& tx);
+    void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted);
+    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected);
     void CheckActiveState();
     void TrySignChainTip();
     void EnforceBestChainLock();

src/llmq/quorums_instantsend.cpp

@@ -949,42 +949,27 @@ void CInstantSendManager::UpdateWalletTransaction(const CTransactionRef& tx, con
     mempool.AddTransactionsUpdated(1);
 }
 
-void CInstantSendManager::SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex, int posInBlock)
+void CInstantSendManager::ProcessNewTransaction(const CTransactionRef& tx, const CBlockIndex* pindex)
 {
     if (!IsNewInstantSendEnabled()) {
         return;
     }
 
     if (tx->IsCoinBase() || tx->vin.empty()) {
-        // coinbase can't and TXs with no inputs be locked
+        // coinbase and TXs with no inputs can't be locked
         return;
     }
 
     bool inMempool = mempool.get(tx->GetHash()) != nullptr;
 
-    // Are we called from validation.cpp/MemPoolConflictRemovalTracker?
-    // TODO refactor this when we backport the BlockConnected signal from Bitcoin, as it gives better info about
-    // conflicted TXs
-    bool isConflictRemoved = pindex == nullptr && posInBlock == -1 && !inMempool;
-
-    if (isConflictRemoved) {
-        LOCK(cs);
-        RemoveConflictedTx(*tx);
-        return;
-    }
-
     uint256 islockHash;
     {
         LOCK(cs);
         islockHash = db.GetInstantSendLockHashByTxid(tx->GetHash());
 
         // update DB about when an IS lock was mined
         if (!islockHash.IsNull() && pindex) {
-            if (posInBlock == -1) {
-                db.RemoveInstantSendLockMined(islockHash, pindex->nHeight);
-            } else {
-                db.WriteInstantSendLockMined(islockHash, pindex->nHeight);
-            }
+            db.WriteInstantSendLockMined(islockHash, pindex->nHeight);
         }
     }
 
@@ -1001,13 +986,47 @@ void CInstantSendManager::SyncTransaction(const CTransactionRef& tx, const CBloc
     if (!chainlocked && islockHash.IsNull()) {
         // TX is not locked, so make sure it is tracked
         AddNonLockedTx(tx);
-        nonLockedTxs.at(tx->GetHash()).pindexMined = posInBlock != -1 ? pindex : nullptr;
+        nonLockedTxs.at(tx->GetHash()).pindexMined = pindex;
     } else {
         // TX is locked, so make sure we don't track it anymore
         RemoveNonLockedTx(tx->GetHash(), true);
     }
 }
 
+void CInstantSendManager::TransactionAddedToMempool(const CTransactionRef& tx)
+{
+    ProcessNewTransaction(tx, nullptr);
+}
+
+void CInstantSendManager::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted)
+{
+    if (!IsNewInstantSendEnabled()) {
+        return;
+    }
+
+    if (!vtxConflicted.empty()) {
+        LOCK(cs);
+        for (const auto& tx : vtxConflicted) {
+            RemoveConflictedTx(*tx);
+        }
+    }
+
+    for (const auto& tx : pblock->vtx) {
+        ProcessNewTransaction(tx, pindex);
+    }
+}
+
+void CInstantSendManager::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)
+{
+    LOCK(cs);
+    for (auto& tx : pblock->vtx) {
+        auto islockHash = db.GetInstantSendLockHashByTxid(tx->GetHash());
+        if (!islockHash.IsNull()) {
+            db.RemoveInstantSendLockMined(islockHash, pindexDisconnected->nHeight);
+        }
+    }
+}
+
 void CInstantSendManager::AddNonLockedTx(const CTransactionRef& tx)
 {
     AssertLockHeld(cs);

src/llmq/quorums_instantsend.h

@@ -141,7 +141,10 @@ class CInstantSendManager : public CRecoveredSigsListener
     void ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLock& islock);
     void UpdateWalletTransaction(const CTransactionRef& tx, const CInstantSendLock& islock);
 
-    void SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex = nullptr, int posInBlock = 0);
+    void ProcessNewTransaction(const CTransactionRef& tx, const CBlockIndex* pindex);
+    void TransactionAddedToMempool(const CTransactionRef& tx);
+    void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted);
+    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected);
     void AddNonLockedTx(const CTransactionRef& tx);
     void RemoveNonLockedTx(const uint256& txid, bool retryChildren);
     void RemoveConflictedTx(const CTransaction& tx);

src/privatesend/privatesend.cpp

@@ -509,16 +509,41 @@ void CPrivateSend::UpdatedBlockTip(const CBlockIndex* pindex)
     }
 }
 
-void CPrivateSend::SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex, int posInBlock)
+void CPrivateSend::UpdateDSTXConfirmedHeight(const CTransactionRef& tx, int nHeight)
 {
-    if (tx->IsCoinBase()) return;
+    AssertLockHeld(cs_mapdstx);
 
-    LOCK2(cs_main, cs_mapdstx);
+    auto it = mapDSTX.find(tx->GetHash());
+    if (it == mapDSTX.end()) {
+        return;
+    }
+
+    it->second.SetConfirmedHeight(nHeight);
+    LogPrint(BCLog::PRIVATESEND, "CPrivateSend::%s -- txid=%s, nHeight=%d\n", __func__, tx->GetHash().ToString(), nHeight);
+}
+
+void CPrivateSend::TransactionAddedToMempool(const CTransactionRef& tx)
+{
+    LOCK(cs_mapdstx);
+    UpdateDSTXConfirmedHeight(tx, -1);
+}
+
+void CPrivateSend::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted)
+{
+    LOCK(cs_mapdstx);
+    for (const auto& tx : vtxConflicted) {
+        UpdateDSTXConfirmedHeight(tx, -1);
+    }
 
-    uint256 txHash = tx->GetHash();
-    if (!mapDSTX.count(txHash)) return;
+    for (const auto& tx : pblock->vtx) {
+        UpdateDSTXConfirmedHeight(tx, pindex->nHeight);
+    }
+}
 
-    // When tx is 0-confirmed or conflicted, posInBlock is SYNC_TRANSACTION_NOT_IN_BLOCK and nConfirmedHeight should be set to -1
-    mapDSTX[txHash].SetConfirmedHeight((posInBlock == -1 || pindex == nullptr) ? -1 : pindex->nHeight);
-    LogPrint(BCLog::PRIVATESEND, "CPrivateSend::SyncTransaction -- txid=%s\n", txHash.ToString());
+void CPrivateSend::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)
+{
+    LOCK(cs_mapdstx);
+    for (const auto& tx : pblock->vtx) {
+        UpdateDSTXConfirmedHeight(tx, -1);
+    }
 }

src/privatesend/privatesend.h

@@ -422,7 +422,12 @@ class CPrivateSend
     static CPrivateSendBroadcastTx GetDSTX(const uint256& hash);
 
     static void UpdatedBlockTip(const CBlockIndex* pindex);
-    static void SyncTransaction(const CTransactionRef& tx, const CBlockIndex* pindex = nullptr, int posInBlock = 0);
+
+    static void UpdateDSTXConfirmedHeight(const CTransactionRef& tx, int nHeight);
+    static void TransactionAddedToMempool(const CTransactionRef& tx);
+    static void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted);
+    static void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected);
+
 };
 
 #endif