refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options

luke-jr commented at 2:21 pm on June 5, 2024: member

refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options d4c3c2e9ae

DrahtBot commented at 2:21 pm on June 5, 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. 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.

DrahtBot added the label Refactoring on Jun 5, 2024

luke-jr commented at 2:22 pm on June 5, 2024: member

@kristapsk @jonatack @achow101

fanquake commented at 3:16 pm on June 5, 2024: member

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

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

DrahtBot added the label CI failed on Jun 5, 2024

DrahtBot commented at 4:27 pm on June 5, 2024: contributor

🚧 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

luke-jr commented at 5:06 pm on June 5, 2024: member

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?

glozow commented at 12:38 pm on June 6, 2024: member

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

luke-jr commented at 6:33 pm on June 12, 2024: member

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

maflcko commented at 8:18 pm on July 2, 2024: member

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/:

 0./kernel/mempool_limits.h:20:28: error: use of undeclared identifier 'DEFAULT_ANCESTOR_LIMIT'
 1   20 |     int64_t ancestor_count{DEFAULT_ANCESTOR_LIMIT};
 2      |                            ^
 3./kernel/mempool_limits.h:22:34: error: use of undeclared identifier 'DEFAULT_ANCESTOR_SIZE_LIMIT_KVB'
 4   22 |     int64_t ancestor_size_vbytes{DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1'000};
 5      |                                  ^
 6./kernel/mempool_limits.h:24:30: error: use of undeclared identifier 'DEFAULT_DESCENDANT_LIMIT'
 7   24 |     int64_t descendant_count{DEFAULT_DESCENDANT_LIMIT};
 8      |                              ^
 9./kernel/mempool_limits.h:26:36: error: use of undeclared identifier 'DEFAULT_DESCENDANT_SIZE_LIMIT_KVB'
10   26 |     int64_t descendant_size_vbytes{DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1'000};
11      |                                    ^
12
13
14
15
16./kernel/mempool_options.h:44:40: error: use of undeclared identifier 'DEFAULT_INCREMENTAL_RELAY_FEE'
17   44 |     CFeeRate incremental_relay_feerate{DEFAULT_INCREMENTAL_RELAY_FEE};
18      |                                        ^
19./kernel/mempool_options.h:46:32: error: use of undeclared identifier 'DEFAULT_MIN_RELAY_TX_FEE'
20   46 |     CFeeRate min_relay_feerate{DEFAULT_MIN_RELAY_TX_FEE};
21      |                                ^
22./kernel/mempool_options.h:47:33: error: use of undeclared identifier 'DUST_RELAY_TX_FEE'
23   47 |     CFeeRate dust_relay_feerate{DUST_RELAY_TX_FEE};
24      |                                 ^
25./kernel/mempool_options.h:55:94: error: use of undeclared identifier 'MAX_OP_RETURN_RELAY'
26   55 |     std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
27      |                                                                                              ^
28./kernel/mempool_options.h:55:51: error: use of undeclared identifier 'DEFAULT_ACCEPT_DATACARRIER'
29   55 |     std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
30      |                                                   ^
31./kernel/mempool_options.h:56:31: error: use of undeclared identifier 'DEFAULT_PERMIT_BAREMULTISIG'
32   56 |     bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
33      |                               ^

maflcko commented at 10:11 am on July 4, 2024: member

E.g.:

 0commit 94ed6bf4575abee5200e7fc7054a47d66bebd56c
 1Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
 2Date:   Wed Jul 3 18:05:21 2024 +0200
 3
 4    move-only: Default values in MemPoolLimits
 5
 6diff --git a/src/kernel/mempool_limits.h b/src/kernel/mempool_limits.h
 7index 8d4495c3cb..eeeaedd233 100644
 8--- a/src/kernel/mempool_limits.h
 9+++ b/src/kernel/mempool_limits.h
10@@ -4,9 +4,17 @@
11 #ifndef BITCOIN_KERNEL_MEMPOOL_LIMITS_H
12 #define BITCOIN_KERNEL_MEMPOOL_LIMITS_H
13 
14-#include <policy/policy.h>
15-
16 #include <cstdint>
17+#include <limits>
18+
19+/** Default for -limitancestorcount, max number of in-mempool ancestors */
20+static constexpr unsigned int DEFAULT_ANCESTOR_LIMIT{25};
21+/** Default for -limitancestorsize, maximum kilobytes of tx + all in-mempool ancestors */
22+static constexpr unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT_KVB{101};
23+/** Default for -limitdescendantcount, max number of in-mempool descendants */
24+static constexpr unsigned int DEFAULT_DESCENDANT_LIMIT{25};
25+/** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */
26+static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KVB{101};
27 
28 namespace kernel {
29 /**
30diff --git a/src/policy/packages.h b/src/policy/packages.h
31index 3050320122..ce84d5a9e1 100644
32--- a/src/policy/packages.h
33+++ b/src/policy/packages.h
34@@ -7,6 +7,7 @@
35 
36 #include <consensus/consensus.h>
37 #include <consensus/validation.h>
38+#include <kernel/mempool_limits.h>
39 #include <policy/policy.h>
40 #include <primitives/transaction.h>
41 #include <util/hasher.h>
42diff --git a/src/policy/policy.h b/src/policy/policy.h
43index 9cdb8ab767..a39f8930ea 100644
44--- a/src/policy/policy.h
45+++ b/src/policy/policy.h
46@@ -51,14 +51,6 @@ static constexpr unsigned int MAX_STANDARD_SCRIPTSIG_SIZE{1650};
47 static constexpr unsigned int DUST_RELAY_TX_FEE{3000};
48 /** Default for -minrelaytxfee, minimum relay fee for transactions */
49 static constexpr unsigned int DEFAULT_MIN_RELAY_TX_FEE{1000};
50-/** Default for -limitancestorcount, max number of in-mempool ancestors */
51-static constexpr unsigned int DEFAULT_ANCESTOR_LIMIT{25};
52-/** Default for -limitancestorsize, maximum kilobytes of tx + all in-mempool ancestors */
53-static constexpr unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT_KVB{101};
54-/** Default for -limitdescendantcount, max number of in-mempool descendants */
55-static constexpr unsigned int DEFAULT_DESCENDANT_LIMIT{25};
56-/** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */
57-static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KVB{101};
58 /** Default for -datacarrier */
59 static const bool DEFAULT_ACCEPT_DATACARRIER = true;
60 /**

move-only: Default values in MemPoolLimits 053d5d85ad

luke-jr commented at 4:16 pm on July 6, 2024: member

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

maflcko commented at 6:40 am on July 8, 2024: member

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.

maflcko commented at 10:48 am on September 16, 2024: member

Are you still working on this?

achow101 commented at 4:00 pm on October 15, 2024: member

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.

achow101 closed this on Oct 15, 2024

refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options #30232

Code Coverage

Reviews

Conflicts