MiniMiner: use FeeFrac in AncestorFeerateComparator #30412
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee 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. ConflictsReviewers, 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. |
Comparing using FeeFracs is more precise, allows us to simply the code since FeeFrac comparison internally does cross-multiplication, and avoids potential overflow in the multiplication. Previously, we were only comparing feerates up to 0.001sat/vB precision, since CFeeRate comparison just looks at their respective nSatoshisPerK. This could lead to MiniMiner selecting packages in the wrong order (i.e. by txid) if their feerates were less than 0.001sat/vB different.
Delete asserts that are redundant with the == assert. Add assertion that the coinbase isn't already in mock_template_txids.
glozow
force-pushed
the
2024-07-miniminer-feefrac
branch
from
271469b to
0937052
Compare
| @@ -174,7 +174,7 @@ MiniMiner::MiniMiner(const std::vector<MiniMinerMempoolEntry>& manual_entries, | |||
| SanityCheck(); | |||
There was a problem hiding this comment.
In the commit message of "MiniMiner: use FeeFrac in AncestorFeerateComparator":
- Comparing using FeeFracs is more precise, allows us to simply the
+ Comparing feerates using FeeFracs is more precise, allows us to simplify the| auto [iter, new_entry] = mock_template_txids.emplace(blocktemplate->block.vtx[0]->GetHash()); | ||
| assert(new_entry); |
There was a problem hiding this comment.
In 'fuzz: mini_miner_selection fixups':
The comment in L189 threw me off. I assume that it’s meant to indicate that we are able to add the coinbase transaction to mock_template_txids because MiniMiner did not add it, but I first understood it to indicate that we should not be able to add a coinbase transaction to mock_template_txids and therefore the assert was inverse to my expectation.
Also, TIL that the return values of emplace(…) differ between set and vector.
There was a problem hiding this comment.
I'll add an update to the comment if I retouch
Closes #30284. Closes #30367, see #30367 (comment)
Previously, we were only comparing feerates up to 1/1000 precision, since CFeeRate comparison just looks at their respective nSatoshisPerK. This could lead to MiniMiner selecting packages in the wrong order (i.e. by txid) if their feerates were less than 0.001sat/vB different. Fix this by creating + comparing
FeeFracs instead.Also,
FeeFrac::Muldoesn't have the overflow problem.Also added a few minor fuzzer fixups that caught my eye while I was debugging this.