Merge #15159: [RPC] Remove lookup to UTXO set from GetTransaction

04da9f4834 [RPC] Update getrawtransaction interface (Amiti Uttarwar)

Pull request description:

  - stop checking unspent UTXOs for a transaction when txindex is not enabled, as per conversation here: https://github.com/bitcoin/bitcoin/issues/3220#issuecomment-377458383
  - code contributed by sipa

Tree-SHA512: aa07353bccc14b81b7803992a25d076d6bc06d15ec7c1b85828dc10aea7e0498d9b49f71783e352ab8a14b0bb2010cfb7835de3dfd1bc6f2323f460449348e66

12 files changed, 35 insertions, 28 deletions

diff --git a/doc/release-notes.md b/doc/release-notes.md
index 9e04f11635..a54b08848f 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -254,6 +254,12 @@ in the Low-level Changes section below.
 
 - See the [Mining](#mining) section for changes to `getblocktemplate`.
 
+- The `getrawtransaction` RPC no longer checks the unspent UTXO set for
+  a transaction. The remaining behaviors are as follows: 1. If a
+  blockhash is provided, check the corresponding block. 2. If no
+  blockhash is provided, check the mempool. 3. If no blockhash is
+  provided but txindex is enabled, also check txindex.
+
 Graphical User Interface (GUI)
 ------------------------------
 
diff --git a/src/rest.cpp b/src/rest.cpp
index c7a627d14e..326f7ae1d2 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -352,7 +352,7 @@ static bool rest_tx(HTTPRequest* req, const std::string& strURIPart)
 
     CTransactionRef tx;
     uint256 hashBlock = uint256();
-    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true))
+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock))
         return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not found");
 
     switch (rf) {
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 86cf121da2..fa0b62ec48 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1960,7 +1960,7 @@ static UniValue getblockstats(const JSONRPCRequest& request)
             for (const CTxIn& in : tx->vin) {
                 CTransactionRef tx_in;
                 uint256 hashBlock;
-                if (!GetTransaction(in.prevout.hash, tx_in, Params().GetConsensus(), hashBlock, false)) {
+                if (!GetTransaction(in.prevout.hash, tx_in, Params().GetConsensus(), hashBlock)) {
                     throw JSONRPCError(RPC_INTERNAL_ERROR, std::string("Unexpected internal error (tx index seems corrupt)"));
                 }
 
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 68e3fcda33..ac2e0ff4ee 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -67,12 +67,11 @@ static UniValue getrawtransaction(const JSONRPCRequest& request)
     if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
         throw std::runtime_error(
             RPCHelpMan{"getrawtransaction",
-                "\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
-                "enabled, it also works for blockchain transactions. If the block which contains the transaction\n"
-                "is known, its hash can be provided even for nodes without -txindex. Note that if a blockhash is\n"
-                "provided, only that block will be searched and if the transaction is in the mempool or other\n"
-                "blocks, or if this node does not have the given block available, the transaction will not be found.\n"
-            "DEPRECATED: for now, it also works for transactions with unspent outputs.\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"
 
             "\nReturn the raw transaction data.\n"
             "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
@@ -177,7 +176,7 @@ static UniValue getrawtransaction(const JSONRPCRequest& request)
 
     CTransactionRef tx;
     uint256 hash_block;
-    if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, blockindex)) {
         std::string errmsg;
         if (blockindex) {
             if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {
@@ -274,7 +273,7 @@ static UniValue gettxoutproof(const JSONRPCRequest& request)
     if (pblockindex == nullptr)
     {
         CTransactionRef tx;
-        if (!GetTransaction(oneTxid, tx, Params().GetConsensus(), hashBlock, false) || hashBlock.IsNull())
+        if (!GetTransaction(oneTxid, tx, Params().GetConsensus(), hashBlock) || hashBlock.IsNull())
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not yet in block");
         pblockindex = LookupBlockIndex(hashBlock);
         if (!pblockindex) {
diff --git a/src/validation.cpp b/src/validation.cpp
index 6a26bf9baa..de9c0d96db 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1002,13 +1002,11 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
  * Return transaction in txOut, and if it was found inside a block, its hash is placed in hashBlock.
  * If blockIndex is provided, the transaction is fetched from the corresponding block.
  */
-bool GetTransaction(const uint256& hash, CTransactionRef& txOut, const Consensus::Params& consensusParams, uint256& hashBlock, bool fAllowSlow, CBlockIndex* blockIndex)
+bool GetTransaction(const uint256& hash, CTransactionRef& txOut, const Consensus::Params& consensusParams, uint256& hashBlock, const CBlockIndex* const block_index)
 {
-    CBlockIndex* pindexSlow = blockIndex;
-
     LOCK(cs_main);
 
-    if (!blockIndex) {
+    if (!block_index) {
         CTransactionRef ptx = mempool.get(hash);
         if (ptx) {
             txOut = ptx;
@@ -1018,20 +1016,13 @@ bool GetTransaction(const uint256& hash, CTransactionRef& txOut, const Consensus
         if (g_txindex) {
             return g_txindex->FindTx(hash, hashBlock, txOut);
         }
-
-        if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it
-            const Coin& coin = AccessByTxid(*pcoinsTip, hash);
-            if (!coin.IsSpent()) pindexSlow = chainActive[coin.nHeight];
-        }
-    }
-
-    if (pindexSlow) {
+    } else {
         CBlock block;
-        if (ReadBlockFromDisk(block, pindexSlow, consensusParams)) {
+        if (ReadBlockFromDisk(block, block_index, consensusParams)) {
             for (const auto& tx : block.vtx) {
                 if (tx->GetHash() == hash) {
                     txOut = tx;
-                    hashBlock = pindexSlow->GetBlockHash();
+                    hashBlock = block_index->GetBlockHash();
                     return true;
                 }
             }
diff --git a/src/validation.h b/src/validation.h
index c0ffc9b0e4..4f70eb645f 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -269,7 +269,7 @@ void ThreadScriptCheck();
 /** Check whether we are doing an initial block download (synchronizing from disk or network) */
 bool IsInitialBlockDownload();
 /** Retrieve a transaction (from memory pool, or from disk, if possible) */
-bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, bool fAllowSlow = false, CBlockIndex* blockIndex = nullptr);
+bool GetTransaction(const uint256& hash, CTransactionRef& tx, const Consensus::Params& params, uint256& hashBlock, const CBlockIndex* const blockIndex = nullptr);
 /**
  * Find the best known block, and make it the tip of the block chain
  *
diff --git a/test/functional/feature_segwit.py b/test/functional/feature_segwit.py
index 4bcdf9af55..1efc50e71f 100755
--- a/test/functional/feature_segwit.py
+++ b/test/functional/feature_segwit.py
@@ -43,22 +43,26 @@ class SegWitTest(BitcoinTestFramework):
         self.setup_clean_chain = True
         self.num_nodes = 3
         # This test tests SegWit both pre and post-activation, so use the normal BIP9 activation.
+        # TODO: remove -txindex. Currently required for getrawtransaction call.
         self.extra_args = [
             [
                 "-rpcserialversion=0",
                 "-vbparams=segwit:0:999999999999",
                 "-addresstype=legacy",
+                "-txindex"
             ],
             [
                 "-blockversion=4",
                 "-rpcserialversion=1",
                 "-vbparams=segwit:0:999999999999",
                 "-addresstype=legacy",
+                "-txindex"
             ],
             [
                 "-blockversion=536870915",
                 "-vbparams=segwit:0:999999999999",
                 "-addresstype=legacy",
+                "-txindex"
             ],
         ]
 
diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py
index d5a1b53408..f850a54462 100755
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -41,7 +41,8 @@ class RESTTest (BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 2
-        self.extra_args = [["-rest"], []]
+        # TODO: remove -txindex. Currently required for getrawtransaction call.
+        self.extra_args = [["-rest", "-txindex"], []]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index 272ebe65cb..1e10280e60 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -19,6 +19,8 @@ class PSBTTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = False
         self.num_nodes = 3
+       # TODO: remove -txindex. Currently required for getrawtransaction call.
+        self.extra_args = [[], ["-txindex"], ["-txindex"]]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 5b9dbef68d..a97d753626 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -42,7 +42,8 @@ class RawTransactionsTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
-        self.extra_args = [["-addresstype=legacy"], ["-addresstype=legacy"], ["-addresstype=legacy"]]
+        # TODO: remove -txindex. Currently required for getrawtransaction call.
+        self.extra_args = [["-addresstype=legacy", "-txindex"], ["-addresstype=legacy", "-txindex"], ["-addresstype=legacy", "-txindex"]]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py
index e5ac2c8bd4..2b684349ce 100755
--- a/test/functional/wallet_abandonconflict.py
+++ b/test/functional/wallet_abandonconflict.py
@@ -18,7 +18,8 @@ from test_framework.util import assert_equal, assert_raises_rpc_error, connect_n
 class AbandonConflictTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
-        self.extra_args = [["-minrelaytxfee=0.00001"], []]
+        # TODO: remove -txindex. Currently required for getrawtransaction call.
+        self.extra_args = [["-minrelaytxfee=0.00001", "-txindex"], []]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py
index 7184bb8cb6..daae2ed3c0 100755
--- a/test/functional/wallet_basic.py
+++ b/test/functional/wallet_basic.py
@@ -23,6 +23,8 @@ class WalletTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 4
         self.setup_clean_chain = True
+        # TODO: remove -txindex. Currently required for getrawtransaction call.
+        self.extra_args = [[], [], ["-txindex"], []]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()