Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues:
Fix all issues by adding a new RPC.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ajtowns, glozow, achow101 |
| Concept ACK | kevkevinpal, instagibbs, michaelfolkson, svanstaa, kristapsk |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Hi @MarcoFalke
More than 5 tests are failing
742+ "mempool state."},
743+ },
744+ RPCArgOptions{.oneline_description = "\"options\""}},
745+ },
746+ RPCResult{RPCResult::Type::OBJ, "", "", std::vector<RPCResult>{}},
747+ RPCExamples{HelpExampleCli("importmempool", "") + HelpExampleRpc("importmempool", "")},
would it make sense to add a ./pathto/mempool.dat to the example param because right now when I type
bitcoin-cli importmempool
I get
0Examples:
1> bitcoin-cli importmempool
2> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "importmempool", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
This might be confusing
0 RPCExamples{HelpExampleCli("importmempool", "./pathto/mempool.dat") + HelpExampleRpc("importmempool", "./pathto/mempool.dat")},
Concept ACK 846ff37fe7ac2a5b4f855faaadb3434158ed6364 (need to check latest commit)
bitcoin-cli createwallet "testwallet"
bitcoin-cli getnewaddress
bitcoin-cli generateto address 6 <previous commands output>
bitcoin-cli generate 20 (did this a couple times)
bitcoin-cli getbalance (validate you have a balance)
bitcoin-cli getnewaddress
bitcoin-cli send '{"<above output>": 0.1}' null "unset" null '{"fee_rate": 1}' (did this a few times)
shutdown bitcoind
copy mempool.dat from datadir to different location
delete mempool.dat from datadir
restart bitcoind
bitcoin-cli importmempool ./mempool.dat
Saw this in the logs
2023-04-13T19:12:01Z Imported mempool transactions from disk: 5 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
got {} as response from cli
Might test again and check the exact values
did git diff ./datadir/regtest/mempool.dat mempool.dat and it gave me no changes in the vim editor to see
but
diff ./datadir/regtest/mempool.dat mempool.dat gave me
Binary files ./datadir/regtest/mempool.dat and ./mempool.dat differ
not sure if that matters
737+ "Warning: Importing untrusted metadata may corrupt the mempool "
738+ "state."},
739+ {"apply_unbroadcast_set", RPCArg::Type::BOOL, RPCArg::Default{false},
740+ "Whether to apply the unbroadcast set metadata from the mempool file."
741+ "Warning: Importing untrusted metadata may corrupt the "
742+ "mempool state."},
"Warning: Importing untrusted metadata may lead to unexpected issues and undesirable bugs. Do not set this bool unless you understand exactly what it does."?
731+ RPCArg::Type::OBJ,
732+ RPCArg::Optional::OMITTED,
733+ "",
734+ {
735+ {"apply_fee_delta_priority", RPCArg::Type::BOOL, RPCArg::Default{false},
736+ "Whether to apply the fee delta metadata from the mempool file. "
prioritisetransaction here explicitly?
23- Chainstate& active_chainstate,
24- fsbridge::FopenFn mockable_fopen_function = fsbridge::fopen);
25+struct ImportMempoolOptions {
26+ fsbridge::FopenFn mockable_fopen_function{fsbridge::fopen};
27+ bool apply_fee_delta_priority{true};
28+ bool apply_unbroadcast_set{true};
Should this also include the ability to override the (added to the mempool) nTime data with the current time (or an explicitly nominated time)?
Having a child transaction with an earlier nTime than its parent transaction might be confusing. I think the only way that can currently happen is with system/mock time shenanigans.
718@@ -719,6 +719,56 @@ static RPCHelpMan getmempoolinfo()
719 };
720 }
721
722+static RPCHelpMan importmempool()
723+{
724+ return RPCHelpMan{
725+ "importmempool",
726+ "Import the file and attempt to add its contents to the mempool.\n"
Change “the file” to “a mempool.dat file” ?
Maybe reference the “savemempool” rpc as a way of updating mempool.dat with the current mempool?
-persistmempool
163+ assert_equal({}, self.nodes[0].importmempool(mempooldat0))
164+ assert_equal(len(self.nodes[0].getrawmempool()), 7)
165+ fees = self.nodes[0].getmempoolentry(txid=last_txid)["fees"]
166+ assert_equal(fees["base"], fees["modified"])
167+ assert_equal({}, self.nodes[0].importmempool(mempooldat0, {"apply_fee_delta_priority": True, "apply_unbroadcast_set": True}))
168+ fees = self.nodes[0].getmempoolentry(txid=last_txid)["fees"]
71@@ -73,7 +72,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active
72 file >> nFeeDelta;
73
74 CAmount amountdelta = nFeeDelta;
75- if (amountdelta) {
76+ if (amountdelta && opts.apply_fee_delta_priority) {
77 pool.PrioritiseTransaction(tx->GetHash(), amountdelta);
PrioritiseTransaction applies the delta on top, so if txmempool already has a tx prioritised + this mempool.dat contains priority for it, this will stack them instead of rewriting the existing priority. I don’t know if this is problematic, but probably good to keep in mind? (Wrote a test)
use_current_time defaulting to true? As far as i understand, this would set all the broadcast times of the transactions in mempool.dat to the current time, effectively extending their lifetimes, to the point that (time-) expired txns would be dumped into the network again.
So I would suggest defaulting the value to false, unless there is some rationale behind it that is not obvious.
Also while we’re sneaking PR review club questions on here what do people here use this for? I’m assuming it is useful for mempool dev work (specific details would be interesting) but I can’t imagine it is common for a user to want this.
Concept ACK (the RPC approach seems superior assuming people are currently importing mempools without a RPC being available)
The use case would be wanting to sync both the chain and the mempool to include everything/most known by the network or one of your other nodes. This is trivially possible for the chain, however for the mempool it is not.
One option would be to use the mempool P2P message type, or add a new message type. This would likely require lifting some restrictions or adding others to make it usable in a normal setting and still prevent privacy/DoS concerns. To what extent the existing mempool message type behavior can be modified will need to be considered in light of the discussion about its removal.
Another option would be to use a new RPC to import a mempool. In light of people potentially loading untrusted mempool data, I opted to use the same mempool acceptance defaults that would be picked as if the transactions were synced/submitted over P2P.
158@@ -159,6 +159,15 @@ def run_test(self):
159 assert self.nodes[0].getmempoolinfo()["loaded"]
160 assert_equal(len(self.nodes[0].getrawmempool()), 0)
161
162+ self.log.debug("Import mempool at runtime to node0.")
163+ assert_equal({}, self.nodes[0].importmempool(mempooldat0))
164+ assert_equal(len(self.nodes[0].getrawmempool()), 7)
165+ fees = self.nodes[0].getmempoolentry(txid=last_txid)["fees"]
166+ assert_equal(fees["base"], fees["modified"])
167+ assert_equal({}, self.nodes[0].importmempool(mempooldat0, {"apply_fee_delta_priority": True, "apply_unbroadcast_set": True}))
Approach ACK I think here since “apply_unbroadcast_set”: True` option is added, we should test mempool unbroadcast set list, else there would be no need for the second option.
0 assert_equal({}, self.nodes[0].importmempool(mempooldat0, {"apply_fee_delta_priority": True, "apply_unbroadcast_set": True}))
1 assert_equal(2, self.nodes[0].getmempoolinfo()['unbroadcastcount'])
ThreadImport has finished? I tried importing another mempool before the mempool from the datadir had finished loading, and I didn’t encounter any problems on first sight - but I’m not sure how useful that would be and I guess the final result would be dependent on races between the two mempools being loaded in parallel?
Should the rpc be executable before the mempool load from ThreadImport has finished?
I’d say yes, because the transaction may also arrive via the P2P interface before the persisted mempool is loaded. (The RPC interface is designed to mimic the P2P one).
However, a better check may be to refuse loading a mempool while in IBD? It seems better to error early instead of silently throwing away transactions that can not be applied to the current tip, because it is too old.
4079@@ -4079,7 +4080,10 @@ void PruneBlockFilesManual(Chainstate& active_chainstate, int nManualPruneHeight
4080 void Chainstate::LoadMempool(const fs::path& load_path, FopenFn mockable_fopen_function)
4081 {
4082 if (!m_mempool) return;
4083- ::ImportMempool(*m_mempool, load_path, *this, mockable_fopen_function);
4084+ ImportMempool(*m_mempool, load_path, *this,
4085+ ImportMempoolOptions{
4086+ .mockable_fopen_function = std::move(mockable_fopen_function),
ImportMempoolOptions::mockable_fopen_function isn’t actually used anywhere since LoadMempool overrides it. Maybe drop LoadMempool entirely and call kernel::ImportMempool directly from init.cpp and the fuzz test?
762+ const fs::path load_path{fs::u8path(request.params[0].get_str())};
763+ const UniValue& use_current_time{request.params[1]["use_current_time"]};
764+ const UniValue& apply_fee_delta{request.params[1]["apply_fee_delta_priority"]};
765+ const UniValue& apply_unbroadcast{request.params[1]["apply_unbroadcast_set"]};
766+ kernel::ImportMempoolOptions opts{
767+ .use_current_time = use_current_time.isNull() ? true : use_current_time.get_bool(),
Might be worth adding a helper for the isNull ? default : get_native() pattern?
0template<typename T>
1T RPCDefaultArg(const UniValue&, T default);
2template<>
3bool RPCDefaultArg(const UniValue& uv, bool default) { return uv.isNull() ? default : uv.get_bool(); }
4
5const auto use_current_time = RPCDefaultArg<bool>(request.params[1]["use_current_time"], true);
The 3-line function is only called once outside of tests, so it is
clearer to inline it.
Also, clarify the LoadMempool doxygen.
This allows optional named arguments with default values.
test_importmempool_union contributed by glozow
Co-authored-by: glozow <gloriajzhao@gmail.com>
