[kernel 3a/n] Decouple `CTxMemPool` from `ArgsManager`

dongcarl commented at 1:59 am on June 7, 2022: contributor

This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18

As mentioned in the Stage 1 Step 2 description of the libbitcoinkernel project, ArgsManager will not be part of libbitcoinkernel. Therefore, it is important that we remove any dependence on ArgsManager by code that will be part of libbitcoinkernel. This is the first in a series of PRs aiming to achieve this.

This PR removes CTxMemPool+MempoolAccept’s dependency on ArgsManager by introducing a CTxMemPool::Options struct, which is used to specify CTxMemPool’s various options at construction time.

These options are:

-maxmempool -> CTxMemPool::Options::max_size
-mempoolexpiry -> CTxMemPool::Options::expiry
-limitancestorcount -> CTxMemPool::Options::limits::ancestor_count
-limitancestorsize -> CTxMemPool::Options::limits::ancestor_size
-limitdescendantcount -> CTxMemPool::Options::limits::descendant_count
-limitdescendantsize -> CTxMemPool::Options::limits::descendant_size

More context can be gleaned from the commit messages. The important commits are:

56eb479ded8bfb2ef635bb6f3b484f9d5952c70d “pool: Add and use MemPoolOptions, ApplyArgsManOptions”
a1e08b70f3068f4e8def1c630d8f50cd54da7832 “mempool: Pass in -maxmempool instead of referencing gArgs”
6f4bf3ede5812b374828f08fc728ceded2f10024 “mempool: Pass in -mempoolexpiry instead of referencing gArgs”
5958a7fe4806599fc620ee8c1a881ca10fa2dd16 “mempool: Introduce (still-unused) MemPoolLimits”

Reviewers: Help needed in the following commits (see commit messages):

a1e08b70f3068f4e8def1c630d8f50cd54da7832 “mempool: Pass in -maxmempool instead of referencing gArgs”
0695081a797e9a5d7787b78b0f8289dafcc6bff7 “node/ifaces: Use existing MemPoolLimits”

Note to Reviewers: There are perhaps an infinite number of ways to architect CTxMemPool::Options, the current one tries to keep it simple, usable, and flexible. I hope we don’t spend too much time arguing over the design here since that’s not the point. In the case that you’re 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.

TODO:

Use the more ergonomic CTxMemPool::Options where appropriate
Doxygen comments for ApplyArgsManOptions, MemPoolOptions

Questions for Reviewers:

Should we use std::chrono::seconds for CTxMemPool::Options::expiry and CTxMemPool::m_expiry instead of an int64_t? Something else? (std::chrono::hours?)
Should I merge CTxMemPool::Limits inside CTxMemPool::Options?

dongcarl force-pushed on Jun 7, 2022

DrahtBot added the label Refactoring on Jun 7, 2022

DrahtBot commented at 8:32 am on June 7, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25386 (refactor: Extract MIB_BYTES constant for init.cpp by Empact)
#25308 (refactor: Reduce number of LoadChainstate parameters and return values by ryanofsky)
#25285 (Add AutoFile without ser-type and ser-version and use it where possible by MarcoFalke)
#25193 (indexes: Read the locator’s top block during init, allow interaction with reindex-chainstate by mzumsande)
#24513 (CChainState -> Chainstate by jamesob)
#24158 (Optimize Mempool Reorg logic using Epochs, improving memory usage and runtime. by JeremyRubin)
#23443 (p2p: Erlay support signaling by naumenkogs)
#13990 (Allow fee estimation to work with lower fees by ajtowns)

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 Needs rebase on Jun 7, 2022

dongcarl force-pushed on Jun 7, 2022

DrahtBot removed the label Needs rebase on Jun 7, 2022

dongcarl force-pushed on Jun 13, 2022

dongcarl commented at 5:53 pm on June 13, 2022: contributor

Pushed 532f83872d3921030fc4240fa8a8c4f6a1aaacf0 -> 4bbc2e0cfc2e395649a20e616648937c81dca377

Added doxygen comments for MemPoolOptions, MemPoolLimits, ApplyArgsManOptions
Use CTxMemPool::{Options,Limits} instead of MemPool{Options,Limits} wherever possible
Move ApplyArgsManOptions(const ArgsManager&, MemPoolLimits&) to an anonymous namespace in mempool_args.cpp

dongcarl force-pushed on Jun 13, 2022

dongcarl force-pushed on Jun 16, 2022

dongcarl commented at 6:21 pm on June 16, 2022: contributor

Pushed f803134e4cf1f2566858e0c597fcd19e29afed3f -> c98d5f1744dc3beed116ff708e3d4d8dcfe53eae

Rebased after merge of #25215 and #25223 (thanks all reviewers!)
Marking as Ready for Review as all dependent PRs have been merged

dongcarl marked this as ready for review on Jun 16, 2022

fanquake commented at 9:50 am on June 17, 2022: member

https://cirrus-ci.com/task/6709025164230656:

 0Traceback (most recent call last):
 1                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 552, in assert_start_raises_init_error
 2                                       ret = self.process.wait(timeout=self.rpc_timeout)
 3                                     File "/usr/lib/python3.6/subprocess.py", line 1469, in wait
 4                                       raise TimeoutExpired(self.args, timeout)
 5                                   subprocess.TimeoutExpired: Command '['/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind', '-datadir=/tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_\u20bf_🏃_20220616_191835/mempool_limit_214/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-uacomment=testnode0', '-logthreadnames', '-logsourcelocations', '-maxmempool=4']' timed out after 2400 seconds
 6                                   During handling of the above exception, another exception occurred:
 7                                   Traceback (most recent call last):
 8                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
 9                                       self.run_test()
10                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_limit.py", line 82, in run_test
11                                       self.nodes[0].assert_start_raises_init_error(["-maxmempool=4"], "Error: -maxmempool must be at least 5 MB")
12                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 582, in assert_start_raises_init_error
13                                       self._raise_assertion_error(assert_msg)
14                                     File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 167, in _raise_assertion_error
15                                       raise AssertionError(self._node_msg(msg))
16                                   AssertionError: [node 0] bitcoind should have exited within 2400s with expected error Error: -maxmempool must be at least 5 MB

in src/init.cpp:542 in 85aa9d85a6 outdated

