RPC: Improve error messages on RPC endpoints that use GetTransaction

jimpo commented at 11:56 pm on May 1, 2018: contributor

This contains some follow up items from #13033.

The PR refactors the GetTransaction function and moves it from validation to rpc/rawtransaction. This breaks a cyclic dependency between validation and index/txindex pointed out by @sipa. Also, the REST transaction API and gettxoutproof RPC now have more clear error messages when the txindex is not available, as requested by @TheBlueMatt.

This would also be a good opportunity to drop the slow tx lookup through the unspent coins view if people are for it, as proposed in #3220.

fanquake added the label RPC/REST/ZMQ on May 2, 2018

in test/functional/rpc_rawtransaction.py:236 in c0601575e9 outdated

175@@ -176,6 +176,28 @@ def run_test(self):
176         self.nodes[0].reconsiderblock(block1)
177         assert_equal(self.nodes[0].getbestblockhash(), block2)
178 
179+        # Create new transaction and don't broadcast.
180+        to_address = self.nodes[0].getnewaddress()
181+        prevout_index = [i for i, out in enumerate(gottx['vout']) if out['value'] == 1][0]

fanquake commented at 0:27 am on May 2, 2018:

F841 local variable ‘prevout_index’ is assigned to but never used

fanquake commented at 0:31 am on May 2, 2018: member

Same error on two of the linux builds:

 02018-05-02T00:14:12.349000Z TestFramework (INFO): sendrawtransaction with missing input
 12018-05-02T00:14:13.011000Z TestFramework (ERROR): JSONRPC error
 2Traceback (most recent call last):
 3  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 154, in main
 4    self.run_test()
 5  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_rawtransaction.py", line 195, in run_test
 6    self.nodes[0].sendrawtransaction(spending_tx)
 7  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/coverage.py", line 47, in __call__
 8    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 9  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 138, in __call__
10    raise JSONRPCException(response['error'])
11test_framework.authproxy.JSONRPCException: absurdly-high-fee, 4800006160 > 10000000 (code 256) (-26)
122018-05-02T00:14:13.012000Z TestFramework (INFO): Stopping nodes

jimpo force-pushed on May 2, 2018

in src/rpc/rawtransaction.cpp:237 in 1226aa943f outdated

232-            if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {
233-                throw JSONRPCError(RPC_MISC_ERROR, "Block not available");
234+    if (blockindex) {
235+        if (GetTransactionInBlock(hash, blockindex, tx)) {
236+            hash_block = blockindex->GetBlockHash();
237+        } else if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {

promag commented at 0:46 am on May 3, 2018:

nStatus requires cs_main lock?

in test/functional/rpc_rawtransaction.py:188 in 0453681b95 outdated

183+        outputs = {to_address: 0.9999}
184+        spending_rawtx = self.nodes[1].createrawtransaction(inputs, outputs)
185+        spending_tx = self.nodes[1].signrawtransactionwithwallet(spending_rawtx)['hex']
186+        spending_txid = bytes_to_hex_str(hash256(hex_str_to_bytes(spending_tx)))
187+
188+        # Searching unknown transaction with out txindex.

promag commented at 10:42 am on May 13, 2018:

... without -txindex.?

in test/functional/rpc_rawtransaction.py:250 in 0453681b95 outdated

190+
191+        # Searching unknown transaction with txindex.
192+        assert_raises_rpc_error(-5, "No such mempool or blockchain transaction.", self.nodes[2].getrawtransaction, spending_txid)
193+
194+        # Broadcast transaction to mempool.
195+        self.nodes[0].sendrawtransaction(spending_tx)

promag commented at 10:46 am on May 13, 2018:

IMO this could be removed (lines 193 to 200), does’t test error messages.

jimpo commented at 6:52 pm on May 16, 2018:

The purpose of the assertion on line 199 is to test that the error received on 192 goes away after transaction is in the mempool.

promag commented at 7:44 pm on May 16, 2018:

Right, a comment there would be nice then, like:

0# No error should be raised now that the transaction is known.

in src/rpc/rawtransaction.cpp:38 in bc6466ed4e outdated

35@@ -36,7 +36,8 @@
36 
37 #include <univalue.h>
38 
39-bool GetTransaction(const uint256& tx_hash, CTransactionRef& tx, uint256& block_hash, bool allow_slow)
40+bool GetTransaction(const uint256& tx_hash, CTransactionRef& tx, uint256& block_hash,

promag commented at 10:48 am on May 13, 2018:

nit, could put allow_slow after tx_hash.

in src/rpc/protocol.h:60 in 7b435e6df8 outdated

56@@ -57,6 +57,7 @@ enum RPCErrorCode
57     RPC_VERIFY_ALREADY_IN_CHAIN     = -27, //!< Transaction already in chain
58     RPC_IN_WARMUP                   = -28, //!< Client still warming up
59     RPC_METHOD_DEPRECATED           = -32, //!< RPC method is deprecated
60+    RPC_DATA_UNAVAILABLE            = -33, //!< Requested data is unavailable due to application configuration or status

promag commented at 10:51 am on May 13, 2018:

nit s/status/state.

promag commented at 10:54 am on May 13, 2018: member

Some comments.

jimpo force-pushed on May 16, 2018

DrahtBot commented at 8:08 pm on June 15, 2018: member

DrahtBot added the label Needs rebase on Jun 15, 2018

jimpo force-pushed on Jun 18, 2018

DrahtBot removed the label Needs rebase on Jun 19, 2018

jimpo force-pushed on Jun 24, 2018

DrahtBot added the label Needs rebase on Jul 7, 2018

DrahtBot commented at 8:40 am on July 7, 2018: member

jimpo force-pushed on Jul 9, 2018

DrahtBot removed the label Needs rebase on Jul 9, 2018

jimpo force-pushed on Jul 10, 2018

jb55 commented at 6:15 pm on July 15, 2018: member

Concept ACK, checked and it rebases cleanly

RPC: Improve error messages returned by gettxoutproof.

The error messages now indicate the status of the txindex.

0f5c476483

RPC: Move GetTransaction from validation to rpc/rawtransaction.

This breaks a circular dependency between validation and txindex.

6d787d4c52

RPC: Refactor GetTransaction helper method.

Break out GetTransactionInBlock into a separate method.

ac3535187b

RPC: Return error message from GetTransaction helper.

Since both the REST and RPC APIs use GetTransaction, there is no need
to duplicate the error handling logic.

8f3efeb11d

test: Test error message on getrawtransaction without txindex. 12c4386360

jimpo force-pushed on Jul 16, 2018

DrahtBot commented at 10:08 pm on July 18, 2018: member

DrahtBot added the label Needs rebase on Jul 18, 2018

DrahtBot commented at 1:44 pm on October 28, 2018: member

DrahtBot added the label Up for grabs on Oct 28, 2018

DrahtBot closed this on Oct 28, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Feb 15, 2022

RPC: Improve error messages on RPC endpoints that use GetTransaction #13144