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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30079.

Reviews

See 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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #33216 (mini miner: enable Linearize return package feerates by ismaelsadeeq)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• In fees.h comment: “all transaction parents was added into the vector first” -> “all transaction parents were added into the vector first” [plural verb agreement]
• In feature_fee_estimation.py log message: “ignore all transaction with in block child” -> “ignore all transactions with an in-block child” [plural noun and hyphenation]

^{drahtbot_id_4_m}

cc @josibake @instagibbs @naumenkogs @darosior @murchandamus since you reviewed the last time

concept ACK

strikes me as the right idea, keep it simple for now, focusing on correctness. Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

Concept ACK

Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

I tracked recent 1000 blocks from block 842457 to 843457

~61% of transactions in the last 1000 blocks were confirmed in a cluster size > 1.
~38% of transactions in the last 1000 blocks were confirmed in a cluster size 1

Transactions: 3974143
Cluster size 1 transactions: 1516505
Approximate percentage of Cluster size 1 transactions: ~38
Transactions in a cluster size > 1 transactions: 2457638
Approximate percentage of transactions in a cluster size > 1: ~61

The data is available here here https://docs.google.com/spreadsheets/d/1QTE2EQeQlamA123pIn0SY4F-9jtnzS7e7CV1etT6ytM/edit?usp=sharing

I used this script to collect the data https://gist.github.com/ismaelsadeeq/e2767040b720f65986976b3f8c6b7b20

Thanks for review @rkrux

• I addressed all your comments
• Force pushed from c12a677 to 2563305
  compare diff

Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

I tracked recent 1000 blocks from block 842457 to 843457

~61% of transactions in the last 1000 blocks were confirmed in a cluster size > 1. ~38% of transactions in the last 1000 blocks were confirmed in a cluster size 1

Transactions: 3974143 Cluster size 1 transactions: 1516505 Approximate percentage of Cluster size 1 transactions: ~38 Transactions in a cluster size > 1 transactions: 2457638 Approximate percentage of transactions in a cluster size > 1: ~61

The data is available here here https://docs.google.com/spreadsheets/d/1QTE2EQeQlamA123pIn0SY4F-9jtnzS7e7CV1etT6ytM/edit?usp=sharing

I used this script to collect the data https://gist.github.com/ismaelsadeeq/e2767040b720f65986976b3f8c6b7b20

This data shows that most transactions are in cluster size > 1.

So I switched to ignoring transaction that are CPFP'd only.

Thanks for your reviews @rkrux @josibake

• Forced pushed from 2563305 to 055d9e6 Compare diff

• Addressed all comments.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/25734218532}

I didn't look too closely into the CI failure reason, but seems like a relatively simple fix, i.e. fixing an signed int overflow:

SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow node/mini_miner.cpp:196:33 
MS: 0 ; base unit: 0000000000000000000000000000000000000000
artifact_prefix='./'; Test unit written to ./crash-2315c89458602360eb93362328da5c0ef21f9864

Traceback (most recent call last):
  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 411, in <module>
    main()
  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 199, in main
    run_once(
  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 376, in run_once
    assert len(done_stat) == 1
           ^^^^^^^^^^^^^^^^^^^
AssertionError

Marking as draft, as I attempt to fix the fuzzing overflow error.

Rebased and fix comment by @yancyribbens

Feel free to ignore me if this PR is close to merge, but it does feel like there are two PRs here. The first 3 commits seem more about updates to miniminer, and then the 4th commit does what the title of this PR indicates.

It may help to attract more review by breaking out the first 3 commits into their own PR, using this PR as a motivating use case. Either way, I plan to dig in more next week and review, but wanted to mention this.

Feel free to ignore me if this PR is close to merge, but it does feel like there are two PRs here. The first 3 commits seem more about updates to miniminer, and then the 4th commit does what the title of this PR indicates.

It may help to attract more review by breaking out the first 3 commits into their own PR, using this PR as a motivating use case. Either way, I plan to dig in more next week and review, but wanted to mention this.

Not close to merge I think, so far this PR received a couple of concept ACK and some initial review last year.

On the contrary the mini miner changes are tiny, but thats the first step, I opened #33216 for that.

The next two commits are preparing the removed mempool transactions for linearization by the mini miner.
by going over the transactions sequentially and appending their ancestors and descendant, the bulk of the diff is from the test in 80cb8d2

The subsequent three commits are the main feature and test.

Hence I think they can be bundled together.

In the latest pushes I moved the helper functions to fees for simplicity and make some improvement to the code.

Putting in draft because of a dependent PR.

Thanks for taking the suggestion of splitting this up! I think it would be nice to add a blurb to the description that this PR depends on #33216 and is in draft pending review on the parent PR. You mention this in the comments, but having it in the description ensures reviewers dont miss it / get confused on the status of the PR.