538@@ -539,9 +539,9 @@ void SetupServerArgs(ArgsManager& argsman)
539     argsman.AddArg("-stopafterblockimport", strprintf("Stop running after importing blocks from disk (default: %u)", DEFAULT_STOPAFTERBLOCKIMPORT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
540     argsman.AddArg("-stopatheight", strprintf("Stop running after reaching the given height in the main chain (default: %u)", DEFAULT_STOPATHEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
541     argsman.AddArg("-limitancestorcount=<n>", strprintf("Do not accept transactions if number of in-mempool ancestors is <n> or more (default: %u)", DEFAULT_ANCESTOR_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
542-    argsman.AddArg("-limitancestorsize=<n>", strprintf("Do not accept transactions whose size with all in-mempool ancestors exceeds <n> kilobytes (default: %u)", DEFAULT_ANCESTOR_SIZE_LIMIT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
543+    argsman.AddArg("-limitancestorsize=<n>", strprintf("Do not accept transactions whose size with all in-mempool ancestors exceeds <n> kilobytes (default: %u)", DEFAULT_ANCESTOR_SIZE_LIMIT_KB), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);

glozow commented at 11:24 am on June 17, 2022:

in 85aa9d85a6903f9f85e5f609175206aed0b09410 “scripted-diff: Rename anc/desc size limit vars to indicate SI unit "

This is possibly out of scope/nitty but since you’re adding the KB unit, I think it’s worth noting that it’s kilo virtual bytes, not kilobytes. So perhaps DEFAULT_ANCESTOR_SIZE_LIMIT_KVB is more accurate

(see https://github.com/bitcoin/bitcoin/blob/874529665c1c326fc86fc0d0d6c3512fab087ab8/src/txmempool.h#L98, https://github.com/bitcoin/bitcoin/blob/874529665c1c326fc86fc0d0d6c3512fab087ab8/src/txmempool.cpp#L232-L243)

dongcarl commented at 1:18 pm on June 21, 2022:

Fixed in 67ad3f84b1e69ad830ee27b25e0b6a33519e1d0b

in src/node/interfaces.cpp:658 in 95430b611c outdated

653@@ -653,24 +654,25 @@ class ChainImpl : public Chain
654     }
655     void getPackageLimits(unsigned int& limit_ancestor_count, unsigned int& limit_descendant_count) override
656     {
657-        limit_ancestor_count = gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
658-        limit_descendant_count = gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT);
659+        limit_ancestor_count = limit_descendant_count = 0;
660+        if (!m_node.mempool) return;

glozow commented at 12:09 pm on June 17, 2022:

in 95430b611c6562845143669e88e07fcde770d1af “node/ifaces: Use existing MemPoolLimits”

I see your “Question for Reviewers: Is the default for limit_ancestor_count and limit_descendant_count in getPackageLimits correct?” in the commit message. To clarify, are you asking whether this has a behavior change, or are you asking if the change is better?

IIUC This returns limits = 0 when there is no mempool, where it would have previously returned the results of querying gArgs / defaults. This makes logical sense to me: if there’s no mempool, then there’s no mempool limits. It seems the one use for getPackageLimits is in SelectCoins, and the wallet understands it can still make an unconfirmed transaction (i.e. anc/desc count = 1) so it’s fine: https://github.com/bitcoin/bitcoin/blob/874529665c1c326fc86fc0d0d6c3512fab087ab8/src/wallet/spend.cpp#L509-L511

I guess a future -nomempool option would just need to be consistent with this.

ryanofsky commented at 6:10 pm on June 17, 2022:

In commit “node/ifaces: Use existing MemPoolLimits” (95430b611c6562845143669e88e07fcde770d1af)

I don’t think think it is a good idea to return 0 for these. Seems like that would mess up wallet coin selection when the mempool is disabled for no benefit. Seems better to just use mempool values when mempool is present and use default values when mempool is not present like

0limit_ancestor_count = DEFAULT_ANCESTOR_LIMIT;
1limit_descendant_count = DEFAULT_DESCENDANT_LIMIT;
2if (!m_node.mempool) return;

so the wallet is not doing something crazy when there is no mempool

ariard commented at 6:43 pm on June 20, 2022:

In commit 95430b61, “node/ifaces: Use existing MemPoolLimits”

I think the wallet relying on getPackageLimits is to blame here, as eventually the wallet should be able to evaluate the propagation odds of its issued transactions even in a future -nomempool setting. I think it’s a rational for a node operator to run without mempool on let’s say a low-performance host, however this operator still eager to verify if the wallet transactions are compliant with the most-widely deployed policies on the network. So a future direction could be to rename MemPoolLimits as something like TransactionPolicy and ship potentially different instances to the wallet and mempool component.

I think for now, I’ll go with Russ suggestion as otherwise it would be a behavior change. I don’t think it would fail AttemptSelection() as the latest call in SelectCoins() ignore ancestors/descendants limits though it might severely delay the selection or alter the set of yielded UTXOs in a detrimental way.

dongcarl commented at 1:18 pm on June 21, 2022:

Fixed in 4825207fc6c15fce4cc1cb9b14a66a51f3b5a47b, feel free to reopen if still a problem!

in src/init.cpp:936 in efcbd12767 outdated

931@@ -932,10 +932,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
932     }
933 
934     // mempool limits
935-    int64_t nMempoolSizeMax = args.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000;
936-    int64_t nMempoolSizeMin = args.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KB) * 1000 * 40;

glozow commented at 12:13 pm on June 17, 2022:

In efcbd12767d87b33b6c6deb86d441292ff681b13 “init: Remove redundant -mempool, -limit* queries "

Seems like you’ve dropped a * 40 here? Is that maybe what’s causing the CI failure?

ryanofsky commented at 6:28 pm on June 17, 2022:

In commit “init: Remove redundant -mempool, -limit* queries” (efcbd12767d87b33b6c6deb86d441292ff681b13)

Nice catch. Using explicit units would also have helped here (max_size_bytes < descendant_size_vbytes is more obviously wrong than max_size < descendant_size)

dongcarl commented at 1:38 pm on June 21, 2022:

Fixed in 4a50089d8c3421f2e37869a2344cd7143a2c393e 😅

glozow commented at 12:42 pm on June 17, 2022: member

Concept ACK

Re: “Should I merge CTxMemPool::Limits inside CTxMemPool::Options?”

(TLDR imo keeping them separate is fine). Would it be accurate to think of Limits as policy (used externally in MemPoolAccept) while Options is related to managing the data structure of CTxMemPool itself (i.e. it knows its own check ratio and max size)? Imo separating the ancestor/descendant limits makes sense because they’re not just internal to CTxMemPool; they’re also used by MemPoolAccept to implement carveouts. For example, it will override them by adding +1 to the descendant limit and setting ancestor limit = 2 for CPFP carve out: https://github.com/bitcoin/bitcoin/blob/874529665c1c326fc86fc0d0d6c3512fab087ab8/src/validation.cpp#L906-L908

in src/init.cpp:1431 in c318c982f6 outdated

1434+    CTxMemPool::Options mempool_opts{
1435+        Desig(estimator) node.fee_estimator.get(),
1436+        Desig(check_ratio) chainparams.DefaultConsistencyChecks() ? 1 : 0,
1437+    };
1438+    ApplyArgsManOptions(args, mempool_opts);
1439+    mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1000000);

ryanofsky commented at 3:43 pm on June 17, 2022:

In commit “pool: Add and use MemPoolOptions, ApplyArgsManOptions” (c318c982f62610d1c937ab803346307d5458f877)

I think the clamping and the “mempool must be at least %d MB” error checking that is moved here later should be part of mempool code, not init code, so initializing the mempool is straightforward, consistent across bitcoind code and test code and future kernel code, and the same error checking and conveniences are provided in every case.

Easiest way to do this would be to move the std::clamp call to the CTxMemPool::CTxMemPool constructor and add a std::optional<string>& error output parameter to the constructor so it can provide feedback about invalid options instead of causing problems later.

dongcarl commented at 7:09 pm on June 27, 2022:

I think this is a fine idea but out of the scope of this PR, adding to project-wide TODOs!

in src/test/fuzz/tx_pool.cpp:129 in c318c982f6 outdated

122@@ -121,6 +123,14 @@ void MockTime(FuzzedDataProvider& fuzzed_data_provider, const CChainState& chain
123     SetMockTime(time);
124 }
125 
126+static inline CTxMemPool MakeMempool(const ArgsManager& argsman)
127+{
128+    CTxMemPool::Options mempool_opts{};
129+    ApplyArgsManOptions(argsman, mempool_opts);

ryanofsky commented at 4:52 pm on June 17, 2022:

In commit “pool: Add and use MemPoolOptions, ApplyArgsManOptions” (c318c982f62610d1c937ab803346307d5458f877)

Applying command line options to tx_pool fuzz test appears to be a change in behavior that is only obliquely referenced in the commit message. Would be good to say up front how exactly behavior is supposed to be changing in the commit up front.

maflcko commented at 9:56 am on June 22, 2022:

7e6d2d72368631b03d00c8fc7689a670ba92e4ac:

Applying command line options to tx_pool fuzz test appears to be a change in behavior that is only obliquely referenced in the commit message. Would be good to say up front how exactly behavior is supposed to be changing in the commit up front.

I don’t think this changes behaviour, as the argsman doesn’t have any stuff set. (A config setting is only forced in ChainTestingSetup).

I think the goal to add the no-op argsman stuff here is that if settings are supplied in the future, they are not ignored.

ryanofsky commented at 0:48 am on June 23, 2022:

I don’t think this changes behaviour, as the argsman doesn’t have any stuff set. (A config setting is only forced in ChainTestingSetup).

You’re right. I was misinterpreting a comment about the other fuzz test in the commit description, thinking it applied to this test. I thought that options passed to the fuzzer would be ignored previously and now applied. But now I notice SetMempoolConstraints setting argsman options above, so I think this is exactly preserving existing behavior.

Still would be nice if commit said explicitly if it’s not intended to change behavior or is intended to change something. Easier to check if code matches intent than to work backwards and figure out what intent is supposed to be.

dongcarl commented at 7:04 pm on June 27, 2022:

Fixed as of c7dbe3ae368096f1b3111e0ef7e08a7a5b4ee2a7

in src/mempool_args.cpp:15 in 5b733c301d outdated

 9@@ -10,4 +10,7 @@
10 void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opts)
11 {
12     mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);
13+
14+    auto mempool_max_size_mb = argsman.GetIntArg("-maxmempool");
15+    if (mempool_max_size_mb) mempool_opts.max_size = *mempool_max_size_mb * 1000000;

ryanofsky commented at 5:17 pm on June 17, 2022:

In commit “mempool: Pass in -maxmempool instead of referencing gArgs” (5b733c301debc3119872d436c555f1bf67deb00d)

I think long local variable names like mempool_max_size_mb that differ from each other only a few characters make code frustrating to read and make it easy for mistakes to slip through. Would suggest less verbose code and short visually distinct names like:

0if (auto mb = argsman.GetIntArg("-maxmempool")) mempool_opts.max_size = mb * 1000000;

instead of

0auto mempool_max_size_mb = argsman.GetIntArg("-maxmempool");
1if (mempool_max_size_mb) mempool_opts.max_size = *mempool_max_size_mb * 1000000;

dongcarl commented at 6:23 pm on June 21, 2022:

Fixed in 8c81736608e05607d0d84a6edb7ef6b66e46857c, 846a47db00c76b76115fb882aa9e032d234b0a5e, and 6843403a15cab34bc4e1e7acf7a0e509a2d437df

in src/validation.cpp:2287 in 5b733c301d outdated

2295@@ -2298,7 +2296,7 @@ CoinsCacheSizeState CChainState::GetCoinsCacheSizeState()
2296     AssertLockHeld(::cs_main);
2297     return this->GetCoinsCacheSizeState(
2298         m_coinstip_cache_size_bytes,
2299-        gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000);
2300+        m_mempool ? m_mempool->m_max_size : 0);

ryanofsky commented at 5:31 pm on June 17, 2022:

In commit “mempool: Pass in -maxmempool instead of referencing gArgs” (5b733c301debc3119872d436c555f1bf67deb00d)

Would be good if commit message said first thing how behavior is and isn’t supposed to be changing. There’s a reviewer hint to look at this but it’s vague.

dongcarl commented at 3:35 pm on June 24, 2022:

It was a bit hard for me to discern the original intent here, so this is definitely something we want to look into. From what I can tell, we’re allowing the CoinsView to use some of mempool’s memory quota when the mempool’s memory usage is low? Perhaps @jamesob can shed some light on this logic and help determine what the best solution is here.

jamesob commented at 2:13 pm on June 27, 2022:

I think as-written, this is the right behavior if no mempool exists. The argument passed here (see https://github.com/bitcoin/bitcoin/pull/16945/commits/b17e91d842724d2888a179a73585cc4c2ef1dc21#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2203-R2204 for more context) ultimately just accounts for the fact that we repurpose “extra” mempool headroom for the coinscache (see https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L1425).

dongcarl commented at 7:06 pm on June 27, 2022:

Thanks for the help James! 😄

dongcarl commented at 8:30 pm on June 27, 2022:

Added some context in commit message of 710b30a02b

in src/mempool_options.h:15 in 98be1b8412 outdated

10 class CBlockPolicyEstimator;
11 
12 /** Default for -maxmempool, maximum megabytes of mempool memory usage */
13 static const unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB = 300;
14+/** Default for -mempoolexpiry, expiration time for mempool transactions in hours */
15+static const unsigned int DEFAULT_MEMPOOL_EXPIRY = 336;

ryanofsky commented at 5:35 pm on June 17, 2022:

In commit “mempool: Pass in -mempoolexpiry instead of referencing gArgs” (98be1b84126e3a40f0a45fcd289b1073bdb10361)

Seems fine, but is there intentionally not a scripted diff to change this to DEFAULT_MEMPOOL_EXPIRY_HOURS similar to the other ones?

dongcarl commented at 6:24 pm on June 21, 2022:

Fixed in 5fae7725822771b76825f1e0452d8583141e54e3

in src/mempool_options.h:24 in 5b733c301d outdated

20@@ -16,6 +21,7 @@ class CBlockPolicyEstimator;
21 struct MemPoolOptions {
22     CBlockPolicyEstimator* const estimator = nullptr;
23     int check_ratio = 0;
24+    size_t max_size = DEFAULT_MAX_MEMPOOL_SIZE_MB * 1000000;

ryanofsky commented at 5:38 pm on June 17, 2022:

In commit “mempool: Pass in -maxmempool instead of referencing gArgs” (5b733c301debc3119872d436c555f1bf67deb00d)

For this new member and the new mempool member max_size_bytes would seem like a nicer name than max_size for making code understandable and preventing bugs, especially as mb and kb and vbytes are used elsewhere.

dongcarl commented at 6:24 pm on June 21, 2022:

Fixed in 8c81736608e05607d0d84a6edb7ef6b66e46857c

in src/mempool_options.h:32 in 98be1b8412 outdated

24@@ -22,6 +25,7 @@ struct MemPoolOptions {
25     CBlockPolicyEstimator* const estimator = nullptr;
26     int check_ratio = 0;
27     size_t max_size = DEFAULT_MAX_MEMPOOL_SIZE_MB * 1000000;
28+    int64_t expiry = DEFAULT_MEMPOOL_EXPIRY;

ryanofsky commented at 5:43 pm on June 17, 2022:

In commit “mempool: Pass in -mempoolexpiry instead of referencing gArgs” (98be1b84126e3a40f0a45fcd289b1073bdb10361)

To clarify this option and prevent bugs it would seem better to use std::chrono::hours type instead of int64_t type for this or call the variable expiry_hours.

dongcarl commented at 2:28 pm on June 21, 2022:

How about std::chrono::seconds? It seems that’s how it’s used/compared in our code anyway

ryanofsky commented at 3:00 pm on June 21, 2022:

No opinion on what unit should be used. Just saying using some explicit time unit in the variable name or type could make code more readable and prevent bugs.

maflcko commented at 3:05 pm on June 21, 2022:

It could be NodeClock::duration with the reason that the value is compared against the value of GetTime(), which is deprecated in favour of (and an alias to) NodeClock::now().

dongcarl commented at 6:26 pm on June 21, 2022:

I used std::chrono::seconds in 846a47db00c76b76115fb882aa9e032d234b0a5e. I think for the mempool at least we want to use something that isn’t platform-specific. Feel free to reopen if still a problem!

in src/mempool_limits.h:26 in 2fdb45f171 outdated

21+ *
22+ * Most of the time, this struct should be referenced as CTxMemPool::Limits.
23+ */
24+struct MemPoolLimits {
25+    size_t ancestor_count = DEFAULT_ANCESTOR_LIMIT;
26+    size_t ancestor_size = DEFAULT_ANCESTOR_SIZE_LIMIT_KB * 1000;

ryanofsky commented at 5:56 pm on June 17, 2022:

In commit “mempool: Introduce (still-unused) MemPoolLimits” (2fdb45f17188b3983c9f60e73342b4d1571cb629)

Would be good to add units to these names like ancestor_size_vbytes descendant_size_vbytes to distinguish from other size units used in mempool code and init code

dongcarl commented at 1:40 pm on June 21, 2022:

Fixed in 291b605e21b5f4eb739cd20418ca1fc6f467fcab

in src/node/interfaces.cpp:672 in 95430b611c outdated

674-        auto limit_descendant_count = gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT);
675-        auto limit_descendant_size = gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KB) * 1000;
676+        const CTxMemPool::Limits& limits{m_node.mempool->m_limits};
677         std::string unused_error_string;
678         LOCK(m_node.mempool->cs);
679         return m_node.mempool->CalculateMemPoolAncestors(

ryanofsky commented at 6:17 pm on June 17, 2022:

In commit “node/ifaces: Use existing MemPoolLimits” (95430b611c6562845143669e88e07fcde770d1af)

Would be nice to pass MemPoolLimits struct instead of these separate parameters

dongcarl commented at 6:28 pm on June 21, 2022:

Do you mean something like below? This convenience method would really only benefit this one call site due to the limitations I mentioned here: #25290 (review)

 0diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
 1index cd2ad1f6a2..9977968775 100644
 2--- a/src/node/interfaces.cpp
 3+++ b/src/node/interfaces.cpp
 4@@ -674,8 +674,7 @@ public:
 5         std::string unused_error_string;
 6         LOCK(m_node.mempool->cs);
 7         return m_node.mempool->CalculateMemPoolAncestors(
 8-            entry, ancestors, limits.ancestor_count, limits.ancestor_size_vbytes,
 9-            limits.descendant_count, limits.descendant_size_vbytes, unused_error_string);
10+            entry, ancestors, limits, unused_error_string);
11     }
12     CFeeRate estimateSmartFee(int num_blocks, bool conservative, FeeCalculation* calc) override
13     {
14diff --git a/src/txmempool.cpp b/src/txmempool.cpp
15index baf5718a8d..878e828b65 100644
16--- a/src/txmempool.cpp
17+++ b/src/txmempool.cpp
18@@ -295,6 +295,20 @@ bool CTxMemPool::CheckPackageLimits(const Package& package,
19     return ret;
20 }
21 
22+bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry& entry,
23+                                           setEntries& setAncestors,
24+                                           const CTxMemPool::Limits& limits,
25+                                           std::string& errString,
26+                                           bool fSearchForParents /* = true */) const
27+{
28+    return CalculateMemPoolAncestors(entry, setAncestors,
29+                                     limits.ancestor_count,
30+                                     limits.ancestor_size_vbytes,
31+                                     limits.descendant_count,
32+                                     limits.descendant_size_vbytes,
33+                                     errString, fSearchForParents);
34+}
35+
36 bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry,
37                                            setEntries &setAncestors,
38                                            uint64_t limitAncestorCount,
39diff --git a/src/txmempool.h b/src/txmempool.h
40index 76f51e9d1e..a71c80e2b8 100644
41--- a/src/txmempool.h
42+++ b/src/txmempool.h
43@@ -669,7 +669,19 @@ public:
44      *  fSearchForParents = whether to search a tx's vin for in-mempool parents, or
45      *    look up parents from mapLinks. Must be true for entries not in the mempool
46      */
47-    bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry, setEntries& setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string& errString, bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
48+    bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry,
49+                                   setEntries& setAncestors,
50+                                   const CTxMemPool::Limits& limits,
51+                                   std::string& errString,
52+                                   bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
53+    bool CalculateMemPoolAncestors(const CTxMemPoolEntry& entry,
54+                                   setEntries& setAncestors,
55+                                   uint64_t limitAncestorCount,
56+                                   uint64_t limitAncestorSize,
57+                                   uint64_t limitDescendantCount,
58+                                   uint64_t limitDescendantSize,
59+                                   std::string& errString,
60+                                   bool fSearchForParents = true) const EXCLUSIVE_LOCKS_REQUIRED(cs);
61 
62     /** Calculate all in-mempool ancestors of a set of transactions not already in the mempool and
63      * check ancestor and descendant limits. Heuristics are used to estimate the ancestor and

ryanofsky commented at 4:56 pm on June 23, 2022:

re: #25290 (review)

Do you mean something like below?

Not really, I’m suggesting existing CalculateMemPoolAncestors method could to be changed to take fewer parameters, not that a wrapper could be added around it taking fewer parameters. Even if callers don’t already have the struct they could pass a struct literal and parameter list would be better organized. Not very important though.

dongcarl commented at 7:52 pm on June 27, 2022:

Lots of call sites so not going to do in this PR, added to project-wide TODOs!

fanquake commented at 2:55 pm on January 17, 2023:

The move to using a Limits struct for CalculateMemPoolAncestors was done in #26103.

in src/validation.cpp:437 in 084fd3d78e outdated

436-        m_limit_descendants(gArgs.GetIntArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT)),
437-        m_limit_descendant_size(gArgs.GetIntArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT_KB)*1000) {
438+        m_limit_ancestors(m_pool.m_limits.ancestor_count),
439+        m_limit_ancestor_size(m_pool.m_limits.ancestor_size),
440+        m_limit_descendants(m_pool.m_limits.descendant_count),
441+        m_limit_descendant_size(m_pool.m_limits.descendant_size) {

ryanofsky commented at 6:18 pm on June 17, 2022:

In commit “mempoolaccept: Use limits from mempool in constructor” (084fd3d78e272dc1d75e3e17a046b6dfa798da7d)

Would be nice to use MemPoolLimits struct instead of having these separate duplicate members.

dongcarl commented at 4:40 pm on June 21, 2022:

Unfortunately MemPoolAccept needs to hold m_limit_descendant{s,_size} as non-const since they change:

https://github.com/bitcoin/bitcoin/blob/34869114a72435b9d8364385abaefed70d703fa8/src/validation.cpp#L880-L881

If we just had a MemPoolLimits struct that would enforce const-uniformity across its members and I think it would still be good to keep m_limit_ancestor{s,_size} as const.

in src/init.cpp:1434 in efcbd12767 outdated

1429@@ -1434,6 +1430,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1430     };
1431     ApplyArgsManOptions(args, mempool_opts);
1432     mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1000000);
1433+    if (mempool_opts.max_size < 0 || mempool_opts.max_size < mempool_opts.limits.descendant_size) {
1434+        return InitError(strprintf(_("-maxmempool must be at least %d MB"), std::ceil(mempool_opts.limits.descendant_size / 1000000.0)));

ryanofsky commented at 6:35 pm on June 17, 2022:

In commit “init: Remove redundant -mempool, -limit* queries” (efcbd12767d87b33b6c6deb86d441292ff681b13)

As mentioned previously, I think it would be good to have mempool actually do it’s own error checking instead of putting this responsibility on init code, and think this could be easily accomplished giving the constructor a std::optional<std::string>& error output parameter.

dongcarl commented at 7:10 pm on June 27, 2022:

See: #25290 (review)

in src/init.cpp:1296 in c98d5f1744 outdated

1292@@ -1293,7 +1293,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1293     assert(!node.fee_estimator);
1294     // Don't initialize fee estimation with old data if we don't relay transactions,
1295     // as they would never get updated.
1296-    if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>();
1297+    if (!ignores_incoming_txs) node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME);

ryanofsky commented at 6:38 pm on June 17, 2022:

In commit “fees: Pass in a filepath instead of referencing gArgs” (c98d5f1744dc3beed116ff708e3d4d8dcfe53eae)

Prefer args instead of gArgs here for consistency with surrounding code

dongcarl commented at 1:41 pm on June 21, 2022:

Fixed in 125c198efe1b2157a669758e7dadd1d15fbec368

in src/test/util/setup_common.cpp:166 in c98d5f1744 outdated

162@@ -163,7 +163,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
163     m_node.scheduler->m_service_thread = std::thread(util::TraceThread, "scheduler", [&] { m_node.scheduler->serviceQueue(); });
164     GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler);
165 
166-    m_node.fee_estimator = std::make_unique<CBlockPolicyEstimator>();
167+    m_node.fee_estimator = std::make_unique<CBlockPolicyEstimator>(gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME);

ryanofsky commented at 6:41 pm on June 17, 2022:

In commit “fees: Pass in a filepath instead of referencing gArgs” (c98d5f1744dc3beed116ff708e3d4d8dcfe53eae)

Prefer *m_node.args instead of gArgs here for consistency with surrounding code

dongcarl commented at 1:41 pm on June 21, 2022:

Fixed in 125c198efe1b2157a669758e7dadd1d15fbec368

in src/test/fuzz/policy_estimator_io.cpp:25 in c98d5f1744 outdated

21@@ -22,7 +22,7 @@ FUZZ_TARGET_INIT(policy_estimator_io, initialize_policy_estimator_io)
22     FuzzedAutoFileProvider fuzzed_auto_file_provider = ConsumeAutoFile(fuzzed_data_provider);
23     CAutoFile fuzzed_auto_file = fuzzed_auto_file_provider.open();
24     // Re-using block_policy_estimator across runs to avoid costly creation of CBlockPolicyEstimator object.
25-    static CBlockPolicyEstimator block_policy_estimator;
26+    static CBlockPolicyEstimator block_policy_estimator{gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME};

ryanofsky commented at 6:57 pm on June 17, 2022:

In commit “fees: Pass in a filepath instead of referencing gArgs” (c98d5f1744dc3beed116ff708e3d4d8dcfe53eae)

I guess gArgs is used in some fuzz tests, but it could break if or when BasicTestingSetup ever switches to using local args instead of global args. This is alright in its current form but could consider following const TestingSetup* g_setup; g_setup = testing_setup.get() g_setup->m_node.args->... pattern of other fuzz tests

dongcarl commented at 7:07 pm on June 27, 2022:

Fixed as of 4846995a06ee1886693f096f17cce6005b3cf796

ryanofsky approved

ryanofsky commented at 7:09 pm on June 17, 2022: contributor

Conditional code review ACK c98d5f1744dc3beed116ff708e3d4d8dcfe53eae if CI error is fixed. Feel free to ignore all the suggestions, this basically looks good.

in src/txmempool.h:569 in c318c982f6 outdated

567      * Sanity checks will be off by default for performance, because otherwise
568      * accepting transactions becomes O(N^2) where N is the number of transactions
569      * in the pool.
570-     *
571-     * @param[in] estimator is used to estimate appropriate transaction fees.
572-     * @param[in] check_ratio is the ratio used to determine how often sanity checks will run.

ariard commented at 5:55 pm on June 20, 2022:

In commit c318c982. " pool: Add and use MemPoolOptions, ApplyArgsManOptions"

nit: The comment here could be transposed to the corresponding members in MemPoolOptions, as they don’t have default just above informing about their usage.

dongcarl commented at 1:42 pm on June 21, 2022:

Fixed in 7e6d2d72368631b03d00c8fc7689a670ba92e4ac

in src/validation.h:68 in 85aa9d85a6 outdated

65+static const unsigned int DEFAULT_ANCESTOR_SIZE_LIMIT_KB = 101;
66 /** Default for -limitdescendantcount, max number of in-mempool descendants */
67 static const unsigned int DEFAULT_DESCENDANT_LIMIT = 25;
68 /** Default for -limitdescendantsize, maximum kilobytes of in-mempool descendants */
69-static const unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT = 101;
70+static const unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KB = 101;

ariard commented at 6:13 pm on June 20, 2022:

In commit 85aa9d8 “scripted-diff: Rename anc/desc size limit vars to indicate SI unit "

If you take glozow suggestion above, about the SI from KB to KvB, would be nice to update comments in consequence here.

dongcarl commented at 1:42 pm on June 21, 2022:

Fixed in 67ad3f84b1e69ad830ee27b25e0b6a33519e1d0b

in src/mempool_options.h:12 in 5b733c301d outdated

 3@@ -4,8 +4,13 @@
 4 #ifndef BITCOIN_MEMPOOL_OPTIONS_H
 5 #define BITCOIN_MEMPOOL_OPTIONS_H
 6 
 7+#include <cstddef>
 8+
 9 class CBlockPolicyEstimator;
10 
11+/** Default for -maxmempool, maximum megabytes of mempool memory usage */
12+static const unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB = 300;

darosior commented at 6:23 pm on June 20, 2022:

nit: maybe make it (along with the expiry one) constexpr?

dongcarl commented at 1:41 pm on June 21, 2022:

Fixed in 52349ea72f58b4057a88890fe0c7b722698e3fac

darosior commented at 6:49 pm on June 20, 2022: member

Concept ACK. I’d like to take another look at the mempool limits changes, but i agree it looks good modulo the dropped * 40 pointed by Gloria.

Should I merge CTxMemPool::Limits inside CTxMemPool::Options?

Rebasing after #25388 makes introducing mempool_limits.h just for the MemPoolLimits struct, but on the other hand those limits really are policy and i agree with Gloria it’s preferable to not mix them with options internal to the mempool. So i too think it’s fine as is.

in src/init.cpp:1433 in efcbd12767 outdated

1429@@ -1434,6 +1430,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1430     };
1431     ApplyArgsManOptions(args, mempool_opts);
1432     mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1000000);
1433+    if (mempool_opts.max_size < 0 || mempool_opts.max_size < mempool_opts.limits.descendant_size) {

ariard commented at 6:54 pm on June 20, 2022:

In commit efcbd127, “init: Remove redundant -mempool, -limit* queries "

I got a compilation warning here as an unsigned int should not be inferior to 0.

ariard commented at 4:26 pm on June 21, 2022:

I still got a compilation warning, is that issue on your local host or misconfigs of my compiler flags ?

0init.cpp: In function ‘bool AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)’:
1init.cpp:1435:37: warning: comparison of unsigned expression in ‘< 0’ is always false [-Wtype-limits]
2 1435 |     if (mempool_opts.max_size_bytes < 0 || mempool_opts.max_size_bytes < descendant_limit_bytes) {
3      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~

dongcarl commented at 6:59 pm on June 27, 2022:

Fixed by making mempool_opts.max_size_bytes a int64_t to avoid an unintended change in behavior

ariard commented at 7:01 pm on June 20, 2022: member

SGTM modulo I think this comment and this other one to fix.

Should I merge CTxMemPool::Limits inside CTxMemPool::Options?

I think it’s better to keep them split as it might be interesting to give the transaction/package-related limits to the wallet component, even in the lack of a mempool. It would solve the discussion around getPackageLimits().

DrahtBot added the label Needs rebase on Jun 20, 2022

dongcarl force-pushed on Jun 21, 2022

DrahtBot removed the label Needs rebase on Jun 21, 2022

dongcarl force-pushed on Jun 21, 2022

ariard approved

ariard commented at 4:47 pm on June 21, 2022: member

Code Review ACK 125c198e

Changes since last review:

add MB suffix to DEFAULT_MAX_MEMPOOL_SIZE
add comments in MemPoolOptions
one-lining mempool_max_size_mb assignment in ApplyArgsManOptions
add bytes suffix to max_size
make DEFAULT_MEMPOOL_EXPIRY constexpr
add vbytes suffix to ancestor/descendant_size
change SI of DEFAULT_ANCESTOR_SIZE_LIMIT from KB to KVB
return default package limits in lack of mempool in getPackageLimits()
use correct descendant_limit_bytes to evaluate min mempool max size

I don’t know about the compilation warning, if you get ride of it in another libbitcoinkernel PR.

dongcarl force-pushed on Jun 21, 2022

dongcarl commented at 6:30 pm on June 21, 2022: contributor

Push c98d5f1744dc3beed116ff708e3d4d8dcfe53eae -> 167076739f02ddcf754200b378d504d320d0ff22

Resolved most of the review comments (Thanks Gloria, Ryan, and Antoine^2) ☺️

in src/mempool_limits.h:20 in 6843403a15 outdated

15+ */
16+struct MemPoolLimits {
17+    size_t ancestor_count = DEFAULT_ANCESTOR_LIMIT;
18+    size_t ancestor_size_vbytes = DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000;
19+    size_t descendant_count = DEFAULT_DESCENDANT_LIMIT;
20+    size_t descendant_size_vbytes = DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1000;

dongcarl commented at 6:48 pm on June 21, 2022:

Reviewers: What do people think about using uint64_t here instead of size_t? It seems like there’s no obvious need for these to differ from platform to platform and it might be better to not have to worry about them being inconsistent across platforms.

dongcarl commented at 6:51 pm on June 21, 2022:

Ping @glozow, we might also have to change the member vars in MemPoolAccept if you think this is an okay idea?

laanwj commented at 9:25 pm on June 21, 2022:

You have a point, but I don’t know. All the array/vector size functions return size_t, right? So making these uint64_t introduces implicit casts. That while they’re unlikely to ever be outside the range of uint32_t anyway.

glozow commented at 8:56 am on June 22, 2022:

What do people think about using uint64_t here instead of size_t? It seems like there’s no obvious need for these to differ from platform to platform and it might be better to not have to worry about them being inconsistent across platforms.

Yeah makes sense. Do you mean to do those in this PR? It seems there are thoughts about changing all virtual size representation to int64_t or int32_t across the board (signed because we do arithmetic with them). See #23962 and #23957

dongcarl commented at 2:04 pm on June 22, 2022:

Okay, as the PR currently stands there are a few other integral type changes. Given that they’re somewhat tricky to reason about (overflows and such), I’m going to revert them to their original types and leave the changing for other PRs.

As for this specific one… It seems that we take ancestor_size_limit as uint64_t in most CTxMemPool function declarations, unsigned int in interface, and size_t in MemPoolAccept. Not exactly sure which one to choose for MemPoolOptions…

dongcarl commented at 9:26 pm on June 24, 2022:

See: #25290 (comment)

dongcarl commented at 7:01 pm on June 27, 2022:

Made these int64_ts to avoid an unintended change in behavior. Added a project-wide TODO to have bounds checking in ApplyArgsManOptions

in src/util/system.cpp:637 in a64c96f861 outdated

633+        return "1";
634+    } else if (value.isNum()) {
635+        return value.getValStr();
636+    } else {
637+        return value.get_str();
638+    }

maflcko commented at 9:22 am on June 22, 2022:

nit in a64c96f861d2d2658704085d42463ab03b1e6350: I think this can be written shorter by dropping the else and nesting.

0if (value.isNull()) return std::nullopt;
1...
2if (value.IsNum()) return value.getValStr();
3return value.get_str();

dongcarl commented at 7:29 pm on June 23, 2022:

Fixed as of fc02f77ca6

in src/util/system.cpp:668 in a64c96f861 outdated

666+        return 1;
667+    } else if (value.isNum()) {
668+        return value.getInt<int64_t>();
669+    } else {
670+        return LocaleIndependentAtoi<int64_t>(value.get_str());
671+    }

maflcko commented at 9:22 am on June 22, 2022:

Same

dongcarl commented at 7:29 pm on June 23, 2022:

Fixed as of fc02f77ca6

in src/util/system.cpp:689 in a64c96f861 outdated

689+    return SettingToBool(value);
690+}
691+
692+std::optional<bool> SettingToBool(const util::SettingsValue& value)
693+{
694+    return value.isNull() ? std::nullopt : std::make_optional(value.isBool() ? value.get_bool() : InterpretBool(value.get_str()));

maflcko commented at 9:25 am on June 22, 2022:

Same. Also any reason to use if above, but ?+std::make_optional here?

0if (value.isNull()) return std::nullopt;
1if (value.isBool()) return value.get_bool();
2return InterpretBool(...);

w0xlt commented at 2:21 pm on June 22, 2022:

I also think using if makes the code clearer and more readable.

dongcarl commented at 7:29 pm on June 23, 2022:

Fixed as of fc02f77ca6

in src/test/fuzz/rbf.cpp:27 in 7e6d2d7236 outdated

23@@ -23,7 +24,7 @@ FUZZ_TARGET(rbf)
24     if (!mtx) {
25         return;
26     }
27-    CTxMemPool pool;
28+    CTxMemPool pool{{}};

maflcko commented at 9:51 am on June 22, 2022:

unrelated comment in 7e6d2d72368631b03d00c8fc7689a670ba92e4ac:

It is probably a bug that no argsmanager is set up. (any log messages should be muted, not accumulated and turned into an OOM when fuzzing for a long time)
Using {{}} seems fine.

ryanofsky commented at 4:32 pm on June 23, 2022:

In commit “pool: Add and use MemPoolOptions, ApplyArgsManOptions” (7e6d2d72368631b03d00c8fc7689a670ba92e4ac)

This seems ok, but maybe later it would be better to use const TestingSetup* g_setup; pattern and g_setup->m_node->mempool to use a mempool that has check_ratio enabled (and has the muted log messages)

maflcko commented at 10:27 am on June 27, 2022:

Closing as resolved

in src/test/fuzz/validation_load_mempool.cpp:37 in 7e6d2d7236 outdated

31@@ -30,7 +32,10 @@ FUZZ_TARGET_INIT(validation_load_mempool, initialize_validation_load_mempool)
32     SetMockTime(ConsumeTime(fuzzed_data_provider));
33     FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
34 
35-    CTxMemPool pool{};
36+    CTxMemPool::Options mempool_opts{};
37+    ApplyArgsManOptions(*g_setup->m_node.args, mempool_opts);
38+    CTxMemPool pool{mempool_opts};

maflcko commented at 9:53 am on June 22, 2022:

question in 7e6d2d72368631b03d00c8fc7689a670ba92e4ac:

I think {{}} should also work here?

If not it might be good to mention in the commit message why this was chosen. Presumably the goal is to add the no-op argsman stuff here is that if settings are supplied in the future, they are not ignored.

ryanofsky commented at 0:55 am on June 23, 2022:

If not it might be good to mention in the commit message why this was chosen. Presumably the goal is to add the no-op argsman stuff here is that if settings are supplied in the future, they are not ignored.

Agree it would be good to document, but I think it would be preferable to say in a code comment instead of a commit message if you’re explaining reasoning behind code, so people reading the code can see the explanation.

in src/mempool_args.cpp:14 in 8c81736608 outdated

 9@@ -10,4 +10,6 @@
10 void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opts)
11 {
12     mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);
13+
14+    if (auto mb = argsman.GetIntArg("-maxmempool")) mempool_opts.max_size_bytes = *mb * 1000000;

maflcko commented at 10:06 am on June 22, 2022:

nit in 8c81736608e05607d0d84a6edb7ef6b66e46857c:

Could use 1'000'000?

dongcarl commented at 7:29 pm on June 23, 2022:

Fixed as of 468ee5b9a8

in src/txmempool.h:711 in 8c81736608 outdated

702@@ -701,6 +703,9 @@ class CTxMemPool
703       *  takes the fee rate to go back down all the way to 0. When the feerate
704       *  would otherwise be half of this, it is set to 0 instead.
705       */
706+    CFeeRate GetMinFee() const {
707+        return GetMinFee(m_max_size_bytes);
708+    }
709     CFeeRate GetMinFee(size_t sizelimit) const;

maflcko commented at 10:14 am on June 22, 2022:

8c81736608e05607d0d84a6edb7ef6b66e46857c:

I am not a fan of exposing this test-only method to normal code. What about removing it and allowing unit tests to modify m_max_size_bytes (or something like that)?

(Note: GetMinFee is marked const, but actually a setter of the rolling fee

dongcarl commented at 7:53 pm on June 23, 2022:

It seems that only the version of GetMinFee with a custom size limit is test-only… Perhaps we make that version protected in CTxMemPool and have the tests use a:

0class CMemPoolTest : public CTxMemPool
1{
2public:
3    using CTxMemPool::GetMinFee;
4};

Or did you have something else in mind? I’m hoping it’s just a small change here since it’s already existing behaviour…

maflcko commented at 8:32 am on June 24, 2022:

class MemPoolTest sounds fine

dongcarl commented at 8:29 pm on June 27, 2022:

Done in 0019650c1b

in src/rpc/mempool.cpp:660 in 8c81736608 outdated

656@@ -657,9 +657,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
657     ret.pushKV("bytes", (int64_t)pool.GetTotalTxSize());
658     ret.pushKV("usage", (int64_t)pool.DynamicMemoryUsage());
659     ret.pushKV("total_fee", ValueFromAmount(pool.GetTotalFee()));
660-    int64_t maxmempool{gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000};
661-    ret.pushKV("maxmempool", maxmempool);
662-    ret.pushKV("mempoolminfee", ValueFromAmount(std::max(pool.GetMinFee(maxmempool), ::minRelayTxFee).GetFeePerK()));
663+    ret.pushKV("maxmempool", (int64_t)pool.m_max_size_bytes);

maflcko commented at 10:16 am on June 22, 2022:

nit 8c81736608e05607d0d84a6edb7ef6b66e46857c: Maybe use a int64_t(...) cast if a cast is needed?

dongcarl commented at 8:29 pm on June 27, 2022:

No longer needed as of 5491373670

in src/mempool_options.h:26 in 8c81736608 outdated

22@@ -18,6 +23,7 @@ struct MemPoolOptions {
23     CBlockPolicyEstimator* const estimator = nullptr;
24     /* The ratio used to determine how often sanity checks will run.  */
25     int check_ratio = 0;
26+    size_t max_size_bytes = DEFAULT_MAX_MEMPOOL_SIZE_MB * 1000000;

maflcko commented at 10:41 am on June 22, 2022:

nit in https://github.com/bitcoin/bitcoin/commit/8c81736608e05607d0d84a6edb7ef6b66e46857c: 1'000'000?

dongcarl commented at 7:30 pm on June 23, 2022:

Fixed as of 468ee5b9a8

in src/Makefile.am:183 in 167076739f outdated

177@@ -178,6 +178,9 @@ BITCOIN_CORE_H = \
178   logging.h \
179   logging/timer.h \
180   mapport.h \
181+  mempool_args.h \
182+  mempool_limits.h \
183+  mempool_options.h \

maflcko commented at 11:02 am on June 22, 2022:

chainstatemanager_opts.h is located in kernel/, but this not. why?

in src/mempool_args.cpp:18 in 167076739f outdated

 8+#include <util/system.h>
 9+
10+namespace {
11+void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolLimits& mempool_limits)
12+{
13+    mempool_limits.ancestor_count = argsman.GetIntArg("-limitancestorcount", mempool_limits.ancestor_count);

maflcko commented at 11:13 am on June 22, 2022:

nit in 6843403a15cab34bc4e1e7acf7a0e509a2d437df:

0    if (auto cnt = argsman.GetIntArg("-limitancestorcount") mempool_limits.ancestor_count = *cnt;

Any reason to use a different style for those, when it is less code to keep the style consistent with the other parsing?

dongcarl commented at 7:47 pm on June 23, 2022:

I wanted to make visually clear the cases where we directly use the ArgsMan arg in CTxMemPool::Options and which cases get transformed before getting set in CTxMemPool::Options. However, I don’t feel strongly so let me know if you prefer the consistency instead!

maflcko commented at 8:33 am on June 24, 2022:

I don’t care too much. My suggestion uses less code, but this is only a style nit.

in src/mempool_args.cpp:30 in 167076739f outdated

20+}
21+}
22+
23+void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opts)
24+{
25+    mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);

maflcko commented at 11:14 am on June 22, 2022:

same nit:

0    if (auto ratio = argsman.GetIntArg("-checkmempool") mempool_opts.check_ratio = *ratio;

dongcarl commented at 7:47 pm on June 23, 2022:

See #25290 (review)

in src/node/interfaces.cpp:665 in 167076739f outdated

662+            limit_descendant_count = limits.descendant_count;
663+        } else {
664+            const CTxMemPool::Limits& limits{m_node.mempool->m_limits};
665+            limit_ancestor_count = limits.ancestor_count;
666+            limit_descendant_count = limits.descendant_count;
667+        }

maflcko commented at 11:21 am on June 22, 2022:

nit in b810f102c47c61f276ffa2f9a78a3b86ad5a88b8 for less code:

0            const CTxMemPool::Limits default_limits{};
1            
2            const CTxMemPool::Limits& limits{m_node.mempool?m_node.mempool->m_limits:default_limits};
3            limit_ancestor_count = limits.ancestor_count;
4            limit_descendant_count = limits.descendant_count;

dongcarl commented at 7:30 pm on June 23, 2022:

Fixed as of d669b93469

in src/init.cpp:934 in 7e90c69f1b outdated

931@@ -932,10 +932,6 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
932     }
933 
934     // mempool limits

maflcko commented at 11:24 am on June 22, 2022:

7e90c69f1be6f8ba52e1f24b983379d00aad2f65 : forgot to remove this?

dongcarl commented at 7:30 pm on June 23, 2022:

Fixed as of a956afedfe

in src/init.cpp:1434 in 7e90c69f1b outdated

1429@@ -1434,6 +1430,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1430     };
1431     ApplyArgsManOptions(args, mempool_opts);
1432     mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1000000);
1433+
1434+    size_t descendant_limit_bytes = mempool_opts.limits.descendant_size_vbytes * 40;
1435+    if (mempool_opts.max_size_bytes < 0 || mempool_opts.max_size_bytes < descendant_limit_bytes) {

maflcko commented at 11:26 am on June 22, 2022:

7e90c69f1be6f8ba52e1f24b983379d00aad2f65:

this doesn’t work. You can’t parse a signed integer and assign it to an unsigned type, then use < 0 to check if it originally was negative.

ariard commented at 1:21 am on June 23, 2022:

Already reported here : #25290 (review), get caught as a compilation warning.

maflcko commented at 5:20 am on June 23, 2022:

To solve, it would either need to be cast back with int64_t(...) < 0 or the type changed.

in src/init.cpp:1436 in 7e90c69f1b outdated

1429@@ -1434,6 +1430,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1430     };
1431     ApplyArgsManOptions(args, mempool_opts);
1432     mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1000000);
1433+
1434+    size_t descendant_limit_bytes = mempool_opts.limits.descendant_size_vbytes * 40;
1435+    if (mempool_opts.max_size_bytes < 0 || mempool_opts.max_size_bytes < descendant_limit_bytes) {
1436+        return InitError(strprintf(_("-maxmempool must be at least %d MB"), std::ceil(descendant_limit_bytes / 1000000.0)));

maflcko commented at 11:27 am on June 22, 2022:

7e90c69f1be6f8ba52e1f24b983379d00aad2f65: Maybe use 1'000'000.0 (not sure if that compiles?)

dongcarl commented at 7:30 pm on June 23, 2022:

It does! Fixed as of a956afedfe

in src/policy/fees.h:23 in 167076739f outdated

19@@ -19,6 +20,8 @@
20 #include <string>
21 #include <vector>
22 
23+extern const char* FEE_ESTIMATES_FILENAME;

maflcko commented at 11:32 am on June 22, 2022:

167076739f02ddcf754200b378d504d320d0ff22:

Instead of exporting this, what about exporting a function that calculates the path?

0fs::path FeeestPath(ArgsManager&);

So caller could call FeeestPath(gArgs) instead of gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME

maflcko commented at 10:25 am on June 27, 2022:

Closing as resolved

maflcko approved

maflcko commented at 11:34 am on June 22, 2022: member

Looks very good, but I think commit 7e90c69f1be6f8ba52e1f24b983379d00aad2f65 doesn’t work.

review ACK 167076739f02ddcf754200b378d504d320d0ff22 📽

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 167076739f02ddcf754200b378d504d320d0ff22 📽
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh5rAwApjNyIo4vWTlBPYX0A7LPHtMuhGjhptrfleuzrHmFNmrtt0af0IF4wdDw
 8Q2p4O4h2b+tdGfwahr4aW9RJ8/UXUkyNnLDtRhiGwl3Km5u0wWhgMtBbCAoIDs9k
 9oBbOnSlG1Fry+sey7dPCpvFcfcPBV3ldPvG6M1Bi2lWWFrLVVCnIsQYzUfC52Roz
10RIsBtEr2u7kX0i3cmklWpSps25yDSIWnwxCbrwnqfVYfw2ntZN7PGT3PdIu6kFuK
11qcAwEHe7GR3hZjCVlr2uo9QPGbTNFYhPX+zgjDjZVATmoR0rqL++bVrv/EiGBcNZ
12QApyOuattxjPBbqBYgj8WQ2rYr/QfB+1WPMCmmibtC5ghkfFRiu5iEvrKqicU7f4
13ZpRdH/chVY4xUvXbDgGBevaCDW6m0Jlu2Y0TVYXA7HzDP1novRtr7qQoUBhJW+f6
14WAmRfwf02VjVRGLbeTesbTpKnZ0/u6R4RZI41xlaGtB/KgMt3S72tvbby/De9aEI
15YkeDfTK9
16=P9lh
17-----END PGP SIGNATURE-----

in src/mempool_options.h:29 in 167076739f outdated

24+ *
25+ * Most of the time, this struct should be referenced as CTxMemPool::Options.
26+ */
27+struct MemPoolOptions {
28+    /* Used to estimate appropriate transaction fees. */
29+    CBlockPolicyEstimator* const estimator = nullptr;

maflcko commented at 11:39 am on June 22, 2022:

For all commits: Any reason to not use {} over = in this struct for C++11 member inits?

dongcarl commented at 7:45 pm on June 23, 2022:

Nope! Is {} preferred?

maflcko commented at 8:35 am on June 24, 2022:

Yes, for new code it is preferred style. {} has narrowing warnings enabled, as opposed to = or (), which do not.

maflcko commented at 10:25 am on June 27, 2022:

Closing as resolved

w0xlt approved

w0xlt commented at 5:33 pm on June 22, 2022: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/25290/commits/167076739f02ddcf754200b378d504d320d0ff22

. MemPoolOptions, MemPoolLimits and ApplyArgsManOptions make the mempool code more modularized. . Adding SI and time units improves code readability. . Replacing est_filepath by CBlockPolicyEstimator::m_estimation_filepath decouples this class from ArgsManager gArgs.

ArgsMan: Add Get*Arg functions returning optional

This allows the caller to not provide a default at all and just check
inside the optional to see if the arg was set or not.

fc02f77ca6

scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit

Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.

-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MAX_MEMPOOL_SIZE" \
    && git grep -l -E "$find_regex" \
        | xargs sed -i -E "s@$find_regex@\0_MB@g"
-END VERIFY SCRIPT-

ccbaf546a6

ariard approved

ariard commented at 1:24 am on June 23, 2022: member

Code Review ACK 1670767

Changes since last review, adding more bytes suffix, switching m_expiry from std::chrono::hours to std::chrono::seconds and correcting some comments from kb to vkb.

in src/test/util/setup_common.cpp:169 in 7e6d2d7236 outdated

163@@ -161,7 +164,12 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
164     GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler);
165 
166     m_node.fee_estimator = std::make_unique<CBlockPolicyEstimator>();
167-    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), m_node.args->GetIntArg("-checkmempool", 1));
168+    CTxMemPool::Options mempool_opts{
169+        Desig(estimator) m_node.fee_estimator.get(),
170+        Desig(check_ratio) 1,

ryanofsky commented at 4:21 pm on June 23, 2022:

In commit “pool: Add and use MemPoolOptions, ApplyArgsManOptions” (7e6d2d72368631b03d00c8fc7689a670ba92e4ac)

Maybe would be good to mention this is intentionally ignoring chainparams.DefaultConsistencyChecks(), and always enabling checking regardless of current network for test purposes.

dongcarl commented at 7:02 pm on June 27, 2022:

Fixed as of c7dbe3ae368096f1b3111e0ef7e08a7a5b4ee2a7

in src/test/fuzz/tx_pool.cpp:156 in 7e6d2d7236 outdated

151@@ -142,7 +152,7 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
152     // The sum of the values of all spendable outpoints
153     constexpr CAmount SUPPLY_TOTAL{COINBASE_MATURITY * 50 * COIN};
154 
155-    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
156+    CTxMemPool tx_pool_{MakeMempool(*node.args)};

ryanofsky commented at 4:37 pm on June 23, 2022:

In commit “pool: Add and use MemPoolOptions, ApplyArgsManOptions” (7e6d2d72368631b03d00c8fc7689a670ba92e4ac)

Any idea why this test is creating separate mempools instead of using g_setup->m_node->mempool? If there’s a reason, would be good to mention in a code comment.

I guess same question also applies to the other validation_load_mempool.cpp fuzz test below

dongcarl commented at 4:21 pm on June 24, 2022:

I think this is required for all fuzz tests since the initialize function is run once but the body is run multiple times. Using g_setup->m_node->mempool would mean that mempool state would persist between runs. So we have to set up a fresh mempool each time.

Do you think we should put in a comment along the lines of: “Use a local mempool instead of g_setup’s so that mempool state does not persist across runs”?

dongcarl commented at 7:03 pm on June 27, 2022:

Closing, feel free to reopen if still a problem!

in src/mempool_limits.h:16 in 6843403a15 outdated

11+ * Options struct containing limit options for a CTxMemPool. Default constructor
12+ * populates the struct with sane default values which can be modified.
13+ *
14+ * Most of the time, this struct should be referenced as CTxMemPool::Limits.
15+ */
16+struct MemPoolLimits {

ryanofsky commented at 5:11 pm on June 23, 2022:

In commit “mempool: Introduce (still-unused) MemPoolLimits” (6843403a15cab34bc4e1e7acf7a0e509a2d437df)

Might be good to document these members using documentation that is deleted in later commit “mempool: Use m_limit for UpdateTransactionsFromBlock” (7375ec6b6ada8f45b585f272ab20f1b9c1328ed6). Like for “The maximum allowed number of transactions including the entry and its ancestors” interesting to know the count is inclusive.

dongcarl commented at 7:03 pm on June 27, 2022:

Done as of 5759f4af1f2e788de369c4998611bfb1bbf6a705

ryanofsky commented at 5:47 pm on June 23, 2022: contributor

Conditional code review ACK 167076739f02ddcf754200b378d504d320d0ff22 if negative mempool size InitError is fixed: #25290 (review).

I’d probably fix this by just making option types int64_t instead of size_t since GetintArg is returning int64_t anyway. This would also facilitate moving error handling into the mempool constructor in the future (https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797, #25290 (review))

dongcarl force-pushed on Jun 23, 2022

maflcko approved

maflcko commented at 8:28 am on June 24, 2022: member

Some style fixups, but the bug is still there, I think.

re-ACK 468ee5b9a8 🎨

Changes:

Style fixups (less verbose code) in the first commit
Set mempool_opts.estimator = nullptr in the tx_pool fuzz target
Style: Use 1'000'000 over 1000000, and 1'000 over 1000
Style: Less verbose code in getPackageLimits
Doc: Remove leftover code comment

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 468ee5b9a8 🎨
 4
 5Changes:
 6* Style fixups (less verbose code) in the first commit
 7* Set mempool_opts.estimator = nullptr in the tx_pool fuzz target
 8* Style: Use 1'000'000 over 1000000, and 1'000 over 1000
 9* Style: Less verbose code in getPackageLimits
10* Doc: Remove leftover code comment
11-----BEGIN PGP SIGNATURE-----
12
13iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
14pUjJswwAyoo1eM+cuPf2dyA2ZXSC9fYwq0geBB/rpaUzr1tYTPiD1NTDcKcYEPh5
15MkFgcPzCl65QbxbLsqH2H6WQ5y8ITuYsL6tqQaqqgg+4XLAK1fIO0mBH4/nGmivv
168Tq5AMolDwqroSYhHqa4x4sVnOg/YG6mCi+BzqgfR4hRRWbGayTTKAapm6LCyLmI
17anMlIh6jItYQ7Vzy5HUPkQrNsxad29FrG8Jldy9Sfz8kbz84KRe3D/yHsL5bzkYH
18TOsPBjouLQK7YEXQiqri/3SSCvO557zhI1Fg3CiEuoZrYLz6+PxNTba40kVxV2X/
193oVQAObfQ5a2D60sVYKeaMweLs2xx/+9lFojD7AyYEeobEqMi+nCJipAdvv3EEHU
201doqX9XyMw/E1PQXCb73l2YGpp5Q1ear66584cIMYT4vM2UXy0jF+a4lpFBSDCT+
21nk2tNSiH6JcZUj+LT8i9tCZoQBonf5thKUBphoh9w0E5TFYdDboiDyPGM9Q8DSN7
22sA1kGAEa
23=32vq
24-----END PGP SIGNATURE-----

dongcarl force-pushed on Jun 24, 2022

dongcarl commented at 9:23 pm on June 24, 2022: contributor

I’m having some challenges with using int64_t vs size_t for CTxMemPool::Options::max_size_bytes. From my cursory reading, it seems like overflow of signed integral types are UB, so I’m not sure what the first check here is doing:

https://github.com/bitcoin/bitcoin/blob/1da1c0dd66265b6a610bf6c21bf2090910cb4fb3/src/init.cpp#L934

Is it to somehow catch the platforms where the apparent behaviour is to wrap around to negative when signed integral types overflow? That doesn’t seem right to me and I’m likely missing something.

In my mind, ideally we would eventually have ApplyArgsManOptions check that the command line arguments that require multiplication fit within their destination integral types after multiplication. This is a safety check that’s not done even prior to this PR.

There’s also the fact that prior to this PR, -maxmempool was passed to many functions as size_t, which could have underflowed…

Given that this PR is already a little bloaty, how about this: we use int64_t wherever we replace GetIntArg for now, which will make sure that there’s no unintentional behaviour change, and we follow up with a PR adding bounds checked multiplication in ApplyArgsManOptions and propagate errors up?

dongcarl force-pushed on Jun 24, 2022

maflcko commented at 10:23 am on June 27, 2022: member

so I’m not sure what the first check here is doing:

It is simply checking that the mempool is at least +4MB. It doesn’t check for overflow or truncation later on, which can still happen. If you want to fix that, sounds good, but I’d say it is not required.

in b61080b1993c10d953352701a54067955ca7401b: Please add a not, see my original comment, which does have a “not”: #25290 (review)

re-ACK 7e4f61b2901c8e1506a7b3246a4518e2aa4061f5 🥐

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 7e4f61b2901c8e1506a7b3246a4518e2aa4061f5 🥐
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjuagwAsUkOT4dLcniGY3SR8K3PuOKlb4e3U6wMnwfNERnevJKXF89YraytloFJ
 8QlLm6TEK9RM1NYwSApTwngC8hQIjc6uxS/JotfxrsZUJn529Njdw1jbi2Ecz/STY
 9EaBODMg8hWBecn+RNzYTN/oRrqBg1MK0tPDyIX+3HbQ/CjpbnWsdjRS1habaikEi
10zQ21Xh7rvlRzDViA9pbevkQEHSNFItDoGVn4SdLpZbSV21AcsfO9y/g9iF0YjyJm
11ByyHO08HE9FJr+S8JwXNPXHsqANPGnNTk+gtkzZIq3STD4mv9asIGFeS2IhGXM3Y
12mCDsPQLYAqbMEvZdsM5icazI1uHthYR4V1HgmJivQvW9e+yvptuOP1uDXCw/n7dZ
1352YY8hvE5gAY1NTukSJ9o3PZ3H6ijXIHyoFMwwma+wEu5aAlrm/L4l31xTQKmf/0
149bq2t+DRey/ZwTN4oILdt27xorSdIfW6Vvz6c8tbD4SGs9CA6LTBnGt//4Dte2qG
15eL+dryIC
16=vFr5
17-----END PGP SIGNATURE-----

in src/test/fuzz/tx_pool.cpp:134 in 479807a544 outdated

129+    // Take the default options for tests...
130+    CTxMemPool::Options mempool_opts{MemPoolOptionsForTest(node)};
131+
132+    // ...override specific options for this specific fuzz suite
133+    mempool_opts.estimator = nullptr;
134+    mempool_opts.check_ratio = 1;

ryanofsky commented at 3:21 pm on June 27, 2022:

In commit “pool: Add and use MemPoolOptions, ApplyArgsManOptions” (479807a5440660ded0932232a51280eabc1f6b7f)

Note sure if this is intentional. The comment says “override” but this is just setting the same value. Ok if it is intentional

dongcarl commented at 6:20 pm on June 27, 2022:

For .estimator, it’s overriding since MemPoolOptionsForTest would have set it. For .check_ratio, yeah it’s setting the same value, but it’s essentially being explicit in case some day MemPoolOptionsForTest is changed.

in src/mempool_args.cpp:32 in 1471293c69 outdated

12@@ -13,4 +13,6 @@ using kernel::MemPoolOptions;
13 void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opts)
14 {
15     mempool_opts.check_ratio = argsman.GetIntArg("-checkmempool", mempool_opts.check_ratio);
16+
17+    if (auto mb = argsman.GetIntArg("-maxmempool")) mempool_opts.max_size_bytes = *mb * 1'000'000;

ryanofsky commented at 3:49 pm on June 27, 2022:

In commit “mempool: Pass in -maxmempool instead of referencing gArgs” (1471293c698330354cd7c319c1a12b9c69c0dbdb)

IMO, this function would be an ideal place to check -maxmempool is >0, let MemPoolOptions::max_size_bytes be a size_t instead of an int64_t so it matches the CTxMemPool::m_max_size_bytes, return an error string, and follow good general practice of handling errors early. As is, these checks are done separately in src/init.cpp though so lack of error checking here is not actually a bug

dongcarl commented at 6:11 pm on June 27, 2022:

Yup, absolutely agree. Added to project-wide TODOs.

ryanofsky approved

ryanofsky commented at 4:27 pm on June 27, 2022: contributor

Code review ACK 7e4f61b2901c8e1506a7b3246a4518e2aa4061f5. Many cleanups since last review, main one is constructing mempools more consistently in test. Also max_size_bytes and MemPoolLimits are now int64_t instead of size_t to avoid an unintended change in behavior. m_max_size_bytes is still size_t

re: #25290 (comment)

it seems like overflow of signed integral types are UB, so I’m not sure what the first check here is doing: https://github.com/bitcoin/bitcoin/blob/1da1c0dd66265b6a610bf6c21bf2090910cb4fb3/src/init.cpp#L932-L934

I think it’s just needed to show an error in the case where both options are passed negative values.

fuzz/rbf: Add missing TestingSetup

MarcoFalke mentioned that this is likely a bug since "any log messages
should be muted, not accumulated and turned into an OOM when fuzzing for
a long time".

0199bd35bb

dongcarl force-pushed on Jun 27, 2022

dongcarl commented at 6:58 pm on June 27, 2022: contributor

@MarcoFalke

in b61080b: Please add a not, see my original comment, which does have a “not”: #25290 (comment)

😅 That is definitely my bad, fixed in latest push!

dongcarl force-pushed on Jun 27, 2022

dongcarl commented at 8:26 pm on June 27, 2022: contributor

Pushed 7e4f61b2901c8e1506a7b3246a4518e2aa4061f5 -> 54913736701ed336a2be94feb0f77fae21226ba4

Fixed quoting Marco: #25290 (comment)
Added comment about CChainState::GetCoinsCacheSizeState() correctness (thanks James!): #25290 (review)
Made CTxMemPool::m_max_size_bytes also int64_t to avoid an unintended change in behavior (thanks for pointing this one out Ryan!). Added to project TODO a task to add bounds checking in ApplyArgsManOptions(..., MemPoolOptions&) so that we can safely transition CTxMemPool::{Options,Limits} to using unsigned types for sizes.
Added commit “mempool: Make GetMinFee() with custom size protected” addressing: #25290 (review)

dongcarl commented at 8:29 pm on June 27, 2022: contributor

Maintainers: All reviews have been addressed, I think this PR is in a good place for merge pending CI pass.

dongcarl commented at 10:39 pm on June 27, 2022: contributor

I believe the CI failure in wallet_reorgsrestore.py is spurious: https://cirrus-ci.com/task/6073861945950208?logs=ci#L560

I’ve run FILE_ENV=./ci/test/00_setup_env_native_asan.sh ./ci/test_run_all.sh locally and am unable to reproduce the error.

ariard approved

ariard commented at 10:49 pm on June 27, 2022: member

Code Review ACK 5491373

Changes since last review:

moving mempool_optjons.h under the kernel namespace
adding MemPoolOptionsForTest in src/test/util/setup_common.cpp
initializing CTxMemPool with MemPoolOptionsForTest
moving MemPoolOptions::max_size_bytes from size_t to int64_t
moving CTxMemPool::m_max_size_bytes from size_t to int64_t
renaming DEFAULT_MEMPOOL_EXPIRY to DEFAULT_MEMPOOL_EXPIRY_HOURS
using ternary in getPackageLimits()
adding comments in MemPoolLimits
making descendant_limits_bytes in src/init.cpp a int64_t
adding new policy/fes_args.cpp and the fee estimator path constructor FeeestPath

glozow commented at 9:40 am on June 28, 2022: member

light code review ACK 5491373670. Very happy with the reduced querying of gArgs for mempool options, agree with the code organization, I’m much less familiar with init than mempool but stepped through each commit and the code looks correct to me.

in src/util/system.h:375 in fc02f77ca6 outdated

371@@ -365,6 +372,7 @@ class ArgsManager
372      * @return command-line argument or default value
373      */
374     bool GetBoolArg(const std::string& strArg, bool fDefault) const;
375+    std::optional<bool> GetBoolArg(const std::string& strArg) const;

laanwj commented at 10:24 am on June 28, 2022:

I like the idea of this commit. My original idea for the future of defaults was to make ArgsManager handle them internally at registration time, and get rid of the default arguments to these functions (at query time) in that way. But this works.

in src/policy/fees_args.h:14 in 5491373670 outdated

 9+
10+class ArgsManager;
11+
12+fs::path FeeestPath(const ArgsManager& argsman);
13+
14+

laanwj commented at 11:08 am on June 28, 2022:

nit: extra newline?

dongcarl commented at 3:52 pm on June 28, 2022:

Fixed as of b96e009a8a36f4feeee49d1a3c8f85fb9ab0d920

in src/policy/fees_args.h:13 in 5491373670 outdated

 7+
 8+#include <fs.h>
 9+
10+class ArgsManager;
11+
12+fs::path FeeestPath(const ArgsManager& argsman);

laanwj commented at 11:10 am on June 28, 2022:

Please add a doc comment for this function. E.g.

0/** Return path of fee estimates data file. */

dongcarl commented at 3:52 pm on June 28, 2022:

Fixed as of b96e009a8a36f4feeee49d1a3c8f85fb9ab0d920

in src/test/fuzz/policy_estimator_io.cpp:22 in 5491373670 outdated

17+} // namespace
18+
19 void initialize_policy_estimator_io()
20 {
21     static const auto testing_setup = MakeNoLogFileContext<>();
22+    g_setup = testing_setup.get();

laanwj commented at 11:15 am on June 28, 2022:

Having a global dangling pointer here is kind of ugly. Is there really no other way to get this information in? If you need a global, what about storing the path in the global instead of a pointer. It seems slightly safer at least.

dongcarl commented at 3:53 pm on June 28, 2022:

We could just store the path, but this is sort of the pattern that most fuzz tests take, and we might want something else from the testingsetup in the future :-/

maflcko commented at 4:13 pm on June 28, 2022:

Yeah, this is the general pattern. Let me know if there is a better way.

And there should be no risk in doing “risky” stuff in the unit or fuzz tests, such as globals, raw pointers/memory, “wild” casts, etc…, as they are run in sanitizers that should detect any issue if it arises. (As opposed to real code, which is not 100% covered by sanitizers)

laanwj commented at 4:58 pm on June 28, 2022:

Ok, that’s a good point. I was mainly afraid of false positives/negatives due to pointers pointing to the wrong memory. But yes, the sanitizers should catch that.

dongcarl force-pushed on Jun 28, 2022

in src/txmempool.cpp:456 in c7dbe3ae36 outdated

450@@ -451,8 +451,9 @@ void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee,
451     assert(int(nSigOpCostWithAncestors) >= 0);
452 }
453 
454-CTxMemPool::CTxMemPool(CBlockPolicyEstimator* estimator, int check_ratio)
455-    : m_check_ratio(check_ratio), minerPolicyEstimator(estimator)
456+CTxMemPool::CTxMemPool(const Options& opts)
457+    : m_check_ratio(opts.check_ratio),
458+      minerPolicyEstimator(opts.estimator)

maflcko commented at 4:32 pm on June 28, 2022:

nit in c7dbe3ae368096f1b3111e0ef7e08a7a5b4ee2a7:

Could use {} over () for additional compiler checks?

dongcarl commented at 8:12 pm on June 28, 2022:

Fixed in f1941e8bfd and others

in src/init.cpp:1437 in c7dbe3ae36 outdated

1433+    CTxMemPool::Options mempool_opts{
1434+        Desig(estimator) node.fee_estimator.get(),
1435+        Desig(check_ratio) chainparams.DefaultConsistencyChecks() ? 1 : 0,
1436+    };
1437+    ApplyArgsManOptions(args, mempool_opts);
1438+    mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1000000);

maflcko commented at 4:41 pm on June 28, 2022:

nit in c7dbe3ae368096f1b3111e0ef7e08a7a5b4ee2a7:

Could use 1'000'000?

dongcarl commented at 8:13 pm on June 28, 2022:

Fixed in f1941e8bfd

in src/test/mempool_tests.cpp:16 in 0019650c1b outdated

15@@ -16,6 +16,12 @@ BOOST_FIXTURE_TEST_SUITE(mempool_tests, TestingSetup)
16

maflcko commented at 4:42 pm on June 28, 2022:

commit message of 0019650c1b56b65262b75dd284040848aaaec129:

drived -> derived?

dongcarl commented at 8:13 pm on June 28, 2022:

Fixed in 386c9472c8

in src/validation.cpp:4642 in e741c49716 outdated

4640@@ -4645,7 +4641,7 @@ static const uint64_t MEMPOOL_DUMP_VERSION = 1;
4641 
4642 bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mockable_fopen_function)
4643 {
4644-    int64_t nExpiryTimeout = gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60;
4645+    int64_t nExpiryTimeout = std::chrono::seconds{pool.m_expiry}.count();

maflcko commented at 6:26 pm on June 28, 2022:

nit in e741c49716a65b624fcf91db0de6dd11617675ad: Might be nice to make nExpiryTimeout type-safe in a follow-up.

dongcarl commented at 8:13 pm on June 28, 2022:

True! Not addressing in this PR. Leaving open for posterity.

in src/kernel/mempool_limits.h:7 in 1ad4268e1a outdated

0@@ -0,0 +1,25 @@
1+// Copyright (c) 2022 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+#ifndef BITCOIN_KERNEL_MEMPOOL_LIMITS_H
5+#define BITCOIN_KERNEL_MEMPOOL_LIMITS_H
6+
7+#include <cstdint>
8+#include <policy/policy.h>

maflcko commented at 6:28 pm on June 28, 2022:

nit in 1ad4268e1a7da0e97a97fa677f7b50f226d084b4: Usually std-includes come last in a separate paragraph

dongcarl commented at 8:13 pm on June 28, 2022:

Fixed in 9333427014

in src/txmempool.cpp:158 in 5a3d18eb2c outdated

155     }
156     mapTx.modify(updateIt, update_descendant_state(modifySize, modifyFee, modifyCount));
157 }
158 
159-void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashesToUpdate, uint64_t ancestor_size_limit, uint64_t ancestor_count_limit)
160+void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashesToUpdate)

