rpc: improve submitpackage documentation and other improvements #29292

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

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.

DrahtBot added the label RPC/REST/ZMQ on Jan 22, 2024

stickies-v force-pushed on Jan 22, 2024

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:

                "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

stickies-v force-pushed on Jan 23, 2024

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:

stickies-v force-pushed on Jan 25, 2024

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.

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.

stickies-v force-pushed on Feb 10, 2024

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.

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?

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.

stickies-v force-pushed on Mar 1, 2024

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!

fanquake requested review from instagibbs on May 6, 2024

fanquake requested review from fjahr on May 6, 2024

fjahr commented at 1:50 AM on May 6, 2024: contributor

re-ACK 311f52371f37123f1f6186ac818db0741f643aed

Apologies for even slower re-review :)

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

        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.

test: add bounds checking for submitpackage RPC 17f74512f0

doc: rpc: submitpackage takes sorted array f9ece258aa

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

doc: rpc: fix submitpackage examples 78e52f663f

stickies-v force-pushed on May 6, 2024

stickies-v commented at 11:27 PM on May 6, 2024: contributor

Force pushed to address @instagibbs's review, only very minor changes.

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

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

glozow commented at 4:35 PM on May 8, 2024: member

utACK 78e52f663f3e3ac86260b913dad777fd7218f210

achow101 commented at 10:32 PM on May 8, 2024: member

ACK 78e52f663f3e3ac86260b913dad777fd7218f210

achow101 merged this on May 8, 2024

achow101 closed this on May 8, 2024

stickies-v deleted the branch on May 8, 2024

bitcoin locked this on May 8, 2025