This PR enables BlockAssembler to add all selected packages’ feer and vrtual size to a vector, and then return the vector as a member of CBlockTemplate struct.
This PR is the first step in the #30157 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.
| Type | Reviewers |
|---|---|
| Stale ACK | rkrux |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
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.
- The commit also tests this new behaviour.
Co-authored-by: willcl-ark <will@256k1.dev>
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.
🐙 This pull request conflicts with the target branch and needs rebase.
