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.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #30239 (Ephemeral Dust by instagibbs)
• #29942 (Remove redundant -datacarrier option by vostrnad)
• #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.

@kristapsk @jonatack @achow101

https://github.com/bitcoin/bitcoin/pull/30232/checks?check_run_id=25845075037:

A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.

🚧 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/25845075037}

A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.

How do you propose resolving this? It's not really a circular dependency, just equivocated as such due to the CI test stripping file extensions, but maybe there's a better approach that just fixing CI?

Mind providing some motivation for the refactor? PR description is empty

Besides making the code cleaner, I'm hoping to get to a point where it's practical to fix the remaining vsize bugs.

A new circular dependency in the form of "kernel/mempool_options -> policy/policy -> kernel/mempool_options" appears to have been introduced.

The policy header include should probably be removed from mempool_options. Any policy settings defaults that can be modified should be included in the corresponding header file itself. (This is also true for the other default values in the kernel/*_opts.h files. That is, the following symbols should be moved to the header in ./kernel/:

./kernel/mempool_limits.h:20:28: error: use of undeclared identifier 'DEFAULT_ANCESTOR_LIMIT'
   20 |     int64_t ancestor_count{DEFAULT_ANCESTOR_LIMIT};
      |                            ^
./kernel/mempool_limits.h:22:34: error: use of undeclared identifier 'DEFAULT_ANCESTOR_SIZE_LIMIT_KVB'
   22 |     int64_t ancestor_size_vbytes{DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1'000};
      |                                  ^
./kernel/mempool_limits.h:24:30: error: use of undeclared identifier 'DEFAULT_DESCENDANT_LIMIT'
   24 |     int64_t descendant_count{DEFAULT_DESCENDANT_LIMIT};
      |                              ^
./kernel/mempool_limits.h:26:36: error: use of undeclared identifier 'DEFAULT_DESCENDANT_SIZE_LIMIT_KVB'
   26 |     int64_t descendant_size_vbytes{DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1'000};
      |                                    ^




./kernel/mempool_options.h:44:40: error: use of undeclared identifier 'DEFAULT_INCREMENTAL_RELAY_FEE'
   44 |     CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE};
      |                                        ^
./kernel/mempool_options.h:46:32: error: use of undeclared identifier 'DEFAULT_MIN_RELAY_TX_FEE'
   46 |     CFeeRate min_relay_feerate{DEFAULT_MIN_RELAY_TX_FEE};
      |                                ^
./kernel/mempool_options.h:47:33: error: use of undeclared identifier 'DUST_RELAY_TX_FEE'
   47 |     CFeeRate dust_relay_feerate{DUST_RELAY_TX_FEE};
      |                                 ^
./kernel/mempool_options.h:55:94: error: use of undeclared identifier 'MAX_OP_RETURN_RELAY'
   55 |     std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
      |                                                                                              ^
./kernel/mempool_options.h:55:51: error: use of undeclared identifier 'DEFAULT_ACCEPT_DATACARRIER'
   55 |     std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
      |                                                   ^
./kernel/mempool_options.h:56:31: error: use of undeclared identifier 'DEFAULT_PERMIT_BAREMULTISIG'
   56 |     bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
      |                               ^

E.g.:

commit 94ed6bf4575abee5200e7fc7054a47d66bebd56c
Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Date:   Wed Jul 3 18:05:21 2024 +0200

    move-only: Default values in MemPoolLimits

diff --git a/src/kernel/mempool_limits.h b/src/kernel/mempool_limits.h
index 8d4495c3cb..eeeaedd233 100644
--- a/src/kernel/mempool_limits.h
+++ b/src/kernel/mempool_limits.h
@@ -4,9 +4,17 @@
 #ifndef BITCOIN_KERNEL_MEMPOOL_LIMITS_H
 #define BITCOIN_KERNEL_MEMPOOL_LIMITS_H
 
-#include <policy/policy.h>
-
 #include <cstdint>
+#include <limits>
+
+/** Default for -limitancestorcount, max number of in-mempool ancestors */
+static constexpr unsigned int DEFAULT_ANCESTOR_LIMIT{25};
+/** Default for -limitancestorsize, maximum kilobytes of tx + all in-mempool ancestors */
+static constexpr unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT_KVB{101};
+/** Default for -limitdescendantcount, max number of in-mempool descendants */
+static constexpr unsigned int DEFAULT_DESCENDANT_LIMIT{25};
+/** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */
+static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KVB{101};
 
 namespace kernel {
 /**
diff --git a/src/policy/packages.h b/src/policy/packages.h
index 3050320122..ce84d5a9e1 100644
--- a/src/policy/packages.h
+++ b/src/policy/packages.h
@@ -7,6 +7,7 @@
 
 #include <consensus/consensus.h>
 #include <consensus/validation.h>
+#include <kernel/mempool_limits.h>
 #include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <util/hasher.h>
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 9cdb8ab767..a39f8930ea 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -51,14 +51,6 @@ static constexpr unsigned int MAX_STANDARD_SCRIPTSIG_SIZE{1650};
 static constexpr unsigned int DUST_RELAY_TX_FEE{3000};
 /** Default for -minrelaytxfee, minimum relay fee for transactions */
 static constexpr unsigned int DEFAULT_MIN_RELAY_TX_FEE{1000};
-/** Default for -limitancestorcount, max number of in-mempool ancestors */
-static constexpr unsigned int DEFAULT_ANCESTOR_LIMIT{25};
-/** Default for -limitancestorsize, maximum kilobytes of tx + all in-mempool ancestors */
-static constexpr unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT_KVB{101};
-/** Default for -limitdescendantcount, max number of in-mempool descendants */
-static constexpr unsigned int DEFAULT_DESCENDANT_LIMIT{25};
-/** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */
-static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KVB{101};
 /** Default for -datacarrier */
 static const bool DEFAULT_ACCEPT_DATACARRIER = true;
 /**

Seems more like kernel shouldn't have policy stuff at all, but that's out of scope for this PR. Added your commit

IIRC the kernel will ship with the mempool, so it'll need to ship with policy. In any case, my commit was just for mempool_limits. The same would have to be done for kernel/mempool_options.

Are you still working on this?

This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs. The motivation is vague/missing.

Closing due to lack of interest.