maflcko commented at 6:35 pm on June 28, 2022:

nit in 5a3d18eb2c12e046d35d628dc67daa9e2bb343b3: Can update format while touching this line?

dongcarl commented at 8:14 pm on June 28, 2022:

Fixed in 6c5c60c412

in src/init.cpp:1433 in 470347d2ff outdated

1427@@ -1433,6 +1428,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1428     };
1429     ApplyArgsManOptions(args, mempool_opts);
1430     mempool_opts.check_ratio = std::clamp<int>(mempool_opts.check_ratio, 0, 1000000);
1431+
1432+    int64_t descendant_limit_bytes = mempool_opts.limits.descendant_size_vbytes * 40;

maflcko commented at 6:40 pm on June 28, 2022:

nit in 470347d2ffd462d635fdf344e38332a4eb4b8ad3: could add a {} scope to avoid leaking this symbol into the full AppInitMain scope?

dongcarl commented at 8:14 pm on June 28, 2022:

I’m not going to do this in this PR, feel free to follow up.

maflcko approved

maflcko commented at 6:45 pm on June 28, 2022: member

lgtm

review ACK 54913736701ed336a2be94feb0f77fae21226ba4 📏

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 54913736701ed336a2be94feb0f77fae21226ba4 📏
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUijogv8D4DYVC5hv7qOlIEMk+Yj7ZthU8aCSgXn6bhN611QGGuj40G9dfTNtOZ5
 8oLKTttz9W1ML68IdlN+syYL112EN9lcJi+jls5wdof3dr1qrKZJEdBEi/sL+CBjt
 9h1vr8zHHWSCw2IYhEtHmQMj8oNbtID4NCG3ngyAEh9xzw7UGrzM9bxUI0qJhQJ1n
