rpc: improve submitpackage documentation and other improvements #29292

1. 
   stickies-v commented at 8:35 pm on January 22, 2024: contributor

   submitpackage requires the package to be topologically sorted with the child being the last element in the array, but this is not documented in the RPC method or the error messages.

   Also sneaking in some other minor improvements that I found while going through the code:

   • Informing the user that package needs to be an array of length between 1 and MAX_PACKAGE_COUNT is confusing when IsChildWithPackage() requires that the package size >= 2. Remove this check to avoid code duplication and sending a confusing error message.
   • fixups to the submitpackage examples
2. 
   DrahtBot commented at 8:35 pm on January 22, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	fjahr, instagibbs, glozow, achow101

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label RPC/REST/ZMQ on Jan 22, 2024
4. stickies-v force-pushed on Jan 22, 2024
5. in src/rpc/mempool.cpp:824 in 7e8d3591a1 outdated

   821         "Warning: successful submission does not mean the transactions will propagate throughout the network.\n"
   822         ,
   823         {
   824-            {"package", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of raw transactions.",
   825+            {"package", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of raw transactions.\n"
   826+                "The package must solely consist of a child and its parents. None of the parents may depend on eachother.\n"
   

   ---

   ---

   

   brunoerg commented at 9:11 pm on January 22, 2024:

   0                "The package must solely consist of a child and its parents. None of the parents may depend on each other.\n"
   

   ---

   

   stickies-v commented at 10:29 am on January 23, 2024:

   thx, fixed
6. stickies-v force-pushed on Jan 23, 2024
7. in src/rpc/mempool.cpp:864 in a77dc1bdbb outdated

   862         },
   863         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
   864         {
   865             const UniValue raw_transactions = request.params[0].get_array();
   866-            if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) {
   867+            if (raw_transactions.size() > MAX_PACKAGE_COUNT) {
   

   ---

   ---

   

   glozow commented at 11:54 am on January 24, 2024:

   Seems we are missing test coverage for this. Maybe add one to rpc_packages.py?

   ---

   

   glozow commented at 11:55 am on January 24, 2024:

   Also, why delete the empty vector case? That should throw a JSONRPCError too, no?

   ---

   

   stickies-v commented at 2:05 pm on January 24, 2024:

   Seems we are missing test coverage for this. Maybe add one to rpc_packages.py?

   I’ll have a look!

   Also, why delete the empty vector case? That should throw a JSONRPCError too, no?

   We throw a JSONRPCError here if package size < 2, so all this should do is remove the confusing error message that the array needs to be at least size 1 (when actually it’s size 2)

   ---

   

   stickies-v commented at 12:19 pm on January 25, 2024:

   Added test coverage: the test diff after changing the bounds checking shows how the error messages change

   ---

   

   glozow commented at 4:53 pm on February 2, 2024:

   thanks for adding a test :+1:
8. stickies-v force-pushed on Jan 25, 2024
9. in src/rpc/mempool.cpp:864 in 021ecfea06 outdated

   860@@ -861,9 +861,9 @@ static RPCHelpMan submitpackage()
   861         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
   862         {
   863             const UniValue raw_transactions = request.params[0].get_array();
   864-            if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) {
   865+            if (raw_transactions.size() > MAX_PACKAGE_COUNT) {
   

   ---

   ---

   

   glozow commented at 4:43 pm on February 2, 2024:

   Instead of the 021ecfea062151bce0d67bf1575fe7e5e1ee22db approach, I actually think must contain between 2 and MAX_PACKAGE_COUNT transactions would be a more helpful interface for users

   ---

   

   stickies-v commented at 9:59 am on February 10, 2024:

   I’ve thought about that approach but decided not to because:

   • even though it’s a helpful message, the user trying to submit a package of size 1 almost by definition means they don’t understand the package topology requirements. Therefore, I think pointing that out is more helpful so the user can read up on the documentation right away. (of course, we could provide more specific error messages during package topology validation, but that’s orthogonal to this PR)
   • we’d be performing the same validation (min package size of 2) twice, which is a bit of a code smell imo

   I don’t feel super strongly about this so I’m happy to go along with your suggestion if you feel don’t like the current approach after my explanation.

   ---

   

   fjahr commented at 11:58 am on February 19, 2024:

   Don’t we check for MAX_PACKAGE_COUNT later on as well (so also twice)? Either way, I am leaning slightly towards what @glozow asks, simply because it’s nicer for the users to give helpful feedback as early as we can.

   ---

   

   stickies-v commented at 9:41 am on March 1, 2024:

   I still prefer my deduplicated approach but since I’m okay with the alternative suggested here and it seems I’m alone in my view I’ve updated this in 1aa7403ac1506a2283c8553e3d2f645ed1cfc719 to reflect that.

   Don’t we check for MAX_PACKAGE_COUNT later on as well (so also twice)?

   We do, but I think that’s not the same - doing an early max array size check is helpful to protect against resource exhaustion.

10. in src/rpc/mempool.cpp:859 in b2fb55cabc outdated

    854@@ -855,8 +855,8 @@ static RPCHelpMan submitpackage()
    855             },
    856         },
    857         RPCExamples{
    858-            HelpExampleCli("testmempoolaccept", "[rawtx1, rawtx2]") +
    859-            HelpExampleCli("submitpackage", "[rawtx1, rawtx2]")
    860+            HelpExampleRpc("submitpackage", R"(["rawtx1", "rawtx2"])") +
    861+            HelpExampleCli("submitpackage", R"(["rawtx1", "rawtx2"])")
    

    ---

    ---

    

    glozow commented at 4:52 pm on February 2, 2024:

    I don’t really understand why b2fb55cabc76d842c58b51ff9c64126e6639d1bb is better - I’d interpret this as an array of literal strings “rawtx1”, “rawtx2” ?

    ---

    

    stickies-v commented at 9:52 am on February 10, 2024:

    The main point of b2fb55cabc76d842c58b51ff9c64126e6639d1bb is to remove the testmempoolaccept reference. I use the opportunity to align the string notation with the (I think) more recent usage of raw literal string notation, which is a bit clearer imo. Using quotation marks around the “variable” name seems consistent with every other instance I’ve found, e.g. testmempoolaccept, generateblock, combinerawtransaction and combinepsbt

    So I think these changes make sense?

    ---

    

    stickies-v commented at 11:35 pm on March 20, 2024:

    Marking as resolved for now, lmk if you still have concerns about this.

    ---

    

    instagibbs commented at 3:37 pm on May 6, 2024:

    if we’re lining up ala testmempoolaccept shouldn’t we be adding single quotes around the whole array as well?

    ---

    

    stickies-v commented at 11:25 pm on May 6, 2024:

    You’re right, force pushed to add single quotes, thanks.
11. stickies-v force-pushed on Feb 10, 2024
12. 
    stickies-v commented at 10:00 am on February 10, 2024: contributor

    Rebased to master to make CI green, no changes otherwise. Thanks for the review @glozow, addressed all comments.
13. in test/functional/rpc_packages.py:28 in f0a6324eca outdated

    24@@ -25,6 +25,9 @@
    25 )
    26 
    27 
    28+MAX_PACKAGE_COUNT = 25
    

    ---

    ---

    

    fjahr commented at 12:02 pm on February 19, 2024:

    nit: Might be good to stick this somewhere into the framework so it can be reused in other test, similar to the constants on top of test_framework/messages.py.

    ---

    

    stickies-v commented at 8:59 am on March 1, 2024:

    The constant is specific to packages and not used anywhere else. I think I’d prefer keeping it in here until we need to access it in a different test?
14. 
    fjahr commented at 12:07 pm on February 19, 2024: contributor

    utACK 5a7a7208554d09bd0a0bb8de9d4d94ed4146e739

    This is ok to merge as is but if you decide to address https://github.com/bitcoin/bitcoin/pull/29292/commits/9deb9104572b4543b7135c31fbf3a34cb1f371b6#r1476296848 I will quickly re-review.

15. stickies-v force-pushed on Mar 1, 2024
16. 
    stickies-v commented at 9:42 am on March 1, 2024: contributor

    Force pushed to incorporate the approach suggested in #29292 (review)

    Apologies for the slow follow-up while I was on holidays, thank you for the review!

17. fanquake requested review from instagibbs on May 6, 2024
18. fanquake requested review from fjahr on May 6, 2024
19. 
    fjahr commented at 1:50 am on May 6, 2024: contributor

    re-ACK 311f52371f37123f1f6186ac818db0741f643aed

    Apologies for even slower re-review :)

20. in test/functional/rpc_packages.py:346 in f0a6324eca outdated

    342@@ -340,6 +343,13 @@ def test_submitpackage(self):
    343         assert_raises_rpc_error(-25, "package topology disallowed", node.submitpackage, chain_hex)
    344         assert_equal(legacy_pool, node.getrawmempool())
    345 
    346+        assert_raises_rpc_error(-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [chain_hex[0]] * 0)
    

    ---

    ---

    

    instagibbs commented at 3:32 pm on May 6, 2024:

    found it weird

    0        assert_raises_rpc_error(-8, f"Array must contain between 1 and {MAX_PACKAGE_COUNT} transactions.", node.submitpackage, [])
    

    ---

    

    stickies-v commented at 11:26 pm on May 6, 2024:

    I quite liked the symmetry/progression of the * 0 notation but I don’t really care either way and agree that [] is readable as well, so addressed in force push.
21. test: add bounds checking for submitpackage RPC 17f74512f0
22. doc: rpc: submitpackage takes sorted array f9ece258aa
23. rpc: update min package size error message in submitpackage

    Currently, the only allowed package topology has a min size of 2.
    Update the error message to reflect that.

    1a875d4049
24. doc: rpc: fix submitpackage examples 78e52f663f
25. stickies-v force-pushed on May 6, 2024
26. 
    stickies-v commented at 11:27 pm on May 6, 2024: contributor

    Force pushed to address @instagibbs’s review, only very minor changes.
27. 
    fjahr commented at 3:51 am on May 7, 2024: contributor

    re-ACK 78e52f663f3e3ac86260b913dad777fd7218f210

    Trial diff to check: https://github.com/bitcoin/bitcoin/compare/311f52371f37123f1f6186ac818db0741f643aed..78e52f663f3e3ac86260b913dad777fd7218f210

28. 
    instagibbs commented at 1:30 pm on May 7, 2024: member

    ACK https://github.com/bitcoin/bitcoin/pull/29292/commits/78e52f663f3e3ac86260b913dad777fd7218f210

    only the suggested changes, verified via git range-diff master 311f523 78e52f6

29. 
    glozow commented at 4:35 pm on May 8, 2024: member

    utACK 78e52f663f3e3ac86260b913dad777fd7218f210
30. 
    achow101 commented at 10:32 pm on May 8, 2024: member

    ACK 78e52f663f3e3ac86260b913dad777fd7218f210
31. achow101 merged this on May 8, 2024
32. achow101 closed this on May 8, 2024

    ---

33. stickies-v deleted the branch on May 8, 2024

Contributors
stickies-v DrahtBot brunoerg glozow fjahr instagibbs achow101

Review Requested
fjahr

Labels
RPC/REST/ZMQ