[WIP] transaction fees in getblock

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.

0src/bitcoin-cli getblock 00000000000000000007fce39a80dc9110fe8709ee504c01e1f04f1f59be7bbf 2
1...
2      "fees": 0.00124850,
3...

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:

0error C4703: potentially uninitialized local pointer variable 'ptr_txundo'

Suggestion:

0const 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: member

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: member

DrahtBot added the label Needs rebase on Jun 6, 2019

0xB10C commented at 4:19 pm on June 18, 2019: member

Concept ACK.

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

MarcoFalke added the label Up for grabs on Mar 5, 2020

fanquake closed this on Mar 13, 2020

fanquake deleted a comment on May 21, 2020

fanquake 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

DrahtBot locked this on Feb 15, 2022

[WIP] transaction fees in getblock #16083

Conflicts