Merge bitcoin#22383: rpc: Prefer to use txindex if available for GetTransaction

78f4c8b prefer to use txindex if available for GetTransaction (Jameson Lopp)

Pull request description:

  Fixes bitcoin#22382

  Motivation: prevent excessive disk reads if txindex is enabled.

  Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?

ACKs for top commit:
  jonatack:
    ACK 78f4c8b
  theStack:
    Code review ACK 78f4c8b
  LarryRuane:
    tACK 78f4c8b
  luke-jr:
    utACK 78f4c8b
  jnewbery:
    utACK 78f4c8b
  rajarshimaitra:
    Code review ACK bitcoin@78f4c8b
  lsilva01:
    Code Review ACK and Tested ACK bitcoin@78f4c8b on Ubuntu 20.04

Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0

Author: MarcoFalke

src/rpc/rawtransaction.cpp

@@ -113,11 +113,11 @@ static UniValue getrawtransaction(const JSONRPCRequest& request)
     RPCHelpMan{
                 "getrawtransaction",
                 "\nReturn the raw transaction data.\n"
-                "\nBy default this function only works for mempool transactions. When called with a blockhash\n"
-                "argument, getrawtransaction will return the transaction if the specified block is available and\n"
-                "the transaction is found in that block. When called without a blockhash argument, getrawtransaction\n"
-                "will return the transaction if it is in the mempool, or if -txindex is enabled and the transaction\n"
-                "is in a block in the blockchain.\n"
+
+                "\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"
+                "the specified block is available and the transaction is found 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

@@ -1072,6 +1072,20 @@ CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMe
 {
     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)) {
@@ -1082,15 +1096,6 @@ CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMe
                 }
             }
         }
-        return nullptr;
-    }
-    if (mempool) {
-        CTransactionRef ptx = mempool->get(hash);
-        if (ptx) return ptx;
-    }
-    if (g_txindex) {
-        CTransactionRef tx;
-        if (g_txindex->FindTx(hash, hashBlock, tx)) return tx;
     }
     return nullptr;
 }

src/validation.h

@@ -185,15 +185,16 @@ void StartScriptCheckWorkerThreads(int threads_num);
 /** Stop all of the script checking worker threads */
 void StopScriptCheckWorkerThreads();
 /**
- * Return transaction from the block at block_index.
- * If block_index is not provided, fall back to mempool.
- * If mempool is not provided or the tx couldn't be found in mempool, fall back to g_txindex.
+ * 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 block_index is not provided, look in the mempool, if provided
+ * @param[in]  mempool         If provided, check mempool for tx
  * @param[in]  hash            The txid
  * @param[in]  consensusParams The params
- * @param[out] hashBlock       The hash of block_index, if the tx was found via block_index
+ * @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);