docs: Update the explainer text for the listunspent RPC #22525

Fonta1n3 commented at 6:10 AM on July 22, 2021: none

Users may find it confusing to see that their utxo is labelled as spendable when they know for certain their wallet does not hold private keys.

This is a minor documentation fix for bitcoin-cli listunspent to clarify that native descriptor wallets always consider utxos to be spendable.

Fixes #22518.

fanquake added the label Docs on Jul 22, 2021

Fonta1n3 marked this as a draft on Jul 22, 2021

Fonta1n3 marked this as ready for review on Jul 22, 2021

Fonta1n3 renamed this:
~~Update the explainer text for `bitcoin-cli listunspent`~~
docs: Update the explainer text for `bitcoin-cli listunspent`
on Jul 22, 2021

in src/wallet/rpcwallet.cpp:2885 in eab0c131bc outdated

2881 | @@ -2882,7 +2882,7 @@ static RPCHelpMan listunspent()
2882 |                              {RPCResult::Type::NUM, "confirmations", "The number of confirmations"},
2883 |                              {RPCResult::Type::STR_HEX, "redeemScript", "The redeemScript if scriptPubKey is P2SH"},
2884 |                              {RPCResult::Type::STR, "witnessScript", "witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH"},
2885 | -                            {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output"},
2886 | +                            {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output. Native descriptor wallets will always return true irregardless of whether the wallet holds private keys for the output."},

unknown commented at 6:26 AM on July 22, 2021:

                            {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output. Native descriptor wallets will always return true regardless of the wallet having private keys for the output."},

Replaced 'irregardless' with 'regardless' because it looks weird and people have different opinions about its grammar: https://english.stackexchange.com/questions/1259/irregardless-vs-irrespective

Removed 'whether' from 'regardless of whether' and people have different opinions about it: https://english.stackexchange.com/questions/107183/is-regardless-of-whether-or-not-proper-grammar

ghost commented at 6:27 AM on July 22, 2021: none

Can also squash commits: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Fonta1n3 force-pushed on Jul 22, 2021

Fonta1n3 commented at 10:39 AM on July 22, 2021: none

Can also squash commits: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Done.

in src/wallet/rpcwallet.cpp:2885 in 7ac39cb8aa outdated

2881 | @@ -2882,7 +2882,7 @@ static RPCHelpMan listunspent()
2882 |                              {RPCResult::Type::NUM, "confirmations", "The number of confirmations"},
2883 |                              {RPCResult::Type::STR_HEX, "redeemScript", "The redeemScript if scriptPubKey is P2SH"},
2884 |                              {RPCResult::Type::STR, "witnessScript", "witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH"},
2885 | -                            {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output"},
2886 | +                            {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output. Native descriptor wallets will always return true irregardless of whether the wallet holds private keys for the output"},

jonatack commented at 4:16 PM on July 22, 2021:

Echoing @prayank23's very good feedback, which is simultaneously more correct and shorter.

                            {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output. Native descriptor wallets will always return true regardless of the wallet having private keys for the output."},

nit, as the addition is a full sentence, it makes sense to use sentence punctuation here.

Fonta1n3 commented at 12:52 AM on July 23, 2021: none

Ah sorry I misread that. Will make the change.

docs: update the help command for listunspent

Native descriptor wallets always consider outputs to be spendable. This should be made clear as it is an important distinction.

Update src/wallet/rpcwallet.cpp

Better grammar.

Co-authored-by: Jon Atack <jon@atack.com>

332192cea5

Fonta1n3 force-pushed on Jul 23, 2021

jarolrod commented at 4:57 AM on July 23, 2021: member

ACK 332192cea599666fd9f1038bd53163477ad49b8c

Looks good to me 🥃 This change is conceptually correct. This is the current behavior.

MarcoFalke renamed this:
~~docs: Update the explainer text for `bitcoin-cli listunspent`~~
docs: Update the explainer text for the listunspent RPC
on Jul 23, 2021

MarcoFalke added the label RPC/REST/ZMQ on Jul 23, 2021

flack commented at 2:04 PM on July 28, 2021: contributor

maybe as an alternative, this could be written something like this:

Whether we have the private keys to spend this output (in legacy wallets only, native descriptor wallets always return true).

Because it seems like this field only does something useful for legacy wallets (and will probably be deprecated once they are)

tryphe changes_requested

tryphe commented at 5:21 AM on July 30, 2021: contributor

Thanks for your first PR!

The message should have a line break after each chunk of 80(?) characters. See this one for example.

Small nit, I would prefer the added message to be: Native descriptor wallets will always return true. versus: Native descriptor wallets will always return true regardless of the wallet having private keys for the output.

The first one seems just as informative and more terse, since by definition, descriptor wallets don't have the keys. But maybe it's just personal preference.

Feel free to mark my comments as resolved as you wish.

tryphe commented at 5:29 AM on July 30, 2021: contributor

Hmm, that's weird. Some messages truncate at 80 or 100, but some go beyond 100. Perhaps a change was made specifically to this line and the width was ignored.

Maybe another reviewer knows what the correct width is? Does it not matter now?

Fonta1n3 commented at 1:51 AM on August 1, 2021: none

Thanks for your first PR!

The message should have a line break after each chunk of 80(?) characters. See this one for example.

Small nit, I would prefer the added message to be: Native descriptor wallets will always return true. versus: Native descriptor wallets will always return true regardless of the wallet having private keys for the output.

The first one seems just as informative and more terse, since by definition, descriptor wallets don't have the keys. But maybe it's just personal preference.

Feel free to mark my comments as resolved as you wish.

I agree its neater and more terse but it could be misinterpreted that all native descriptor wallets hold private keys if the help text only states Native descriptor wallets will always return true.

fanquake requested review from achow101 on Aug 12, 2021

achow101 commented at 5:19 PM on August 12, 2021: member

I feel like it would be better to explain what the consequences of something being spendable is rather than when the wallet would mark an UTXO as spendable. Spendable really means that the wallet will consider the UTXO for spending who performing coin selection and include_watchonly is false. Trying to then describe describe the actual behavior of when something is spendable I think is a futile task as it gets into the craziness of ismine. There's more to it than just private keys, scripts are also needed, and only certain scripts.

Even in legacy wallets, it is possible to create a case where you should be able to spend a UTXO because private keys are available, but the UTXO is marked as not spendable.

DrahtBot commented at 4:37 AM on August 26, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22798 (doc: Fix RPC result documentation by MarcoFalke)

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 commented at 8:31 AM on September 23, 2021: member

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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

DrahtBot added the label Needs rebase on Sep 23, 2021

Fonta1n3 commented at 11:01 AM on September 23, 2021: none

I feel like it would be better to explain what the consequences of something being spendable is rather than when the wallet would mark an UTXO as spendable. Spendable really means that the wallet will consider the UTXO for spending who performing coin selection and include_watchonly is false. Trying to then describe describe the actual behavior of when something is spendable I think is a futile task as it gets into the craziness of ismine. There's more to it than just private keys, scripts are also needed, and only certain scripts.

Even in legacy wallets, it is possible to create a case where you should be able to spend a UTXO because private keys are available, but the UTXO is marked

I agree the more accurate the text the better. My PR certainly makes an improvement on the existing text which is down right misleading. I do not have the understanding of the underlying functionality to add anything more useful here, if you have a suggestion I am all ears.

DrahtBot commented at 12:28 PM on December 22, 2021: member

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

MarcoFalke closed this on Apr 29, 2022

MarcoFalke commented at 2:22 PM on April 29, 2022: member

Closing for now. Let us know if it should be reopened.

DrahtBot locked this on Apr 29, 2023