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

dougEfresh · 2021-10-20T05:50:50Z

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
}

maflcko

Concept ACK

src/rpc/rawtransaction.cpp

lsilva01

Concept ACK.

It would be interesting to add a functional test case.

maflcko · 2021-10-20T18:52:54Z

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 · 2021-10-20T18:53:13Z

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 · 2021-10-20T19:08:55Z

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 · 2021-10-20T19:11:07Z

Yes, if that is the behavior of the other RPC, it makes sense to copy it.

sipa · 2021-10-20T19:17:00Z

You might want to also add the UTXOs being spent by the transaction. You're looking those up anyway.

maflcko · 2021-10-21T11:22:58Z

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

DrahtBot · 2021-10-25T18:59:59Z

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.

src/rpc/rawtransaction.cpp

dougEfresh · 2021-10-28T18:02:06Z

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 · 2021-10-28T18:20:00Z

@dougEfresh The scriptPubKey and amount being spent for each input (together with information about the scriptPubKey like the output already has).

maflcko · 2021-10-29T09:11:03Z

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 · 2021-10-29T11:48:53Z

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) ....
```

src/rpc/rawtransaction.cpp

luke-jr · 2021-10-30T03:22:40Z

test/functional/rpc_rawtransaction.py

+                assert_equal(gottx['in_active_chain'], True)
+                if v == 2:
+                    # fee is in verbosity 2 only
+                    assert_equal(gottx['fee'], Decimal('0.00002820'))


What is supposed to guarantee this is correct?

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.

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

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

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.

rajarshimaitra

Strong ReACK eae8f8e

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.

rajarshimaitra · 2022-10-04T11:33:29Z

src/rpc/rawtransaction.cpp

+    const bool is_block_pruned{WITH_LOCK(cs_main, return chainman.m_blockman.IsBlockPruned(blockindex))};
+
+    if (verbosity == 1 || tx->IsCoinBase() ||
+        !blockindex || is_block_pruned ||


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?

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)

test/functional/rpc_rawtransaction.py

aureleoules · 2022-10-04T16:10:54Z

src/rpc/rawtransaction.cpp


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


Suggested change

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

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

test/functional/rpc_rawtransaction.py

vostrnad

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 · 2022-10-30T07:24:42Z

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

* 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

dougEfresh · 2022-10-30T12:15:23Z

src/rpc/rawtransaction.cpp

+    }
+    // If request is verbosity >= 1 but no blockhash was given, then look up the blockindex
+    if (request.params[2].isNull()) {
+        LOCK(cs_main);


I like to avoid this lock, suggestions welcome.

aureleoules

ACK f866971

hernanmarino

re ACK f866971

pablomartin4btc

re-tACK f866971

achow101 · 2022-12-13T22:40:01Z

ACK f866971

…nsaction

maflcko

Nice, thx. Left some doc nits.

maflcko · 2022-12-14T08:31:44Z

src/rpc/rawtransaction.cpp

                     },
-                     RPCResult{"if verbose is set to true",
+                     RPCResult{"if verbosity is set to 1",
+                         // When updating this documentation, update `decoderawtransaction` in the same way.


nit: Is this dev comment still accurate, given that DecodeTxDoc was split out for this reason?

@MarcoFalke Fixed in #26734

src/rpc/rawtransaction.cpp

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

fanquake linked an issue Oct 20, 2021 that may be closed by this pull request

Return fee in getrawtransaction #23264

Closed

maflcko approved these changes Oct 20, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from 2730b68 to 2598983 Compare October 20, 2021 12:39

maflcko reviewed Oct 20, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from 2598983 to d3f084a Compare October 20, 2021 13:51

lsilva01 approved these changes Oct 20, 2021

View reviewed changes

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from d3f084a to 2e8ce71 Compare October 20, 2021 18:47

dougEfresh force-pushed the 23264-fee-getrawtransaction branch 2 times, most recently from 6c61582 to 83e5f6c Compare October 21, 2021 20:31

DrahtBot mentioned this pull request Oct 25, 2021

refactor: cleanup/followup of Remove -deprecatedrpc=addresses flag #22924

Closed

laanwj reviewed Oct 28, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the 23264-fee-getrawtransaction branch 2 times, most recently from 2051110 to 2c56d72 Compare October 29, 2021 11:42

dougEfresh changed the title ~~rpc: Return fee in getrawtransaction~~ rpc: Return fee and prevout (utxos) to getrawtransaction Oct 29, 2021

dougEfresh commented Oct 29, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Oct 29, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Oct 29, 2021

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Oct 30, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 8, 2022

refactor: Run type check against RPCArgs (1/2) #26039

Merged

DrahtBot added the Needs rebase label Sep 13, 2022

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from 6216d8d to 23eb378 Compare September 17, 2022 09:16

DrahtBot removed the Needs rebase label Sep 17, 2022

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from 23eb378 to f5c7620 Compare September 17, 2022 10:12

DrahtBot mentioned this pull request Sep 19, 2022

rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR #25737

Merged

DrahtBot added the Needs rebase label Sep 21, 2022

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from f5c7620 to eae8f8e Compare October 1, 2022 10:46

DrahtBot removed the Needs rebase label Oct 1, 2022

rajarshimaitra reviewed Oct 4, 2022

View reviewed changes

aureleoules reviewed Oct 4, 2022

View reviewed changes

dougEfresh force-pushed the 23264-fee-getrawtransaction branch 3 times, most recently from 845913f to 226b0b7 Compare October 10, 2022 07:58

vostrnad reviewed Oct 22, 2022

View reviewed changes

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from 226b0b7 to 6e5e745 Compare October 30, 2022 07:22

rpc: Return fee and prevout(s) to getrawtransaction

f866971

* 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

dougEfresh force-pushed the 23264-fee-getrawtransaction branch from 6e5e745 to f866971 Compare October 30, 2022 12:06

dougEfresh commented Oct 30, 2022

View reviewed changes

aureleoules approved these changes Nov 28, 2022

View reviewed changes

hernanmarino reviewed Nov 28, 2022

View reviewed changes

pablomartin4btc reviewed Nov 28, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 13, 2022

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

Closed

achow101 merged commit daf881d into bitcoin:master Dec 13, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 14, 2022

Merge bitcoin#23319: rpc: Return fee and prevout (utxos) to getrawtra…

f404487

…nsaction

maflcko reviewed Dec 15, 2022

View reviewed changes

casey mentioned this pull request Jan 27, 2023

Re-enable skipping transactions below first inscription height ordinals/ord#1364

Closed

bitcoin locked and limited conversation to collaborators Dec 20, 2023


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

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

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

Uh oh!

Conversation

dougEfresh commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For Reviewers

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 20, 2021

Uh oh!

dougEfresh commented Oct 20, 2021

Uh oh!

dougEfresh commented Oct 20, 2021

Uh oh!

maflcko commented Oct 20, 2021

Uh oh!

sipa commented Oct 20, 2021

Uh oh!

maflcko commented Oct 21, 2021

Uh oh!

DrahtBot commented Oct 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

dougEfresh commented Oct 28, 2021

Uh oh!

sipa commented Oct 28, 2021

Uh oh!

maflcko commented Oct 29, 2021

Uh oh!

dougEfresh commented Oct 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougEfresh Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougEfresh Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vostrnad left a comment

Choose a reason for hiding this comment

Uh oh!

dougEfresh commented Oct 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougEfresh commented Oct 20, 2021 •

edited

Loading

DrahtBot commented Oct 25, 2021 •

edited

Loading

dougEfresh Oct 30, 2021 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

dougEfresh Oct 10, 2022 •

edited

Loading