[kernel 3b/n] Decouple `{Dump,Load}Mempool` from `ArgsManager`

dongcarl commented at 11:35 pm on June 27, 2022: contributor

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

This PR moves {Dump,Load}Mempool into its own kernel/mempool_persist module and introduces ArgsManager node:: helpers in node/mempool_persist_argsto remove the scattered calls to GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL).

More context can be gleaned from the commit messages.

One thing I was reflecting on as I wrote this was that in the long run, I think we should probably invert the validation <-> mempool relationship. Instead of mempool not depending on validation, it might make more sense to have validation not depend on mempool. Not super urgent since libbitcoinkernel will include both validation and mempool, but perhaps something for the future.

DrahtBot added the label Refactoring on Jun 28, 2022

DrahtBot added the label Needs rebase on Jun 28, 2022

in src/txmempool.cpp:1057 in f20bff9107 outdated

1052+        }
1053+        vinfo = infoAll();
1054+        unbroadcast_txids = GetUnbroadcastTxs();
1055+    }
1056+
1057+    auto mid = GetSystemTime<std::chrono::microseconds>();

MarcoFalke commented at 7:27 am on June 28, 2022:

When touching this line, it could make sense to make it a behaviour change and use steady clock, see for example #25456.

dongcarl commented at 5:41 pm on June 29, 2022:

Fixed as of 05ff88acdfc1b777e7d6e53a2975e18ead6f35b1

dongcarl force-pushed on Jun 28, 2022

dongcarl added this to the "WIP PRs" column in a project

dongcarl force-pushed on Jun 29, 2022

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

@MarcoFalke

Following up on #25290 (review), wondering what clock I should be using there? SteadyClock? NodeClock?

Also, could you sanity check my usage of the new clock facilities in 0ea35ed965725341428a657c4fb29c518969af60? Thanks!

MarcoFalke commented at 3:39 pm on June 29, 2022: member

Jup, steady clock is fine, given that previously system clock was used and the result is only used for logging a duration (time difference).

Ticks<SecondsDouble> should also work?

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

@MarcoFalke Okay, I’ll use Ticks<SecondsDouble> and steady clock for DumpMempool, what about for LoadMemPool you mentioned here? #25290 (review)

MarcoFalke commented at 3:49 pm on June 29, 2022: member

The timestamps for mempool entries are mockable (GetTime), thus they are (supposed to be) using NodeClock.

dongcarl force-pushed on Jun 29, 2022

dongcarl commented at 5:40 pm on June 29, 2022: contributor

Push 05ff88acdfc1b777e7d6e53a2975e18ead6f35b1:

Use type-safe time types
Rebase over master after merge of #25290

I think this PR is ready for review!

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

DrahtBot removed the label Needs rebase on Jun 29, 2022

in src/kernel/mempool_options.h:38 in 5b426d71aa outdated

