rest: add verbose and mempool_sequence query params for mempool/contents #26207
Conversation
glozow
requested review from
brunoerg,
luke-jr and
stickies-v
and removed request for
luke-jr
brunoerg
left a comment
brunoerg
left a comment
There was a problem hiding this comment.
Concept ACK
You should update the doc (REST-interface.md) as well
@brunoerg would it make sense to update that file after there has been a release with this patch? I think that might confuse users who see those options in the doc but are unable to use them in any release. |
andrewtoth
force-pushed
the
rest-verbose-mempool
branch
from
e462602 to
5a30be2
Compare
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
Concept ACK - bringing it further in line with the getrawmempool RPC seems reasonable.
would it make sense to update that file after there has been a release with this patch?
To minimize maintenance I'd suggest labeling this pull with the 25.0 milestone and adding a small disclaimer to the rest-interface.md documentation that these 2 parameters are available only from 25.0, similar to
Line 56 in 3baa0f5
That way when this PR doesn't make it into 25.0 it'll be easy to track and update the docs.
src/rest.cpp
Outdated
There was a problem hiding this comment.
I think the most elegant solution here would be to introduce a HTTPRequest::GetQueryParameter<bool> specialization that converts to and returns a bool, or throws an err if the string can't be parsed. However, that would require some more fundamental changes to how we deal with errors (which unfortunately we return instead of throw) here. Flagging it here for follow-up, feel free to grab it if you're keen.
There was a problem hiding this comment.
Strong concept ACK on introducing a HTTPRequest::GetQueryParameter<bool>, it would be more elegant than using string ("true"/"false"), but I know it would require a lot of more work and maybe could be done in a follow-up.
There was a problem hiding this comment.
I can definitely do that in a follow-up PR after this is merged.
andrewtoth
force-pushed
the
rest-verbose-mempool
branch
from
fd4c6dc to
c35c7d0
Compare
@stickies-v I don't think I have permissions to add labels. Otherwise I have addressed all feedback from yourself and @brunoerg . |
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
ACK c35c7d091 (for 25.0)
I don't think I have permissions to add labels.
The maintainers should be able to add the 25.0 label when they see this, otherwise you can always send a message on #bitcoin-core-dev IRC. It's not super urgent though, just would be a bit easier to track I think.
andrewtoth
force-pushed
the
rest-verbose-mempool
branch
from
c35c7d0 to
b534c54
Compare
andrewtoth
force-pushed
the
rest-verbose-mempool
branch
from
b534c54 to
8cc41a1
Compare
|
Can you please rebase to latest master to unblock the (unrelated) failing Win64 CI test? |
andrewtoth
force-pushed
the
rest-verbose-mempool
branch
from
8cc41a1 to
1ff5d61
Compare
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
re-ACK 1ff5d61
I verified that the only changes since c35c7d0 are suggested documentation updates to REST-interface.md.
pablomartin4btc
left a comment
pablomartin4btc
left a comment
There was a problem hiding this comment.
tested ACK 1ff5d61.
I've performed manual tests getting the node started with -rest, comparing results between the rest api call using curl with the different combinations of params and values vs. calling the getrawtransacion rpc command (using bitcoin-cli), which is essentially what the python functional tests do.
It works as expected and it's consistent with rpc.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
ACK 1ff5d61 |
…ams for mempool/contents 1ff5d61 doc: add mempool/contents rest verbose and mempool_sequence args (Andrew Toth) 52a31dc tests: mempool/contents verbose and mempool_sequence query params tests (Andrew Toth) a518fff rest: add verbose and mempool_sequence query params for mempool/contents (Andrew Toth) Pull request description: The verbose mempool json response can get very large. This adds an option to return the non-verbose response of just the txids. It is identical to the rpc response so the diff here is minimal. This also adds the mempool_sequence parameter for rpc consistency. Verbose defaults to true to remain backwards compatible. It uses query parameters to be compatible with the efforts in bitcoin#25752. ACKs for top commit: achow101: ACK 1ff5d61 stickies-v: re-ACK [1ff5d61](bitcoin@1ff5d61) pablomartin4btc: tested ACK 1ff5d61. Tree-SHA512: 1bf08a7ffde2e7db14dc746e421feedf17d84c4b3f1141e79e36feb6014811dfde80e1d8dbc476c15ff705de2d3c967b3081dcd80536d76b7edf888f1a92e9d1
…ams for mempool/contents 1ff5d61 doc: add mempool/contents rest verbose and mempool_sequence args (Andrew Toth) 52a31dc tests: mempool/contents verbose and mempool_sequence query params tests (Andrew Toth) a518fff rest: add verbose and mempool_sequence query params for mempool/contents (Andrew Toth) Pull request description: The verbose mempool json response can get very large. This adds an option to return the non-verbose response of just the txids. It is identical to the rpc response so the diff here is minimal. This also adds the mempool_sequence parameter for rpc consistency. Verbose defaults to true to remain backwards compatible. It uses query parameters to be compatible with the efforts in bitcoin#25752. ACKs for top commit: achow101: ACK 1ff5d61 stickies-v: re-ACK [1ff5d61](bitcoin@1ff5d61) pablomartin4btc: tested ACK 1ff5d61. Tree-SHA512: 1bf08a7ffde2e7db14dc746e421feedf17d84c4b3f1141e79e36feb6014811dfde80e1d8dbc476c15ff705de2d3c967b3081dcd80536d76b7edf888f1a92e9d1
6 participants
The verbose mempool json response can get very large. This adds an option to return the non-verbose response of just the txids. It is identical to the rpc response so the diff here is minimal. This also adds the mempool_sequence parameter for rpc consistency. Verbose defaults to true to remain backwards compatible.
It uses query parameters to be compatible with the efforts in #25752.