10/AXxVZZ32sM+5G2YYa1Pc+HoxmpcPhZuF5sWCxTDGeDkwA8DEbvyJUOwP+ibO65x
11hMDvsoLIwYsVkXCw9LQFG2nUB2uWKujmi4AgGqsOr/+mO6foxsMqwrZLhXyZn39U
12JWBtkWkizU69fYMBdr5+ku0BdH6c6EL/esfsTQ8n16W+Dlq/1Pm0sgVwQmGOF8F2
13rdrxKl4YSodH7vX8ChNxNxaASBqKoLltSOxJ7QWdB9py4F7c1yd/dAA1GpUVz8g6
14BVcxDbpa0JTkYU2a3yUx02vs3SUPb+P2ZTz+gsxbDmw/gT/J8RZ3RDwNQKafhTzZ
159imCuWxg
16=vOeM
17-----END PGP SIGNATURE-----

ryanofsky approved

ryanofsky commented at 7:06 pm on June 28, 2022: contributor

Code review ACK b96e009a8a36f4feeee49d1a3c8f85fb9ab0d920. Commit message and comment cleanups since last review, and m_max_size_bytes is changed from size_t to int64_t

pool: Add and use MemPoolOptions, ApplyArgsManOptions

Reviewers: Note that CTxMemPool now requires a non-defaulted
CTxMemPool::Options for its constructor. Meaning that there's no need to
worry about a stray CTxMemPool constructor somewhere defaulting to
something incorrect. All instances of CTxMemPool construction are
addressed here in this commit.