33@@ -32,6 +34,8 @@ struct MemPoolOptions {
34     int64_t max_size_bytes{DEFAULT_MAX_MEMPOOL_SIZE_MB * 1'000'000};
35     std::chrono::seconds expiry{std::chrono::hours{DEFAULT_MEMPOOL_EXPIRY_HOURS}};
36     MemPoolLimits limits{};
37+    /* An empty path turns off mempool persistence */
38+    fs::path persistence_path = {};

MarcoFalke commented at 6:41 pm on June 29, 2022:

nit in 5b426d71aab1d2c7a3f7b44043e98a0e9b821518:

No need for =

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

Fixed as of 930f023ea9

in src/mempool_args.h:14 in 5b426d71aa outdated

 9@@ -10,6 +10,9 @@ namespace kernel {
10 struct MemPoolOptions;
11 };
12 
13+/** Default for -persistmempool */
14+static constexpr bool DEFAULT_PERSIST_MEMPOOL = true;

MarcoFalke commented at 6:44 pm on June 29, 2022:

nit in 5b426d71aab1d2c7a3f7b44043e98a0e9b821518:

Could use {}.

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

Fixed as of 930f023ea9

in src/util/time.h:75 in 276cfd2f5f outdated

70@@ -71,6 +71,9 @@ int64_t GetTimeMillis();
71 /** Returns the system time (not mockable) */
72 int64_t GetTimeMicros();
73 
74+template <typename T>
75+T GetSystemTime();

MarcoFalke commented at 6:47 pm on June 29, 2022:

276cfd2f5fd48d0556ccc4a83d350c0e9d45cc99: why is this file changed?

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

Sorry, it was leftover… Fixed as of 6612412dfc

in src/validation.cpp:4784 in 276cfd2f5f outdated

4781-        LogPrintf("Dumped mempool: %gs to copy, %gs to dump\n", (mid-start)*MICRO, (last-mid)*MICRO);
4782+        auto last = SteadyClock::now();
4783+
4784+        auto copy_secs = Ticks<SecondsDouble>(mid - start);
4785+        auto dump_secs = Ticks<SecondsDouble>(last - mid);
4786+        LogPrintf("Dumped mempool: %gs to copy, %gs to dump\n", copy_secs, dump_secs);

MarcoFalke commented at 6:50 pm on June 29, 2022:

nit: Could keep inlined to not evaluate the args if the log is disabled ?

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

Done as of 6612412dfc

in src/validation.cpp:57 in 35a04e0707 outdated

52 #include <util/translation.h>
53 #include <validationinterface.h>
54 #include <warnings.h>
55 
56 #include <algorithm>
57-#include <chrono>

MarcoFalke commented at 6:51 pm on June 29, 2022:

35a04e0707478f18a5c12e8979a823e119d9: Are you sure that nothing else uses those two headers?

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

Ah my bad, reinstated as of a19cc42ca2

in src/txmempool.cpp:1152 in 05ff88acdf outdated

1148@@ -1150,7 +1149,7 @@ bool CTxMemPool::_LoadMempool(std::function<bool(const CTransactionRef&, int64_t
1149             if (amountdelta) {
1150                 PrioritiseTransaction(tx->GetHash(), amountdelta);
1151             }
1152-            if (nTime > nNow - nExpiryTimeout) {
1153+            if (nTime > TicksSinceEpoch<Seconds<int64_t>>(now - m_expiry)) {

MarcoFalke commented at 6:55 pm on June 29, 2022:

05ff88acdfc1b777e7d6e53a2975e18ead6f35b1:

What do you think about if (std::chrono::seconds{nTime} > now - m_expiry) { ?

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

I tried it, and I think the only thing that’ll compile is:

0            if (NodeSeconds{std::chrono::seconds{nTime}} > now - m_expiry) {

Which doesn’t seem better than what we have now

MarcoFalke commented at 10:22 am on June 30, 2022:

I see. Is there any reason why you need Seconds<int64_t>? std::chrono::seconds should already be of the same width.

dongcarl commented at 4:25 pm on June 30, 2022:

From reading the docs, the representation for std::chrono::seconds seems to only need to be “A signed integral type of at least 35 bits”, so there might be platforms/stdlib’s that don’t use int64_t and I thought it’d be better to be explicit about it.

MarcoFalke commented at 1:58 pm on July 1, 2022:

Is there anything that requires this code part to be explicit about it? Does it protect against something?

dongcarl commented at 5:34 pm on July 1, 2022:

It doesn’t really, happy to change it to std::chrono::seconds if you want me to!

MarcoFalke commented at 6:39 pm on July 1, 2022:

If there is no reason to change util/time.h, then I think the preference is to not change it.

dongcarl commented at 7:24 pm on July 1, 2022:

Agreed! Done in ddf4920df58bc6f84297abc9b31e5830237ea514

MarcoFalke approved

MarcoFalke commented at 6:55 pm on June 29, 2022: member

left some nits (feel free to ignore)

dongcarl force-pushed on Jun 29, 2022

DrahtBot commented at 4:52 am on June 30, 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:

#25577 (mempool, refactor: add MemPoolBypass by w0xlt)
#25571 (refactor: Make mapBlocksUnknownParent local, and rename it by LarryRuane)
#25570 (mempool: Add the bypass_{csv,cltv} option to testmempoolaccept by w0xlt)
#25564 (Move DEFAULT_MAX_ORPHAN_TRANSACTIONS to node/txorphanage.h by MarcoFalke)
#25532 (mempool: Add the bypass_feerate_accuracy option to testmempoolaccept by w0xlt)
#25527 ([kernel 3c/n] Decouple validation cache initialization from ArgsManager by dongcarl)
#25494 (indexes: Stop using node internal types by ryanofsky)
#25466 (ci: add unused-using-decls to clang-tidy by fanquake)
#25434 (mempool: Add option to bypass contextual timelocks in testmempoolaccept by w0xlt)
#25308 (refactor: Reduce number of LoadChainstate parameters and return values by ryanofsky)
#25203 (logging: update to severity-based logging by jonatack)
#25193 (indexes: Read the locator’s top block during init, allow interaction with reindex-chainstate by mzumsande)
#24539 (Add a “tx output spender” index by sstone)
#21422 (Add feerate histogram to getmempoolinfo by kiminuo)

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.

in src/Makefile.test_fuzz.include:14 in ad862b8bbc outdated

 9@@ -10,7 +10,8 @@ EXTRA_LIBRARIES += \
10 TEST_FUZZ_H = \
11     test/fuzz/fuzz.h \
12     test/fuzz/FuzzedDataProvider.h \
13-    test/fuzz/util.h
14+    test/fuzz/util.h \
15+    test/fuzz/mempool_utils.h

MarcoFalke commented at 10:45 am on June 30, 2022:

nit: keep sorted with m before u?

dongcarl commented at 4:30 pm on June 30, 2022:

Fixed as of 2f8699a9423c048b1b36a13ea020e687dae245f2

dongcarl force-pushed on Jun 30, 2022

MarcoFalke removed the label Refactoring on Jul 1, 2022

DrahtBot added the label Refactoring on Jul 1, 2022

dongcarl force-pushed on Jul 1, 2022

dongcarl moved this from the "WIP PRs" to the "Ready for Review PRs" column in a project

glozow commented at 2:34 pm on July 7, 2022: member

Concept + Approach ACK

in src/mempool_args.cpp:38 in 930f023ea9 outdated

32@@ -33,5 +33,12 @@ void ApplyArgsManOptions(const ArgsManager& argsman, MemPoolOptions& mempool_opt
33 
34     if (auto hours = argsman.GetIntArg("-mempoolexpiry")) mempool_opts.expiry = std::chrono::hours{*hours};
35 
36+
37+    if (argsman.GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL)) {
38+        if (mempool_opts.persistence_path.empty()) mempool_opts.persistence_path = argsman.GetDataDirNet() / "mempool.dat";

theuni commented at 3:29 pm on July 7, 2022:

Why is this the only option that’s not clobbered by ApplyArgsManOptions?

dongcarl commented at 4:57 pm on July 7, 2022:

Mostly because the other options specifies a value that goes into mempool_opts, whereas -persistmempool is a boolean option.

Note that we use an empty persistence_path to indicate that we don’t want to persist the mempool

So the logic goes:

If we want to persist the mempool, and there’s no existing mempool path set, set persistence_path to <datadir>/ mempool.dat
If we don’t want to persist the mempool, set persistence_path to an empty path

in src/txmempool.cpp:1053 in a19cc42ca2 outdated

1047+
1048+    std::map<uint256, CAmount> mapDeltas;
1049+    std::vector<TxMempoolInfo> vinfo;
1050+    std::set<uint256> unbroadcast_txids;
1051+
1052+    static Mutex dump_mutex;

theuni commented at 3:40 pm on July 7, 2022:

This can probably move to CTxMemPool now as well in a follow-up?

dongcarl commented at 7:23 pm on July 7, 2022:

As a non-static or as a static? I think as a static it may prevent cases where 2 mempools are created with the same persistence path and try to dump at the same time. However, I think that’s perhaps somewhat nonsensical anyway. I don’t care either way.

theuni commented at 7:48 pm on July 7, 2022:

In that case I’d expect a lock at the filesystem level as opposed to a mutex. But sure, it’s not at all a priority, I was just curious because I assumed the reason it was static before is because it wasn’t well encapsulated.

dongcarl commented at 0:29 am on July 8, 2022:

Followup entry added in umbrella issue #24303

MarcoFalke commented at 1:15 pm on July 18, 2022:

This is used consistently in the codebase, see also DumpBanlist.

in src/txmempool.cpp:1109 in a90ec0817b outdated

1105@@ -1105,6 +1106,96 @@ bool CTxMemPool::DumpMempool(FopenFn mockable_fopen_function, bool skip_file_com
1106     return true;
1107 }
1108 
1109+void CTxMemPool::LoadMempool(std::function<bool(const CTransactionRef&, int64_t)> simple_atmp, FopenFn mockable_fopen_function)

theuni commented at 3:46 pm on July 7, 2022:

m_is_loaded/IsLoaded() doesn’t actually indicate whether or not the mempool was successfully, loaded, but rather if a load has been attempted and did not result in a catastrophic ShutdownRequested.

In that case, could these be renamed as a follow-up to be less confusing? Maybe TryLoadMempool and MempoolLoadTried or something?

dongcarl commented at 0:28 am on July 8, 2022:

Fixed!

in src/txmempool.cpp:1112 in a90ec0817b outdated

1105@@ -1105,6 +1106,96 @@ bool CTxMemPool::DumpMempool(FopenFn mockable_fopen_function, bool skip_file_com
1106     return true;
1107 }
1108 
1109+void CTxMemPool::LoadMempool(std::function<bool(const CTransactionRef&, int64_t)> simple_atmp, FopenFn mockable_fopen_function)
1110+{
1111+    _LoadMempool(simple_atmp, mockable_fopen_function);
1112+    SetIsLoaded(!ShutdownRequested());

theuni commented at 4:02 pm on July 7, 2022:

This is really confusing. _LoadMempool may return false if ShutdownRequested(), but that’s ignored here and it’s checked again.

It’s also not clear why ShutdownRequested() may have been called. Might it be coming from some low-level operation stemming from _LoadMempool itself? Or is it simple_atmp we’re worried about?

I see that you described the confusing semantics in the commit message, but I’m not convinced that it’s ok to double-down on the wonky behavior before improving it some :(

dongcarl commented at 0:29 am on July 8, 2022:

Followup entry added in umbrella issue #24303

in src/txmempool.cpp:1129 in ddf4920df5 outdated

1125@@ -1127,7 +1126,7 @@ bool CTxMemPool::_LoadMempool(std::function<bool(const CTransactionRef&, int64_t
1126     int64_t failed = 0;
1127     int64_t already_there = 0;
1128     int64_t unbroadcast = 0;
1129-    int64_t nNow = GetTime();
1130+    auto now = NodeClock::now();

theuni commented at 4:09 pm on July 7, 2022:

Any reason not to use a steady clock here since it’s an interval?

MarcoFalke commented at 4:29 pm on July 7, 2022:

Previously it was already using the NodeClock (just a different type), so this doesn’t change behaviour.

One reason to use NodeClock (system_clock + mockable) is to allow the test to mock the tx pool entries’ time.

I am not sure if steady_clock is always guaranteed to be based off of the system timestamp at boot. If it wasn’t this could lead to misinterpretation of timestamps of a persisted mempool.dat after a system reboot?

theuni commented at 4:40 pm on July 7, 2022:

I am not sure if steady_clock is always guaranteed to be based off of the system timestamp at boot. If it wasn’t this could lead to misinterpretation of timestamps of a persisted mempool.dat after a system reboot?

Ah, I didn’t catch that this persisted. In that case, you’re right of course.

dongcarl commented at 7:27 pm on July 7, 2022:

Closing, feel free to reopen if not resolved!

in src/validation.cpp:3867 in a90ec0817b outdated

3867-    if (!m_mempool->m_persistence_path.empty()) {
3868-        ::LoadMempool(*m_mempool, *this);
3869-    }
3870-    m_mempool->SetIsLoaded(!ShutdownRequested());
3871+    auto simple_atmp = [&](const CTransactionRef& tx, int64_t accept_time) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
3872+        return AcceptToMemoryPool(*this, tx, accept_time, /*bypass_limits=*/false, /*test_accept=*/false).m_result_type == MempoolAcceptResult::ResultType::VALID;

theuni commented at 4:17 pm on July 7, 2022:

Do we need all of AcceptToMemoryPool here? Without digging too much, I should think we might only need a subset, potentially skipping some of the uncaching/flushing logic?

dongcarl commented at 7:30 pm on July 7, 2022:

I’m not sure, and I don’t know enough about this logic to confidently say. I’d prefer to preserve as much existing behavior as possible for an important module like mempool, which is what I think this does.

Okay to leave as a followup for someone with more knowledge of this logic?

dongcarl commented at 0:29 am on July 8, 2022:

Followup entry added in umbrella issue #24303

theuni commented at 4:22 pm on July 7, 2022: member

Concept ACK and light review ACK.

I left a few comments/complaints about cleaning up some of the call/return semantics. I’d feel more comfortable with these changes if they didn’t preserve some of our current weirdness, but I don’t have a ton of concrete suggestions without really digging in.

dongcarl commented at 7:57 pm on July 7, 2022: contributor

For larger things like simplifying AcceptToMemoryPool and changing _LoadMempool’s semantics, I’m going to add extensive documentation of what I’ve found, and leave it for followups. These seem like delicate operations which can either slow down libbitcoinkernel progress or introduce subtle bugs. I think they’re better off as standalone efforts.

theuni commented at 8:12 pm on July 7, 2022: member

For larger things like simplifying AcceptToMemoryPool and changing _LoadMempool’s semantics, I’m going to add extensive documentation of what I’ve found, and leave it for followups. These seem like delicate operations which can either slow down libbitcoinkernel progress or introduce subtle bugs. I think they’re better off as standalone efforts.

I think that’s perfectly reasonable as long as nothing here gets more convoluted (arguably the atmp callback does that a little, but that’s reasonable enough), and the current status quo is fully understood and documented. +1 for more docs, thanks for that :)

dongcarl commented at 9:18 pm on July 7, 2022: contributor

Fixed

dongcarl force-pushed on Jul 8, 2022

dongcarl commented at 0:24 am on July 8, 2022: contributor

Push ddf4920df58bc6f84297abc9b31e5830237ea514 -> 8bf70da49cb0f79c7c09d126a37942a76fad7093

Add a functional test for savemempool when -persistmempool=0
IWYU src/mempool_args.cpp
Made savemempool work when -persistmempool=0
Rename LoadMempool -> TryLoadMempool
Rename m_is_loaded -> m_load_tried
Add a commit that makes mapNextTx private

MarcoFalke commented at 6:19 am on July 8, 2022: member

From CI:

FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\test_runner_₿_🏃_20220708_004922\\mempool_persist_175\\node0\\regtest\\mempool.dat' -> 'C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\test_runner_₿_🏃_20220708_004922\\mempool_persist_175\\node1\\regtest\\mempool.dat'

DrahtBot added the label Needs rebase on Jul 8, 2022

fanquake assigned glozow on Jul 8, 2022

dongcarl force-pushed on Jul 8, 2022

dongcarl commented at 5:34 pm on July 8, 2022: contributor

Push 8bf70da49cb0f79c7c09d126a37942a76fad7093 -> 4a4102d0986d88f548ae3ea3d5bcca10a2e598a2

Fixed: #25487 (comment)

in src/mempool_args.h:14 in ea750b1374 outdated

 9@@ -10,6 +10,9 @@ namespace kernel {
10 struct MemPoolOptions;
11 };
12 
13+/** Default for -persistmempool */
14+static constexpr bool DEFAULT_PERSIST_MEMPOOL{true};

ariard commented at 0:58 am on July 9, 2022:

more comment, “the mempool is saved on shutdown and load on restart”

dongcarl commented at 1:28 am on July 14, 2022:

Fixed in adf4a30d95

in src/txmempool.h:735 in cfb82402e5 outdated

734@@ -735,10 +735,10 @@ class CTxMemPool
735     void GetTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) const;
736 
737     /** @returns true if the mempool is fully loaded */
738-    bool IsLoaded() const;
739+    bool GetLoadTried() const;

ariard commented at 1:22 am on July 9, 2022:

comment could be updated to reflect it indicates load attempt not succes

ryanofsky commented at 10:37 pm on July 13, 2022:

re: #25487 (review)

In commit “scripted-diff: Rename m_is_loaded -> m_load_tried” (ae6bf8ed30d5b8511f22d3a8f16112b33804944f)

comment could be updated to reflect it indicates load attempt not succes

In case you do manually update the comment after the scripted diff, could also renamed the “bool loaded” argument below “bool load_tried”

dongcarl commented at 1:28 am on July 14, 2022:

Fixed in 82111033f3

in src/txmempool.h:828 in f7587c8e1e outdated

823+
824 private:
825+    /** Sets the current loaded state */
826+    void SetLoadTried(bool loaded);
827+
828+    bool LoadMempool(std::function<bool(const CTransactionRef&, int64_t)> simple_atmp, FopenFn mockable_fopen_function = fsbridge::fopen);

ariard commented at 1:28 am on July 9, 2022:

can get a comment

dongcarl commented at 1:29 am on July 14, 2022:

The newest version of this PR doesn’t have simple_atmp, and the new ::LoadMempool signature seem simple enough, feel free to reopen if still a problem!

in src/validation.cpp:3866 in f7587c8e1e outdated

3866     if (!m_mempool) return;
3867-    if (!m_mempool->m_persistence_path.empty()) {
3868-        ::LoadMempool(*m_mempool, *this);
3869-    }
3870-    m_mempool->SetLoadTried(!ShutdownRequested());
3871+    auto simple_atmp = [&](const CTransactionRef& tx, int64_t accept_time) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {

ariard commented at 1:37 am on July 9, 2022:

I guess simple_atmp in opposition to the more complex AcceptPackage / AcceptMultipleTransactions ? In that case single_tx_atmp might be more speaking, I don’t know.

dongcarl commented at 5:31 pm on July 13, 2022:

No longer relevant as of 80b303ea1a065602cf3f84d45b04dce95bfb6952

ariard commented at 1:37 am on July 9, 2022: member

Approach ACK

in test/functional/mempool_persist.py:149 in af62345b56 outdated

132@@ -133,6 +133,15 @@ def run_test(self):
133             self.nodes[2].syncwithvalidationinterfacequeue()  # Flush mempool to wallet
134             assert_equal(node2_balance, wallet_watch.getbalance())
135 
136+        mempooldat0 = os.path.join(self.nodes[0].datadir, self.chain, 'mempool.dat')
137+        mempooldat1 = os.path.join(self.nodes[1].datadir, self.chain, 'mempool.dat')
138+
139+        self.log.debug("Force -persistmempool=0 node1 to savemempool to disk via RPC")
140+        result1 = self.nodes[1].savemempool()

ryanofsky commented at 1:28 am on July 11, 2022:

In commit “test/mempool_persist: Test manual savemempool when -persistmempool=0” (af62345b560b7d47cd8c40823e08b352b4334fff)

Maybe assert the mempooldat1 file doesn’t exist before calling savemempool

dongcarl commented at 5:31 pm on July 13, 2022:

Done in c84390b741ab7b61c9f702d8b447c8cadc1257c8

in src/txmempool.cpp:1042 in 41c0d88c74 outdated

1038@@ -1036,6 +1039,73 @@ size_t CTxMemPool::DynamicMemoryUsage() const {
1039     return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 15 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(vTxHashes) + cachedInnerUsage;
1040 }
1041 
1042+bool CTxMemPool::DumpMempoolPath(const fs::path& dump_path, FopenFn mockable_fopen_function, bool skip_file_commit) const

ryanofsky commented at 1:49 am on July 11, 2022:

In commit “mempool: Make DumpMempool a CTxMemPool method” (41c0d88c7402a4af37aeda36652631fe4659c192)

It seems good that this PR is decoupling mempool persistence from validation and argsmanager code, but bad that this is now coupling mempool persistence to the CTxMemPool class itself, and giving the dump and load functions access to protected CTxMemPool members which they shouldn’t have access to.

Would suggest moving mempool dump and load functions from validation.cpp to a new txmempool_persist.cpp file, and keeping them as standalone functions, instead of moving them to txmempool.cpp and turning them into methods.

In general it is good to keep the amount of code that can access a classes’ private state minimal, and keep the purpose of a class focused, so classes don’t grow into unmaintainable monoliths over time like CWallet.

glozow commented at 3:47 pm on July 11, 2022:

^Mm, this makes a lot of sense to me.

theuni commented at 6:41 pm on July 11, 2022:

I think I agree with this as well, though I’m not very familiar with this code. Since dump/load kinda straddles CChainState and CTxMemPool neither seem like a great home, so standalone makes sense to me.

dongcarl commented at 7:07 pm on July 11, 2022:

~~Urgh I hate it when you’re right Ryan~~ Will do! 😆

dongcarl commented at 5:30 pm on July 13, 2022:

Done as of 80b303ea1a065602cf3f84d45b04dce95bfb6952

in src/txmempool.cpp:1057 in 41c0d88c74 outdated

1052+    static Mutex dump_mutex;
1053+    LOCK(dump_mutex);
1054+
1055+    {
1056+        LOCK(cs);
1057+        for (const auto &i : mapDeltas) {

ryanofsky commented at 1:58 am on July 11, 2022:

In commit “mempool: Make DumpMempool a CTxMemPool method” (41c0d88c7402a4af37aeda36652631fe4659c192)

This seems like a bug. Previously this code was iterating over pool.mapDeltas. Now it is iterating over the local mapDeltas variable above.

This could be fixed by changing the line to this->mapDeltas but I would recommend avoiding this problem by just moving this code, and not trying to change it. See previous comment, but I think it is better for dump and load to be standalone functions not member functions to make the CTxMemPool class cleaner and more maintainable.

glozow commented at 3:27 pm on July 11, 2022:

Good catch! It seems we only test prioritisation of transactions in the mempool, which is serialized alongside each transaction (TxMempoolInfo::nFeeDelta). Here’s a test for persistence of mapDeltas. It passes on master and fails on this branch.

theuni commented at 6:38 pm on July 11, 2022:

+1, nice catch!

dongcarl commented at 5:30 pm on July 13, 2022:

As of 80b303ea1a065602cf3f84d45b04dce95bfb6952, I rebased over #25592 which should test for this, and the bug no longer exists with the new approach

in src/txmempool.h:577 in ea750b1374 outdated

573@@ -573,6 +574,8 @@ class CTxMemPool
574 
575     const Limits m_limits;
576 
577+    const fs::path m_persistence_path{};

ryanofsky commented at 2:13 am on July 11, 2022:

In commit “mempool: Introduce m_persistence_path member” (ea750b1374fa0a49212b1f280660be7b2f6b9b2f)

I suspect it would simplify the PR and the code overall not to add a CTxMemPool::m_persistence_path member or an MemPoolOptions::persistence_path option. You could avoid the noise and statefulness and the twisty const fs::path dump_path{mempool.m_persistence_path.empty() ? MemPoolPersistencePath(args) : mempool.m_persistence_path}; logic by writing simple helper functions:

0bool MempoolPersist(const ArgsManager& args)
1{
2    return args.GetBoolArg("-persistmempool", DEFAULT_PERSIST_MEMPOOL);
3}
4
5fs::path MempoolPath(const ArgsManager& args)
6{
7    return args.GetDataDirNet() / "mempool.dat";
8}

And calling them on as needed from validation and RPC code without adding extra options and state.

theuni commented at 6:45 pm on July 11, 2022:

Mmm, this would simplify the code in question, though I personally prefer the pattern of parsing these upfront to avoid having to involve ArgsManager for something that seems like a property of CTxMemPool.

Edit: Thinking about this more and considering your idea to explicitly keep load/save out of CTxMemPool, I suppose the above isn’t really a great argument. In fact for RPC, one might argue that it’s weird that a path isn’t a required param.

dongcarl commented at 5:29 pm on July 13, 2022:

Implemented as of 80b303ea1a065602cf3f84d45b04dce95bfb6952, usage can be seen in fe185e6b38a7ef1d66d92aa18ba2b6ad9fef56c9 and 9c9ad8fee250fd2c17d3cd9b15333894d972c5a6.

Feel free to reopen if still a problem!

ryanofsky commented at 2:18 am on July 11, 2022: contributor

A lot of the changes seem good here, but I think I found a bug in the dump function, and I would suggest not burdening the CTxMemPool class with this persistence functionality, and instead moving the dump/load functions to a separate mempool_persist.cpp file so mempool code can be more focused and maintainable.

MarcoFalke referenced this in commit 01ae8d9cd2 on Jul 12, 2022

sidhujag referenced this in commit faf95a755c on Jul 12, 2022

test/mempool_persist: Test manual savemempool when -persistmempool=0 c84390b741

dongcarl force-pushed on Jul 13, 2022

DrahtBot removed the label Needs rebase on Jul 13, 2022

dongcarl force-pushed on Jul 13, 2022

dongcarl commented at 5:28 pm on July 13, 2022: contributor

Push 4a4102d0986d88f548ae3ea3d5bcca10a2e598a2 -> 80b303ea1a065602cf3f84d45b04dce95bfb6952

Rework PR to move {Load,Dump}Mempool to their own mempool_persist module instead of moving them to be methods of CTxMemPool

Note: there are a lot of code comments I still need to add, just wanted to push and have people take a look at the general direction, will add comments in the end.

dongcarl renamed this:
~~[kernel 3b/n] Make `{Dump,Load}Mempool` `CTxMemPool` methods, decouple from `ArgsManager`~~
[kernel 3b/n] Decouple `{Dump,Load}Mempool` from `ArgsManager`
on Jul 13, 2022

dongcarl force-pushed on Jul 13, 2022

dongcarl commented at 5:49 pm on July 13, 2022: contributor

Push 80b303ea1a065602cf3f84d45b04dce95bfb6952 -> fda55102298c5a9a280173e5a4a8e30e5943a447

Added commit 267cbd1bd40af4bf52a0905cafb303c8f2c2bfac

in src/node/mempool_persist_args.cpp:13 in fe185e6b38 outdated

 8+#include <util/system.h>
 9+#include <validation.h>
10+
11+namespace node {
12+
13+bool MempoolPersist(const ArgsManager& argsman)

jamesob commented at 7:26 pm on July 13, 2022:

https://github.com/bitcoin/bitcoin/pull/25487/commits/fe185e6b38a7ef1d66d92aa18ba2b6ad9fef56c9

If you happen to retouch… I found the naming of this function a little confusing - when looking at calls in the wild, it initially sounds like this is actually persisting the mempool. ShouldPersistMempool() might be a little clearer.

dongcarl commented at 8:09 pm on July 13, 2022:

Fixed in 04f72e9020

in src/init.cpp:252 in 07fcd8bc09 outdated

248@@ -249,7 +249,7 @@ void Shutdown(NodeContext& node)
249     node.addrman.reset();
250     node.netgroupman.reset();
251 
252-    if (node.mempool && node.mempool->IsLoaded() && MempoolPersist(*node.args)) {
253+    if (node.mempool && node.mempool->GetLoadTried() && MempoolPersist(*node.args)) {

jamesob commented at 7:31 pm on July 13, 2022:

(Note about whole commit https://github.com/bitcoin/bitcoin/pull/25487/commits/07fcd8bc092398964f510a625a90196485466448)

Could be scripted-diff but not a huge deal because the commit is very short.

dongcarl commented at 8:09 pm on July 13, 2022:

Fixed in ae6bf8ed30

in ci/test/06_script_b.sh:44 in fda5510229 outdated

40@@ -41,6 +41,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
41   CI_EXEC "python3 ${DIR_IWYU}/include-what-you-use/iwyu_tool.py"\
42           " src/compat"\
43           " src/init"\
44+          " src/kernel/mempool_persist.cpp"\

jamesob commented at 7:46 pm on July 13, 2022:

https://github.com/bitcoin/bitcoin/pull/25487/commits/fda55102298c5a9a280173e5a4a8e30e5943a447

Maybe worth adding the header file as well?

dongcarl commented at 7:56 pm on July 13, 2022:

Unfortunately iwyu_tool.py doesn’t accept header files as individual arguments, it does check the corresponding header file when a .cpp is specified tho. Feel free to reopen if still a problem!

jamesob approved

jamesob commented at 7:48 pm on July 13, 2022: member

ACK fda55102298c5a9a280173e5a4a8e30e5943a447 (jamesob/ackr/25487.1.dongcarl.kernel_3b_n_decouple_du)

Very straightforward now after a few rounds of review. Looks good!

Read through commits locally and built all.

dongcarl force-pushed on Jul 13, 2022

dongcarl commented at 8:08 pm on July 13, 2022: contributor

Push fda55102298c5a9a280173e5a4a8e30e5943a447 -> 6f018a1f908f264667b024e60bb2394fed48f05c

Addressed James’ reviews:
- #25487 (review)
- #25487 (review))

dongcarl force-pushed on Jul 13, 2022

dongcarl commented at 8:20 pm on July 13, 2022: contributor

Push 6f018a1f908f264667b024e60bb2394fed48f05c -> 2480e6823044b1fe6ba8fc84ee4434437a8c9df0

Fix me being a bit careless

jamesob approved

jamesob commented at 9:42 pm on July 13, 2022: member

reACK 2480e6823044b1fe6ba8fc84ee4434437a8c9df0 (jamesob/ackr/25487.2.dongcarl.kernel_3b_n_decouple_du)

https://github.com/bitcoin/bitcoin/pull/25487/commits/ae6bf8ed30d5b8511f22d3a8f16112b33804944f may be the prettiest scripted-diff I ever have seen.

ryanofsky approved

ryanofsky commented at 10:58 pm on July 13, 2022: contributor

Code review ACK 2480e6823044b1fe6ba8fc84ee4434437a8c9df0. Very simple and clean changes

dongcarl force-pushed on Jul 14, 2022

dongcarl commented at 1:27 am on July 14, 2022: contributor

Push 2480e68230 -> 3188375d36

Added more comments, addressing:

dongcarl force-pushed on Jul 14, 2022

ryanofsky approved

ryanofsky commented at 2:32 pm on July 14, 2022: contributor

Code review ACK 3a9dc8e675f41228cc49c3b8140c3ca39d94001f. Just some comments fixups since last review

in src/test/fuzz/validation_load_mempool.cpp:50 in 6780367717 outdated

42+
43     auto fuzzed_fopen = [&](const fs::path&, const char*) {
44         return fuzzed_file_provider.open();
45     };
46-    (void)LoadMempool(pool, g_setup->m_node.chainman->ActiveChainstate(), fuzzed_fopen);
47+    (void)chainstate.LoadMempool(MempoolPath(g_setup->m_args), fuzzed_fopen);

ryanofsky commented at 2:39 pm on July 14, 2022:

In commit “LoadMempool: Pass in load_path, stop using gArgs” (678036771720433bd789e68e43c0e250cd3706b8)

What’s the reason this fuzz test is changed to use chainstate.LoadMempool instead of kernel::LoadMempool? It seems to make test setup more fragile and LoadMempool/DumpMempool sequence a little less symmetric. Maybe extra test coverage is worth it though or there was a different reason previous code didn’t work. Just curious

dongcarl commented at 3:53 pm on July 14, 2022:

Added more context in c4249c1243981f42091950d1295c2be6bd4bf118, also added a followup to the project ticket to make loadmempool less footgunny (this footgun was existed prior to this PR)

dongcarl force-pushed on Jul 14, 2022

dongcarl commented at 3:46 pm on July 14, 2022: contributor

Push 3a9dc8e675 -> 7ae032e075

Merged fuzz/validation_load_mempool changes into the DummyChainstate commit, added git message for why calling LoadMempool via CChainState is more correct.

Also added an entry to libbitcoinkernel followup to make LoadMempool less foot-gunny.

ryanofsky approved

ryanofsky commented at 12:29 pm on July 15, 2022: contributor

Code review ACK 7ae032e075caa97c790603e768c7c02b80358968. Since last review commits were just rearranged internally and a fuzz change was better explained. No changes to the overall diff

glozow commented at 1:46 pm on July 15, 2022: member

light code review ACK 7ae032e075caa97c790603e768c7c02b80358968

in src/validation.cpp:4731 in 9c83269682 outdated

4727@@ -4726,7 +4728,9 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka
4728 
4729 bool DumpMempool(const CTxMemPool& pool, FopenFn mockable_fopen_function, bool skip_file_commit)
4730 {
4731-    int64_t start = GetTimeMicros();
4732+    using SteadyClock = std::chrono::steady_clock;

MarcoFalke commented at 2:30 pm on July 15, 2022:

9c832696826449236e88ff2fbb43d12d3d4ebc78 This can probably be added to util/time.h:

 0diff --git a/src/util/time.h b/src/util/time.h
 1index 0f87d66c2e..b01be93f83 100644
 2--- a/src/util/time.h
 3+++ b/src/util/time.h
 4@@ -24,6 +24,7 @@ struct NodeClock : public std::chrono::system_clock {
 5 };
 6 using NodeSeconds = std::chrono::time_point<NodeClock, std::chrono::seconds>;
 7 
 8+using SteadyClock = std::chrono::steady_clock;
 9 using SteadySeconds = std::chrono::time_point<std::chrono::steady_clock, std::chrono::seconds>;
10 using SteadyMilliseconds = std::chrono::time_point<std::chrono::steady_clock, std::chrono::milliseconds>;
11 using SteadyMicroseconds = std::chrono::time_point<std::chrono::steady_clock, std::chrono::microseconds>;

dongcarl commented at 4:28 pm on July 15, 2022:

Done in bd4407817e

in src/init.cpp:120 in 04f72e9020 outdated

111@@ -111,6 +112,8 @@ using node::CleanupBlockRevFiles;
112 using node::DEFAULT_PRINTPRIORITY;
113 using node::DEFAULT_STOPAFTERBLOCKIMPORT;
114 using node::LoadChainstate;
115+using node::MempoolPath;
116+using node::ShouldPersistMempool;

MarcoFalke commented at 2:32 pm on July 15, 2022:

04f72e90204c049d5d320388fda1f01e53417938: What is the point of those, when it is less code overall with them removed?

dongcarl commented at 3:32 pm on July 15, 2022:

Do you mean why have these using declarations instead of using node::MempoolPath, node::ShouldPersistMempool directly?

MarcoFalke commented at 3:39 pm on July 15, 2022:

Yes. If only one symbol requires them, it seems less code to just write the namespace out.

dongcarl commented at 4:32 pm on July 15, 2022:

In the longer term I think init.cpp will go in the node namespace so it’s probably less code change in the longer term to have the using declarations. In any case, probably not going to address it in this PR.

in src/Makefile.am:386 in 04f72e9020 outdated

380@@ -380,6 +381,7 @@ libbitcoin_node_a_SOURCES = \
381   node/context.cpp \
382   node/eviction.cpp \
383   node/interfaces.cpp \
384+  node/mempool_persist_args.cpp \

MarcoFalke commented at 2:34 pm on July 15, 2022:

04f72e90204c049d5d320388fda1f01e53417938: Add to iwyu CI?

dongcarl commented at 3:34 pm on July 15, 2022:

I didn’t do this most because IWYU insists that:

0node/mempool_persist_args.cpp should add these lines:
1#include <memory>         // for allocator

And I’m not sure why or if we need it… Does @ryanofsky know?

MarcoFalke commented at 3:39 pm on July 15, 2022:

Ok nvm then. Looks like an iwyu bug

in src/test/fuzz/validation_load_mempool.cpp:46 in c4249c1243 outdated

42+
43     auto fuzzed_fopen = [&](const fs::path&, const char*) {
44         return fuzzed_file_provider.open();
45     };
46-    (void)LoadMempool(pool, g_setup->m_node.chainman->ActiveChainstate(), fuzzed_fopen);
47+    (void)chainstate.LoadMempool(g_setup->m_args);

MarcoFalke commented at 2:38 pm on July 15, 2022:

c4249c1243981f42091950d1295c2be6bd4bf118: You are removing fuzzed_open. This will disable the fuzz tests.

Also there is a typo in the commit message “it definitely is differece”.

dongcarl commented at 4:28 pm on July 15, 2022:

Fixed in b857ac60d9

in src/node/blockstorage.cpp:14 in 679469ff57 outdated

10@@ -11,6 +11,7 @@
11 #include <flatfile.h>
12 #include <fs.h>
13 #include <hash.h>
14+#include <node/mempool_persist_args.h>

MarcoFalke commented at 2:52 pm on July 15, 2022:

c4249c1243981f42091950d1295c2be6bd4bf118: Why is this needed?

dongcarl commented at 4:30 pm on July 15, 2022:

Fixed in 06b88ffb8a

in src/node/blockstorage.cpp:896 in 679469ff57 outdated

893@@ -893,6 +894,6 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
894             return;
895         }
896     } // End scope of CImportingNow
897-    chainman.ActiveChainstate().LoadMempool(args);
898+    chainman.ActiveChainstate().LoadMempool(mempool_path);

MarcoFalke commented at 2:53 pm on July 15, 2022:

c4249c1243981f42091950d1295c2be6bd4bf118: I think you probably wanted to call ShouldPersistMempool(args) ? MempoolPath(args) : fs::path{} here? Which also makes more sense to me, as args is already available in this scope.

dongcarl commented at 4:04 pm on July 15, 2022:

I guess that depends on whether we want ThreadImport to be part of libbitcoinkernel or not. I’m operating under the assumption that we do want it to be part of libbitcoinkernel, so whenever there’s an opportunity to untangle it from ArgsManager, I take it.

in src/validation.h:418 in 3298c26002 outdated

414@@ -415,8 +415,6 @@ enum class CoinsCacheSizeState
415     OK = 0
416 };
417 
418-using FopenFn = std::function<FILE*(const fs::path&, const char*)>;

MarcoFalke commented at 2:55 pm on July 15, 2022:

3298c26002a24dd196348b9849e6bc0b2e5e56fd: Instead of moving this twice in the same pull, what about only moving it only once to the final place in one go?

dongcarl commented at 4:30 pm on July 15, 2022:

Moved once in b3267258b0

MarcoFalke commented at 2:57 pm on July 15, 2022: member

7ae032e075caa97c790603e768c7c02b80358968 📃

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 37ae032e075caa97c790603e768c7c02b80358968 📃
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgfUgwAkIj0v/a6TKMxsnCnOXt7Fx6T4Om/7AP2yIltNiFCbNSi3pK25loSjt0y
 8XojYPih/7ZrUggAqOANaRBg1nplwDqpiD7XjFaP9LFvXu1xkfEww1FHgXY2q25CI
 9V8xA69Oq2BLKfr3Zj8Lv5PQPq1dZgyaR3QmBBR7SscbSnte+SzsnOuVxmUdaCh4A
10koGKy7uFEQi2IkeeFOTippHuGkLYm2jN/8TwGzmWy27KaOkjRyezbHp1ozmimB+g
11/OdoV0Fve6Jzlv1ThIJeR9K8eCJhJLDC8nRecMwLehMsGOuoeJ8SozmbSihqXcFI
128VQ+WKn0WBfA0UAynC66QKPUBYthsQ5VXZ9QIS81J4Ktjkv2+CHCufyLX0mwxByy
13NSu8L4zBEpojnoQTDQX35tDuWMZYWK1wQ2gBmTBBMulF6Q+VLGVecsAUXg4mGnuX
14KkqC1aW6lLF//yYtcHpV/qPtBJ7Ym1yHJMR9kj5JUB8EFVaIZAnljgRjXYXy70mT
154NeqZUCC
16=PbsG
17-----END PGP SIGNATURE-----

MarcoFalke commented at 2:58 pm on July 15, 2022: member

Looks good, but I think there is a bug #25487 (review), which should be addressed or replied to before merge.

DumpMempool: Use std::chrono instead of weird int64_t arthmetics

This makes it so that DumpMempool doesn't depend on MICRO anymore

bd4407817e

DumpMempool: Pass in dump_path, stop using gArgs

Also introduce node::{ShouldPersistMempool,MempoolPath} helper functions
in node/mempool_persist_args.{h,cpp} which are used by non-kernel
DumpMempool callers to determine whether or not to automatically dump
the mempool and where to dump it to.

413f4bb52b

scripted-diff: Rename m_is_loaded -> m_load_tried

m_is_loaded/IsLoaded() doesn't actually indicate whether or not the
mempool was successfully, loaded, but rather if a load has been
attempted and did not result in a catastrophic ShutdownRequested.

-BEGIN VERIFY SCRIPT-
find_regex="\bm_is_loaded\b" \
    && git grep -l -E "$find_regex" \
        | xargs sed -i -E "s@$find_regex@m_load_tried@g"

find_regex="\bIsLoaded\b" \
    && git grep -l -E "$find_regex" \
        | xargs sed -i -E "s@$find_regex@GetLoadTried@g"

find_regex="\bSetIsLoaded\b" \
    && git grep -l -E "$find_regex" \
        | xargs sed -i -E "s@$find_regex@SetLoadTried@g"
-END VERIFY SCRIPT-

813962da0b

mempool: Improve comments for [GS]etLoadTried

Also change the param name for SetLoadTried to load_tried.

f9e8e5719f

mempool: Use NodeClock+friends for LoadMempool ae1e8e3756

dongcarl force-pushed on Jul 15, 2022

Move FopenFn to fsbridge namespace

[META] In a future commit in this patchset, it will be used by more than
       just validation, and it needs to align with fopen anyway.

b3267258b0

test/fuzz: Invoke LoadMempool via CChainState

Not only does this increase coverage, it is also more correct in that
when ::LoadMempool is called with a mempool and chainstate, it calls
AcceptToMemoryPool with just the chainstate.

AcceptToMemoryPool will then act on the chainstate's mempool via
CChainState::GetMempool, which may be different from the mempool
originally passed to ::LoadMempool. (In this fuzz test's case, it
definitely is different)

Also, move DummyChainstate to its own file since it's now used by the
validation_load_mempool fuzz test to replace CChainState's m_mempool.

b857ac60d9

LoadMempool: Pass in load_path, stop using gArgs

Also:
1. Have CChainState::LoadMempool and ::ThreadImport take in paths and
   pass it through untouched to LoadMempool.
2. Make LoadMempool exit early if the load_path is empty.
3. Adjust the call to ::ThreadImport in ::AppInitMain to correctly pass
   in an empty path if mempool persistence is disabled.

06b88ffb8a

Move DEFAULT_PERSIST_MEMPOOL out of libbitcoinkernel

It is no longer used by anything inside libbitcoinkernel, move it to
node/mempool_persist_args.h where it belongs.

aa30676541

Move {Load,Dump}Mempool to kernel namespace

Also:
1. Add the newly introduced kernel/mempool_persist.cpp to IWYU CI script
2. Add chrono mapping for iwyu

cb3e9a1e3f

dongcarl force-pushed on Jul 15, 2022

dongcarl commented at 4:28 pm on July 15, 2022: contributor

Push 7ae032e075caa97c790603e768c7c02b80358968 -> cb3e9a1e3f

Addressed:

MarcoFalke approved

MarcoFalke commented at 1:35 pm on July 18, 2022: member

ACK cb3e9a1e3f 🔒

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK cb3e9a1e3f 🔒
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgBIgv+I0xufL+4M1xQ03SLaGjEKhoV4ts5l8hXo1Md5hP8qXMKMoBz8GOzM6vX
 8ctVleKfacPJQP65lTM4JqaORnTY/CA1eZz59klYy6+W5TjxasH0Tj55oVRGjA1Be
 9LdJpc6x9I8B9nJfvcPM5FFcBo2y7QbOKDcZQtjqi6gxdoOHsteHEts3D2CPPfuXc
10UmpffGr8mQXyC6L3ISoSaMA19a4FyA8+LrhZTS2WRKvwRC55h8G53k6tIFk5HU/K
11T65bW0IgVyXT1XjhF94J2EjwTX9pLWUDEeJhib7rc42onmIBKhqCEU4AWConz8G9
12rGAJCkuuoPRTNwO26tmi2ZG2pBRSfGZot6AccKH57UAHgjX7vTB5Vs00IwXa84B6
13cHHzR/9SYUx9af+2ciyUn2BNmKktRiVeInYjGoDONoKpuT8emF0M/QgFAelYRu6u
14N9E7LKKYztGiKnxlXVxVA5AyvJGweCK2AhLU8QHA7d6jpB9ghuHFzl+7upBbgHPd
15raQhDcGj
16=8bBU
17-----END PGP SIGNATURE-----

ryanofsky approved

ryanofsky commented at 2:48 pm on July 18, 2022: contributor

Code review ACK cb3e9a1e3f8d72daaa361fc45dd853775e754b9d Main change is fuzzed_fopen fix, and there were some other cleanups.

glozow commented at 3:02 pm on July 18, 2022: member

re ACK cb3e9a1e3f8d72daaa361fc45dd853775e754b9d via git range-diff 7ae032e...cb3e9a1

glozow merged this on Jul 18, 2022

glozow closed this on Jul 18, 2022

glozow moved this from the "Ready for Review PRs" to the "Done or Closed or Rethinking" column in a project

sidhujag referenced this in commit 02896ae612 on Jul 18, 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

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

fanquake referenced this in commit c2f2abd0a4 on May 11, 2023

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

kittywhiskers referenced this in commit a42eadba91 on Jul 19, 2023

kittywhiskers referenced this in commit 2002b5f99c on Jul 19, 2023

kittywhiskers referenced this in commit f601b18f4a on Jul 23, 2023

kittywhiskers referenced this in commit 8eccf6c633 on Jul 23, 2023

kittywhiskers referenced this in commit 7c04cadcda on Jul 24, 2023

bitcoin locked this on Jul 26, 2023

[kernel 3b/n] Decouple {Dump,Load}Mempool from ArgsManager #25487

Conflicts

[kernel 3b/n] Decouple `{Dump,Load}Mempool` from `ArgsManager` #25487