Drop support for getrawtransaction for confirmed tx without txindex #13931

Accidental removal of this scope?

No, not accidental. Just wrong.

After this removal, where do you still use blockIndex? It seems to me that "lookup by block index" is now also removed. Is that intentional?

Oops, no, fixing.

It's a shame that this PR adds -txindex to a bunch of tests (since running with txindex is not the standard way to run bitcoind, so the tests are no longer testing the mainline case).

I don't think it's a show-stopper, but it'd be preferable to update the tests to not have to use txindex, or perhaps add a TODO to remove txindex at some point in the future. We could also add a comment on why txindex is needed for these tests to pass.

what is the standard way to run bitcoind? seems like there is some confusion in the original conversation too: #3220 (comment)

my understanding is three use cases for this method (still supported after this change):

1. with blockhash -> search specific block for txn
2. without blockhash, txindex not enabled -> check mempool
3. without blockhash, txindex enabled -> check all the things

the use case removed by this PR is

• without blockhash, txindex not enabled -> check unspent UTXOs

I have to take a closer look at how the tests work, but is the predominant usage to pass in a blockhash, or to be searching the mempool?

my understanding is...

yes, that's correct, although I'd be more specific in 3: ~check all the things~ -> check mempool and tx index.

is the predominant usage to pass in a blockhash

It depends on whether you're trying to find a confirmed or unconfirmed tx. I expect both usages are common.

~check all the things~ -> check mempool and tx index.

is there somewhere a transaction could be other than the mempool or tx index?

if -txindex is enabled then no - the txindex contains all the confirmed transactions, and your mempool contains the unconfirmed txs that the node is aware of (obviously excluding conflicts, replaced txs, non-standard txs, expired txs, etc)

ok kewl. just confirming that the only qualm was with the imprecision of language :) thanks!

I think this comment could be clearer now that the logic is more straigtforward. Something like:

• When called with a blockhash argument, getrawtransaction will return the transaction if the specified block is available and the transaction is found in that block.
• When called without a blockhash argument, getrawtransaction will return the transaction if it is in the mempool, or if -txindex is enabled and the transaction is in a block in the blockchain.

I'd start without blockhash argument (mempool only), then blockhash, then say blockhash can be omitted if txindex.

I'd start without blockhash argument (mempool only), then blockhash, then say blockhash can be omitted if txindex.

I don't think this captures the logic. "without blockhash argument (mempool only)" is not correct, since we'll check the whole blockchain if txindex is enabled. This also doesn't communicate that if blockhash is provided, then txindex won't be used to check the entire blockchain.

-txindex is a command line option, not a command :)

This function can be much simplified now that we're not looking for unspent coins in the UTXO set:

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 591943594..2d246e016 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -245,28 +245,17 @@ static UniValue gettxoutproof(const JSONRPCRequest& request)
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
         }
     } else {
-        LOCK(cs_main);
-
-        // Loop through txids and try to find which block they're in. Exit loop once a block is found.
-        for (const auto& tx : setTxids) {
-            const Coin& coin = AccessByTxid(*pcoinsTip, tx);
-            if (!coin.IsSpent()) {
-                pblockindex = chainActive[coin.nHeight];
-                break;
-            }
+        if (!g_txindex) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "blockhash must be provided when txindex is disabled.");
         }
-    }
-
 
-    // Allow txindex to catch up if we need to query it and before we acquire cs_main.
-    if (g_txindex && !pblockindex) {
-        g_txindex->BlockUntilSyncedToCurrentChain();
-    }
+        // Allow txindex to catch up if we need to query it and before we acquire cs_main.
+        if (!pblockindex) {
+            g_txindex->BlockUntilSyncedToCurrentChain();
+        }
 
-    LOCK(cs_main);
+        LOCK(cs_main);
 
-    if (pblockindex == nullptr)
-    {
         CTransactionRef tx;
         if (!GetTransaction(oneTxid, tx, Params().GetConsensus(), hashBlock) || hashBlock.IsNull())
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not yet in block");

(also at https://github.com/jnewbery/bitcoin/tree/pr13931.1 / https://github.com/jnewbery/bitcoin/commit/eeef25bfb5470efddfb2bf3e998dfb124531e473)

This also improves error reporting if a blockhash is not provided and txindex is not enabled ("blockhash must be provided when txindex is disabled." instead of "Transaction not yet in block")

Tests also need to be updated with this change: https://github.com/jnewbery/bitcoin/commit/0ffbc2055908b2fa1797f92b4391c2bbbbec588a

That "blockhash must be provided" message is a very useful addition to gettxoutproof (I just got confused by it on another branch).