We set options for CTxMemPool and construct it in many different ways. A
good example can be seen in how we determine CTxMemPool's check_ratio in
AppInitMain(...).

1. We first set the default based on chainparams's
   DefaultConsistencyChecks()
2. Then, we apply the ArgsManager option on top of that default
3. Finally, we clamp the result of that between 0 and 1 Million

With this patch, most CTxMemPool construction are along the lines of:

    MemPoolOptions mempool_opts{...default overrides...};
    ApplyArgsManOptions(argsman, mempool_opts);
    ...hard overrides...
    CTxMemPool pool{mempool_opts};

This "compositional" style of building options means that we can omit
unnecessary/irrelevant steps wherever we want but also maintain full
customizability.

For example:

- For users of libbitcoinkernel, where we eventually want to remove
  ArgsManager, they simply won't call (or even know about)
  ApplyArgsManOptions.

- See src/init.cpp to see how the check_ratio CTxMemPool option works
  after this change.

A MemPoolOptionsForTest helper was also added and used by tests/fuzz
tests where a local CTxMemPool needed to be created.

The change in src/test/fuzz/tx_pool.cpp seemingly changes behaviour by
applying ArgsManager options on top of the CTxMemPool::Options defaults.
However, in future commits where we introduce flags like -maxmempool,
the call to ApplyArgsManOptions is actually what preserves the existing
behaviour. Previously, although it wasn't obvious, our CTxMemPool would
consult gArgs for flags like -maxmempool when it needed it, so it
already relied on ArgsManager information. This patchset just laid bare
the obfuscatory perils of globals.

