Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: improve submitpackage documentation and other improvements #29292

Merged

Conversation

stickies-v
Copy link
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
Copy link
Contributor

DrahtBot commented Jan 22, 2024

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.

src/rpc/mempool.cpp Outdated Show resolved Hide resolved
@stickies-v stickies-v force-pushed the 2024-01/patch-submitpackage-docs branch from 7e8d359 to a77dc1b Compare January 23, 2024 10:29
src/rpc/mempool.cpp Outdated Show resolved Hide resolved
@stickies-v stickies-v force-pushed the 2024-01/patch-submitpackage-docs branch from a77dc1b to b2fb55c Compare January 25, 2024 12:16
src/rpc/mempool.cpp Outdated Show resolved Hide resolved
src/rpc/mempool.cpp Outdated Show resolved Hide resolved
@stickies-v stickies-v force-pushed the 2024-01/patch-submitpackage-docs branch from b2fb55c to 5a7a720 Compare February 10, 2024 10:00
@stickies-v
Copy link
Contributor Author

stickies-v commented Feb 10, 2024

Rebased to master to make CI green, no changes otherwise. Thanks for the review @glozow, addressed all comments.

Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 5a7a720

This is ok to merge as is but if you decide to address 9deb910#r1476296848 I will quickly re-review.

src/rpc/mempool.cpp Outdated Show resolved Hide resolved
test/functional/rpc_packages.py Show resolved Hide resolved
@stickies-v stickies-v force-pushed the 2024-01/patch-submitpackage-docs branch from 5a7a720 to 311f523 Compare March 1, 2024 08:59
@stickies-v
Copy link
Contributor Author

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

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

@fanquake fanquake requested review from instagibbs and fjahr May 6, 2024 01:16
@fjahr
Copy link
Contributor

fjahr commented May 6, 2024

re-ACK 311f523

Apologies for even slower re-review :)

test/functional/rpc_packages.py Outdated Show resolved Hide resolved
src/rpc/mempool.cpp Outdated Show resolved Hide resolved
@stickies-v stickies-v force-pushed the 2024-01/patch-submitpackage-docs branch from 311f523 to 78e52f6 Compare May 6, 2024 23:24
@stickies-v
Copy link
Contributor Author

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

@fjahr
Copy link
Contributor

fjahr commented May 7, 2024

re-ACK 78e52f6

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

@instagibbs
Copy link
Member

ACK 78e52f6

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

Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 78e52f6

@achow101
Copy link
Member

achow101 commented May 8, 2024

ACK 78e52f6

@achow101 achow101 merged commit 4300325 into bitcoin:master May 8, 2024
16 checks passed
@stickies-v stickies-v deleted the 2024-01/patch-submitpackage-docs branch May 8, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants