Don't rely on NewPoWValidBlock and use SyncTransaction to build blockTxs

NewPoWValidBlock is not guaranteed to be called when blocks come in fast.
When a block is accepted in AcceptBlock, NewPoWValidBlock is only called
when the new block is a successor of the currently active tip. This is not
the case when after the first block a second block is accepted immediately
as the first block is not connected yet.

This might be a bug actually in the handling of NewPoWValidBlock, so we
might need to check/fix this later, but currently I prefer to not touch
that part.

Instead, we now use SyncTransaction to gather TXs for blockTxs. This works
because SyncTransaction is called for all transactions in a freshly
connected block in one go. The call also happens before UpdatedBlockTip is
called, so it's fine with the existing logic.

src/dsnotificationinterface.cpp

@@ -71,11 +71,6 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
     llmq::quorumDKGSessionManager->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload);
 }
 
-void CDSNotificationInterface::NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr<const CBlock>& block)
-{
-    llmq::chainLocksHandler->NewPoWValidBlock(pindex, block);
-}
-
 void CDSNotificationInterface::SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock)
 {
     llmq::quorumInstantSendManager->SyncTransaction(tx, pindex, posInBlock);

src/dsnotificationinterface.h

@@ -21,7 +21,6 @@ class CDSNotificationInterface : public CValidationInterface
     void AcceptedBlockHeader(const CBlockIndex *pindexNew) override;
     void NotifyHeaderTip(const CBlockIndex *pindexNew, bool fInitialDownload) override;
     void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
-    void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& block) override;
     void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) override;
     void NotifyMasternodeListChanged(const CDeterministicMNList& newList) override;
     void NotifyChainLock(const CBlockIndex* pindex) override;

src/llmq/quorums_chainlocks.cpp

@@ -316,41 +316,34 @@ void CChainLocksHandler::TrySignChainTip()
     quorumSigningManager->AsyncSignIfMember(Params().GetConsensus().llmqChainLocks, requestId, msgHash);
 }
 
-void CChainLocksHandler::NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr<const CBlock>& block)
+void CChainLocksHandler::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
 {
-    LOCK(cs);
-    if (blockTxs.count(pindex->GetBlockHash())) {
-        // should actually not happen (blocks are only written once to disk and this is when NewPoWValidBlock is called)
-        // but be extra safe here in case this behaviour changes.
-        return;
+    bool handleTx = true;
+    if (tx.IsCoinBase() || tx.vin.empty()) {
+        handleTx = false;
     }
 
-    int64_t curTime = GetAdjustedTime();
+    LOCK(cs);
 
-    // We listen for NewPoWValidBlock so that we can collect all TX ids of all included TXs of newly received blocks
+    if (handleTx) {
+        int64_t curTime = GetAdjustedTime();
+        txFirstSeenTime.emplace(tx.GetHash(), curTime);
+    }
+
+    // We listen for SyncTransaction 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.
-
-    auto txs = std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>();
-    for (const auto& tx : block->vtx) {
-        if (tx->IsCoinBase() || tx->vin.empty()) {
-            continue;
+    if (pindex && posInBlock != CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK) {
+        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());
         }
-        txs->emplace(tx->GetHash());
-        txFirstSeenTime.emplace(tx->GetHash(), curTime);
-    }
-    blockTxs[pindex->GetBlockHash()] = txs;
-}
-
-void CChainLocksHandler::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock)
-{
-    if (tx.IsCoinBase() || tx.vin.empty()) {
-        return;
     }
-
-    LOCK(cs);
-    int64_t curTime = GetAdjustedTime();
-    txFirstSeenTime.emplace(tx.GetHash(), curTime);
 }
 
 bool CChainLocksHandler::IsTxSafeForMining(const uint256& txid)

src/llmq/quorums_chainlocks.h

@@ -87,7 +87,6 @@ class CChainLocksHandler : public CRecoveredSigsListener
     void ProcessNewChainLock(NodeId from, const CChainLockSig& clsig, const uint256& hash);
     void AcceptedBlockHeader(const CBlockIndex* pindexNew);
     void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork);
-    void NewPoWValidBlock(const CBlockIndex* pindex, const std::shared_ptr<const CBlock>& block);
     void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock);
     void TrySignChainTip();
     void EnforceBestChainLock();