rpc: add return message to savemempool RPC #23398
Conversation
|
Concept ACK |
lsilva01
force-pushed
the
add_return_message_savemempool
branch
from
8d619af to
653dbb1
Compare
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
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 |
|---|---|
 |
 |
brunoerg
left a comment
brunoerg
left a comment
There was a problem hiding this comment.
tACK 653dbb14186485deba5ff46853fd72fea2c0fb7f (MacOS 11.6)
Also, ran mempool_persist test successfully.
There was a problem hiding this comment.
tACK 653dbb1
(Windows 10)
PS C:\testspace\bitcoin\bin> .\bitcoin-cli.exe -datadir="E:" savemempool
{
"filename": "E:\\testnet3\\mempool.dat"
}
Will reACK if changes are made based on #23398 (comment)
lsilva01
force-pushed
the
add_return_message_savemempool
branch
from
653dbb1 to
eae8ca9
Compare
|
1b55103 addresses the scenario mentioned in #23398 (comment) |
lsilva01
force-pushed
the
add_return_message_savemempool
branch
2 times, most recently
from
ba2dc56 to
2901bc4
Compare
lsilva01
force-pushed
the
add_return_message_savemempool
branch
from
2901bc4 to
ce06412
Compare
lsilva01
force-pushed
the
add_return_message_savemempool
branch
from
ce06412 to
1e30cf1
Compare
|
reACK 1e30cf1 Changes since last review: https://github.com/bitcoin/bitcoin/compare/653dbb1..1e30cf1 |
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
reACK 1e30cf1
Changes since my last review:
- The added line
const NodeContext& node = EnsureAnyNodeContext(request.context);has been moved up belowconst CTxMemPool& mempool = EnsureAnyMemPool(request.context); - .string() has been replaced with
fs::PathToStringto 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.
Github-Pull: bitcoin#23398 Rebased-From: 09c3395
Github-Pull: bitcoin#23398 Rebased-From: 1e30cf1
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
Concept and approach ACK 1e30cf1
(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:
Line 4453 in 77a2f5d
Line 4582 in 77a2f5d
https://github.com/lsilva01/bitcoin/blob/1e30cf12706f0e7f843112452934bc9d02d759dc/src/rpc/blockchain.cpp#L2229
Does this PR also need mentioning in the release notes?
lsilva01
force-pushed
the
add_return_message_savemempool
branch
from
1e30cf1 to
aa1a4c9
Compare
|
Code review ACK aa1a4c9 |
aa1a4c9 Add file validation to savemempool RPC test (lsilva01) 871e64d Add filename to savemempool RPC result (lsilva01) Pull request description: 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. ACKs for top commit: laanwj: Code review ACK aa1a4c9 Tree-SHA512: e8b1dd0a8976e5eb15f7476c9651e492d2c621a67e0b726721fa7a2ae0ddd272ee28b87a2d0c650bd635e07fa96bdefe77bece4deb6486ef3ee9a4f83423a840
9b575f1 Improve fs::PathToString documentation (Russell Yanofsky) Pull request description: Add a developer note about avoiding `fs::PathToString` in RPCs, and improve some other `fs::PathToString` comments. Developer note might have been useful in two recent review comments: - bitcoin/bitcoin#23398 (comment) - bitcoin/bitcoin#23155 (comment) ACKs for top commit: laanwj: Documentation review ACK 9b575f1 jamesob: ACK bitcoin/bitcoin@9b575f1 prayank23: ACK bitcoin/bitcoin@9b575f1 hebasto: ACK 9b575f1 shaavan: ACK 9b575f1 Tree-SHA512: b8b3ecb6208c3897241e4f24dcec64fe7cf091bc79388862cf5f4b315cb8e804939981c4bed4c81dbff99ec9f750bad99015d0f04890704ac9df63c2a6719b6d
9b575f1 Improve fs::PathToString documentation (Russell Yanofsky) Pull request description: Add a developer note about avoiding `fs::PathToString` in RPCs, and improve some other `fs::PathToString` comments. Developer note might have been useful in two recent review comments: - bitcoin#23398 (comment) - bitcoin#23155 (comment) ACKs for top commit: laanwj: Documentation review ACK 9b575f1 jamesob: ACK bitcoin@9b575f1 prayank23: ACK bitcoin@9b575f1 hebasto: ACK 9b575f1 shaavan: ACK 9b575f1 Tree-SHA512: b8b3ecb6208c3897241e4f24dcec64fe7cf091bc79388862cf5f4b315cb8e804939981c4bed4c81dbff99ec9f750bad99015d0f04890704ac9df63c2a6719b6d
9 participants
Currently, if the user calls the
savemempoolRPC method, there is no way to knowwhere 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.pyto validate this new return message.