rpc: Return fee and prevout (utxos) to getrawtransaction #23319

dougEfresh commented at 5:50 AM on October 20, 2021: contributor

Add fee response in BTC to getrawtransaction #23264

For Reviewers

Verbose arg is now an int
Verbose = 2 includes a fee field and prevout
./test/functional/rpc_rawtransaction.py contains a new test to validate fields of new verbosity 2 (not the values)

bitcoin-cli -chain=test getrawtransaction 9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4 2 000000000000001d442e556146d5f2841d85150c200e8d8b8a4b5005b13878f6

  "in_active_chain": true,
  "txid": "9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4",
  "hash": "7f23e3f3a0a256ddea1d35ffd43e9afdd67cc68389ef1a804bb20c76abd6863e",
 ....
  "vin": [
    {
      "txid": "23fc75d6d74f6f97e225839af69ff36a612fe04db58a4414ec4828d1749a05a0",
      "vout": 0,
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "prevout": {
        "generated": false,
        "height": 2099486,
        "value": 0.00017764,
        "scriptPubKey": {
          "asm": "0 7846ce1ced3253d8bd43008db2ca364cc722f5a2",
          "hex": "00147846ce1ced3253d8bd43008db2ca364cc722f5a2",
          "address": "tb1q0prvu88dxffa302rqzxm9j3kfnrj9adzk49mlp",
          "type": "witness_v0_keyhash"
        }
      },
      "sequence": 4294967295
    },
...
 "fee": 0.00000762
}

fanquake added the label RPC/REST/ZMQ on Oct 20, 2021

in src/rpc/rawtransaction.cpp:225 in 2730b680d2 outdated

 222 | +    result.pushKV("in_active_chain", in_active_chain);
 223 | +
 224 | +    std::optional<size_t>  opt_tx_position{std::nullopt};
 225 | +    CBlockUndo blockUndo;
 226 | +    CBlock block;
 227 | +    if (IsBlockPruned(blockindex) || !(UndoReadFromDisk(blockUndo, blockindex) && ReadBlockFromDisk(block, blockindex, Params().GetConsensus()))) {

maflcko commented at 7:03 AM on October 20, 2021:

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 2730b680d2 outdated

  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},

maflcko commented at 7:03 AM on October 20, 2021:

it is optional?

maflcko approved

maflcko commented at 7:04 AM on October 20, 2021: member

Concept ACK

dougEfresh force-pushed on Oct 20, 2021

in src/rpc/rawtransaction.cpp:226 in 25989832aa outdated

 220 | +    CBlock block;
 221 | +    if (!blockindex || IsBlockPruned(blockindex) || !(UndoReadFromDisk(blockUndo, blockindex) && ReadBlockFromDisk(block, blockindex, Params().GetConsensus()))) {
 222 | +        TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate());
 223 | +        return result;
 224 | +    }
 225 | +    result.pushKV("in_active_chain", in_active_chain);

maflcko commented at 12:49 PM on October 20, 2021:

this is an unrelated behavior change

dougEfresh commented at 1:30 PM on October 20, 2021:

right. good catch.

maflcko commented at 2:14 PM on October 20, 2021:

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

maflcko commented at 2:22 PM on October 20, 2021:

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.

if (a) pushKV();

is clearer than

if (something)
  if (a) pushKV();
  return
pushKV();

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

The tests can also be taken from the other RPC: https://github.com/bitcoin/bitcoin/pull/22918/files

dougEfresh force-pushed on Oct 21, 2021

