[WIP] transaction fees in getblock #16083

pull FelixWeis wants to merge 2 commits into bitcoin:master from FelixWeis:201905_grt_prevout changing 4 files +135 −97

FelixWeis commented at 2:12 PM on May 24, 2019: contributor
Using block undo data (like in #14802) we can now show fee information for each transaction in a block without the need for additional -txindex and/or a ton of costly lookups. For a start we'll add transaction fee information to getblock verbosity level 2. This comes at a negligible speed penalty (<1%). Optimally, we add a "prevout" KV to each input spent, displaying additional information like value, scriptPubKey and height for each input spent. Because this involves calculating addresses it is a lot more costly (~ 22% slower) so new verbosity level 3 is introduced.
```
src/bitcoin-cli getblock 00000000000000000007fce39a80dc9110fe8709ee504c01e1f04f1f59be7bbf 2
...
      "fees": 0.00124850,
...
```
move only Block/Undo checked aaa2fd1d2a
getblock: tx fee calculation for verbosity 2 via Undo data, new verbosity level 3 showing prevout info for inputs dd83c4c925
DrahtBot added the label RPC/REST/ZMQ on May 24, 2019

in src/rpc/blockchain.cpp:181 in dd83c4c925

 180 | +    if (verbosity >= 2) {
 181 | +        const CBlockUndo blockUndo = GetUndoChecked(blockindex);
 182 | +        for (size_t i = 0; i < block.vtx.size(); ++i) {
 183 | +
 184 | +            const auto& tx = block.vtx.at(i);
 185 | +            const CTxUndo* ptr_txundo;

promag commented at 9:03 AM on May 25, 2019:

Error:

error C4703: potentially uninitialized local pointer variable 'ptr_txundo'

Suggestion:

const CTxUndo* tx_undo = tx->IsCoinBase() ? nullptr : &block_undo.vtxundo.at(i - 1);

luke-jr commented at 8:56 PM on September 1, 2019:

Could (should?) probably just use i instead of IsCoinBase

in src/rpc/blockchain.cpp:192 in dd83c4c925

 193 | +            TxToUniv(*tx, uint256(), objTx, true, RPCSerializationFlags(), verbosity, ptr_txundo);
 194 |              txs.push_back(objTx);
 195 |          }
 196 | -        else
 197 | +    } else {
 198 | +        for (size_t i = 0; i < block.vtx.size(); ++i) {

promag commented at 9:04 AM on May 25, 2019:

Could keep the range loop?

in src/core_write.cpp:253 in dd83c4c925
```
 244 | @@ -225,9 +245,17 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry,
 245 |          ScriptPubKeyToUniv(txout.scriptPubKey, o, true);
 246 |          out.pushKV("scriptPubKey", o);
 247 |          vout.push_back(out);
 248 | +
 249 | +        amt_total_out += txout.nValue;
 250 |      }
 251 |      entry.pushKV("vout", vout);
 252 |  
 253 | +    if (txundo != nullptr && !tx.IsCoinBase()) {
```
promag commented at 9:06 AM on May 25, 2019:

Should be enough if (txundo) {?

luke-jr commented at 10:34 PM on August 19, 2019:

Need to remove the assert below too, since fees will be negative on a generation transaction.

luke-jr commented at 8:52 PM on September 1, 2019:

Also might need to subtract subsidy amount from generation.

luke-jr commented at 8:57 PM on September 1, 2019:

Nevermind, I see that by initialising txundo to nullptr, this is safe.
promag commented at 9:22 AM on May 25, 2019: member

Concept ACK.

Instead of moving code up, you could forward declare those functions at the top? I have some code nits when this is ready for review.

Needs tests (also in test/functional/feature_pruning.py ?) and release note.

in src/core_write.cpp:223 in dd83c4c925

 218 | +                if (verbosity >= 3) {
 219 | +                    UniValue p(UniValue::VOBJ);
 220 | +                    p.pushKV("height", (int64_t)prevout.nHeight);
 221 | +                    p.pushKV("value", ValueFromAmount(prevout.out.nValue));
 222 | +                    p.pushKV("coinbase", (bool)prevout.fCoinBase);
 223 | +                    UniValue o(UniValue::VOBJ);

practicalswift commented at 10:27 AM on May 25, 2019:

This o shadows o in outer scope :-)

FelixWeis commented at 7:29 PM on May 25, 2019: contributor

thanks for the review, @promag / @practicalswift. yes will get tests. as im not a cpp dev i just wanted to get some quick feedback on feasability and style (optional nullptr in funciton arg etc...)
DrahtBot commented at 9:10 PM on June 1, 2019: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #16129 (refactor: Remove unused includes by practicalswift)
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.
DrahtBot commented at 3:21 PM on June 6, 2019: contributor

Needs rebase
DrahtBot added the label Needs rebase on Jun 6, 2019
0xB10C commented at 4:19 PM on June 18, 2019: contributor

Concept ACK.
jnewbery cross-referenced this on Jul 8, 2019 from issue Future PRs by jnewbery

in src/core_write.cpp:222 in dd83c4c925

 217 | +                amt_total_in += prevout.out.nValue;
 218 | +                if (verbosity >= 3) {
 219 | +                    UniValue p(UniValue::VOBJ);
 220 | +                    p.pushKV("height", (int64_t)prevout.nHeight);
 221 | +                    p.pushKV("value", ValueFromAmount(prevout.out.nValue));
 222 | +                    p.pushKV("coinbase", (bool)prevout.fCoinBase);

luke-jr commented at 10:32 PM on August 19, 2019:

Should be "generated" or something else - it's never a coinbase itself.

in src/rpc/blockchain.cpp:860 in dd83c4c925

 940 | -                },
 941 | -                RPCExamples{
 942 | -                    HelpExampleCli("getblock", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\"")
 943 | -            + HelpExampleRpc("getblock", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\"")
 944 | -                },
 945 | +    const RPCHelpMan help{

luke-jr commented at 10:39 PM on August 19, 2019:

Why is this all reformatted?

luke-jr changes_requested

in src/rpc/blockchain.cpp:157 in dd83c4c925

 153 | @@ -121,7 +154,7 @@ UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex
 154 |      return result;
 155 |  }
 156 |  
 157 | -UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, bool txDetails)
 158 | +UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, const int verbosity)

luke-jr commented at 8:47 PM on September 1, 2019:

Changing a bool to int here will silently turn true into 1 and misbehave. Either rename the function, or find a way to make it give a compile error when a boolean is passed.

luke-jr commented at 8:49 PM on September 1, 2019:

(In fact, this is breaking rest_block in your PR!)

in src/core_write.cpp:256 in dd83c4c925

 251 |      entry.pushKV("vout", vout);
 252 |  
 253 | +    if (txundo != nullptr && !tx.IsCoinBase()) {
 254 | +        CAmount fees = amt_total_in - amt_total_out;
 255 | +        assert(MoneyRange(fees));
 256 | +        entry.pushKV("fees", ValueFromAmount(fees));

luke-jr commented at 8:53 PM on September 1, 2019:

Probably should be "fee"

laanwj added the label Waiting for author on Sep 30, 2019

in src/rpc/blockchain.cpp:177 in dd83c4c925

 176 | -    {
 177 | -        if(txDetails)
 178 | -        {
 179 | +
 180 | +    if (verbosity >= 2) {
 181 | +        const CBlockUndo blockUndo = GetUndoChecked(blockindex);

luke-jr commented at 5:42 PM on November 18, 2019:

Looks like this will fail if the block isn't in the main chain?

MarcoFalke commented at 6:56 PM on November 18, 2019: member

@FelixWeis Wen rebase, sir?

I'd like to see this in 0.20.0
MarcoFalke removed the label Waiting for author on Nov 18, 2019
luke-jr commented at 3:04 AM on January 5, 2020: member

Addressed issues and at least partially rebased (to 0.19 branch-point) in https://github.com/bitcoin/bitcoin/compare/master...luke-jr:rpc_getblock_prevouts_fees-0.19
luke-jr cross-referenced this on Mar 5, 2020 from issue Add new filter type v0 for segwit only Scripts to blockfilterindex by dangershony
MarcoFalke added the label Up for grabs on Mar 5, 2020
fanquake closed this on Mar 13, 2020
MarcoFalke cross-referenced this on Apr 26, 2020 from issue Pick up "transaction fees in getblock" pull request by MarcoFalke
MarcoFalke cross-referenced this on Apr 26, 2020 from issue RPC Feature: option for prevouts in getrawtransaction by jimpo
robot-visions cross-referenced this on Apr 26, 2020 from issue rpc: calculate fees in getblock using BlockUndo data by robot-visions
MarcoFalke cross-referenced this on May 18, 2020 from issue [RPC] Getting the spent transaction outputs of transactions in latest blocks by NicolasDorier
bitcoin deleted a comment on May 21, 2020
bitcoin deleted a comment on Jun 9, 2020
fanquake removed the label Up for grabs on Sep 14, 2020
fanquake commented at 1:38 AM on September 14, 2020: member

Being picked up in #18772.
MarcoFalke referenced this in commit f656165e9c on Dec 24, 2020
bitcoin locked this on Feb 15, 2022

Contributors

Labels

RPC/REST/ZMQ Needs rebase

Linked (view graph)

#13108 RPC Feature: option for prevouts in getrawtransaction #14802 rpc: faster getblockstats using BlockUndo data #15206 Immediately disconnect on invalid net message checksum #18223 Add new filter type v0 for segwit only Scripts to blockfilterindex #18771 Pick up "transaction fees in getblock" pull request #18772 rpc: calculate fees in getblock using BlockUndo data #19003 [RPC] Getting the spent transaction outputs of transactions in latest blocks

[WIP] transaction fees in getblock #16083

Conflicts