refactor: replace helper GetTransactionBlock to direct usage of GetTransaction

Author: knst

src/llmq/chainlocks.cpp

@@ -26,7 +26,12 @@
 using node::ReadBlockFromDisk;
 
 // Forward declaration to break dependency over node/transaction.h
-std::pair<CTransactionRef, uint256> GetTransactionBlock(const uint256& hash, const CTxMemPool* const mempool);
+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::GetTransaction;
 
 static bool ChainLocksSigningEnabled(const CSporkManager& sporkman)
 {
@@ -640,8 +645,8 @@ void CChainLocksHandler::Cleanup()
         }
     }
     for (auto it = txFirstSeenTime.begin(); it != txFirstSeenTime.end(); ) {
-        auto [tx, hashBlock] = GetTransactionBlock(it->first, &mempool);
-        if (!tx) {
+        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()) {

src/llmq/instantsend.cpp

@@ -31,7 +31,12 @@ using node::fImporting;
 using node::fReindex;
 
 // Forward declaration to break dependency over node/transaction.h
-std::pair<CTransactionRef, uint256> GetTransactionBlock(const uint256& hash, const CTxMemPool* const mempool);
+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::GetTransaction;
 
 namespace llmq
 {
@@ -580,7 +585,8 @@ bool CInstantSendManager::CheckCanLock(const COutPoint& outpoint, bool printDebu
         return false;
     }
 
-    auto [tx, hashBlock] = GetTransactionBlock(outpoint.hash, &mempool);
+    uint256 hashBlock{};
+    const auto tx = GetTransaction(nullptr, &mempool, outpoint.hash, params, hashBlock);
     // 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) {
@@ -637,7 +643,9 @@ void CInstantSendManager::HandleNewInputLockRecoveredSig(const CRecoveredSig& re
         g_txindex->BlockUntilSyncedToCurrentChain();
     }
 
-    auto [tx, hashBlock] = GetTransactionBlock(txid, &mempool);
+
+    uint256 hashBlock{};
+    const auto tx = GetTransaction(nullptr, &mempool, txid, Params().GetConsensus(), hashBlock);
     if (!tx) {
         return;
     }
@@ -1019,7 +1027,8 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm
         return;
     }
 
-    auto [tx, hashBlock] = GetTransactionBlock(islock->txid, &mempool);
+    uint256 hashBlock{};
+    const auto tx = GetTransaction(nullptr, &mempool, islock->txid, Params().GetConsensus(), hashBlock);
     const CBlockIndex* pindexMined{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()) {

src/node/transaction.cpp

@@ -150,10 +150,3 @@ CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMe
     return nullptr;
 }
 } // namespace node
-
-std::pair<CTransactionRef, uint256> GetTransactionBlock(const uint256& hash, const CTxMemPool* const mempool)
-{
-    uint256 hashBlock{};
-    if (!g_txindex && !mempool) return {nullptr, hashBlock}; // Fast-fail as we don't have any other way to search
-    return {GetTransaction(/*block_index=*/nullptr, mempool, hash, Params().GetConsensus(), hashBlock), hashBlock};
-}

src/node/transaction.h

@@ -60,13 +60,4 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
 CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock);
 } // namespace node
 
-/**
- * Retrieve transaction and block from txindex (or mempool)
- * That's just a wrapper over GetTransaction to break circular dependencies
- * and to simplify caller for case if no block_index
- */
-std::pair</*tx=*/CTransactionRef, /*hash_block=*/uint256> GetTransactionBlock(const uint256& hash,
-                                                                              const CTxMemPool* const mempool);
-
-
 #endif // BITCOIN_NODE_TRANSACTION_H

src/validation.cpp

@@ -138,7 +138,11 @@ uint256 hashAssumeValid;
 arith_uint256 nMinimumChainWork;
 
 // Forward declaration to break dependency over node/transaction.h
-std::pair<CTransactionRef, uint256> GetTransactionBlock(const uint256& hash, const CTxMemPool* const mempool);
+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::GetTransaction;
 
 const CBlockIndex* CChainState::FindForkInGlobalIndex(const CBlockLocator& locator) const
 {
@@ -754,7 +758,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     if (auto conflictLockOpt = m_chain_helper.ConflictingISLockIfAny(tx); conflictLockOpt.has_value()) {
         auto& [_, conflict_txid] = conflictLockOpt.value();
 
-        if (auto [txConflict, _] = GetTransactionBlock(conflict_txid, &m_pool); txConflict) {
+        uint256 hashBlock;
+        if (auto txConflict = GetTransaction(nullptr, &m_pool, conflict_txid, chainparams.GetConsensus(), hashBlock); txConflict) {
             GetMainSignals().NotifyInstantSendDoubleSpendAttempt(ptx, txConflict);
         }
         LogPrintf("ERROR: AcceptToMemoryPool : Transaction %s conflicts with locked TX %s\n", hash.ToString(), conflict_txid.ToString());