DrahtBot commented at 6:59 PM on October 25, 2021: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	aureleoules, hernanmarino, pablomartin4btc, achow101
Concept ACK	MarcoFalke, lsilva01, jonatack
Stale ACK	stickies-v, rajarshimaitra

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 83e5f6c584 outdated

 230 | +    }
 231 | +
 232 | +    std::optional<size_t>  opt_tx_position{std::nullopt};
 233 | +    CBlockUndo blockUndo;
 234 | +    CBlock block;
 235 | +    if (!blockindex || IsBlockPruned(blockindex) || !(UndoReadFromDisk(blockUndo, blockindex) && ReadBlockFromDisk(block, blockindex, Params().GetConsensus()))) {

laanwj commented at 5:31 PM on October 28, 2021:

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 guess mostly to prevent unneeded allocation of

    std::optional<size_t>  opt_tx_position{std::nullopt};
    CBlockUndo blockUndo;
    CBlock block;

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

Showing the utxos being spent is also being done by the other RPC. Any questions you might have should be answered by looking at the code changes of https://github.com/bitcoin/bitcoin/pull/22918/files

dougEfresh force-pushed on Oct 29, 2021

dougEfresh renamed this:
~~rpc: Return fee in getrawtransaction~~
rpc: Return fee and prevout (utxos) to getrawtransaction
on Oct 29, 2021

dougEfresh commented at 11:48 AM on October 29, 2021: contributor

Showing the utxos being spent is also being done by the other RPC. Any questions you might have should be answered by looking at the code changes of https://github.com/bitcoin/bitcoin/pull/22918/files

thanks, added utxos with TxVerbosity::SHOW_DETAILS_AND_PREVOUT

My latest change includes

test for prevout (utxo)
release documentation
consolidate if statement in ./src/rpc/rawtransaction.cpp getrawtransaction
```
if (verbosity == 1 || !blockindex || IsBlockPruned(blockindex) ....
```

in src/rpc/rawtransaction.cpp:87 in 2c56d72aca outdated

  85 |  
  86 | -                "\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 2c56d72aca outdated

  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"
  85 |  
  86 | -                "\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"

luke-jr commented at 4:29 PM on October 29, 2021:

verbosity

in src/rpc/rawtransaction.cpp:90 in 2c56d72aca outdated

  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"},

luke-jr commented at 4:30 PM on October 29, 2021:

Stick to just the numbers in docs. (bool is essentially just backwards compat)

in test/functional/rpc_rawtransaction.py:146 in 2c56d72aca outdated

 161 | -                )
 162 | -                assert_raises_rpc_error(-5, err_msg, self.nodes[n].getrawtransaction, txid=tx, verbose=True)
 163 | +                assert_equal(gottx['in_active_chain'], True)
 164 | +                if v == 2:
 165 | +                    # fee is in verbosity 2 only
 166 | +                    assert_equal(gottx['fee'], Decimal('0.00002820'))

luke-jr commented at 3:22 AM on October 30, 2021:

What is supposed to guarantee this is correct?

dougEfresh commented at 7:28 AM on October 30, 2021:

The value of fee is not important. I want to see that the field fee exists. Change it to assert('fee' in gottxt) ?

There are another tests (outside the scope of this PR) which validate the fee amount.

luke-jr commented at 3:59 PM on October 30, 2021:

Maybe do tx_fee = -self.nodes[2].gettransaction(tx)['fee'] immediately after sending and compare to that?

dougEfresh commented at 10:26 AM on November 2, 2021:

No fee field in gettransaction . I added assert('fee' in gottxt) but kept assert_equal(gottx['fee'], Decimal('0.00002820'))

rajarshimaitra commented at 3:33 PM on November 16, 2021:

There is fee in getransaction. see https://developer.bitcoin.org/reference/rpc/gettransaction.html

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 f30f007c82 outdated

 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;

amovfx commented at 1:52 AM on November 14, 2021:

