Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] transaction fees in getblock #16083

Draft
wants to merge 2 commits into
base: master
from

Conversation

@FelixWeis
Copy link
Contributor

commented May 24, 2019

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,
...
FelixWeis added 2 commits May 24, 2019
…sity level 3 showing prevout info for inputs
Copy link
Member

left a comment

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.

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

const auto& tx = block.vtx.at(i);
const CTxUndo* ptr_txundo;

This comment has been minimized.

Copy link
@promag

promag May 25, 2019

Member

Error:

error C4703: potentially uninitialized local pointer variable 'ptr_txundo'

Suggestion:

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 1, 2019

Member

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

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

This comment has been minimized.

Copy link
@promag

promag May 25, 2019

Member

Could keep the range loop?

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

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

This comment has been minimized.

Copy link
@promag

promag May 25, 2019

Member

Should be enough if (txundo) {?

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 19, 2019

Member

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 1, 2019

Member

Also might need to subtract subsidy amount from generation.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 1, 2019

Member

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

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 comment has been minimized.

Copy link
@practicalswift

practicalswift May 25, 2019

Member

This o shadows o in outer scope :-)

@FelixWeis

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

Needs rebase
@0xB10C

This comment has been minimized.

Copy link

commented Jun 18, 2019

Concept ACK.

@jnewbery jnewbery referenced this pull request Jul 8, 2019
UniValue p(UniValue::VOBJ);
p.pushKV("height", (int64_t)prevout.nHeight);
p.pushKV("value", ValueFromAmount(prevout.out.nValue));
p.pushKV("coinbase", (bool)prevout.fCoinBase);

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 19, 2019

Member

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

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

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 19, 2019

Member

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

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 19, 2019

Member

Why is this all reformatted?

@@ -121,7 +154,7 @@ UniValue blockheaderToJSON(const CBlockIndex* tip, const CBlockIndex* blockindex
return result;
}

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)

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 1, 2019

Member

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.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 1, 2019

Member

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

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

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 1, 2019

Member

Probably should be "fee"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.