Implement Mini version of BlockAssembler to calculate mining scores #27021

murchandamus · 2023-02-02T00:25:13Z

Implement Mini version of BlockAssembler to calculate mining scores

Run the mining algorithm on a subset of the mempool, only disturbing the
mempool to copy out fee information for relevant entries. Intended to be
used by wallet to calculate amounts needed for fee-bumping unconfirmed
transactions.

From comments of sipa and glozow below:

In what way does the code added here differ from the real block assembly code?

Only operates on the relevant transactions rather than full mempool

Has the ability to remove transactions that will be replaced so they don't impact their ancestors

Does not hold mempool lock outside of the constructor, makes copies of the entries it needs instead (though I'm not sure if this has an effect in practice)

Doesn't do the sanity checks like keeping weight within max block weight and IsFinalTx()

After the block template is built, additionally calculates fees to bump remaining ancestor packages to target feerate

DrahtBot · 2023-02-02T00:25:15Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, furszy, theStack
Concept ACK	glozow
Stale ACK	LarryRuane

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

Conflicts

No conflicts as of last run.

glozow

Wrote some more tests for this, please feel free to take: glozow@6761c59

src/txmempool.cpp

dergoegge · 2023-02-03T15:41:43Z

I wrote a fuzz test for the MiniMiner and it crashes on some of the Assumes: https://github.com/dergoegge/bitcoin/tree/2023-01-fuzz-mini-miner

$ echo "AQEWCQEBAAEACf//////////////CBwAAgAAlRwB7QEA/wAAAAL7AAEAAAEB7QEA/wAAAAL7AAAA
AAAAXAD//w==" | base64 -d > mini_miner_crash.input
$ FUZZ=mini_miner ./src/test/fuzz/fuzz ./mini_miner_crash.input

Now that could just mean that my assumptions about what should be passed into the MiniMiner are wrong (i.e. my fuzz target is using the MiniMiner incorrectly). In that case it might make sense to add documentation that outlines what is expected from a MiniMiner user.

Two questions:

Are the assumptions (i.e. Assumes in mini_miner.cpp) internal to the MiniMiner or do they rely on external assumptions as well? e.g. Does the mini miner expect the mempool it receives in its constructor to only hold transactions that passed out ATMP checks?
Since this is a "mini" version of the BlockAssembler, would it be possible to differentially fuzz the two? Or using one as an oracle to test the other? e.g. Checking if transactions bumped with the help of MiniMiner make it into the next block constructed by the actual BlockAssembler.

sipa · 2023-02-03T16:13:03Z

In what way does the code added here differ from the real block assembly code?

glozow · 2023-02-03T16:14:18Z

Thanks for fuzzing 💯
It's fully possible the crashing is due to real bugs, I hit a crash yesterday while testing as well. I think there is something wrong with the way it's handling to-be-replaced outputs.

Does the mini miner expect the mempool it receives in its constructor to only hold transactions that passed out ATMP checks?

Actually no. It just uses what's cached in the mempool entries. The fees don't even need to match inputs - outputs, and mini miner definitely doesn't require ATMP checks.

Since this is a "mini" version of the BlockAssembler, would it be possible to differentially fuzz the two?

Differential fuzzing is perfect for this really.MiniMiner::BuildMockTemplate(target_feerate) is supposed to do the exact same thing as BlockAssembler::addPackageTxs with blockmintxfee = target feerate.

Checking if transactions bumped with the help of MiniMiner make it into the next block constructed by the actual BlockAssembler.

That would also be a really good way of testing the results (after the wallet stuff is added?). Add the bumping tx, mine another block with the target feerate as min feerate, and see that they all get mined.

glozow · 2023-02-03T16:21:26Z

In what way does the code added here differ from the real block assembly code?

Only operates on the relevant transactions rather than full mempool
Has the ability to remove transactions that will be replaced so they don't impact their ancestors
Does not hold mempool lock outside of the constructor, makes copies of the entries it needs instead (though I'm not sure if this has an effect in practice)
Doesn't do the sanity checks like keeping weight within max block weight and IsFinalTx()
After the block template is built, additionally calculates fees to bump remaining ancestor packages to target feerate

murchandamus · 2023-02-03T21:29:41Z

Added tests from glozow’s branch

glozow · 2023-02-06T10:14:15Z

Is there a reason to leave the CalculateTotalBumpFee commit out of this PR?

murchandamus · 2023-02-06T22:26:39Z

Oh good point, that just grew organically, but really it could be part of the mini-miner changes. I’ll squash it in there.

murchandamus · 2023-02-08T16:58:22Z

I’ve amended this branch to include the CalculateTotalBumpFee method from #26152 in the mini-miner code here.

glozow · 2023-02-16T16:06:44Z

I pushed some changes (with @xekyo's permission, thanks):

Capped traversal at 500 items in CalculateCluster(). Number is arbitrary, open for commentary
Fixed up a few things in the MiniMiner implementation, mostly shuffling things around and updating comments
Dropped the chain interface changes (I think those can go in Bump unconfirmed ancestor transactions to target feerate #26152)
Expanded unit tests
Added a fuzzer (expanded from @dergoegge's, thanks)
Added a fuzzer to differentially test block templates built by BlockAssembler and MiniMiner. Hopefully this gives reviewers a bit more confidence that they are doing the same thing even if the implementations are difficult to review/compare.

murchandamus · 2023-02-16T21:29:52Z

Awesome, thanks for the reworking this, @glozow, and the work on the fuzzer, @dergoegge. I've fixed a minor tidy issue and I’ll pick the chain interface changes into #26152 and rebase on this shortly.

Ready for review

murchandamus · 2023-02-16T22:26:03Z

Pushed again to provide signed commits

glozow · 2023-02-17T16:19:40Z

review-beg-pinging @LarryRuane @josibake @stickies-v who have looked at MiniMiner previously

LarryRuane · 2023-02-17T17:53:33Z

For anyone wanting to review this PR and would like some help with basic mempool concepts, I made a video: https://youtu.be/sQ05azzTp9o -- it mentions 26152 but I think would be helpful for reviewers here as well.

achow101 · 2023-02-23T21:25:18Z

ACK ce882ef

Follow-up from bitcoin#27021

Follow-up from bitcoin#27021: accessing of fields in MiniMinerMempoolEntry was done inconsistently. Even though we had a getter, we would directly write to the fields when we needed to update them. This commits sets the fields to private and introduces a method for updating the ancestor information in transactions using the same method name as used for Mempool Entries.