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

Fonta1n3 · 2021-07-22T06:10:18Z

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.

ghost · 2021-07-22T06:26:09Z

src/wallet/rpcwallet.cpp

@@ -2882,7 +2882,7 @@ static RPCHelpMan listunspent()
                            {RPCResult::Type::NUM, "confirmations", "The number of confirmations"},
                            {RPCResult::Type::STR_HEX, "redeemScript", "The redeemScript if scriptPubKey is P2SH"},
                            {RPCResult::Type::STR, "witnessScript", "witnessScript if the scriptPubKey is P2WSH or P2SH-P2WSH"},
-                            {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output"},
+                            {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."},


Suggested change

{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."},

{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 · 2021-07-22T06:27:50Z

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

Fonta1n3 · 2021-07-22T10:39:29Z

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

Done.

src/wallet/rpcwallet.cpp

Fonta1n3 · 2021-07-23T00:52:35Z

Ah sorry I misread that. Will make the change.

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>

jarolrod

ACK 332192c

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

flack · 2021-07-28T14:04:19Z

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

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 · 2021-07-30T05:29:06Z

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 · 2021-08-01T01:51:38Z

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.

achow101 · 2021-08-12T17:19:23Z

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 · 2021-08-26T04:37:41Z

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 · 2021-09-23T08:31:05Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Fonta1n3 · 2021-09-23T11:01:18Z

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 · 2021-12-22T12:28:41Z

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.

maflcko · 2022-04-29T14:22:04Z

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

fanquake added the Docs label Jul 22, 2021

Fonta1n3 marked this pull request as draft July 22, 2021 06:12

Fonta1n3 marked this pull request as ready for review July 22, 2021 06:16

Fonta1n3 mentioned this pull request Jul 22, 2021

0.21.1 native descriptor wallet showing incorrect value for spendable #22518

Open

Fonta1n3 changed the title ~~Update the explainer text for bitcoin-cli listunspent~~ docs: Update the explainer text for bitcoin-cli listunspent Jul 22, 2021

ghost reviewed Jul 22, 2021

View reviewed changes

Fonta1n3 force-pushed the master branch from fe86f79 to 7ac39cb Compare July 22, 2021 10:38

jonatack reviewed Jul 22, 2021

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

docs: update the help command for listunspent

332192c

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>

Fonta1n3 force-pushed the master branch from f301527 to 332192c Compare July 23, 2021 01:10

jarolrod reviewed Jul 23, 2021

View reviewed changes

maflcko changed the title ~~docs: Update the explainer text for bitcoin-cli listunspent~~ docs: Update the explainer text for the listunspent RPC Jul 23, 2021

maflcko added the RPC/REST/ZMQ label Jul 23, 2021

tryphe suggested changes Jul 30, 2021

View reviewed changes

fanquake requested a review from achow101 August 12, 2021 05:05

fanquake linked an issue Aug 12, 2021 that may be closed by this pull request

0.21.1 native descriptor wallet showing incorrect value for spendable #22518

Open

DrahtBot mentioned this pull request Aug 25, 2021

doc: Fix RPC result documentation #22798

Merged

DrahtBot added the Needs rebase label Sep 23, 2021

maflcko closed this Apr 29, 2022

bitcoin locked and limited conversation to collaborators Apr 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Fonta1n3 commented Jul 22, 2021 •

edited by fanquake

ghost Jul 22, 2021

ghost commented Jul 22, 2021

Fonta1n3 commented Jul 22, 2021

Fonta1n3 commented Jul 23, 2021

jarolrod left a comment

flack commented Jul 28, 2021

tryphe left a comment •

edited

tryphe commented Jul 30, 2021 •

edited

Fonta1n3 commented Aug 1, 2021

achow101 commented Aug 12, 2021 •

edited

DrahtBot commented Aug 26, 2021 •

edited

DrahtBot commented Sep 23, 2021

Fonta1n3 commented Sep 23, 2021

DrahtBot commented Dec 22, 2021

maflcko commented Apr 29, 2022

	{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."},
	{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."},

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

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

Conversation

Fonta1n3 commented Jul 22, 2021 • edited by fanquake

ghost Jul 22, 2021

Choose a reason for hiding this comment

ghost commented Jul 22, 2021

Fonta1n3 commented Jul 22, 2021

Fonta1n3 commented Jul 23, 2021

jarolrod left a comment

Choose a reason for hiding this comment

flack commented Jul 28, 2021

tryphe left a comment • edited

Choose a reason for hiding this comment

tryphe commented Jul 30, 2021 • edited

Fonta1n3 commented Aug 1, 2021

achow101 commented Aug 12, 2021 • edited

DrahtBot commented Aug 26, 2021 • edited

Conflicts

DrahtBot commented Sep 23, 2021

Fonta1n3 commented Sep 23, 2021

DrahtBot commented Dec 22, 2021

maflcko commented Apr 29, 2022

Fonta1n3 commented Jul 22, 2021 •

edited by fanquake

tryphe left a comment •

edited

tryphe commented Jul 30, 2021 •

edited

achow101 commented Aug 12, 2021 •

edited

DrahtBot commented Aug 26, 2021 •

edited