[WIP] transaction fees in getblock by FelixWeis · Pull Request #16083 · bitcoin/bitcoin

FelixWeis · 2019-05-24T14:12:41Z

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

…sity level 3 showing prevout info for inputs

promag

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.

promag · 2019-05-25T09:03:34Z

src/rpc/blockchain.cpp

+        for (size_t i = 0; i < block.vtx.size(); ++i) {
+
+            const auto& tx = block.vtx.at(i);
+            const CTxUndo* ptr_txundo;


Error:

error C4703: potentially uninitialized local pointer variable 'ptr_txundo'

Suggestion:

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

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

promag · 2019-05-25T09:04:37Z

src/rpc/blockchain.cpp

        }
-        else
+    } else {
+        for (size_t i = 0; i < block.vtx.size(); ++i) {


Could keep the range loop?

promag · 2019-05-25T09:06:45Z

src/core_write.cpp

    }
    entry.pushKV("vout", vout);

+    if (txundo != nullptr && !tx.IsCoinBase()) {


Should be enough if (txundo) {?

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

Also might need to subtract subsidy amount from generation.

Nevermind, I see that by initialising txundo to nullptr, this is safe.

practicalswift · 2019-05-25T10:27:06Z

src/core_write.cpp

+                    p.pushKV("height", (int64_t)prevout.nHeight);
+                    p.pushKV("value", ValueFromAmount(prevout.out.nValue));
+                    p.pushKV("coinbase", (bool)prevout.fCoinBase);
+                    UniValue o(UniValue::VOBJ);


This o shadows o in outer scope :-)

FelixWeis · 2019-05-25T19:29:26Z

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 · 2019-06-01T21:10:16Z

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 · 2019-06-06T15:21:39Z

Needs rebase

0xB10C · 2019-06-18T16:19:47Z

Concept ACK.

luke-jr · 2019-08-19T22:32:15Z

src/core_write.cpp

+                    UniValue p(UniValue::VOBJ);
+                    p.pushKV("height", (int64_t)prevout.nHeight);
+                    p.pushKV("value", ValueFromAmount(prevout.out.nValue));
+                    p.pushKV("coinbase", (bool)prevout.fCoinBase);


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

luke-jr · 2019-08-19T22:34:57Z

src/core_write.cpp

    }
    entry.pushKV("vout", vout);

+    if (txundo != nullptr && !tx.IsCoinBase()) {


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

luke-jr · 2019-08-19T22:39:16Z

src/rpc/blockchain.cpp

-                    HelpExampleCli("getblock", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\"")
-            + HelpExampleRpc("getblock", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\"")
-                },
+    const RPCHelpMan help{


Why is this all reformatted?

luke-jr · 2019-09-01T20:47:50Z

src/rpc/blockchain.cpp

 }

-UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, bool txDetails)
+UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIndex* blockindex, const int verbosity)


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.

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

luke-jr · 2019-09-01T20:53:11Z

src/core_write.cpp

+    if (txundo != nullptr && !tx.IsCoinBase()) {
+        CAmount fees = amt_total_in - amt_total_out;
+        assert(MoneyRange(fees));
+        entry.pushKV("fees", ValueFromAmount(fees));


Probably should be "fee"

luke-jr · 2019-11-18T17:42:12Z

src/rpc/blockchain.cpp

-        {
+
+    if (verbosity >= 2) {
+        const CBlockUndo blockUndo = GetUndoChecked(blockindex);


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

maflcko · 2019-11-18T18:56:21Z

@FelixWeis Wen rebase, sir?

I'd like to see this in 0.20.0

luke-jr · 2020-01-05T03:04:38Z

Addressed issues and at least partially rebased (to 0.19 branch-point) in master...luke-jr:rpc_getblock_prevouts_fees-0.19

fanquake · 2020-09-14T01:38:49Z

Being picked up in #18772.

…data 66d012a test: RPC: getblock fee calculations (Elliott Jin) bf7d6e3 RPC: getblock: tx fee calculation for verbosity 2 via Undo data (Elliott Jin) Pull request description: This change is progress towards bitcoin#18771 . It adapts the fee calculation part of bitcoin#16083 and addresses some feedback. The additional "verbosity level 3" features are planned for a future PR. **Original PR description:** > Using block undo data (like in bitcoin#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%). ACKs for top commit: luke-jr: tACK 66d012a fjahr: tACK 66d012a MarcoFalke: review ACK 66d012a 🗜 Tree-SHA512: be1fe4b866946a8dc36427f7dc72a20e10860e320a28fa49bc85bd2a93a0d699768179be29fa52e18b2ed8505d3ec272e586753ef2239b4230e0aefd233acaa2

FelixWeis added 2 commits May 24, 2019 15:46

move only Block/Undo checked

aaa2fd1

getblock: tx fee calculation for verbosity 2 via Undo data, new verbo…

dd83c4c

…sity level 3 showing prevout info for inputs

DrahtBot added the RPC/REST/ZMQ label May 24, 2019

promag reviewed May 25, 2019

View reviewed changes

practicalswift reviewed May 25, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jun 6, 2019

jnewbery mentioned this pull request Jul 8, 2019

Future PRs bitcoin-core-review-club/website#14

Closed

luke-jr suggested changes Aug 19, 2019

View reviewed changes

luke-jr reviewed Sep 1, 2019

View reviewed changes

laanwj added the Waiting for author label Sep 30, 2019

luke-jr reviewed Nov 18, 2019

View reviewed changes

maflcko removed the Waiting for author label Nov 18, 2019

luke-jr mentioned this pull request Mar 5, 2020

Add new filter type v0 for segwit only Scripts to blockfilterindex #18223

Closed

maflcko added the Up for grabs label Mar 5, 2020

fanquake closed this Mar 13, 2020

This was referenced Apr 26, 2020

Pick up "transaction fees in getblock" pull request #18771

Closed

RPC Feature: option for prevouts in getrawtransaction #13108

Closed

robot-visions mentioned this pull request Apr 26, 2020

rpc: calculate fees in getblock using BlockUndo data #18772

Merged

maflcko mentioned this pull request May 18, 2020

[RPC] Getting the spent transaction outputs of transactions in latest blocks #19003

Closed

bitcoin deleted a comment from WasanTantiwararom May 21, 2020

bitcoin deleted a comment from Irshad4401 Jun 9, 2020

fanquake removed the Up for grabs label Sep 14, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

FelixWeis commented May 24, 2019

Uh oh!

promag left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

FelixWeis commented May 25, 2019

Uh oh!

DrahtBot commented Jun 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Jun 6, 2019

Uh oh!

0xB10C commented Jun 18, 2019

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!

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!

maflcko commented Nov 18, 2019

Uh oh!

luke-jr commented Jan 5, 2020

Uh oh!

fanquake commented Sep 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Jun 1, 2019 •

edited

Loading