rpc: add return message to savemempool RPC #23398

pull lsilva01 wants to merge 2 commits into bitcoin:master from lsilva01:add_return_message_savemempool changing 2 files +13 −3
  1. lsilva01 commented at 6:29 pm on October 30, 2021: contributor

    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.

  2. DrahtBot added the label RPC/REST/ZMQ on Oct 30, 2021
  3. in src/rpc/blockchain.cpp:2227 in 8d619af8bc outdated
    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());
    


    MarcoFalke commented at 7:19 am on October 31, 2021:
    Would be nice to use request.context.node.args and not the global.

    lsilva01 commented at 11:06 pm on October 31, 2021:
    Done.
  4. ghost commented at 8:13 am on October 31, 2021: none
    Concept ACK
  5. lsilva01 force-pushed on Oct 31, 2021
  6. lsilva01 requested review from MarcoFalke on Oct 31, 2021
  7. shaavan approved
  8. shaavan commented at 1:45 pm on November 1, 2021: contributor

    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
    Master PR
  9. brunoerg approved
  10. brunoerg commented at 11:16 am on November 2, 2021: member

    tACK 653dbb14186485deba5ff46853fd72fea2c0fb7f (MacOS 11.6)

    Also, ran mempool_persist test successfully.

  11. in src/rpc/blockchain.cpp:2226 in 653dbb1418 outdated
    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);
    


    luke-jr commented at 5:14 pm on November 2, 2021:
    Completely hypothetical scenario at the moment, but if there is no node context, we probably shouldn’t throw an error here since the dump has presumably already succeeded.

    lsilva01 commented at 7:05 pm on November 2, 2021:
    Done in 1b55103
  12. unknown approved
  13. unknown commented at 6:13 pm on November 2, 2021: none

    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)

  14. lsilva01 force-pushed on Nov 2, 2021
  15. lsilva01 commented at 7:07 pm on November 2, 2021: contributor
    1b55103 addresses the scenario mentioned in #23398 (review)
  16. in src/rpc/blockchain.cpp:2229 in eae8ca9ac2 outdated
    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;
    


    MarcoFalke commented at 7:21 pm on November 2, 2021:

    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.


    lsilva01 commented at 9:43 pm on November 2, 2021:
    Checked that EnsureAnyMemPool calls EnsureAnyNodeContext(context). Changed the code accordingly.
  17. lsilva01 force-pushed on Nov 2, 2021
  18. lsilva01 force-pushed on Nov 2, 2021
  19. in src/rpc/blockchain.cpp:2226 in 2901bc4306 outdated
    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);
    


    luke-jr commented at 9:06 pm on November 2, 2021:
    Maybe just move this to the top with EnsureAnyMemPool…

    lsilva01 commented at 9:22 pm on November 2, 2021:
    Done in 0f6f763
  20. lsilva01 force-pushed on Nov 2, 2021
  21. in src/rpc/blockchain.cpp:2218 in ce064128bd outdated
    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);
    


    luke-jr commented at 9:24 pm on November 2, 2021:
    Sorry, I mean the line with the EnsureNodeContext use…

    lsilva01 commented at 9:47 pm on November 2, 2021:

    I’m not sure which line you’re referring to.

    This code no longer calls EnsureAnyNodeContext because EnsureAnyMemPool already does,


    luke-jr commented at 1:16 am on November 3, 2021:

    That’s an internal implementation detail of EnsureAnyMemPool. We also generally avoid pointers where nullptr is unacceptable.

    0    const NodeContext& node = EnsureAnyNodeContext(request.context);
    

    lsilva01 commented at 2:47 am on November 3, 2021:
    Done in 09c3395 . Thanks for suggestion.
  22. in src/rpc/blockchain.cpp:2229 in ce064128bd outdated
    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());
    


    luke-jr commented at 1:20 am on November 3, 2021:

    We’ve started using fs::PathToString instead of .string()

    See #22937


    lsilva01 commented at 2:47 am on November 3, 2021:
    Done in 09c3395 . Thanks for suggestion.

    ryanofsky commented at 2:49 am on November 3, 2021:

    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());
    
  23. lsilva01 force-pushed on Nov 3, 2021
  24. shaavan approved
  25. shaavan commented at 6:15 pm on November 3, 2021: contributor

    reACK 1e30cf12706f0e7f843112452934bc9d02d759dc

    Changes since my last review:

    • The added line const NodeContext& node = EnsureAnyNodeContext(request.context); has been moved up below const CTxMemPool& mempool = EnsureAnyMemPool(request.context);
    • .string() has been replaced with 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.

  26. luke-jr referenced this in commit 27bbab1345 on Nov 4, 2021
  27. luke-jr referenced this in commit c8346cd6a2 on Nov 4, 2021
  28. theStack approved
  29. theStack commented at 2:23 am on November 7, 2021: member

    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?

  30. in src/rpc/blockchain.cpp:2229 in 09c3395bc5 outdated
    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"));
    


    ryanofsky commented at 7:01 pm on November 8, 2021:

    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.


    lsilva01 commented at 4:13 pm on November 9, 2021:
    Thanks for review. Done in 871e64d
  31. ryanofsky approved
  32. ryanofsky commented at 7:03 pm on November 8, 2021: member
    Code review ACK 1e30cf12706f0e7f843112452934bc9d02d759dc, but better to avoid PathToString here (see below)
  33. Add filename to savemempool RPC result 871e64d22f
  34. Add file validation to savemempool RPC test aa1a4c9204
  35. lsilva01 force-pushed on Nov 9, 2021
  36. laanwj commented at 12:56 pm on November 10, 2021: member
    Code review ACK aa1a4c920495076ed5b5e3f05cb64d644bab6810
  37. laanwj merged this on Nov 10, 2021
  38. laanwj closed this on Nov 10, 2021

  39. sidhujag referenced this in commit 4cc8ba5d22 on Nov 10, 2021
  40. fanquake referenced this in commit b869a784ef on Nov 17, 2021
  41. sidhujag referenced this in commit 867659eaf6 on Nov 17, 2021
  42. DrahtBot locked this on Nov 10, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me