New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RPC: Improve error messages on RPC endpoints that use GetTransaction #13144
Conversation
@@ -176,6 +176,28 @@ def run_test(self): | |||
self.nodes[0].reconsiderblock(block1) | |||
assert_equal(self.nodes[0].getbestblockhash(), block2) | |||
|
|||
# Create new transaction and don't broadcast. | |||
to_address = self.nodes[0].getnewaddress() | |||
prevout_index = [i for i, out in enumerate(gottx['vout']) if out['value'] == 1][0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F841 local variable 'prevout_index' is assigned to but never used
Same error on two of the linux builds:
|
c060157
to
0453681
Compare
src/rpc/rawtransaction.cpp
Outdated
if (blockindex) { | ||
if (GetTransactionInBlock(hash, blockindex, tx)) { | ||
hash_block = blockindex->GetBlockHash(); | ||
} else if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nStatus
requires cs_main
lock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments.
spending_tx = self.nodes[1].signrawtransactionwithwallet(spending_rawtx)['hex'] | ||
spending_txid = bytes_to_hex_str(hash256(hex_str_to_bytes(spending_tx))) | ||
|
||
# Searching unknown transaction with out txindex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... without -txindex.
?
assert_raises_rpc_error(-5, "No such mempool or blockchain transaction.", self.nodes[2].getrawtransaction, spending_txid) | ||
|
||
# Broadcast transaction to mempool. | ||
self.nodes[0].sendrawtransaction(spending_tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this could be removed (lines 193 to 200), does't test error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, a comment there would be nice then, like:
# No error should be raised now that the transaction is known.
src/rpc/rawtransaction.cpp
Outdated
@@ -36,7 +36,8 @@ | |||
|
|||
#include <univalue.h> | |||
|
|||
bool GetTransaction(const uint256& tx_hash, CTransactionRef& tx, uint256& block_hash, bool allow_slow) | |||
bool GetTransaction(const uint256& tx_hash, CTransactionRef& tx, uint256& block_hash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, could put allow_slow
after tx_hash
.
src/rpc/protocol.h
Outdated
@@ -57,6 +57,7 @@ enum RPCErrorCode | |||
RPC_VERIFY_ALREADY_IN_CHAIN = -27, //!< Transaction already in chain | |||
RPC_IN_WARMUP = -28, //!< Client still warming up | |||
RPC_METHOD_DEPRECATED = -32, //!< RPC method is deprecated | |||
RPC_DATA_UNAVAILABLE = -33, //!< Requested data is unavailable due to application configuration or status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit s/status/state.
0453681
to
1fdf98d
Compare
Needs rebase |
1fdf98d
to
344d645
Compare
344d645
to
9ca6754
Compare
Needs rebase |
9ca6754
to
59b3ab2
Compare
59b3ab2
to
a42534e
Compare
Concept ACK, checked and it rebases cleanly |
The error messages now indicate the status of the txindex.
This breaks a circular dependency between validation and txindex.
Break out GetTransactionInBlock into a separate method.
Since both the REST and RPC APIs use GetTransaction, there is no need to duplicate the error handling logic.
a42534e
to
12c4386
Compare
Needs rebase |
There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened. |
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.