This PR enables BlockAssembler to add all selected packages’ fee and virtual size to a vector, and then return the vector as a member of CBlockTemplate struct.
This PR is the first step in the #30392 project.
The packages’ vsize and fee are used in #30157 to select a percentile fee rate of the top block in the mempool.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30391.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
first commit: could adjust the test
0src/test/amount_tests.cpp:86: // Maximum size in bytes, should not crash
To std::numeric_limits<uint64_t>::max() >> 1 ?
And
0src/test/fuzz/fee_rate.cpp:23: const auto bytes = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
to fuzzed_data_provider.ConsumeIntegral<uint64_t>() >> 1?
This causes a fuzz failure during the conversion of the size to feerate per KB because CAmount is int64_t.
So I reverted back and static_cast the packageSize to be uint32_t, it is safe to do so because we are certain that nSizeWithAncestors will always be positive in BlockAssembler::addPackageTxs.
It’s already done https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/node/miner.cpp#L365
So I reverted back and
static_castthepackageSizeto beuint32_t, it is safe to do so because we are certain thatnSizeWithAncestorswill always be positive inBlockAssembler::addPackageTxs.
Another thing to consider is that the package size is small enough to be representable in uint32_t.
Yeah thanks.
unrelated but I wonder why we are using this big data type to represent size, not only while working in this PR, there are size datype inconsistencies in several places which sometimes cause overflows during calculation.
Is it safe to instead represent a type alias for size just like it was done for CAmount in consensus/amount.h.
And prevent generating large values for size in the fuzz test?
Apart from the fuzz test size can never be greater than 4,000,000?
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/consensus/consensus.h#L12-L15
The fuzz failure is because you also changed
MultiplicationOverflowtouint64_t, which is wrong.
Yeah, I did that because MultiplicationOverflow requires that the type of both parameters be the same.
Apart from the fuzz test size can never be greater than
4,000,000?
In blocks, yes. In the mempool, it may depend on the user’s config.
39@@ -38,6 +40,7 @@ struct CBlockTemplate
40 std::vector<CAmount> vTxFees;
41 std::vector<int64_t> vTxSigOpsCost;
42 std::vector<unsigned char> vchCoinbaseCommitment;
43+ std::vector<std::tuple<CFeeRate, uint32_t>> vFeeratePerSize;
FeeFrac would be better? CFeeRate loses precision and can be inferred from the fee and size
probably
FeeFracwould be better?CFeeRateloses precision and can be inferred from the fee and size
We will be using the package size to calculate the percentile package and select its fee rate as an estimate to return to the user.
We then have to convert the fee and size to fee rate either by CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes).GetFeePerK or introduce and use a similar GetFeeRate method for the FeeFrac struct.
So, I think using CFeeRate is okay for getting MEMPOOL_FORECAST estimate.
I think the place we could benefit from changing this to FeeFrac is when comparing forecasters’ estimates.
But this can’t be done now because the BLOCK_POLICY_ESTIMATOR forecaster performs its operations in nSatoshiPerK and returns a CFeeRate value as an estimate.
It will require a refactor to change BLOCK_POLICY_ESTIMATOR to use and return FeeFrac as an estimate.
But the forecasters estimates are values greater than 1 s/vb.
Given this, I think using CFeeRate is okay and will avoid the conversions, or adding duplicate GetFeePerK method for FeeFrac and refactoring CBlockPolicyEstimator to use FeeFrac?
CFeeRate a wrapper around FeeFrac better, as suggested #30535 (comment)? If so, then no changes are needed, and we can proceed with this approach while pursuing that direction and mark this comment as resolved.
Otherwise, I’m happy to change this to FeeFrac as you suggested if it’s a blocking comment.
I’ve updated the code to use FeeFrac, and it simplified things – no need for tuples from the initial approach, just a vector of FeeFrac values which achieved same aim.
Smaller diff and no precision loss 💯 I will update all the dependent commits to follow suite.
181+
182+ CFeeRate packageRate(packageFee, packageSize);
183+ auto packageFeeRateFound = findFeeRate(blockFeeAndVsizes, packageRate);
184+
185+ CFeeRate mediumRate(mediumFeeTx.GetFee(), mediumFeeTx.GetTxSize());
186+ auto mediumTxFeeRateFound = findFeeRate(blockFeeAndVsizes, mediumRate);
findFeeRate and explicitly checked the vector for the packages and verify the fee and size were as expected. I also took the opportunity to change the variables to be initialized uniformly.
Just a comment about how this interacts with the Mining interface changes I’m working on.
The Mining interface contains BlockTemplate:
I wonder if adding a field to CBlockTemplate would cause a breaking change for clients connecting via IPC. cc @ryanofsky?
Update: I don’t think it does, because the interface doesn’t use CBlockTemplate directly.
In any case it won’t matter as long as this PR is merged before v29, since we don’t ship the interface until then, so there’s clients to break.
so there’s clients to break. @Sjors, could you clarify your coment? Which clients would this affect?
@ismaelsadeeq are you referring to my statement at the end “so there’s clients to break”?
Once we release the Mining interface, and multiprocess, people can build clients. If we then make changes in the next release, we could break those clients. But that’s not an issue yet.
In any case it won’t matter as long as this PR is merged before v29
fwiw added to milestone and this seems like a fairly simple change. @Sjors @ryanofsky would you mind taking another look? I don’t think I have full context on Mining interface.
149@@ -148,6 +150,7 @@ class BlockAssembler
150 uint64_t nBlockSigOpsCost;
151 CAmount nFees;
152 std::unordered_set<Txid, SaltedTxidHasher> inBlock;
153+ std::vector<FeeFrac> feerateHistogram;
In commit “miner: CreateNewBlock will now track fee and virtual size of selected packages” (5171f16e6f96030fcc91b30bb196a2790e37848c)
It seems like it would be simpler and less fragile if BlockAssembler::addPackageTxs just populated pblocktemplate->FeerateHistogram directly, instead of filling this new BlockAssembler::feerateHistogram variable, which is effectively a temporary variable, and then moved from BlockAssembler::feerateHistogram to pblocktemplate->FeerateHistogram. It would be nice to be able to drop this new variable, and drop the new code moving the histogram from one variable to another.
Code review ACK 5171f16e6f96030fcc91b30bb196a2790e37848c
fwiw added to milestone and this seems like a fairly simple change. @Sjors @ryanofsky would you mind taking another look? I don’t think I have full context on Mining interface.
There’s actually no risk of this change breaking the mining interface because the mining interface isn’t directly exposing the CBlockTemplate struct. Also even if the CBlockTemplate struct were directly exposed it wouldn’t necessarily be a breaking change because old clients or servers encountering the new field would just ignore it, and new clients or servers expecting the new field could detect that the field is missing and handle that case appropriately.
420@@ -421,6 +421,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
421 }
422
423 ++nPackagesSelected;
424+ pblocktemplate->vFeerateHistogram.emplace_back(packageFees, static_cast<int32_t>(packageSize));
39@@ -39,6 +40,7 @@ struct CBlockTemplate
40 std::vector<CAmount> vTxFees;
41 std::vector<int64_t> vTxSigOpsCost;
42 std::vector<unsigned char> vchCoinbaseCommitment;
43+ std::vector<FeeFrac> vFeerateHistogram;
Getting thrown off a bit by the name here. Is it really a histogram at this point or just a vector of fee rates/fracs? Going by this def: https://en.wikipedia.org/wiki/Histogram
Sorry for not raising this earlier.
CalculatePercentiles function here: https://github.com/bitcoin/bitcoin/pull/30157/files#diff-34fc4771c305399b6f7634acd8c5fd2d0b89115f998397d41676180d742c28daR10
Is it really a histogram at this point or just a vector of fee rates/fracs?
It is a vector of package fee frac’s, which can be used to build a histogram.
Might be helpful to add a comment here telling that the values in this vector are sorted. That’s what I assumed while going through the
CalculatePercentilesfunction here.
Yes, it is sorted by the order in which the packages are selected in the block template. I believe this is obvious because we are emplacing the fee frac’s after adding the selected package. However, adding a comment could be useful for clarity.
I will update if there is a need to retouch.
It is a vector of package fee frac’s, which can be used to build a histogram.
Building a histogram seems to be one of the use cases of this field. I don’t feel it should be called a histogram only because of this. Is there any other reason as well? I hope I’m not missing any context here.
vFeerateHistogram in itself is fine and in the #21422 PR a histogram is indeed created.
What I find confusing here is calling a simple vector of feefrac’s a histogram. If a histogram will be created using this field later, let’s call that field a histogram and not this one?
I’ve seen the vectors above vFeerateHistogram, are also simple vector types containing integers, unsigned chars, or CAmounts.
However, we do not name these vectors vCAmounts, vInts, or vUnsignedChars. Instead, they are named based on some context, such as vTxFees, vTxSigOpsCost, and vchCoinbaseCommitment.
For this reason, I believe naming this vector vFeeFrac does not add much clarity because thats obvious.
Instead, I consider vFeerateHistogram to be the closest meaningful name, based on the context of #21422.
Consider this block template vector of package of fee rates: [20/5, 20/5, 20/5, 20/6, 20/6, 15/6]. after sorting by increasing feerate this implicitly represent a histogram-like structure:
Same as with @glozow comment in #30391 (review).
This vector is also implicitly a fee rate diagram. As we can use it to construct a feerate diagram after sorting by increasing feerate.
vPackagesFeerates would be the most accurate IMO.We can continue the discussion , but since this PR already has two A CKs I was hesitating to update, I will proceed to update the variable name and add a comment if a rebase or blocking suggestion arises.
Let me know if this a blocking comment, I am happy to update.
snake_case with m_ prefix: m_feerate_histogram.
However, we do not name these vectors vCAmounts, vInts, or vUnsignedChars. Instead, they are named based on some context, such as vTxFees, vTxSigOpsCost, and vchCoinbaseCommitment. For this reason, I believe naming this vector vFeeFrac does not add much clarity because thats obvious. Instead, I consider vFeerateHistogram to be the closest meaningful name, based on the context of #21422.
I agree with naming variables based on context, this is quite a common practice. I have raised this point because as a reviewer I found it confusing to see a field that’s called a histogram but is not, and therefore I don’t think it is the closest meaningful name. Either feeRateDiagram or packageFeeRates sound good to me.
We can continue the discussion , but since this PR already has two A CKs I was hesitating to update, I will proceed to update the variable name and add a comment if a rebase or blocking suggestion arises.
I understand that naming variables can be in the bikeshed territory and as an author it’s difficult to incorporate changes after getting multiple ACKs. That’s why I didn’t request changes and instead started a discussion because to me this name is confusing and was even more so to read it in the test, and thus I could not ACK the PR.
Nit: In the PR description, a draft PR is mentioned as the project. Can add this link for project issue: #30392
Added, and addressed your comments!
156+ BOOST_CHECK(blockFeerateHistogram.size() == 2);
157+ // lowFeeTx and highFeeChildTx are added to the block as a package.
158+ const auto packageFee{lowFeeTx.GetFee() + highFeeChildTx.GetFee()};
159+ const auto packageSize{lowFeeTx.GetTxSize() + highFeeChildTx.GetTxSize()};
160+ FeeFrac packageFeeFrac{packageFee, packageSize};
161+ BOOST_CHECK(blockFeerateHistogram[0] == packageFeeFrac);
Ah, @ismaelsadeeq did you mean to call this a feerate diagram instead?
@rkrux We’ve previously represented feerate diagrams as a vector of FeeFracs / chunks
121@@ -121,28 +122,43 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const
122 tx.vout[0].nValue = 5000000000LL - 1000;
123 // This tx has a low fee: 1000 satoshis
124 Txid hashParentTx = tx.GetHash(); // save this txid for later use
125- AddToMempool(tx_mempool, entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx));
126+ const auto lowFeeTx{entry.Fee(1000).Time(Now<NodeSeconds>()).SpendsCoinbase(true).FromTx(tx)};
nit: This should be named similarly to the txid to avoid confusion that this might be a different tx altogether. I suggest either calling this parent_tx or renaming hashParentTx to hash_low_fee_tx.
Also snake_case here and elsewhere in this test.
snake_case henceforth.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35265010233
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
- The package feerates are ordered by the sequence in which
packages are selected for inclusion in the block template.
- The commit also tests this new behaviour.
Co-authored-by: willcl-ark <will@256k1.dev>
tACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7
Thank you for incorporating the changes.
