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.
This PR also adds test coverage for the fees and sigops fields in getblocktemplate, so it closes #32053.
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 |
|---|---|
| ACK | ismaelsadeeq, ryanofsky, glozow |
| Concept NACK | luke-jr |
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.
vTxFees and vTxSigOpsCost.
Code review ACK 6d7648795aa053f535510d11379fdd9d0399004c. Since last review, this was changed to drop dummy values instead of setting them to 0.
re: #31897 (comment)
Concept NACK, just makes the code less straightforward and forces re-calculation later if needed. Harmless to leave it
I’m confused about why total fees were stored in a dummy field, and why the fields were represented as a negative number, and why the field didn’t seem to be documented and also didn’t seem to be used. But I also don’t have a clear idea of why individual transaction fees as opposed to total fees are useful here either, so it is hard for me to form a real opinion other than that this change seems like a simplification and improvement that could be prevent bugs from using the undocumented dummy values incorrectly.
and make use (eg, in #31283 and other feature I haven’t PR’d yet)
IMO it doesn’t simplify #31283 to use -vTxFees[0] instead of adding up the fees, but it could be nice to add a separate total_fees field. Maybe total_fees could be useful for the other feature not PRed yet too?
forces re-calculation later if needed
It wouldn’t. We can introduce total_fees and set it in the same place where we now set this dummy value.
The recalculation in #31283 is done in order to avoid relying on this code. I considered adding total_fees for that PR, but it doesn’t seem useful because I intend to replace that code when cluster mempool comes around.
other feature I haven’t PR’d yet
That would justify a total_fees field. I’m happy to refactor waitNext() after said PR is merged, if that happens before the cluster mempool changes.
In either scenario I have no need for -vTxFees[0].
Code review ACK 6d7648795aa053f535510d11379fdd9d0399004c
This dummy values were introduced in #2115. I did not see the usage or rationale for adding the negative fees of all block transactions as the fee of the coinbase tx. I think this dummy values should be deleted.
However I think having total_fees values can be useful.
892@@ -893,7 +893,7 @@ static RPCHelpMan getblocktemplate()
893 }
894 entry.pushKV("depends", std::move(deps));
895
896- int index_in_template = i - 1;
897+ int index_in_template = i - 2;
int index_in_template = 0; or make changes to the values pushed for “fee” and “depends” none of the functional tests fail. Could be worth adding coverage?
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.
127+ utxo_to_spend=tx_b["new_utxo"])
128+
129+ # Generate transaction without sigops. It will go first because it pays
130+ # higher fees (100 sat/vbyte) and descends from a different coinbase.
131+ tx_d = self.wallet.send_self_transfer(from_node=node,
132+ fee_rate=Decimal("0.00100"))
Pass onfirmed_only to ensure that the transaction descends from a different coinbase.
Additionally, you can go further by ensuring that you have at least two confirmed UTXOs at the beginning; otherwise, split the first coinbase.
0 tx_d = self.wallet.send_self_transfer(from_node=node,
1 fee_rate=Decimal("0.00100"), confirmed_only=True)
MiniWallet instances can’t spend each others coins, so I don’t think these additions are needed - but useful to keep in mind.
re-ACK 226d81f8b708ea1c3a39ec58446e291fe7440fdd
Latest change added functional test coverage for the sigops and fee field of txs returned from getblocktemplate RPC
ACK 226d81f8b708ea1c3a39ec58446e291fe7440fdd
Thanks for adding the test! Checked that it’d catch any errors in the RPC code.