This PR enables BlockAssembler to add all selected packages’ feerate and vsize to a vector, and then return the vector as vFeeratePerSize in the CBlockTemplate struct.
This PR is the first step in the #30157 project.
The packages’ vsize and fee rates are used in #30157 to determine the 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 detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| 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?
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 test that block assembler now returns selected package
vsize's and fee rates.
Co-authored-by: ismaelsadeeq <ask4ismailsadiq@gmail.com>