[META] As this patchset progresses, we will move more and more
       CTxMemPool-relevant options into MemPoolOptions and add their
       ArgsMan-related logic to ApplyArgsManOptions.

f1941e8bfd

mempool: Pass in -maxmempool instead of referencing gArgs

- Store the mempool size limit (-maxmempool) in CTxMemPool as a member.

- Remove the requirement to explicitly specify a mempool size limit for
  CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the
  stored mempool size limit where possible.

- Remove all now-unnecessary instances of:
    gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000

The code change in CChainState::GetCoinsCacheSizeState() is correct
since the coinscache should not repurpose "extra" mempool memory
headroom for itself if the mempool doesn't even exist.

82f00de7a6

mempool: Make GetMinFee() with custom size protected

The version of GetMinFee() with a custom size specification is and
should only be used by tests. Mark it as protected and use a derived
class exposing GetMinFee() as public in tests.

386c9472c8

init: Only determine maxmempool once

Now that MemPoolOptions has a correctly-determined max_size member, use
that instead of redetermining it to print the log line.

51c7a41a5e

mempool: Pass in -mempoolexpiry instead of referencing gArgs

- Store the mempool expiry (-mempoolexpiry) in CTxMemPool as a
  std::chrono::seconds member.

- Remove the requirement to explicitly specify a mempool expiry for
  LimitMempoolSize(...), just use the newly-introduced member.

