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

jimpo · 2018-05-01T23:56:02Z

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 · 2018-05-02T00:27:19Z

test/functional/rpc_rawtransaction.py

F841 local variable 'prevout_index' is assigned to but never used

fanquake · 2018-05-02T00:31:06Z

Same error on two of the linux builds:

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

promag · 2018-05-03T00:46:28Z

src/rpc/rawtransaction.cpp

nStatus requires cs_main lock?

promag

Some comments.

promag · 2018-05-13T10:42:27Z

test/functional/rpc_rawtransaction.py

... without -txindex.?

promag · 2018-05-13T10:46:22Z

test/functional/rpc_rawtransaction.py

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

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.

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

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

promag · 2018-05-13T10:48:03Z

src/rpc/rawtransaction.cpp

nit, could put allow_slow after tx_hash.

promag · 2018-05-13T10:51:27Z

src/rpc/protocol.h

nit s/status/state.

DrahtBot · 2018-06-15T20:08:28Z

Needs rebase

DrahtBot · 2018-07-07T08:40:18Z

Needs rebase

jb55 · 2018-07-15T18:15:38Z

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.

DrahtBot · 2018-07-18T22:08:12Z

Needs rebase

DrahtBot · 2018-10-28T13:44:58Z

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.

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

fanquake reviewed May 2, 2018

View reviewed changes

test/functional/rpc_rawtransaction.py Outdated

Copy link

Member

fanquake May 2, 2018

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

jimpo force-pushed the gettransaction-refactor branch from c060157 to 0453681 Compare May 2, 2018 02:59

promag reviewed May 3, 2018

View reviewed changes

src/rpc/rawtransaction.cpp Outdated

Copy link

Contributor

promag May 3, 2018

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?

promag reviewed May 13, 2018

View reviewed changes

jimpo force-pushed the gettransaction-refactor branch from 0453681 to 1fdf98d Compare May 16, 2018 18:53

jimpo mentioned this pull request May 17, 2018

RPC: Introduce getblockstats to plot things #10757

Merged

DrahtBot mentioned this pull request Jun 7, 2018

Document validationinterace callback blocking deadlock potential. #13402

Merged

DrahtBot mentioned this pull request Jun 14, 2018

Consistently validate txid / blockhash length and encoding in rpc calls #13424

Merged

DrahtBot added the Needs rebase label Jun 15, 2018

jimpo force-pushed the gettransaction-refactor branch from 1fdf98d to 344d645 Compare June 18, 2018 18:22

DrahtBot removed the Needs rebase label Jun 19, 2018

jimpo force-pushed the gettransaction-refactor branch from 344d645 to 9ca6754 Compare June 24, 2018 18:26

DrahtBot mentioned this pull request Jun 28, 2018

BIP 174 PSBT Serializations and RPCs #13557

Merged

DrahtBot added the Needs rebase label Jul 7, 2018

jimpo force-pushed the gettransaction-refactor branch from 9ca6754 to 59b3ab2 Compare July 9, 2018 19:18

DrahtBot removed the Needs rebase label Jul 9, 2018

jimpo force-pushed the gettransaction-refactor branch from 59b3ab2 to a42534e Compare July 10, 2018 21:25

Jim Posen added 5 commits July 16, 2018 09:27

RPC: Improve error messages returned by gettxoutproof.

0f5c476

The error messages now indicate the status of the txindex.

RPC: Move GetTransaction from validation to rpc/rawtransaction.

6d787d4

This breaks a circular dependency between validation and txindex.

RPC: Refactor GetTransaction helper method.

ac35351

Break out GetTransactionInBlock into a separate method.

RPC: Return error message from GetTransaction helper.

8f3efeb

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

test: Test error message on getrawtransaction without txindex.

12c4386

jimpo force-pushed the gettransaction-refactor branch from a42534e to 12c4386 Compare July 16, 2018 16:49

DrahtBot added the Needs rebase label Jul 18, 2018

jimpo mentioned this pull request Aug 30, 2018

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942

Closed

DrahtBot added the Up for grabs label Oct 28, 2018

DrahtBot closed this Oct 28, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

jimpo commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 15, 2018

Uh oh!

DrahtBot commented Jul 7, 2018

Uh oh!

jb55 commented Jul 15, 2018

Uh oh!

DrahtBot commented Jul 18, 2018

Uh oh!

DrahtBot commented Oct 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jimpo commented May 1, 2018 •

edited

Loading