test: get and decode tx with a single gettransaction RPC call#23287
Conversation
|
Tested ACK c045207fb4a96f3cbdf7841ecab5c36323ea01a3. Ran the tests and reviewed the changes. |
|
Concept ACK I think mempool_packages.py (line 68 and 69) could be also included here, couldn't it? |
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Code review ACK c045207fb4a96f3cbdf7841ecab5c36323ea01a3. Didn't check if same approach could be applied in other places.
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
Concept ACK
This PR simplifies the code and RPC calls around getting a transaction and decoding it. I was able to successfully compile the PR on Ubuntu 20.04 without any errors. Nice catch, @theStack!
I think mempool_packages.py (line 68 and 69) could be also included here, couldn't it?
I think this is a valid suggestion. I tried to implement this myself while testing, and I was able to successfully do so in the following way:
# We need the wtxids to check P2P announcements
- fulltx = self.nodes[0].getrawtransaction(txid)
- witnesstx = self.nodes[0].decoderawtransaction(fulltx, True)
+ witnesstx = self.nodes[0].gettransaction(txid=txid, verbose=True)['decoded']
witness_chain.append(witnesstx['hash'])
brunoerg
left a comment
brunoerg
left a comment
There was a problem hiding this comment.
tACK c045207fb4a96f3cbdf7841ecab5c36323ea01a3
Great! I am gonna check if there are other places it could be applied |
Rather than subsequently calling `gettransaction` and `decoderawtransaction` to get the decoded information for a specific tx-id, we can simply use the verbose version of `gettransaction`, which returns this in a 'decoded' key. I.e. node.decoderawtransaction(node.gettransaction(txid)['hex']) can be replaced by: node.gettransaction(txid=txid, verbose=True)['decoded']
theStack
force-pushed
the
202110-test-fetch_and_decode_tx_with_single_RPC_call
branch
from
c045207 to
130ee48
Compare
|
Thanks for all the reviews! @stratospher: Note that I intentionally didn't change anything in rpc_rawtransaction.py, as I think this test's focus are RPCs that cope with raw transactions (like decoderawtransaction). |
|
tested ACK 130ee48
@theStack, Thanks for clarifying. That makes total sense! |
|
tACK 130ee48 |
| # We need the wtxids to check P2P announcements | ||
| fulltx = self.nodes[0].getrawtransaction(txid) | ||
| witnesstx = self.nodes[0].decoderawtransaction(fulltx, True) | ||
| witnesstx = self.nodes[0].gettransaction(txid=txid, verbose=True)['decoded'] |
There was a problem hiding this comment.
this is a wallet rpc and eventually the mempool tests shouldn't depend on bdb or sqlite
…ction` RPC call 130ee48 test: get and decode tx with a single `gettransaction` RPC call (Sebastian Falbesoner) Pull request description: Rather than subsequently calling `gettransaction` and `decoderawtransaction` to get the decoded information for a specific tx-id, we can simply use the verbose version of `gettransaction`, which returns this in a 'decoded' key. I.e. ``` node.decoderawtransaction(node.gettransaction(txid)['hex']) ``` can simply be replaced by: ``` node.gettransaction(txid=txid, verbose=True)['decoded'] ``` Rationale: shorter code, shorter test logs, less RPC calls. ACKs for top commit: stratospher: tested ACK 130ee48 amadeuszpawlik: tACK 130ee48 lsilva01: Tested ACK 130ee48 on Ubuntu 20.04. shaavan: ACK 130ee48 Tree-SHA512: cf0bd26e1e21b8022fb8062857906e0706f0ee32d3277f985c461e2519405afe445ab005f5f763fb268c7b4d6e48b2d47eda7af8621b3bce67cece8dfc9bc153
9 participants
Rather than subsequently calling
gettransactionanddecoderawtransactionto get the decoded information for a specific tx-id, we can simply use the verbose version ofgettransaction, which returns this in a 'decoded' key. I.e.can simply be replaced by:
Rationale: shorter code, shorter test logs, less RPC calls.