- Remove all now-unnecessary instances of:
    std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}

aa9141cd81

scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit

Better to be explicit when it comes to time to avoid unintentional bugs.

-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MEMPOOL_EXPIRY" \
    && git grep -l -E "$find_regex" \
        | xargs sed -i -E "s@$find_regex@\0_HOURS@g"
-END VERIFY SCRIPT-

1ecc77321d

scripted-diff: Rename anc/desc size limit vars to indicate SI unit

Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.

-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_(ANCESTOR|DESCENDANT)_SIZE_LIMIT" \
    && git grep -l -E "$find_regex" \
        | xargs sed -i -E "s@$find_regex@\0_KVB@g"
-END VERIFY SCRIPT-

716bb5fbd3

mempool: Introduce (still-unused) MemPoolLimits

They live as a CTxMemPool member.

[META] These limits will be used in subsequent commits to replace calls
       to gArgs.

9333427014

mempoolaccept: Use limits from mempool in constructor 38af2bcf35

node/ifaces: Use existing MemPoolLimits 9e93b10301

mempool: Use m_limit for UpdateTransactionsFromBlock

Since:

- UpdateTransactionsFromBlock is only called by
  MaybeUpdateMempoolForReorg, which calls it with the gArgs-determined
  ancestor limits
- UpdateForDescendants is only called by UpdateTransactionsFromBlock
  with the ancestor limits unchanged

