Skip to content

doc: corrected lockunspent rpc quoting#31275

Closed
Talej wants to merge 1 commit into
bitcoin:masterfrom
Talej:listunspentrpctypo
Closed

doc: corrected lockunspent rpc quoting#31275
Talej wants to merge 1 commit into
bitcoin:masterfrom
Talej:listunspentrpctypo

Conversation

@Talej

@Talej Talej commented Nov 12, 2024

Copy link
Copy Markdown

bitcoin-cli help lockunspent RPC example shows:

As a JSON-RPC call
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "lockunspent", "params": [false, "[{\"txid\":\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\",\"vout\":1}]"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

Including quotes around the transactions argument. Using the RPC in this way fails:

curl --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "lockunspent", "params": [false, "[{\"txid\":\"6f244af8e2162138369fc736a673f85ead59ad88fb5a1edd6e03f81b33c537b0\",\"vout\":1}]"]}' -H 'content-type: text/plain;' http://127.0.0.1:18443/wallet/locktest

{"result":null,"error":{"code":-3,"message":"Expected type array, got string"},"id":"curltest"}

This change corrects the documentation to remove the additional quotes so the example works as intended:

curl --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "lockunspent", "params": [false, [{"txid":"6f244af8e2162138369fc736a673f85ead59ad88fb5a1edd6e03f81b33c537b0","vout":0}]]}' -H 'content-type: text/plain;' http://127.0.0.1:18443/wallet/locktest

{"result":true,"error":null,"id":"curltest"}

@DrahtBot

DrahtBot commented Nov 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31275.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK rkrux, achow101
Concept NACK sipa
Stale ACK luke-jr

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24539 (Add a "tx output spender" index by sstone)

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.

@DrahtBot DrahtBot added the Docs label Nov 12, 2024
@Talej Talej marked this pull request as ready for review November 12, 2024 02:45
@Talej Talej changed the title doc: corrected listunspent rpc quoting doc: corrected lockunspent rpc quoting Nov 12, 2024
@Talej Talej force-pushed the listunspentrpctypo branch from c1e2c48 to 70ae8ca Compare November 12, 2024 02:52
@fanquake

fanquake commented Nov 12, 2024

Copy link
Copy Markdown
Member

The same kind of quoting is used in multiple places in this file, so if it is incorrect / needs updating, all instances should be changed at the same time.

@Talej

Talej commented Nov 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

Corrected additional RPC examples for gettxspendingprevout, createrawtransaction, signrawtransactionwithkey, addmultisigaddress & listunspent

All relevant changes should be complete here

@DrahtBot DrahtBot mentioned this pull request Nov 14, 2024
Merged
luke-jr
luke-jr approved these changes Jan 8, 2025
View reviewed changes

@luke-jr luke-jr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Commit messages leave a bit to be desired, but the code and output looks correct

crACK 1f3f5c049b4080ecaf30604fd22d65aa0fc4af45

@fanquake

fanquake commented Feb 20, 2025

Copy link
Copy Markdown
Member

Please squash your commits. You can improve the commit message at the same time. i.e "doc: ...."

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025
@Talej @luke-jr
corrected lockunspent rpc quoting
599a081 
Github-Pull: bitcoin#31275
Rebased-From: 70ae8ca8b7435a38416c4eb469b3cdad6fb87b4f
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025
@Talej @luke-jr
further RPC example corrections
2f7f2fa 
Github-Pull: bitcoin#31275
Rebased-From: 1f3f5c049b4080ecaf30604fd22d65aa0fc4af45
@Talej Talej force-pushed the listunspentrpctypo branch from 1f3f5c0 to 49ffbc6 Compare February 27, 2025 07:09
@Talej

Talej commented Feb 27, 2025

Copy link
Copy Markdown
Author

Please squash your commits. You can improve the commit message at the same time. i.e "doc: ...."

Done!

@fanquake

fanquake commented May 13, 2025

Copy link
Copy Markdown
Member

@Talej Can you rebase this?

@Talej
doc: corrected lockunspent, gettxspendingprevout, createrawtransactio…
7e93e29 
…n, signrawtransactionwithkey & listunspent rpc quoting
@Talej Talej force-pushed the listunspentrpctypo branch from 49ffbc6 to 7e93e29 Compare May 17, 2025 00:52
@Talej

Talej commented May 17, 2025

Copy link
Copy Markdown
Author

@Talej Can you rebase this?

All done!

@achow101

achow101 commented Oct 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

ACK 7e93e29

Edit: Retracting ack, agree with #31275 (comment)

@DrahtBot DrahtBot requested a review from luke-jr October 27, 2025 17:46
rkrux
rkrux approved these changes Nov 3, 2025
View reviewed changes

@rkrux rkrux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

crACK 7e93e29

Can update the PR title now that more RPCs are updated.

@sipa

sipa commented Nov 10, 2025

Copy link
Copy Markdown
Member

I wanted to verify if all invocations of HelpExampleRpc produce valid JSON, so I tested by adding a UniValue::read inside the function. The following RPCs all fail it still (with this PR):

  • getblockfrompeer
  • importmempool
  • getindexinfo
  • addnode
  • addconnection
  • sendmsgtopeer
  • createwalletdescriptor
  • restorewallet
  • listlabels
  • loadwallet
  • unloadwallet

I think this means we either need to:

  • Get rid of the curl example codes in the RPC help texts entirely, or
  • Add testing to actually enforce they're correct.

@sipa

sipa commented Dec 8, 2025

Copy link
Copy Markdown
Member

Weak Concept NACK.

Given how broken the examples are, I think we should fix them throughout and add testing to prevent regressions, or get rid of them entirely.

@sipa sipa removed their request for review December 11, 2025 19:07
@DrahtBot DrahtBot mentioned this pull request Jan 25, 2026
Merged
@DrahtBot

DrahtBot commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@sedited

sedited commented Mar 14, 2026

Copy link
Copy Markdown
Contributor

There's been no comment from the author here in a while and given the NACK, I don't think reviewers are likely to still approve this change. A comprehensive solution also seems better to me. The formatting pretty much never changes, so a linter could maybe be implemented for that?

The PR hasn't made progress, so I am going to close this. Feel free to open another PR that takes the review comments into consideration.

@sedited sedited closed this Mar 14, 2026
@maflcko maflcko mentioned this pull request Mar 16, 2026
Open
@0xjc65eth 0xjc65eth mentioned this pull request May 28, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@achow101 achow101 Awaiting requested review from achow101
@murchandamus murchandamus Awaiting requested review from murchandamus
@sr-gi sr-gi Awaiting requested review from sr-gi
@danielabrozzoni danielabrozzoni Awaiting requested review from danielabrozzoni
@luke-jr luke-jr Awaiting requested review from luke-jr
2 more reviewers
@rkrux rkrux rkrux approved these changes
@Madxgame Madxgame Madxgame approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Docs Needs rebase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Talej @DrahtBot @fanquake @achow101 @sipa @sedited @luke-jr @rkrux @Madxgame