wouldn’t it make sense to combine the !blockindex check here?
dougEfresh
commented at 12:42 pm on October 20, 2021:
yes, and moved
in
src/rpc/rawtransaction.cpp:101
in
2730b680d2outdated
97@@ -97,6 +98,7 @@ static RPCHelpMan getrawtransaction()
98 {
99 {RPCResult::Type::BOOL, "in_active_chain", /* optional */ true, "Whether specified block is in the active chain or not (only present with explicit \"blockhash\" argument)"},
100 {RPCResult::Type::STR_HEX, "hex", "The serialized, hex-encoded data for 'txid'"},
101+ {RPCResult::Type::STR_AMOUNT, "fee", "transaction fee in " + CURRENCY_UNIT},
I mean why are you changing this at all? Seems unrelated and the existing code should work fine.
dougEfresh
commented at 2:18 pm on October 20, 2021:
you said “wouldn’t it make sense to combine the !blockindex check here?”
in your inital review, i thought you meant combine !blockindex with my new if statement
Yes, but the blockindex check can stay as-is. Also, I think it makes more sense to clarify the condition on when the field is existing/missing.
0if (a) pushKV();
is clearer than
0if (something)
1 if (a) pushKV();
2 return
3pushKV();
dougEfresh
commented at 6:50 pm on October 20, 2021:
Gotcha, Hopefully now it looks better
dougEfresh force-pushed
on Oct 20, 2021
lsilva01 approved
lsilva01
commented at 2:36 pm on October 20, 2021:
contributor
Concept ACK.
It would be interesting to add a functional test case.
dougEfresh force-pushed
on Oct 20, 2021
maflcko
commented at 6:52 pm on October 20, 2021:
member
I think this has been said on IRC already: Shouldn’t this be hidden behind a flag like the RPC it is copied from?
dougEfresh
commented at 6:53 pm on October 20, 2021:
contributor
Concept ACK.
It would be interesting to add a functional test case.
Agreed, wanted to discuss the implementation before diving in.
Anything special I should know about testing ./test/functional/rpc_rawtransaction.py ? This is my 1st time doing functional test.
dougEfresh
commented at 7:08 pm on October 20, 2021:
contributor
I think this has been said on IRC already: Shouldn’t this be hidden behind a flag like the RPC it is copied from?
To confirm we want the second (hybrid) parameter to have three verbosity levels:
0 or false - hex encoded
1 or true - returns an Object with information about txid
2 - adds fee field to Object
maflcko
commented at 7:11 pm on October 20, 2021:
member
Yes, if that is the behavior of the other RPC, it makes sense to copy it.
sipa
commented at 7:17 pm on October 20, 2021:
member
You might want to also add the UTXOs being spent by the transaction. You’re looking those up anyway.
maflcko
commented at 11:22 am on October 21, 2021:
member
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#26039 (refactor: Run type check against RPCArgs (1/2) by MarcoFalke)
#19262 (rpc: Replace OMITTED_NAMED_ARG with OMITTED by MarcoFalke)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
in
src/rpc/rawtransaction.cpp:232
in
83e5f6c584outdated
Is there a reason you don’t add verbosity == 1 to this conjunction instead of duplicating the same TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate()); return result; above?
dougEfresh
commented at 5:51 pm on October 28, 2021:
I can move it to the if (!blockindex || IsBlockPruned(blockindex)... if you like.
dougEfresh
commented at 11:43 am on October 29, 2021:
For code clarity, I moved verbosity == 1 to the if block.
The performance impact is marginal
dougEfresh
commented at 6:02 pm on October 28, 2021:
contributor
You might want to also add the UTXOs being spent by the transaction. You’re looking those up anyway.
@sipa Can you clarify a bit more on add the UTXOs being spent ? Specifically, what you like to add to the response body?
sipa
commented at 6:20 pm on October 28, 2021:
member
@dougEfresh The scriptPubKey and amount being spent for each input (together with information about the scriptPubKey like the output already has).
maflcko
commented at 9:11 am on October 29, 2021:
member
in
src/rpc/rawtransaction.cpp:87
in
2c56d72acaoutdated
8586- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
87- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n",
88+ "If verbose is 0, 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n"
89+ "If verbose is 1 or 'true', returns an Object with information about 'txid'.\n"
90+ "If verbose is 2, returns an Object with information about 'txid', including fees and prevout information for inputs(only for unpruned blocks in the current best chain).\n",
dougEfresh
commented at 11:52 am on October 29, 2021:
I will add a space between inputs(only after review by others.
in
src/rpc/rawtransaction.cpp:85
in
2c56d72acaoutdated
79@@ -79,13 +80,14 @@ static RPCHelpMan getrawtransaction()
80 "and no blockhash argument is passed, it will return the transaction if it is in the mempool or any block.\n"
81 "If a blockhash argument is passed, it will return the transaction if\n"
82 "the specified block is available and the transaction is in that block.\n"
83- "\nHint: Use gettransaction for wallet transactions.\n"
84+ "\nHint: Use gettransaction for wallet transactions.\n\n"
8586- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
87- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n",
88+ "If verbose is 0, 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n"
in
src/rpc/rawtransaction.cpp:90
in
2c56d72acaoutdated
89+ "If verbose is 1 or 'true', returns an Object with information about 'txid'.\n"
90+ "If verbose is 2, returns an Object with information about 'txid', including fees and prevout information for inputs(only for unpruned blocks in the current best chain).\n",
91 {
92 {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"},
93- {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "If false, return a string, otherwise return a json object"},
94+ {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0|false for hex-encoded data, 1|true for a json object, and 2 for json object with fee"},
I also agree with @luke-jr . There is no guarantee that fee calculation will always remain same, and thus better not to assert against hard coded values.
dougEfresh force-pushed
on Nov 2, 2021
in
src/rpc/rawtransaction.cpp:322
in
f30f007c82outdated
236+ for (size_t i = 0; i < block.vtx.size(); ++i) {
237+ const CTransactionRef t = block.vtx.at(i);
238+ if (*t == *tx) {
239+ // blockundo does not have coinbase tx
240+ opt_tx_position = std::optional<size_t>{i-1};
241+ break;
There was a question about the performance implications of this loop. I was digging through the code and I was perhaps thinking that CBlockUndo could maybe benefit from a faster data structure to find a transaction? Maybe a method that returns the data faster.
Perhaps that is a later improvement and off topic?
dougEfresh
commented at 9:56 pm on November 14, 2021:
Someone did suggest a more performant solution, I can’t find it at the moment though.
in
doc/release-notes.md:105
in
f30f007c82outdated
98@@ -99,6 +99,11 @@ Updated RPCs
99 causes the lock to be written persistently to the wallet database. This
100 allows UTXOs to remain locked even after node restarts or crashes. (#23065)
101102+- `getrawtransaction` RPC verbose argument changed to verbosity levels: 0, 1 and 2 (backwards compatible).
103+ Verbosity 0 and 1 correspond to previous (verbose) argument's values `false` and `true`.
104+ Verbosity `3` contains an optional `fee` field and optional `prevout` subfield
105+ describing the spent output. See `getblock` RPC for `prevout` details.
rajarshimaitra
commented at 3:13 pm on November 14, 2021:
Probably I am missing something, but I am not finding any prevout reference in the getblock help doc. Is it referring to it’s internal implementation? Maybe this line could be made a little more clear?
dougEfresh
commented at 9:51 pm on November 14, 2021:
I was referring to the getblock in the above paragraph of the release notes.
I updated to explicitly reference the above paragraph.
in
src/rpc/rawtransaction.cpp:83
in
f30f007c82outdated
79@@ -79,13 +80,14 @@ static RPCHelpMan getrawtransaction()
80 "and no blockhash argument is passed, it will return the transaction if it is in the mempool or any block.\n"
81 "If a blockhash argument is passed, it will return the transaction if\n"
82 "the specified block is available and the transaction is in that block.\n"
83- "\nHint: Use gettransaction for wallet transactions.\n"
84+ "\nHint: Use gettransaction for wallet transactions.\n\n"
rajarshimaitra
commented at 3:40 pm on November 14, 2021:
To keep parity with lines below, maybe also remove leading \n from here?
dougEfresh
commented at 9:47 pm on November 14, 2021:
I moved the \n(s)
in
src/rpc/rawtransaction.cpp:87
in
f30f007c82outdated
8586- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
87- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n",
88+ "If verbosity is 0 or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n"
89+ "If verbosity is 1, returns an Object with information about 'txid'.\n"
90+ "If verbosity is 2, returns an Object with information about 'txid', including fees and prevout information for inputs (only for unpruned blocks in the current best chain).\n",
rajarshimaitra
commented at 3:55 pm on November 14, 2021:
I think the framing can be improved a bit.
Using “transaction” instead of “txid”. They are not in general replaceable with each other for all contexts. It’s clearer to say “details of transaction” than “details of txid”.
The verbosity 2 comment can feel a little confusing for users on the “prevout for inputs” part. It might sound at first glance as the prevout for the tx containing inputs of the given tx. I think keeping it simple like " … , including fee and prevout information … " only is clear enough. The only structure related to prevout in a transaction is inputs, so its redundant to explicitly specify.
dougEfresh
commented at 9:50 pm on November 14, 2021:
See latest latest changes
I added separate RPC documentation for verbosity=2
0Result (for verbosity = 2):
1{ (json object)
2 ..., Same output as verbosity = 1
3 "fee" : n, (numeric, optional) transaction fee in BTC, omitted if block undo data is not available
4 "vin" : [ (json array)
5 { (json object, optional) utxo being spent, omitted if block undo data is not available
6 ..., Same output as verbosity = 1
7 "prevout" : { (json object)
8 "value" : n, (numeric) The value in BTC
9 "height" : n, (numeric) previous block height
10 "scriptPubKey" : { (json object)
11 "asm" : "str", (string) the asm
12 "hex" : "str", (string) the hex
13 "type" : "str", (string) The type, eg 'pubkeyhash'
14 "address" : "str" (string, optional) The Bitcoin address (only if a well-defined address exists)
15 }
16 }
17 },
18 ...
19 ]
20}
in
src/rpc/rawtransaction.cpp:239
in
f30f007c82outdated
rajarshimaitra
commented at 5:31 pm on November 14, 2021:
I am not entirely sure, should we still keep the previous usage in the examples above this line?
Even though the bool values are still accepted, the help doc doesn’t include it anymore, so it might be confusing to users.
dougEfresh
commented at 9:48 pm on November 14, 2021:
Agreed and changed
rajarshimaitra changes_requested
rajarshimaitra
commented at 6:06 pm on November 14, 2021:
contributor
Initial concept ACK. This is really helpful for wallets trying to sync with core rpc out there, and it will reduce get calls by half for input fetching. Thanks for working on this.
Below are few minor nits.
Will review in more detail later.
dougEfresh
commented at 10:04 pm on November 14, 2021:
contributor
Initial concept ACK. This is really helpful for wallets trying to sync with core rpc out there, and it will reduce get calls by half for input fetching. Thanks for working on this.
Below are few minor nits.
Will review in more detail later.
@rajarshimaitra thanks for feedback. The test I added could use a good review.
dougEfresh force-pushed
on Nov 14, 2021
in
src/rpc/rawtransaction.cpp:321
in
fb2f153136outdated
261+ }
262+ for (size_t i = 0; i < block.vtx.size(); ++i) {
263+ const CTransactionRef t = block.vtx.at(i);
264+ if (*t == *tx) {
265+ // blockundo does not have coinbase tx
266+ opt_tx_position = std::optional<size_t>{i-1};
stickies-v
commented at 5:16 pm on November 15, 2021:
This overflows in the case of coinbase transaction (i==0). To bring it in line with getblock (where the coinbase transaction is serialized without fees/prevouts when verbosity is set to 3), you could add another OR clause on #255 to also check for tx->IsCoinBase(), for example?
dougEfresh
commented at 10:20 am on November 16, 2021:
Thanks @stickies-v
I’ll also add a function test to catch this condition
in
src/rpc/rawtransaction.cpp:54
in
fb2f153136outdated
51 //
52 // Blockchain contextual information (confirmations and blocktime) is not
53 // available to code in bitcoin-common, so we query them here and push the
54 // data into the returned UniValue.
55- TxToUniv(tx, uint256(), entry, true, RPCSerializationFlags());
56+ TxToUniv(tx, uint256(), entry, true, RPCSerializationFlags(), txundo, TxVerbosity::SHOW_DETAILS_AND_PREVOUT);
stickies-v
commented at 7:29 pm on November 15, 2021:
I understand that functionally this works fine, but it feels unnecessarily confusing to always pass TxVerbosity::SHOW_DETAILS_AND_PREVOUT even in the case when in actuality we’re trying to get TxVerbosity::SHOW_DETAILS but relying on txundo being null to achieve that. Would it not be more prudent to instead add verbosity as an optional argument to TxToJSON and simply pass that along?
dougEfresh
commented at 1:57 pm on November 22, 2021:
I added an arg for explicit TxVerbosity in TxToJSON
stickies-v
commented at 7:41 pm on November 15, 2021:
contributor
Approach ACKfb2f15313
Did some testing and it looks like the RPC is failing for coinbase transactions when verbosity==2. In this case we should probably fallback to verbosity==1 behaviour instead?
I am not sure why, but I am hitting this weird error in the test
0$ ./test/functional/rpc_rawtransaction.py
12021-11-16T16:12:39.573000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_1ldkso58
22021-11-16T16:12:40.405000Z TestFramework (INFO): Prepare some coins for multiple *rawtransaction commands
32021-11-16T16:12:43.883000Z TestFramework (INFO): Test getrawtransaction with -txindex
42021-11-16T16:12:43.890000Z TestFramework (ERROR): Assertion failed
5Traceback (most recent call last):
6 File "/home/raj/github-repo/bitcoin/test/functional/test_framework/util.py", line 133, in try_rpc
7 fun(*args, **kwds)
8 File "/home/raj/github-repo/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
9 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
10 File "/home/raj/github-repo/bitcoin/test/functional/test_framework/authproxy.py", line 144, in __call__
11 raise JSONRPCException(response['error'], status)
12test_framework.authproxy.JSONRPCException: Unknown named parameter verbose (-8)
It seems its coming from the named arguments in getrawtransaction() calls, like txid, verbose etc. First error is hitting at line 130. Removing the named argument and simply passing in the value works. But I am not sure why would named arguments suddenly stop working. Things are fine at master.
dougEfresh
commented at 9:43 am on November 17, 2021:
contributor
I am not sure why, but I am hitting this weird error in the test
0$ ./test/functional/rpc_rawtransaction.py
I saw that as well, I’m fixing.
dougEfresh force-pushed
on Nov 18, 2021
in
test/functional/rpc_rawtransaction.py:178
in
fee0b82acaoutdated
173@@ -171,6 +174,67 @@ def getrawtransaction_tests(self):
174 block = self.nodes[0].getblock(self.nodes[0].getblockhash(0))
175 assert_raises_rpc_error(-5, "The genesis block coinbase is not considered an ordinary transaction", self.nodes[0].getrawtransaction, block['merkleroot'])
176177+ def getrawtransaction_verbosity_tests(self):
dougEfresh
commented at 10:25 am on November 18, 2021:
I added a separate test to validate output fields, only that they exists, not their values.
Is this worth having?
in
src/rpc/rawtransaction.cpp:140
in
fee0b82acaoutdated
92+ "If verbosity is 1, returns an Object with information about transaction.\n"
93+ "If verbosity is 2, returns an Object with information about transaction, including fees and prevout information.\n",
94 {
95 {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"},
96- {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "If false, return a string, otherwise return a json object"},
97+ {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for hex-encoded data, 1 for a json object, and 2 for json object with fee and prevout"},
dougEfresh
commented at 10:26 am on November 18, 2021:
The function test, ./test/functional/rpc_rawtransaction.py, uses named paramaters, so verbose must be here, unless we want to change the functional test.
in
test/functional/rpc_rawtransaction.py:117
in
fee0b82acaoutdated
113@@ -113,21 +114,23 @@ def getrawtransaction_tests(self):
114 assert_equal(self.nodes[n].getrawtransaction(txId, False), rawTxSigned['hex'])
115116 # 4. valid parameters - supply txid and 1 for verbose.
117- # We only check the "hex" field of the output so we don't need to update this test every time the output format changes.
118+ # We only check the "hex" field of the output, getrawtransaction_verbosity_tests checks output
rajarshimaitra
commented at 4:44 pm on November 18, 2021:
maybe this a little more clear?
0 # We only check the "hex" field of the output, getrawtransaction_verbosity_tests checks remaining fields
dougEfresh
commented at 6:42 pm on November 18, 2021:
But I am still unsure why the tests needs to be separated (or how it is related to previous test failure).
Mostly clarity, I felt the getrawtransaction_tests was a bit confusing to follow, especially as I added the verbosity=2 test cases. Having a separate test for verbosity fields instead of smashing everything in one test block made reading more clear as well was intent.
rajarshimaitra
commented at 1:45 pm on November 19, 2021:
Fair enough.. That makes sense..
in
test/functional/rpc_rawtransaction.py:179
in
fee0b82acaoutdated
173@@ -171,6 +174,67 @@ def getrawtransaction_tests(self):
174 block = self.nodes[0].getblock(self.nodes[0].getblockhash(0))
175 assert_raises_rpc_error(-5, "The genesis block coinbase is not considered an ordinary transaction", self.nodes[0].getrawtransaction, block['merkleroot'])
176177+ def getrawtransaction_verbosity_tests(self):
178+ tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
179+ block1, _ = self.generate(self.nodes[2], 2)
rajarshimaitra
commented at 4:51 pm on November 18, 2021:
Do we need 2 blocks here?
dougEfresh
commented at 7:42 am on November 19, 2021:
Nope!
in
test/functional/rpc_rawtransaction.py:210
in
fee0b82acaoutdated
205+ 'hex',
206+ 'type'
207+ ]
208+ for n in [0, 3]:
209+ for v in [1,2]:
210+ self.log.info(f"Test[{n}] getrawtransaction_verbosity {v} {'with' if n == 0 else 'without'} -txindex, with blockhash")
rajarshimaitra
commented at 4:53 pm on November 18, 2021:
Internal node numbers are in general not printed in the test log as they don’t really convey any meaningful information.
dougEfresh
commented at 7:42 am on November 19, 2021:
Removed
in
test/functional/rpc_rawtransaction.py:211
in
fee0b82acaoutdated
206+ 'type'
207+ ]
208+ for n in [0, 3]:
209+ for v in [1,2]:
210+ self.log.info(f"Test[{n}] getrawtransaction_verbosity {v} {'with' if n == 0 else 'without'} -txindex, with blockhash")
211+ gottx = self.nodes[n].getrawtransaction(txid=tx, verbose=v, blockhash=block1)
rajarshimaitra
commented at 4:55 pm on November 18, 2021:
To keep parity with previous tests, an without blockhash test segment would be nice.
dougEfresh
commented at 7:59 am on November 19, 2021:
I added without blockhash verbosity=2 test in getrawtransaction_tests
in
test/functional/rpc_rawtransaction.py:233
in
fee0b82acaoutdated
228+ script_pub_key = prevout['scriptPubKey']
229+ for script_field in script_pub_key_fields:
230+ if script_field not in script_pub_key:
231+ raise AssertionError(f"field {script_field} is not in transaction scriptPubKey {script_pub_key}")
232+
233+ # check that coinbase has no fee or throws any errors for verbosity 2
rajarshimaitra
commented at 4:56 pm on November 18, 2021:
nit:
0 # check that coinbase has no fee or doesn't throw any errors for verbosity 2
dougEfresh
commented at 7:42 am on November 19, 2021:
changed
rajarshimaitra changes_requested
rajarshimaitra
commented at 5:01 pm on November 18, 2021:
contributor
But I am still unsure why the tests needs to be separated (or how it is related to previous test failure).
Also both verbose and verbosity are used interchangeably throughout the test code. This can be a little confusing for new comers. IIUC we intend to remove the verbose flag over time. So maybe we can have verbose for cases where we are passing bool and verbosity for everywhere else?
dougEfresh force-pushed
on Nov 19, 2021
dougEfresh force-pushed
on Nov 19, 2021
in
test/functional/rpc_rawtransaction.py:234
in
eaf10aacddoutdated
229+ script_pub_key = prevout['scriptPubKey']
230+ for script_field in script_pub_key_fields:
231+ if script_field not in script_pub_key:
232+ raise AssertionError(f"field {script_field} is not in transaction scriptPubKey {script_pub_key}")
233+
234+ # check that coinbase has no fee or does not throws any errors for verbosity 2
rajarshimaitra
commented at 1:51 pm on November 19, 2021:
nit:
0 # check that coinbase has no fee or does not throw any errors for verbosity 2
rajarshimaitra
commented at 12:15 pm on November 23, 2021:
I think this change is still pending..
dougEfresh
commented at 6:25 pm on November 23, 2021:
I think this change is still pending..
Fixed
in
src/rpc/rawtransaction.cpp:94
in
eaf10aacddoutdated
97+ {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for hex-encoded data, 1 for a json object, and 2 for json object with fee and prevout"},
98 {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED_NAMED_ARG, "The block in which to look for the transaction"},
99 },
100 {
101- RPCResult{"if verbose is not set or set to false",
102+ RPCResult{"if verbose is not set or set to 0",
rajarshimaitra
commented at 2:16 pm on November 19, 2021:
0 RPCResult{"if verbosity is not set or set to 0",
in
src/rpc/rawtransaction.cpp:87
in
eaf10aacddoutdated
8889- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
90- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n",
91+ "If verbosity is 0 or omitted, returns a string that is serialized, hex-encoded data for transaction.\n"
92+ "If verbosity is 1, returns an Object with information about transaction.\n"
93+ "If verbosity is 2, returns an Object with information about transaction, including fees and prevout information.\n",
rajarshimaitra
commented at 2:17 pm on November 19, 2021:
0 "If verbosity is 2, returns an Object with information about transaction, including fee and prevout information.\n",
rajarshimaitra changes_requested
rajarshimaitra
commented at 2:32 pm on November 19, 2021:
contributor
I think the commits can be rearranged a bit, consolidating all file specific changes. Would be easier for new reviewers.
Should I just squash into one commit ?
dougEfresh force-pushed
on Nov 20, 2021
rajarshimaitra
commented at 3:15 pm on November 20, 2021:
contributor
Should I just squash into one commit ?
I think the best thing to do here is to put one commit for each file, and order them logically (ex: client.cpp changes should come before rawtransaction.cpp, functional test commit should be the last, etc). For more details on commit structuring refer https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches
maflcko
commented at 7:38 am on November 21, 2021:
member
I think it is also fine to have just one commit, since this is just one feature. And it is fine to add tests in the same commit that adds features.
dougEfresh force-pushed
on Nov 21, 2021
dougEfresh
commented at 9:19 am on November 21, 2021:
contributor
I think it is also fine to have just one commit, since this is just one feature. And it is fine to add tests in the same commit that adds features.
squashed to one commit
rajarshimaitra
commented at 12:15 pm on November 23, 2021:
contributor
DrahtBot added the label
Needs rebase
on Dec 7, 2021
dougEfresh force-pushed
on Dec 10, 2021
DrahtBot removed the label
Needs rebase
on Dec 10, 2021
DrahtBot added the label
Needs rebase
on Jan 11, 2022
in
src/rpc/rawtransaction.cpp:115
in
e76df3127eoutdated
111@@ -111,26 +112,27 @@ static RPCHelpMan getrawtransaction()
112 {
113 return RPCHelpMan{
114 "getrawtransaction",
115- "\nReturn the raw transaction data.\n"
116+ "\nReturn the raw transaction data.\n\n"
maflcko
commented at 10:45 am on January 11, 2022:
0 "Return the raw transaction data.\n\n"
nit: no need for this \n after rebase
in
src/rpc/rawtransaction.cpp:125
in
e76df3127eoutdated
126127- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
128- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n",
129+ "If verbosity is 0 or omitted, returns a string that is serialized, hex-encoded data for transaction.\n"
130+ "If verbosity is 1, returns an Object with information about transaction.\n"
131+ "If verbosity is 2, returns an Object with information about transaction, including fee and prevout information.\n",
maflcko
commented at 10:46 am on January 11, 2022:
0 "If verbosity is 2, returns an Object with information about transaction, including fee and prevout information.",
same: can be removed
dougEfresh force-pushed
on Jan 12, 2022
DrahtBot removed the label
Needs rebase
on Jan 12, 2022
dougEfresh force-pushed
on Jan 12, 2022
dougEfresh force-pushed
on Jan 13, 2022
DrahtBot added the label
Needs rebase
on Jan 17, 2022
dougEfresh force-pushed
on Jan 17, 2022
DrahtBot removed the label
Needs rebase
on Jan 17, 2022
DrahtBot added the label
Needs rebase
on Jan 26, 2022
dougEfresh force-pushed
on Feb 2, 2022
DrahtBot removed the label
Needs rebase
on Feb 2, 2022
dougEfresh force-pushed
on Feb 2, 2022
in
src/rpc/rawtransaction.cpp:305
in
02ed9b997boutdated
dougEfresh
commented at 9:57 pm on February 2, 2022:
I had to add the lock due to IsBlockPruned now requiring a lock
in
src/rpc/rawtransaction.cpp:135
in
02ed9b997boutdated
136+ "the specified block is available and the transaction is in that block.\n\n"
137+ "Hint: Use gettransaction for wallet transactions.\n\n"
138139- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
140- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n",
141+ "If verbosity is 0 or omitted, returns a string that is serialized, hex-encoded data for transaction.\n"
stickies-v
commented at 10:28 am on February 3, 2022:
nit: I find “data for transaction” a bit ambiguous. We’re just returning the entire transaction, so I’d phrase it as:
0 "If verbosity is 0 or omitted, returns the serialized transaction as a hex-encoded string.\n"
in
src/rpc/rawtransaction.cpp:137
in
02ed9b997boutdated
138139- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
140- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.\n",
141+ "If verbosity is 0 or omitted, returns a string that is serialized, hex-encoded data for transaction.\n"
142+ "If verbosity is 1, returns an Object with information about transaction.\n"
143+ "If verbosity is 2, returns an Object with information about transaction, including fee and prevout information.",
stickies-v
commented at 10:31 am on February 3, 2022:
nit: consistent reference to a JSON Object
0 "If verbosity is 1, returns a JSON Object with information about transaction.\n"
1 "If verbosity is 2, returns a JSON Object with information about transaction, including fee and prevout information.",
in
src/rpc/rawtransaction.cpp:140
in
02ed9b997boutdated
142+ "If verbosity is 1, returns an Object with information about transaction.\n"
143+ "If verbosity is 2, returns an Object with information about transaction, including fee and prevout information.",
144 {
145 {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"},
146- {"verbose", RPCArg::Type::BOOL, RPCArg::Default{false}, "If false, return a string, otherwise return a json object"},
147+ {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for hex-encoded data, 1 for a json object, and 2 for json object with fee and prevout"},
stickies-v
commented at 10:31 am on February 3, 2022:
nit: consistent reference to a JSON Object
0 {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::Default{0}, "0 for hex-encoded data, 1 for a JSON Object, and 2 for JSON Object with fee and prevout"},
in
src/rpc/rawtransaction.cpp:145
in
02ed9b997boutdated
148 {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED_NAMED_ARG, "The block in which to look for the transaction"},
149 },
150 {
151- RPCResult{"if verbose is not set or set to false",
152+ RPCResult{"if verbosity is not set or set to 0",
153 RPCResult::Type::STR, "data", "The serialized, hex-encoded data for 'txid'"
stickies-v
commented at 10:37 am on February 3, 2022:
As per above suggestion, for consistency:
0 RPCResult::Type::STR, "data", "The serialized transaction as a hex-encoded string"
in
src/rpc/rawtransaction.cpp:227
in
02ed9b997boutdated
215+ {
216+ {RPCResult::Type::STR, "asm", "the asm"},
217+ {RPCResult::Type::STR, "hex", "the hex"},
218+ {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
219+ {RPCResult::Type::STR, "address", /* optional */ true, "The Bitcoin address (only if a well-defined address exists)"},
220+ }},
stickies-v
commented at 10:44 am on February 3, 2022:
stickies-v
commented at 12:30 pm on February 3, 2022:
This declaration could also be moved down a few lines to right before it is used. Or, if you incorporate my other comment to use find_if, this line can be removed entirely (as can including the <optional> header)
in
src/rpc/rawtransaction.cpp:315
in
02ed9b997boutdated
stickies-v
commented at 12:35 pm on February 3, 2022:
Suggested change to avoid the manual iteration and to simplify the TxToJSON call. I think it’s slightly easier to follow, but I don’t have a strong opinion either way.
0 CTxUndo* undo {nullptr};
1 auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return *t == *tx; });
2 if (it != block.vtx.end()) {
3 // -1 as blockundo does not have coinbase tx
4 undo = &blockUndo.vtxundo.at(it - block.vtx.begin() - 1);
5 }
6 TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate(), undo, TxVerbosity::SHOW_DETAILS_AND_PREVOUT);
7 return result;
stickies-v
commented at 1:39 pm on February 3, 2022:
As per my other comment, I think desc should be added here too?
in
test/functional/rpc_rawtransaction.py:239
in
02ed9b997boutdated
234+ if prevout_field not in prevout:
235+ raise AssertionError(f"field {prevout_field} is not in transaction prevout {prevout}")
236+ script_pub_key = prevout['scriptPubKey']
237+ for script_field in script_pub_key_fields:
238+ if script_field not in script_pub_key:
239+ raise AssertionError(f"field {script_field} is not in transaction scriptPubKey {script_pub_key}")
stickies-v
commented at 2:03 pm on February 3, 2022:
Now these assertions are repeated for every f in fields, I think you want to decrease the indentation?
in
test/functional/rpc_rawtransaction.py:225
in
02ed9b997boutdated
220+ for f in fields:
221+ if f not in gottx:
222+ raise AssertionError(f"field {f} is not in transaction")
223+ if v == 1:
224+ assert('fee' not in gottx)
225+ assert(len(gottx['vin']) > 0)
stickies-v
commented at 2:04 pm on February 3, 2022:
This is not specific to if v == 1 so I’d move it up one level and remove the duplication from if v == 2:
in
test/functional/rpc_rawtransaction.py:222
in
02ed9b997boutdated
217+ for v in [1,2]:
218+ self.log.info(f"Test getrawtransaction_verbosity {v} {'with' if n == 0 else 'without'} -txindex, with blockhash")
219+ gottx = self.nodes[n].getrawtransaction(txid=tx, verbosity=v, blockhash=block1)
220+ for f in fields:
221+ if f not in gottx:
222+ raise AssertionError(f"field {f} is not in transaction")
stickies-v
commented at 2:11 pm on February 3, 2022:
If you do a set comparison you can catch all missing fields in one run, as opposed to potentially having to rerun the test multiple times. The same applies to the other comparisons further down.
0 missing_fields = set(fields).difference(gottx.keys())
1 if missing_fields:
2 raise AssertionError(f"fields {', '.join(missing_fields)} are not in transaction")
in
test/functional/rpc_rawtransaction.py:202
in
02ed9b997boutdated
stickies-v
commented at 2:16 pm on February 3, 2022:
nit: In Python, it’s recommended to add trailing commas on multiline lists to minimize the git diff when new items are added later on.
0 'weight',
stickies-v
commented at 2:29 pm on February 3, 2022:
contributor
tACK02ed9b997
Left some suggested minor improvements and nits in comments, but no showstoppers (except for the excessive unintended assertions in the functional test). LGTM.
DrahtBot added the label
Needs rebase
on Mar 3, 2022
dougEfresh force-pushed
on Mar 5, 2022
DrahtBot removed the label
Needs rebase
on Mar 5, 2022
dougEfresh force-pushed
on Mar 15, 2022
dougEfresh force-pushed
on Mar 16, 2022
DrahtBot added the label
Needs rebase
on Mar 31, 2022
dougEfresh force-pushed
on Mar 31, 2022
DrahtBot removed the label
Needs rebase
on Mar 31, 2022
DrahtBot added the label
Needs rebase
on Mar 31, 2022
dougEfresh force-pushed
on Apr 2, 2022
DrahtBot removed the label
Needs rebase
on Apr 2, 2022
in
src/rpc/rawtransaction.cpp:303
in
58a638f77boutdated
299@@ -263,13 +300,33 @@ static RPCHelpMan getrawtransaction()
300 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, errmsg + ". Use gettransaction for wallet transactions.");
301 }
302303- if (!fVerbose) {
304+ if (verbosity == 0) {
aureleoules
commented at 2:32 am on April 12, 2022:
Specifying a negative verbosity level fallbacks to 2 instead of 0.
0 if (verbosity <= 0) {
aureleoules
commented at 2:32 am on April 12, 2022:
member
tACK58a638f77b4d9d69b64cd4e3eb9ff8da79f465c7.
I tested locally getrawtransaction with :
regular transaction
mempool transaction
coinbase transaction
with/without txindex
Everything worked as expected.
in
src/rpc/rawtransaction.cpp:261
in
58a638f77boutdated
259+ // Accept either a bool (true) or a num (>=0) to indicate verbosity.
260+ int verbosity{0};
261 if (!request.params[1].isNull()) {
262- fVerbose = request.params[1].isNum() ? (request.params[1].get_int() != 0) : request.params[1].get_bool();
263+ if (request.params[1].isBool()) {
264+ verbosity = request.params[1].get_bool() ? 1 : 0;
0 from collections import OrderedDict
1 from decimal import Decimal
2+from itertools import product
34@@ -213,30 +214,29 @@ class RawTransactionsTest(BitcoinTestFramework):
5- for n in [0, 3]:
6- for v in [1,2]:
7+ for n, v in product([0, 3], [1, 2]):
(and reduce indentation in the for-loop scope by four spaces)
dougEfresh
commented at 8:50 am on April 21, 2022:
Change. I also added a comment to indicate intent of the loop.
jonatack
commented at 11:06 am on April 12, 2022:
contributor
Concept ACK
dougEfresh force-pushed
on Apr 21, 2022
dougEfresh force-pushed
on May 4, 2022
dougEfresh force-pushed
on May 4, 2022
DrahtBot added the label
Needs rebase
on May 19, 2022
dougEfresh force-pushed
on May 23, 2022
DrahtBot removed the label
Needs rebase
on May 23, 2022
DrahtBot added the label
Needs rebase
on May 30, 2022
dougEfresh force-pushed
on May 31, 2022
DrahtBot removed the label
Needs rebase
on May 31, 2022
dougEfresh force-pushed
on May 31, 2022
dougEfresh force-pushed
on Jun 4, 2022
dougEfresh force-pushed
on Jun 4, 2022
DrahtBot added the label
Needs rebase
on Jul 19, 2022
hernanmarino approved
hernanmarino
commented at 0:02 am on August 2, 2022:
contributor
tACK . Run automatic tests and manually tested on testnet
pablomartin4btc approved
pablomartin4btc
commented at 3:08 pm on August 2, 2022:
member
tACK
Manually tested using testnet. Checked new functional test.
dougEfresh force-pushed
on Aug 16, 2022
dougEfresh
commented at 5:14 pm on August 16, 2022:
contributor
rebased
DrahtBot removed the label
Needs rebase
on Aug 16, 2022
in
src/rpc/rawtransaction.cpp:260
in
a19823c72coutdated
258+ // Accept either a bool (true) or a num (>=0) to indicate verbosity.
259+ int verbosity{0};
260 if (!request.params[1].isNull()) {
261- fVerbose = request.params[1].isNum() ? (request.params[1].getInt<int>() != 0) : request.params[1].get_bool();
262+ if (request.params[1].isBool()) {
263+ verbosity = verbosity = request.params[1].get_bool();
rajarshimaitra
commented at 11:33 am on October 4, 2022:
It seems if verbosity==2 and !blockindex this will trigger and we won’t get the fee and prevout fields even when g_txindex is available.. Is this expected behavior?
dougEfresh
commented at 8:00 am on October 10, 2022:
You are correct. I have refactored the code to detect if blockhash was sent as a parameter.
What should we expect if the blockhash is not sent and the tx is in mempool? Should the fee field be available? (I would think so)
in
test/functional/rpc_rawtransaction.py:212
in
eae8f8e666outdated
207+ 'type',
208+ ]
209+ # node 0 & 3 with verbosity 1 & 2
210+ for n, v in product([0, 2], [1, 2]):
211+ self.log.info(f"Test getrawtransaction_verbosity {v} {'with' if n == 0 else 'without'} -txindex, with blockhash")
212+ gottx = self.nodes[n].getrawtransaction(txid=tx, verbosity=v, blockhash=block1)
rajarshimaitra
commented at 11:35 am on October 4, 2022:
Could be useful to add cases without blockhash and affirm all the blockhash related expected behaviors..
dougEfresh
commented at 8:00 am on October 10, 2022:
I have added this test case
in
test/functional/rpc_rawtransaction.py:211
in
eae8f8e666outdated
206+ 'hex',
207+ 'type',
208+ ]
209+ # node 0 & 3 with verbosity 1 & 2
210+ for n, v in product([0, 2], [1, 2]):
211+ self.log.info(f"Test getrawtransaction_verbosity {v} {'with' if n == 0 else 'without'} -txindex, with blockhash")
rajarshimaitra
commented at 11:38 am on October 4, 2022:
I reproduced the no blockhash and v=2 situation and confirmed that it doesn’t produce fee and prevout fields.
0 for n, v in product([0, 2], [1, 2]):
1 self.log.info(f"Test getrawtransaction_verbosity {v} {'with' if n == 0 else 'without'} -txindex, with blockhash")
2 if v == 2 and n == 0:
3 gottx = self.nodes[n].getrawtransaction(txid=tx, verbosity=v)
4 self.log.info(f"Tx data for -v=2 -txindex -noblockhash {gottx}")
5 assert('fee' not in gottx)
6 assert('prevout' not in gottx)
rajarshimaitra
commented at 11:41 am on October 4, 2022:
contributor
Its been almost an year since this PR is standing and its quite useful for wallet devs for obvious reasons.. Thanks to the PR author for chugging along..
I did a re-pass and have following non blocking observation regarding a specific blockhash- verbosity-txindex combination.
in
src/rpc/rawtransaction.cpp:57
in
eae8f8e666outdated
in
test/functional/rpc_rawtransaction.py:217
in
eae8f8e666outdated
212+ gottx = self.nodes[n].getrawtransaction(txid=tx, verbosity=v, blockhash=block1)
213+ missing_fields = set(fields).difference(gottx.keys())
214+ if missing_fields:
215+ raise AssertionError(f"fields {', '.join(missing_fields)} are not in transaction")
216+
217+ for f in fields:
aureleoules
commented at 4:30 pm on October 4, 2022:
f is unused, I think you can remove the loop
dougEfresh
commented at 9:15 am on October 9, 2022:
refactored this test to remove this loop
dougEfresh force-pushed
on Oct 5, 2022
dougEfresh force-pushed
on Oct 8, 2022
dougEfresh force-pushed
on Oct 10, 2022
vostrnad
commented at 11:00 am on October 22, 2022:
none
Compiled and manually tested.
The RPC help result object could use some improvement. Right now the section is rendered like this:
0"prevout" : { (json object)
1 "generated" : true|false, (boolean)
2 "value" : n, (numeric) The value in BTC
3 "height" : n, (numeric) previous block height
4 "scriptPubKey" : { (json object)
5 "asm" : "str", (string) the asm
6 "desc" : "str", (string) desc
7 "hex" : "str", (string) the hex
8 "address" : "str", (string, optional) The Bitcoin address (only if a well-defined address exists)
9 "type" : "str" (string) The type, eg 'pubkeyhash'
10 }
11}
I guess there’s no reason it should look much different to what’s in getblock for verbosity=3? For reference:
0"prevout" : { (json object) (Only if undo information is available)
1 "generated" : true|false, (boolean) Coinbase or not
2 "height" : n, (numeric) The height of the prevout
3 "value" : n, (numeric) The value in BTC
4 "scriptPubKey" : { (json object)
5 "asm" : "str", (string) Disassembly of the public key script
6 "desc" : "str", (string) Inferred descriptor for the output
7 "hex" : "hex", (string) The raw public key script bytes, hex-encoded
8 "address" : "str", (string, optional) The Bitcoin address (only if a well-defined address exists)
9 "type" : "str" (string) The type (one of: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_scripthash, witness_v0_keyhash, witness_v1_taproot, witness_unknown)
10 }
11}
dougEfresh force-pushed
on Oct 30, 2022
dougEfresh
commented at 7:24 am on October 30, 2022:
contributor
Compiled and manually tested.
The RPC help result object could use some improvement. Right now the section is rendered like this:
I updated the RPC help to reflect getblock
rpc: Return fee and prevout(s) to getrawtransaction
* Add optional fee response in BTC to getrawtransaction
* Add optional prevout(s) response to getrawtransaction showing utxos being spent
* Add getrawtransaction_verbosity functional test to validate fields
f86697163e
dougEfresh force-pushed
on Oct 30, 2022
in
src/rpc/rawtransaction.cpp:310
in
f86697163e
308+ LOCK(cs_main);
309+ result.pushKV("in_active_chain", chainman.ActiveChain().Contains(blockindex));
310+ }
311+ // If request is verbosity >= 1 but no blockhash was given, then look up the blockindex
312+ if (request.params[2].isNull()) {
313+ LOCK(cs_main);
dougEfresh
commented at 12:15 pm on October 30, 2022:
I like to avoid this lock, suggestions welcome.
aureleoules approved
aureleoules
commented at 10:33 am on November 28, 2022:
member
ACKf86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
hernanmarino
commented at 6:55 pm on November 28, 2022:
contributor
re ACKf86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
pablomartin4btc
commented at 7:51 pm on November 28, 2022:
member
re-tACKf86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
achow101
commented at 10:40 pm on December 13, 2022:
member
ACKf86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
achow101 merged this
on Dec 13, 2022
achow101 closed this
on Dec 13, 2022
sidhujag referenced this in commit
f404487d12
on Dec 14, 2022
in
src/rpc/rawtransaction.cpp:192
in
f86697163e
197+ RPCResult{"if verbosity is not set or set to 0",
198+ RPCResult::Type::STR, "data", "The serialized transaction as a hex-encoded string for 'txid'"
199 },
200- RPCResult{"if verbose is set to true",
201+ RPCResult{"if verbosity is set to 1",
202+ // When updating this documentation, update `decoderawtransaction` in the same way.
maflcko
commented at 8:31 am on December 14, 2022:
nit: Is this dev comment still accurate, given that DecodeTxDoc was split out for this reason?
dougEfresh
commented at 10:50 pm on December 20, 2022:
182183- "\nIf verbose is 'true', returns an Object with information about 'txid'.\n"
184- "If verbose is 'false' or omitted, returns a string that is serialized, hex-encoded data for 'txid'.",
185+ "If verbosity is 0 or omitted, returns the serialized transaction as a hex-encoded string.\n"
186+ "If verbosity is 1, returns a JSON Object with information about transaction.\n"
187+ "If verbosity is 2, returns a JSON Object with information about transaction, including fee and prevout information.",
maflcko
commented at 12:58 pm on December 15, 2022:
nit: “about the transaction”
dougEfresh
commented at 1:41 pm on December 15, 2022:
I’ll fix this up in another PR
in
src/rpc/rawtransaction.cpp:212
in
f86697163e
207+ {
208+ {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"},
209+ {RPCResult::Type::NUM, "fee", /* optional */ true, "transaction fee in " + CURRENCY_UNIT + ", omitted if block undo data is not available"},
210+ {RPCResult::Type::ARR, "vin", "",
211+ {
212+ {RPCResult::Type::OBJ, "", /* optional */ true, "utxo being spent, omitted if block undo data is not available",
maflcko
commented at 12:59 pm on December 15, 2022:
nit: This is wrong, this object isn’t optional, but the "prevout" object
in
src/rpc/rawtransaction.cpp:215
in
f86697163e
210+ {RPCResult::Type::ARR, "vin", "",
211+ {
212+ {RPCResult::Type::OBJ, "", /* optional */ true, "utxo being spent, omitted if block undo data is not available",
213+ {
214+ {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"},
215+ {RPCResult::Type::OBJ, "prevout", "Only if undo information is available)",
maflcko
commented at 12:59 pm on December 15, 2022:
nit: Should be optional?
in
src/rpc/rawtransaction.cpp:209
in
f86697163e
201@@ -198,20 +202,47 @@ static RPCHelpMan getrawtransaction()
202 },
203 DecodeTxDoc(/*txid_field_doc=*/"The transaction id (same as provided)")),
204 },
205+ RPCResult{"for verbosity = 2",
206+ RPCResult::Type::OBJ, "", "",
207+ {
208+ {RPCResult::Type::ELISION, "", "Same output as verbosity = 1"},
209+ {RPCResult::Type::NUM, "fee", /* optional */ true, "transaction fee in " + CURRENCY_UNIT + ", omitted if block undo data is not available"},
maflcko
commented at 12:59 pm on December 15, 2022:
nit: Use clang-tidy named args? /*optional=*/true,
maflcko
commented at 1:00 pm on December 15, 2022:
member
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-06-09 18:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
