MiniMiner: use FeeFrac in AncestorFeerateComparator

glozow commented at 1:01 pm on July 9, 2024: member

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::Mul doesn’t have the overflow problem.

Also added a few minor fuzzer fixups that caught my eye while I was debugging this.

glozow added the label Bug on Jul 9, 2024

DrahtBot commented at 1:01 pm on July 9, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ismaelsadeeq, murchandamus, dergoegge
Stale ACK	sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30079 (Fee Estimation: Ignore all transactions that are CPFP’d by ismaelsadeeq)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)

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.

ismaelsadeeq commented at 3:28 pm on July 9, 2024: member

Concept ACK 271469b8bcb84af8f7a64c530e9cee7e0bf96352

in src/node/mini_miner.cpp:189 in 6254c253e9 outdated

196-                       CFeeRate(ancestor_fee, ancestor_size) :
197-                       CFeeRate(tx_fee, tx_size);
198+        auto min_feerate = [](const MiniMinerMempoolEntry& e) -> FeeFrac {
199+            FeeFrac self_feerate(e.GetModifiedFee(), e.GetTxSize());
200+            FeeFrac ancestor_feerate(e.GetModFeesWithAncestors(), e.GetSizeWithAncestors());
201+            return self_feerate < ancestor_feerate ? self_feerate : ancestor_feerate;

sipa commented at 3:58 pm on July 9, 2024:

Nit: could be return std::min(self_feerate, ancestor_feerate);

glozow commented at 4:23 pm on July 9, 2024:

done

sipa commented at 3:59 pm on July 9, 2024: member

utACK 271469b8bcb84af8f7a64c530e9cee7e0bf96352

DrahtBot requested review from ismaelsadeeq on Jul 9, 2024

MiniMiner: use FeeFrac in AncestorFeerateComparator

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.

de273d5300

fuzz: mini_miner_selection fixups.

Delete asserts that are redundant with the == assert.
Add assertion that the coinbase isn't already in mock_template_txids.

09370529fb

glozow force-pushed on Jul 9, 2024

ismaelsadeeq approved

ismaelsadeeq commented at 5:51 pm on July 9, 2024: member

Tested ACK 09370529fb9f6d06f6d16bdb1fb336f7a265d8ba

DrahtBot requested review from sipa on Jul 9, 2024

glozow requested review from murchandamus on Jul 10, 2024

in src/node/mini_miner.cpp:174 in de273d5300 outdated

173@@ -174,7 +174,7 @@ MiniMiner::MiniMiner(const std::vector<MiniMinerMempoolEntry>& manual_entries,
174     SanityCheck();

murchandamus commented at 3:37 pm on July 11, 2024:

In the commit message of “MiniMiner: use FeeFrac in AncestorFeerateComparator”:

0- Comparing using FeeFracs is more precise, allows us to simply the
1+ Comparing feerates using FeeFracs is more precise, allows us to simplify the

in src/test/fuzz/mini_miner.cpp:192 in 09370529fb

187@@ -188,9 +188,9 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
188     auto mock_template_txids = mini_miner.GetMockTemplateTxids();
189     // MiniMiner doesn't add a coinbase tx.
190     assert(mock_template_txids.count(blocktemplate->block.vtx[0]->GetHash()) == 0);
191-    mock_template_txids.emplace(blocktemplate->block.vtx[0]->GetHash());
192-    assert(mock_template_txids.size() <= blocktemplate->block.vtx.size());
193-    assert(mock_template_txids.size() >= blocktemplate->block.vtx.size());
194+    auto [iter, new_entry] = mock_template_txids.emplace(blocktemplate->block.vtx[0]->GetHash());
195+    assert(new_entry);

murchandamus commented at 4:58 pm on July 11, 2024:

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.

glozow commented at 10:13 am on July 12, 2024:

I’ll add an update to the comment if I retouch

murchandamus commented at 5:00 pm on July 11, 2024: contributor

ACK 09370529fb9f6d06f6d16bdb1fb336f7a265d8ba with nits

dergoegge approved

dergoegge commented at 8:32 am on July 15, 2024: member

tACK 09370529fb9f6d06f6d16bdb1fb336f7a265d8ba

Fuzzed both mini miner harnesses for a while.

fanquake merged this on Jul 15, 2024

fanquake closed this on Jul 15, 2024

glozow deleted the branch on Jul 15, 2024

MiniMiner: use FeeFrac in AncestorFeerateComparator #30412

Code Coverage

Reviews

Conflicts