-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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: Properly document return values (submitblock, gettxout, getblocktemplate, scantxoutset) #20556
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing fa3cbac
Calling gettxout
with an invalid utxo returns no output using bitcoin-cli
, but returns null in the gui. I think the better behavior would be to return an empty json block like we do with other rpc commands such as listtransactions
This can be discussed in #18476 further. The goal of this pull is to simply document the return values as they are and always have been. |
Looks good. |
The (scripted) diff is only 50 lines. If there are any conflicts, it should be trivial to resolve. That the internal representation is a string could be a coincidence. The type is really VNUM (numeric) and not VSTR (string). I checked that the only conflict due to this scripted diff is #19002. |
Just an aside: there used to be talk of, at some point, of making This bled out but seeing them as separate types an abstraction level above JSON string/value distinction makes sense. |
Ok, dropped the scripted diff because it seemed controversial |
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concept ACK. one improvement requested, otherwise generally looks good. I checked most of the documented results actually match up.
Can be reviewed with --ignore-all-space
Can be reviewed with --ignore-all-space
Can be reviewed with --ignore-all-space
Can be reviewed with --ignore-all-space
Force pushed to address feedback. Should be easy to re-ACK with git range-diff |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK fa7ff07
- gettxout -> checked we get an empty result if UTXO is not found
- scantxoutset -> checked
abort
returns bool,status
returns empty orprogress
based on if scan is occurring, the existing docs apply to results fromstart
command. - getblocktemplate -> checked we get string result if
proposal
is rejected, didn't test an accepted proposal, but saw in the code that we would return null, saw the existing docs apply to other modes. - submitblock -> same, saw string for failure, looked at code for success
made sure all the help docs make sense & look good when called from command line
the changes from }, \n },
-> }},
kind of confuse me, but I assume its fine considering code compiles & help docs are showing up correctly.
utACK fa7ff07 Reviewed changes ignoring whitespace. |
Can be reviewed with --ignore-all-space Github-Pull: bitcoin#20556 Rebased-From: faa2059
Can be reviewed with --ignore-all-space Github-Pull: bitcoin#20556 Rebased-From: fabaccf
Can be reviewed with --ignore-all-space Github-Pull: bitcoin#20556 Rebased-From: fae542c
Can be reviewed with --ignore-all-space Github-Pull: bitcoin#20556 Rebased-From: fa7ff07
…template, scantxoutset) Summary: This is a backport of [[bitcoin/bitcoin#20556 | core#20556]] Test Plan: ``` src/bitcoin-cli help submitblock src/bitcoin-cli help gettxout src/bitcoin-cli help getblocktemplate src/bitcoin-cli help scantxoutset ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11337
Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476