Currently, if the user calls the savemempool RPC method, there is no way to know
where the file was created (unless the user knows internal implementation details).
This PR adds a return message stating the file name and path where the mempool was saved and changes mempool_persist.py to validate this new return message.
2222@@ -2219,7 +2223,10 @@ static RPCHelpMan savemempool()
2223 throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
2224 }
2225
2226- return NullUniValue;
2227+ UniValue ret(UniValue::VOBJ);
2228+ ret.pushKV("filename", (gArgs.GetDataDirNet() / "mempool.dat").string());
request.context.node.args and not the global.
ACK 653dbb14186485deba5ff46853fd72fea2c0fb7f
Allowing the user to know the location where the mempool.dat file is saved as soon as it is saved is a beneficial addition. This allows a non-tech-savvy user to see where the file is located easily. At the same time, the message is not too verbose to make a user who doesn’t prefer a lot of text uncomfortable.
I was able to test this PR on Ubuntu 20.04 successfully. The file location was displayed correctly. The new help message clearly explains the new workings. Also, I was able to run the mempool_persist.py test successfully.
Screenshots:
| Master | PR |
|---|---|
 |
 |
tACK 653dbb14186485deba5ff46853fd72fea2c0fb7f (MacOS 11.6)
Also, ran mempool_persist test successfully.
2222@@ -2219,7 +2223,12 @@ static RPCHelpMan savemempool()
2223 throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
2224 }
2225
2226- return NullUniValue;
2227+ NodeContext& node = EnsureAnyNodeContext(request.context);
tACK https://github.com/bitcoin/bitcoin/commit/653dbb14186485deba5ff46853fd72fea2c0fb7f
(Windows 10)
0PS C:\testspace\bitcoin\bin> .\bitcoin-cli.exe -datadir="E:" savemempool
1{
2 "filename": "E:\\testnet3\\mempool.dat"
3}
Will reACK if changes are made based on #23398 (review)
2222@@ -2219,7 +2223,16 @@ static RPCHelpMan savemempool()
2223 throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
2224 }
2225
2226- return NullUniValue;
2227+ auto node_context = util::AnyPtr<NodeContext>(request.context);
2228+
2229+ if (!node_context) {
2230+ return NullUniValue;
How could this happen? If a mempool exists, a node context must have existed as well. See EnsureAnyMemPool above.
This is dead and confusing code.
EnsureAnyMemPool calls EnsureAnyNodeContext(context).
Changed the code accordingly.
2222@@ -2219,7 +2223,12 @@ static RPCHelpMan savemempool()
2223 throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
2224 }
2225
2226- return NullUniValue;
2227+ auto node_context = util::AnyPtr<NodeContext>(request.context);
2214@@ -2211,6 +2215,8 @@ static RPCHelpMan savemempool()
2215 {
2216 const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
2217
2218+ auto node_context = util::AnyPtr<NodeContext>(request.context);
I’m not sure which line you’re referring to.
This code no longer calls EnsureAnyNodeContext because EnsureAnyMemPool already does,
That’s an internal implementation detail of EnsureAnyMemPool. We also generally avoid pointers where nullptr is unacceptable.
0 const NodeContext& node = EnsureAnyNodeContext(request.context);
2224@@ -2219,7 +2225,10 @@ static RPCHelpMan savemempool()
2225 throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
2226 }
2227
2228- return NullUniValue;
2229+ UniValue ret(UniValue::VOBJ);
2230+ ret.pushKV("filename", ((*node_context).args->GetDataDirNet() / "mempool.dat").string());
Good catch! This code would do a silent locale conversion on windows after we stop using boost.
Since JSON strings must be UTF-8, would suggest using .u8string(). (PathToString is meant for internal use and string<->byte roundtrips and is not guaranteed to be UTF-8.)
0ret.pushKV("filename", fs::path((*node_context).args->GetDataDirNet() / "mempool.dat").u8string());
reACK 1e30cf12706f0e7f843112452934bc9d02d759dc
Changes since my last review:
const NodeContext& node = EnsureAnyNodeContext(request.context); has been moved up below const CTxMemPool& mempool = EnsureAnyMemPool(request.context);fs::PathToString to match the latest usage convention.Both these changes make code more structured while also following proper usage guidelines. These modifications do not change the behavior of this PR, so I agree with these changes.
Concept and approach ACK 1e30cf12706f0e7f843112452934bc9d02d759dc (tested with OpenBSD 7.0)
Would be nice if the path to mempool.dat could be somehow deduplicated in the future (though it’s unlikely to ever change), with this PR it will be three instances: https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/validation.cpp#L4453 https://github.com/bitcoin/bitcoin/blob/77a2f5d30c5ecb764b8a7c098492e1f5cdec90f0/src/validation.cpp#L4582 https://github.com/lsilva01/bitcoin/blob/1e30cf12706f0e7f843112452934bc9d02d759dc/src/rpc/blockchain.cpp#L2229
Does this PR also need mentioning in the release notes?
2224@@ -2219,7 +2225,10 @@ static RPCHelpMan savemempool()
2225 throw JSONRPCError(RPC_MISC_ERROR, "Unable to dump mempool to disk");
2226 }
2227
2228- return NullUniValue;
2229+ UniValue ret(UniValue::VOBJ);
2230+ ret.pushKV("filename", fs::PathToString(node.args->GetDataDirNet() / "mempool.dat"));
In commit “Add filename to savemempool RPC result” (09c3395bc5efc0a969548e1d694a75458058342c)
Should replace fs::PathToString(node.args->GetDataDirNet() / "mempool.dat") with (node.args->GetDataDirNet() / "mempool.dat").u8string(). The PathToString and u8string functions do the same thing right now, but u8string is meant to be used for strings which need to be UTF8 (required to produce valid JSON), and PathToString is meant for internal use and can return paths which are not UTF-8.
PathToString here (see below)