I'm just studying for the pr review club.

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 f30f007c82 outdated

  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)
 101 |  
 102 | +- `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 f30f007c82 outdated

  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 f30f007c82 outdated

  85 |  
  86 | -                "\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

Result (for verbosity = 2):
{                               (json object)
  ...,                          Same output as verbosity = 1
  "fee" : n,                    (numeric, optional) transaction fee in BTC, omitted if block undo data is not available
  "vin" : [                     (json array)
    {                           (json object, optional) utxo being spent, omitted if block undo data is not available
      ...,                      Same output as verbosity = 1
      "prevout" : {             (json object)
        "value" : n,            (numeric) The value in BTC
        "height" : n,           (numeric) previous block height
        "scriptPubKey" : {      (json object)
          "asm" : "str",        (string) the asm
          "hex" : "str",        (string) the hex
          "type" : "str",       (string) The type, eg 'pubkeyhash'
          "address" : "str"     (string, optional) The Bitcoin address (only if a well-defined address exists)
        }
      }
    },
    ...
  ]
}

in src/rpc/rawtransaction.cpp:239 in f30f007c82 outdated

 152 | @@ -150,6 +153,7 @@ static RPCHelpMan getrawtransaction()
 153 |              + HelpExampleRpc("getrawtransaction", "\"mytxid\", true")
 154 |              + HelpExampleCli("getrawtransaction", "\"mytxid\" false \"myblockhash\"")
 155 |              + HelpExampleCli("getrawtransaction", "\"mytxid\" true \"myblockhash\"")
 156 | +            + HelpExampleCli("getrawtransaction", "\"mytxid\" 2 \"myblockhash\"")

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 fb2f153136 outdated

 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 fb2f153136 outdated

  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 ACK fb2f15313

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?

For example:

./bitcoin-cli -testnet getrawtransaction ba37a619086f57eeb37915cfb938aa27c69499d87a0e2972dcd3b884d65db0c6 2 00000000763effc6fcd7f757043a4d7a9262582582d05f1fd9dc6c6c70bfaf0b

returns

error code: -1
error message:
vector

The equivalent getblock RPC command however works fine:

./bitcoin-cli -testnet getblock 00000000763effc6fcd7f757043a4d7a9262582582d05f1fd9dc6c6c70bfaf0b 3

in test/functional/rpc_rawtransaction.py:151 in fb2f153136 outdated

 166 | +                    assert_equal(gottx['fee'], Decimal('0.00002820'))
 167 | +                    # verify transacion has prevout field
 168 | +                    assert('vin' in gottx)
 169 | +                    assert(len(gottx['vin']) > 0)
 170 | +                    assert('prevout' in gottx['vin'][0])
 171 | +                    assert_equal(False, gottx['vin'][0]['prevout']['generated'])

rajarshimaitra commented at 4:25 PM on November 16, 2021:

Maybe it would be good to add few more data checks in prevout to ensure nothing changes in at the field level?

rajarshimaitra commented at 4:29 PM on November 16, 2021: contributor

Review ACK https://github.com/bitcoin/bitcoin/pull/23319/commits/fb2f153136dc14921cb7f249fe224919cb7094bb

I am not sure why, but I am hitting this weird error in the test

$ ./test/functional/rpc_rawtransaction.py 
2021-11-16T16:12:39.573000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_1ldkso58
2021-11-16T16:12:40.405000Z TestFramework (INFO): Prepare some coins for multiple *rawtransaction commands
2021-11-16T16:12:43.883000Z TestFramework (INFO): Test getrawtransaction with -txindex
2021-11-16T16:12:43.890000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/raj/github-repo/bitcoin/test/functional/test_framework/util.py", line 133, in try_rpc
    fun(*args, **kwds)
  File "/home/raj/github-repo/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/raj/github-repo/bitcoin/test/functional/test_framework/authproxy.py", line 144, in __call__
    raise JSONRPCException(response['error'], status)
test_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

Review ACK fb2f153

I am not sure why, but I am hitting this weird error in the test
$ ./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 fee0b82aca outdated

 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'])
 176 |  
 177 | +    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 fee0b82aca outdated

  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 fee0b82aca outdated

 113 | @@ -113,21 +114,23 @@ def getrawtransaction_tests(self):
 114 |              assert_equal(self.nodes[n].getrawtransaction(txId, False), rawTxSigned['hex'])
 115 |  
 116 |              # 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?

            # 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 fee0b82aca outdated

 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'])
 176 |  
 177 | +    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 fee0b82aca outdated

 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 fee0b82aca outdated

 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 fee0b82aca outdated

 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:

        # 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

tACK https://github.com/bitcoin/bitcoin/pull/23319/commits/fee0b82aca8f9c8eba4b0bff3e6ec3ddbb7208ae . Functional test is passing now.

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

in test/functional/rpc_rawtransaction.py:234 in eaf10aacdd outdated

 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:

        # 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 eaf10aacdd outdated

  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:

                    RPCResult{"if verbosity is not set or set to 0",

in src/rpc/rawtransaction.cpp:87 in eaf10aacdd outdated

  88 |  
  89 | -                "\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:

                "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

Re ACK https://github.com/bitcoin/bitcoin/pull/23319/commits/eaf10aacddfd32c0722414a3f040d3fae2b51cf7.

I think the commits can be rearranged a bit, consolidating all file specific changes. Would be easier for new reviewers.

And few more nits.

dougEfresh commented at 10:21 AM on November 20, 2021: contributor

Re ACK eaf10aa.

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

reACK https://github.com/bitcoin/bitcoin/pull/23319/commits/b612eddc3c251f187e6f28142f88be75345ad300

dougEfresh force-pushed on Nov 23, 2021

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 e76df3127e outdated

 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:

                "Return the raw transaction data.\n\n"

nit: no need for this \n after rebase

in src/rpc/rawtransaction.cpp:125 in e76df3127e outdated

 126 |  
 127 | -                "\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:

                "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 02ed9b997b outdated

 307 | +    std::optional<size_t> opt_tx_position{std::nullopt};
 308 | +    CBlockUndo blockUndo;
 309 | +    CBlock block;
 310 | +
 311 | +    {
 312 | +        LOCK(cs_main);

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 02ed9b997b outdated

 136 | +                "the specified block is available and the transaction is in that block.\n\n"
 137 | +                "Hint: Use gettransaction for wallet transactions.\n\n"
 138 |  
 139 | -                "\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:

                "If verbosity is 0 or omitted, returns the serialized transaction as a hex-encoded string.\n"

in src/rpc/rawtransaction.cpp:137 in 02ed9b997b outdated

 138 |  
 139 | -                "\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

                "If verbosity is 1, returns a JSON Object with information about transaction.\n"
                "If verbosity is 2, returns a JSON Object with information about transaction, including fee and prevout information.",

in src/rpc/rawtransaction.cpp:140 in 02ed9b997b outdated

 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

                    {"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 02ed9b997b outdated

 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:

                         RPCResult::Type::STR, "data", "The serialized transaction as a hex-encoded string"

in src/rpc/rawtransaction.cpp:227 in 02ed9b997b outdated

 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:

I think this is missing the "desc" output?

"scriptPubKey": {
          "asm": "0 7846ce1ced3253d8bd43008db2ca364cc722f5a2",
          "desc": "addr(tb1q0prvu88dxffa302rqzxm9j3kfnrj9adzk49mlp)#57mlqnnp",
          "hex": "00147846ce1ced3253d8bd43008db2ca364cc722f5a2",
          "address": "tb1q0prvu88dxffa302rqzxm9j3kfnrj9adzk49mlp",
          "type": "witness_v0_keyhash"
        }

stickies-v commented at 10:51 AM on February 3, 2022:

Also nit: the actual results seem to be ordered somewhat differently: in my output type comes after address. Same for height and value in prevout.

in src/rpc/rawtransaction.cpp:305 in 02ed9b997b outdated

 302 | -    TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate());
 303 | +    if (blockindex) {
 304 | +        result.pushKV("in_active_chain", in_active_chain);
 305 | +    }
 306 | +
 307 | +    std::optional<size_t> opt_tx_position{std::nullopt};

stickies-v commented at 10:59 AM on February 3, 2022:

You should include the <optional> header now

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 02ed9b997b outdated

 313 | +        if (verbosity == 1 || tx->IsCoinBase() ||
 314 | +            !blockindex || IsBlockPruned(blockindex) ||
 315 | +            !(UndoReadFromDisk(blockUndo, blockindex) && ReadBlockFromDisk(block, blockindex, Params().GetConsensus()))) {
 316 | +            TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate());
 317 | +            return result;
 318 | +        }

stickies-v commented at 11:07 AM on February 3, 2022:

Would it be possible to reduce your lock usage by only locking during the IsBlockPruned operation? At first sight I think this would work:

        const bool is_block_pruned {WITH_LOCK(cs_main, return IsBlockPruned(blockindex))};
        if (verbosity == 1 || tx->IsCoinBase() || !blockindex || is_block_pruned ||
            !(UndoReadFromDisk(blockUndo, blockindex) && ReadBlockFromDisk(block, blockindex, Params().GetConsensus()))) {
            TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate());
            return result;
        }

in src/rpc/rawtransaction.cpp:336 in 02ed9b997b outdated

 324 | +            opt_tx_position = std::optional<size_t>{i-1};
 325 | +            break;
 326 | +        }
 327 | +    }
 328 | +    TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate(), opt_tx_position ? &blockUndo.vtxundo.at(*opt_tx_position) : nullptr, TxVerbosity::SHOW_DETAILS_AND_PREVOUT);
 329 |      return result;

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.

    CTxUndo* undo {nullptr};
    auto it = std::find_if(block.vtx.begin(), block.vtx.end(), [tx](CTransactionRef t){ return *t == *tx; });
    if (it != block.vtx.end()) {
        // -1 as blockundo does not have coinbase tx
        undo = &blockUndo.vtxundo.at(it - block.vtx.begin() - 1);
    }
    TxToJSON(*tx, hash_block, result, chainman.ActiveChainstate(), undo, TxVerbosity::SHOW_DETAILS_AND_PREVOUT);
    return result;

dougEfresh commented at 3:53 PM on March 5, 2022:

I like, this is more elegant.

in test/functional/rpc_rawtransaction.py:203 in 02ed9b997b outdated

 205 | +            'generated',
 206 | +            'height',
 207 | +            'value',
 208 | +            'scriptPubKey'
 209 | +        ]
 210 | +        script_pub_key_fields = [

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 02ed9b997b outdated

 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 02ed9b997b outdated

 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 02ed9b997b outdated

 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.

                missing_fields = set(fields).difference(gottx.keys())
                if missing_fields:
                    raise AssertionError(f"fields {', '.join(missing_fields)} are not in transaction")

in test/functional/rpc_rawtransaction.py:202 in 02ed9b997b outdated

 197 | +            'time',
 198 | +            'txid',
 199 | +            'vin',
 200 | +            'vout',
 201 | +            'vsize',
 202 | +            'weight'

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.

            'weight',

stickies-v commented at 2:29 PM on February 3, 2022: contributor

tACK 02ed9b997

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 58a638f77b outdated

 299 | @@ -263,13 +300,33 @@ static RPCHelpMan getrawtransaction()
 300 |          throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, errmsg + ". Use gettransaction for wallet transactions.");
 301 |      }
 302 |  
 303 | -    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.

    if (verbosity <= 0) {

aureleoules commented at 2:32 AM on April 12, 2022: member

tACK 58a638f77b4d9d69b64cd4e3eb9ff8da79f465c7. 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 58a638f77b outdated

 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;

jonatack commented at 11:03 AM on April 12, 2022:

            verbosity = request.params[1].get_bool();

in src/rpc/rawtransaction.cpp:314 in 58a638f77b outdated

 312 | +        result.pushKV("in_active_chain", in_active_chain);
 313 | +    }
 314 | +
 315 | +    CBlockUndo blockUndo;
 316 | +    CBlock block;
 317 | +    const bool is_block_pruned {WITH_LOCK(cs_main, return IsBlockPruned(blockindex))};

jonatack commented at 11:04 AM on April 12, 2022:

    const bool is_block_pruned{WITH_LOCK(cs_main, return IsBlockPruned(blockindex))};

(don't hesitate to run clang-format on your changes)

dougEfresh commented at 8:50 AM on April 21, 2022:

Every time I do clang-format on this file, it changes a lot of things that we do not want. I did change this line though.

maflcko commented at 12:43 PM on April 21, 2022:

It is possible to run clang-format on your changes only: https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy

in test/functional/rpc_rawtransaction.py:217 in 58a638f77b outdated

 212 | +            'asm',
 213 | +            'hex',
 214 | +            'type',
 215 | +        ]
 216 | +        for n in [0, 3]:
 217 | +            for v in [1,2]:

jonatack commented at 11:05 AM on April 12, 2022:

 from collections import OrderedDict
 from decimal import Decimal
+from itertools import product
 
@@ -213,30 +214,29 @@ class RawTransactionsTest(BitcoinTestFramework):
-        for n in [0, 3]:
-            for v in [1,2]:
+        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

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

DrahtBot added the label Needs rebase on Jul 19, 2022

hernanmarino approved

hernanmarino commented at 12: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 a19823c72c outdated

 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();

prusnak commented at 8:30 PM on August 16, 2022:

Typo?

            verbosity = request.params[1].get_bool();

dougEfresh commented at 3:54 PM on August 19, 2022:

Must of missed this when I rebased. Fixed now.

dougEfresh force-pushed on Aug 19, 2022

dougEfresh force-pushed on Aug 23, 2022

DrahtBot added the label Needs rebase on Sep 13, 2022

dougEfresh force-pushed on Sep 17, 2022

DrahtBot removed the label Needs rebase on Sep 17, 2022

dougEfresh force-pushed on Sep 17, 2022

DrahtBot added the label Needs rebase on Sep 21, 2022

dougEfresh force-pushed on Oct 1, 2022

DrahtBot removed the label Needs rebase on Oct 1, 2022

in src/rpc/rawtransaction.cpp:323 in eae8f8e666 outdated

 314 | +    CBlockUndo blockUndo;
 315 | +    CBlock block;
 316 | +    const bool is_block_pruned{WITH_LOCK(cs_main, return chainman.m_blockman.IsBlockPruned(blockindex))};
 317 | +
 318 | +    if (verbosity == 1 || tx->IsCoinBase() ||
 319 | +        !blockindex || is_block_pruned ||

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 eae8f8e666 outdated

 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 eae8f8e666 outdated

 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.

         for n, v in product([0, 2], [1, 2]):
            self.log.info(f"Test getrawtransaction_verbosity {v} {'with' if n == 0 else 'without'} -txindex, with blockhash")
            if v == 2 and n == 0:
                gottx = self.nodes[n].getrawtransaction(txid=tx, verbosity=v)
                self.log.info(f"Tx data for -v=2 -txindex -noblockhash {gottx}")
                assert('fee' not in gottx)
                assert('prevout' not in gottx)

rajarshimaitra commented at 11:41 AM on October 4, 2022: contributor

Strong ReACK eae8f8e66621b75776a4a9bb5d8224c7320f89e4

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 eae8f8e666 outdated

  50 | @@ -50,15 +51,17 @@ using node::FindCoins;
  51 |  using node::GetTransaction;
  52 |  using node::NodeContext;
  53 |  using node::PSBTAnalysis;
  54 | +using node::ReadBlockFromDisk;
  55 | +using node::UndoReadFromDisk;
  56 |  
  57 | -static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry, Chainstate& active_chainstate)
  58 | +static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry, Chainstate& active_chainstate, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_TXID)

aureleoules commented at 4:10 PM on October 4, 2022:

static void TxToJSON(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, Chainstate& active_chainstate, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_TXID)

in test/functional/rpc_rawtransaction.py:217 in eae8f8e666 outdated

 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:

"prevout" : {                  (json object)
  "generated" : true|false,    (boolean)
  "value" : n,                 (numeric) The value in BTC
  "height" : n,                (numeric) previous block height
  "scriptPubKey" : {           (json object)
    "asm" : "str",             (string) the asm
    "desc" : "str",            (string) desc
    "hex" : "str",             (string) the hex
    "address" : "str",         (string, optional) The Bitcoin address (only if a well-defined address exists)
    "type" : "str"             (string) The type, eg 'pubkeyhash'
  }
}

I guess there's no reason it should look much different to what's in getblock for verbosity=3? For reference:

"prevout" : {                  (json object) (Only if undo information is available)
  "generated" : true|false,    (boolean) Coinbase or not
  "height" : n,                (numeric) The height of the prevout
  "value" : n,                 (numeric) The value in BTC
  "scriptPubKey" : {           (json object)
    "asm" : "str",             (string) Disassembly of the public key script
    "desc" : "str",            (string) Inferred descriptor for the output
    "hex" : "hex",             (string) The raw public key script bytes, hex-encoded
    "address" : "str",         (string, optional) The Bitcoin address (only if a well-defined address exists)
    "type" : "str"             (string) The type (one of: nonstandard, pubkey, pubkeyhash, scripthash, multisig, nulldata, witness_v0_scripthash, witness_v0_keyhash, witness_v1_taproot, witness_unknown)
  }
}

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

ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9

hernanmarino commented at 6:55 PM on November 28, 2022: contributor

re ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9

pablomartin4btc commented at 7:51 PM on November 28, 2022: member

re-tACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9

achow101 commented at 10:40 PM on December 13, 2022: member

ACK f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9

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:

@MarcoFalke Fixed in https://github.com/bitcoin/bitcoin/pull/26734

in src/rpc/rawtransaction.cpp:181 in f86697163e

 182 |  
 183 | -                "\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

Nice, thx. Left some doc nits.

bitcoin locked this on Dec 20, 2023