Merge bitcoin/bitcoin#22528: refactor: move GetTransaction to node/transaction.cpp

f685a13bef doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f08 refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)

Pull request description:

  ~This PR is based on #22383, which should be reviewed first~ (merged by now).

  In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.

  Relevant IRC log:
  ```
  17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
  17:53 <raj_> jnewbery, +1
  17:53 <stickies-v> agreed!
  17:54 <glozow> jnewbery ya
  17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
  17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
  17:55 <sipa> (before 0.8, validation itself used the txindex)
  17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
  17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
  17:55 <sipa> jnewbery: sure, just providing background
  17:56 <sipa> seems very reasonable to move it elsewhere now
  ```

  The commit should be trivial to review with `--color-moved`.

ACKs for top commit:
  jnewbery:
    Code review ACK f685a13bef
  rajarshimaitra:
    tACK f685a13bef
  mjdietzx:
    crACK f685a13bef
  LarryRuane:
    Code review, test ACK f685a13bef

Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c

src/node/transaction.cpp

@@ -4,9 +4,12 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <consensus/validation.h>
+#include <index/txindex.h>
 #include <net.h>
 #include <net_processing.h>
+#include <node/blockstorage.h>
 #include <node/context.h>
+#include <txmempool.h>
 #include <validation.h>
 #include <validationinterface.h>
 #include <node/transaction.h>
@@ -119,3 +122,38 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
 
     return TransactionError::OK;
 }
+
+CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock)
+{
+    LOCK(cs_main);
+
+    if (mempool && !block_index) {
+        CTransactionRef ptx = mempool->get(hash);
+        if (ptx) return ptx;
+    }
+    if (g_txindex) {
+        CTransactionRef tx;
+        uint256 block_hash;
+        if (g_txindex->FindTx(hash, block_hash, tx)) {
+            if (!block_index || block_index->GetBlockHash() == block_hash) {
+                // Don't return the transaction if the provided block hash doesn't match.
+                // The case where a transaction appears in multiple blocks (e.g. reorgs or
+                // BIP30) is handled by the block lookup below.
+                hashBlock = block_hash;
+                return tx;
+            }
+        }
+    }
+    if (block_index) {
+        CBlock block;
+        if (ReadBlockFromDisk(block, block_index, consensusParams)) {
+            for (const auto& tx : block.vtx) {
+                if (tx->GetHash() == hash) {
+                    hashBlock = block_index->GetBlockHash();
+                    return tx;
+                }
+            }
+        }
+    }
+    return nullptr;
+}

src/node/transaction.h

@@ -10,7 +10,12 @@
 #include <primitives/transaction.h>
 #include <util/error.h>
 
+class CBlockIndex;
+class CTxMemPool;
 struct NodeContext;
+namespace Consensus {
+struct Params;
+}
 
 /** Maximum fee rate for sendrawtransaction and testmempoolaccept RPC calls.
  * Also used by the GUI when broadcasting a completed PSBT.
@@ -38,4 +43,19 @@ static const CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};
  */
 [[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback);
 
+/**
+ * Return transaction with a given hash.
+ * If mempool is provided and block_index is not provided, check it first for the tx.
+ * If -txindex is available, check it next for the tx.
+ * Finally, if block_index is provided, check for tx by reading entire block from disk.
+ *
+ * @param[in]  block_index     The block to read from disk, or nullptr
+ * @param[in]  mempool         If provided, check mempool for tx
+ * @param[in]  hash            The txid
+ * @param[in]  consensusParams The params
+ * @param[out] hashBlock       The block hash, if the tx was found via -txindex or block_index
+ * @returns                    The tx if found, otherwise nullptr
+ */
+CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock);
+
 #endif // BITCOIN_NODE_TRANSACTION_H

src/rpc/rawtransaction.cpp

@@ -76,8 +76,8 @@ static RPCHelpMan getrawtransaction()
 
                 "\nBy default, this call only returns a transaction if it is in the mempool. If -txindex is enabled\n"
                 "and no blockhash argument is passed, it will return the transaction if it is in the mempool or any block.\n"
-                "If -txindex is not enabled and a blockhash argument is passed, it will return the transaction if\n"
+                "If a blockhash argument is passed, it will return the transaction if\n"
-                "the specified block is available and the transaction is found in that block.\n"
+                "the specified block is available and the transaction is in that block.\n"
                 "\nHint: Use gettransaction for wallet transactions.\n"
 
                 "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"

src/validation.cpp

@@ -19,7 +19,6 @@
 #include <flatfile.h>
 #include <hash.h>
 #include <index/blockfilterindex.h>
-#include <index/txindex.h>
 #include <logging.h>
 #include <logging/timer.h>
 #include <node/blockstorage.h>
@@ -1155,38 +1154,6 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx
     return result;
 }
 
-CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock)
-{
-    LOCK(cs_main);
-
-    if (mempool && !block_index) {
-        CTransactionRef ptx = mempool->get(hash);
-        if (ptx) return ptx;
-    }
-    if (g_txindex) {
-        CTransactionRef tx;
-        uint256 block_hash;
-        if (g_txindex->FindTx(hash, block_hash, tx)) {
-            if (!block_index || block_index->GetBlockHash() == block_hash) {
-                hashBlock = block_hash;
-                return tx;
-            }
-        }
-    }
-    if (block_index) {
-        CBlock block;
-        if (ReadBlockFromDisk(block, block_index, consensusParams)) {
-            for (const auto& tx : block.vtx) {
-                if (tx->GetHash() == hash) {
-                    hashBlock = block_index->GetBlockHash();
-                    return tx;
-                }
-            }
-        }
-    }
-    return nullptr;
-}
-
 CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
 {
     int halvings = nHeight / consensusParams.nSubsidyHalvingInterval;

src/validation.h

@@ -140,20 +140,7 @@ void UnloadBlockIndex(CTxMemPool* mempool, ChainstateManager& chainman);
 void StartScriptCheckWorkerThreads(int threads_num);
 /** Stop all of the script checking worker threads */
 void StopScriptCheckWorkerThreads();
-/**
+
- * Return transaction with a given hash.
- * If mempool is provided and block_index is not provided, check it first for the tx.
- * If -txindex is available, check it next for the tx.
- * Finally, if block_index is provided, check for tx by reading entire block from disk.
- *
- * @param[in]  block_index     The block to read from disk, or nullptr
- * @param[in]  mempool         If provided, check mempool for tx
- * @param[in]  hash            The txid
- * @param[in]  consensusParams The params
- * @param[out] hashBlock       The block hash, if the tx was found via -txindex or block_index
- * @returns                    The tx if found, otherwise nullptr
- */
-CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMemPool* const mempool, const uint256& hash, const Consensus::Params& consensusParams, uint256& hashBlock);
 CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams);
 
 bool AbortNode(BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = bilingual_str{});

test/lint/lint-circular-dependencies.sh

@@ -10,7 +10,6 @@ export LC_ALL=C
 
 EXPECTED_CIRCULAR_DEPENDENCIES=(
     "chainparamsbase -> util/system -> chainparamsbase"
-    "index/txindex -> validation -> index/txindex"
     "node/blockstorage -> validation -> node/blockstorage"
     "index/blockfilterindex -> node/blockstorage -> validation -> index/blockfilterindex"
     "index/base -> validation -> index/blockfilterindex -> index/base"