Skip to content
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

BIP 329: Add spendable state to outputs #1452

Merged
merged 3 commits into from
Jun 27, 2023

Conversation

sethforprivacy
Copy link
Contributor

This PR adds a spendable state to outputs in BIP 329, allowing wallets to properly export and import the spendable state of outputs.

This allows wallets to prevent users accidentally spending funds they previously marked as unspendable in wallets like Sparrow or Samourai that support coin control.

We plan to implement the usage of BIP 329 into Envoy shortly, and wanted to ensure that our users have a way to transfer spendable state in and out of the wallet and to other BIP 329-respecting wallets without issues, thus the PR.

Note: this does expand the scope of this BIP very slightly, almost making it a wallet "metadata" BIP instead of purely label-related. I'm happy to expand the PR accordingly by updating the title and copy to reflect that slight scope shift if desired but wanted to start the conversation off with the minimum changes necessary.

This PR currently would conflict with #1412, but I will rebase once that PR is merged to accommodate it and then can squash commits.

@sethforprivacy sethforprivacy changed the title BIP 329: Add spendable state to outputs BIP 329: Add spendable state to outputs May 15, 2023
@craigraw
Copy link
Contributor

LGTM. As you suggest I'd like to see #1412 merged first, but I've had enough requests for a spendable (or frozen) status in BIP329 that I think it's worth the slight scope creep. I don't believe this indicator fits well into any other specification, so having it in the wallet labels format makes the most sense to me as a wallet developer.

@sethforprivacy
Copy link
Contributor Author

LGTM. As you suggest I'd like to see #1412 merged first, but I've had enough requests for a spendable (or frozen) status in BIP329 that I think it's worth the slight scope creep. I don't believe this indicator fits well into any other specification, so having it in the wallet labels format makes the most sense to me as a wallet developer.

Would you prefer that I expanded this PR to include copy changes to make it clear it's not just labels anymore? Or would you prefer it to stay as-is besides the new key-value pair?

Happy to do the leg work expanding copy if you'd prefer it that way.

@craigraw
Copy link
Contributor

I think it's worth adding a line to the motivation yes - something along the lines of "In addition, many wallets allow unspent outputs to be frozen or made unspendable within the wallet. Since this wallet-related metadata is similar to labels and not captured elsewhere, it is also included in this format."

@sethforprivacy
Copy link
Contributor Author

I think it's worth adding a line to the motivation yes - something along the lines of "In addition, many wallets allow unspent outputs to be frozen or made unspendable within the wallet. Since this wallet-related metadata is similar to labels and not captured elsewhere, it is also included in this format."

Added with minor formatting tweaks in ab2f2b1. Thanks for the prompt feedback!

@craigraw
Copy link
Contributor

Noting that #1412 has now been merged, so this PR can be rebased.

@sethforprivacy
Copy link
Contributor Author

Noting that #1412 has now been merged, so this PR can be rebased.

Rebased, all set for merge now!

@craigraw
Copy link
Contributor

Working on this currently, and I was wondering about this default:

the <tt>spendable</tt> state, defaulting to <tt>true</tt> if none is found

Would it not be better for the importing wallet to simply not try to change the spendable status if no spendable property is found? Otherwise a wallet export that doesn't support this property (for any valid reason) may accidentally cause all frozen UTXOs to be unfrozen on import, simply as an unintended side effect of setting the label.

@craigraw
Copy link
Contributor

There's a further nuance too - should an export wish to set the spendable state only, it may unintentionally overwrite valid labels by including a label="" property. I suggest the following edits from line 49:

Each JSON object must only contain key/value pairs, defined as follows:

[table of properties]

[property reference]

Each JSON object must contain both type and ref properties. The label, origin and spendable properties are optional. If the label or spendable properties are omitted, the importing wallet should not alter these values. The origin property should only appear where type is tx, and the spendable property only where type is output.

@sethforprivacy
Copy link
Contributor Author

Agreed on both fronts, I hadn't considered that edge case for the spendable state, nor the JSON overwrites! Updating now.

@craigraw
Copy link
Contributor

LGTM. Implemented in Sparrow in sparrowwallet/sparrow@dbbeaf6.

@kallewoof kallewoof merged commit 1d15f3e into bitcoin:master Jun 27, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants