For the coinbase vTxFees used a dummy value of -nFees.
Similarly the first vTxSigOpsCost entry was calculated from
the dummy coinbase transaction.
This was introduced in #2115, but the values were never returned by the RPC or used in a test.
Drop ’m and add code comments to prevent confusion.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31897.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | ismaelsadeeq, ryanofsky |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
129+ // sigops to it. The coinbase is skipped by the getblocktemplate RPC.
130+ // Mining interface consumers must not use it.
131 pblock->vtx.emplace_back();
132- pblocktemplate->vTxFees.push_back(-1); // updated at end
133- pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
134+ pblocktemplate->vTxFees.push_back(0);
vTxFees.
Code review ACK d1cc874d1f87a654aa2da50dbcf850b84517e938
This does seem like a good simplification, but maybe instead of changing from non-zero dummy values to zero dummy values, it could make sense to drop dummy values entirely like:
0--- a/src/interfaces/mining.h
1+++ b/src/interfaces/mining.h
2@@ -37,11 +37,9 @@ public:
3 // Block contains a dummy coinbase transaction that should not be used.
4 virtual CBlock getBlock() = 0;
5
6- // Fees per transaction. Position 0 represents a dummy coinbase that should
7- // not be used.
8+ // Fees per transaction, not including coinbase transaction.
9 virtual std::vector<CAmount> getTxFees() = 0;
10- // Sigop cost per transaction. Position 0 represents a dummy coinbase that should
11- // not be used.
12+ // Sigop cost per transaction, not including coinbase transaction.
13 virtual std::vector<int64_t> getTxSigops() = 0;
14
15 virtual CTransactionRef getCoinbaseTx() = 0;
16--- a/src/node/miner.cpp
17+++ b/src/node/miner.cpp
18@@ -128,8 +128,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
19 // sigops to it. The coinbase is skipped by the getblocktemplate RPC.
20 // Mining interface consumers must not use it.
21 pblock->vtx.emplace_back();
22- pblocktemplate->vTxFees.push_back(0);
23- pblocktemplate->vTxSigOpsCost.push_back(0);
24
25 LOCK(::cs_main);
26 CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip();
27@@ -175,7 +173,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
28 UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
29 pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());
30 pblock->nNonce = 0;
31- pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
32
33 BlockValidationState state;
34 if (m_options.test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev,
35--- a/src/node/miner.h
36+++ b/src/node/miner.h
37@@ -37,9 +37,9 @@ static const bool DEFAULT_PRINT_MODIFIED_FEE = false;
38 struct CBlockTemplate
39 {
40 CBlock block;
41- // Fees per transaction. Position 0 represents a dummy coinbase.
42+ // Fees per transaction, not including coinbase transaction.
43 std::vector<CAmount> vTxFees;
44- // Sigops per transaction. Position 0 represents a dummy coinbase.
45+ // Sigops per transaction, not including coinbase transaction.
46 std::vector<int64_t> vTxSigOpsCost;
47 std::vector<unsigned char> vchCoinbaseCommitment;
48 /* A vector of package fee rates, ordered by the sequence in which
49--- a/src/rpc/mining.cpp
50+++ b/src/rpc/mining.cpp
51@@ -893,7 +893,7 @@ static RPCHelpMan getblocktemplate()
52 }
53 entry.pushKV("depends", std::move(deps));
54
55- int index_in_template = i - 1;
56+ int index_in_template = i - 2;
57 entry.pushKV("fee", tx_fees.at(index_in_template));
58 int64_t nTxSigOps{tx_sigops.at(index_in_template)};
59 if (fPreSegWit) {
Also if clients of IPC interface just need total fees and sigops, it might make sense to a add a method returning both of those totals to make interface more convenient and efficient and not require multiple calls.
maybe instead of changing from non-zero dummy values to zero dummy values, it could make sense to drop dummy values entirely
I think it’s more intuitive for block.vtx and these two vectors to have the same length.
Also if clients of IPC interface just need total fees and sigops
Agreed, but at this point I haven’t seen a need for it yet. Conversely though, I’m also not convinced yet that we should drop them.
ACK d1cc874d1f87a654aa2da50dbcf850b84517e938
I think it’s more intuitive for block.vtx and these two vectors to have the same length.
No strong opinion but I tend to prefer @ryanofsky diff because it is precise. Since the coinbase transaction does not have fees, we should not simply add 0 for the size of vTxFees to match vtx .
They are two different list with different meaning
vtx is the list of transactions in the block, while vTxFees is the list of fees for each transaction in the block which should exclude the coinbase.
For vTxSigOpsCost I agree it should match vtx and 0 is okay because eventually there will be coinbase but we should clearly indicate the 0 there is dummy.
This is also similar to what was done in https://github.com/bitcoin/bitcoin/pull/31384/commits/777434a2cd14841e35ce39d7a6f51131e6a41de2 to improve precision of the block assembler.
174@@ -174,7 +175,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock()
175 UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
176 pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, chainparams.GetConsensus());
177 pblock->nNonce = 0;
178- pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
block.vtx contains a dummy coinbase, vTxFees and vTxSigOpsCost contain dummy fees and dummy sigop costs for the coinbase. That seems more straight forward to me than having vTxFees be shorter and then having to document why these things have different lengths.
Since the coinbase transaction does not have fees
That’s almost a philosophical question, because miners can (and sometimes accidentally did) claim less fees than allowed. In a way such a coinbase has negative fees, which is perhaps why the original code did that.
Perhaps the most rigorous solution is to add std::optional<CAmount> fee to CTransaction, but that would have implications all over the codebase, so I’d rather not go on that adventure.
block.vtx contains a dummy coinbase, vTxFees and vTxSigOpsCost contain dummy fees and dummy sigop cost
Good point.
That seems more straight forward to me than having vTxFees be shorter and then having to document why these things have different lengths.
We don’t have to document that vTxFees is just a vector of fees paid in the block.
Note: This is an unblocking comment.
This PR is an immediate win because we don’t have to perform checks like we do in https://github.com/bitcoin/bitcoin/pull/31283/files#diff-0ef8ae12c6e9ef2accc78537f42612b3267e8a7c45dc7e9eb998f797e79f2e95R1018-R1028, should this PR come before #31283?
This PR is an immediate win because we don’t have to perform checks like we do in
No this PR doesn’t change anything there. It’s just that people suggested using -vTxFees[] directly there instead of doing the calculation. This PR removes that temptation (and makes it easier to get rid of vTxFees entirely at some point).
Code review ACK 587ad67bf3bee03a7088e1dd5be2828b7a4f4fd4. Only change since last review is removing another dummy vTxSigOpsCost assignment.
IMO, it would be probably be more ideal to drop the dummy values from the CBlockTemplate::vTxFees and CBlockTemplate::vTxSigOpsCost vectors. I don’t think it matters if they have different indexing than CBlock::vtx because that is a different vector in a different class, and it would be confusing for that vector not to contain a placeholder transaction, because it would make indexing in the CBlock unstable over time and updating the CBlock inefficient. These reasons seem inapplicable to the CBlockTemplate members. I think the documentation in suggested diff was sufficient but could be updated with
0-// Fees per transaction, not including coinbase transaction.
1+// Fees per transaction, not including coinbase transaction (unlike CBlock::vtx)
2 std::vector<CAmount> vTxFees;
if an explicit comparison would be helpful. No strong opinion on this though, just explaining my thinking.
For the coinbase vTxFees used a dummy value of -nFees. This
value was never returned by the RPC or used in a test.
Similarly the fist vTxSigOpsCost entry was calculated from
the dummy coinbase transaction.
Drop both and add code comments to prevent confusion.
vTxFees and vTxSigOpsCost.