We can remove the requirement to specify the ancestor limits for both
UpdateTransactionsFromBlock and UpdateForDescendants and just use the
values in the m_limits member.

Also move some removed comments to MemPoolLimits struct members.

The uint64_t cast in UpdateForDescendants is not new behavior,
see the diff in CChainState::MaybeUpdateMempoolForReorg for where they
were previously.

6c5c60c412

init: Remove redundant -*mempool*, -limit* queries

Now that MemPoolOptions has correctly-determined max_size and limits
members, perform sanity checks on that instead of re-determining the
options.

9a3d825c30

fees: Pass in a filepath instead of referencing gArgs d1684beabe

dongcarl force-pushed on Jun 28, 2022

dongcarl commented at 8:10 pm on June 28, 2022: contributor

Pushed b96e009a8a36f4feeee49d1a3c8f85fb9ab0d920 -> d1684beabe5b738c2cc83de83e1aaef11a761b69

Addressed some of Marco’s review comments

dongcarl commented at 8:12 pm on June 28, 2022: contributor

I’m likely not going to repush for nits from this point forwards, since there’s quite a lot of Code Review ACKs already and I want to reduce re-review burden. I hope that’s understandable. We can always follow up in future PRs.

Please ignore the GitHub Project noise of me moving this PR across columns, I just forgot to do so earlier.

ryanofsky approved

ryanofsky commented at 8:22 pm on June 28, 2022: contributor

Code review ACK d1684beabe5b738c2cc83de83e1aaef11a761b69. Just minor cleanups since last review, mostly switching to brace initialization

maflcko commented at 7:12 am on June 29, 2022: member

ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
 7pUgupwv9G/2wfNggsXuzCwHm6yNWsM5mmsQMgwhxAAWWFG0O+Wa3rbImNzW04Qp5
 8LcHOWPiZ4nx/aLL5TtvzEPYJ5iy3inLU/ekMLEzRoRJgdWaPAnXUzXEyK6/wXAie
 9J2Oa1RZTpJ/HuaqnrQ8grupoFHkLw7RQ3j4uhOn1aROUREz+C4JragufIQtXCnMJ
10EBAMn0MnNo3TWmeLEVxdpjkk6Hhuln4Kqv1/MGo9g+CLu6zFsLssBMCO2lGP/2up
115p/HKyPVYh/8lqBjGW2JH7YsHj5pnS614okonpzOEKN6Qktx/YAXnnMjJyW3SvmP
12lVdYjNW38nekegQNSiceliYQauO6SiwmmEL4r921ytAR/zKjYEvKrpj2+w8XclU2
13s/KGZX2x9LqtxkL7rK0HBTqOL4ELmcp2cDqeYiPnL5FHgmERODnE+uACNszpYCHR
14waRWtqkJE5tVYn7gJ3frICTDUWQFbcR5+ofdDDQPWahKfqZwSiRAd68R7x4tL6Dp
15PKfpIgo5
16=seCw
17-----END PGP SIGNATURE-----

maflcko merged this on Jun 29, 2022

maflcko closed this on Jun 29, 2022

maflcko commented at 7:19 am on June 29, 2022: member

Let me know if you want to follow-up for #25290 (review) or want me to do it. I see that you have a follow-up already open, so I don’t want to duplicate work or queue up merge conflicts.

dongcarl commented at 3:36 pm on June 29, 2022: contributor

@MarcoFalke Answered in: #25487 (comment)

sidhujag referenced this in commit f6895b4ddd on Jun 29, 2022

ryanofsky referenced this in commit ba91ffa1ce on Aug 17, 2022

ryanofsky referenced this in commit d98d2aa19f on Aug 17, 2022

ryanofsky referenced this in commit 841c3a1205 on Aug 17, 2022

ryanofsky referenced this in commit b86aabc520 on Aug 17, 2022

ryanofsky referenced this in commit 13d466e247 on Aug 17, 2022

ryanofsky referenced this in commit 370189fe96 on Aug 18, 2022

ryanofsky referenced this in commit a6b3dbeca4 on Aug 22, 2022

ryanofsky referenced this in commit 1c77e2e0db on Aug 22, 2022

ryanofsky referenced this in commit a76f628585 on Aug 26, 2022

ryanofsky referenced this in commit 4e65b3b222 on Aug 26, 2022

glozow referenced this in commit d33c5894e9 on Oct 9, 2022

ryanofsky referenced this in commit cf0b02fb27 on Oct 14, 2022

achow101 referenced this in commit 9321df4487 on Feb 17, 2023

fanquake referenced this in commit b175bdb9b2 on Mar 14, 2023

fanquake referenced this in commit e695d8536e on Mar 16, 2023

fanquake referenced this in commit 369d4c03b7 on Apr 3, 2023

ryanofsky referenced this in commit 153a6882f4 on Jun 9, 2023

bitcoin locked this on Jan 17, 2024

[kernel 3a/n] Decouple CTxMemPool from ArgsManager #25290

Conflicts

[kernel 3a/n] Decouple `CTxMemPool` from `ArgsManager` #25290