-
Notifications
You must be signed in to change notification settings - Fork 5.9k
BIP Draft: Silent Payment Output Script Descriptors #2047
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
base: master
Are you sure you want to change the base?
Conversation
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.
Looks sound and high quality for an initial draft. I don't see the BIP 2 specified mail list discussion for this proposal? (Edit: added to the PR description.)
|
Thanks for the reminder - I've sent the mailing list post and added the link in the BIP. Also, the spaces after commas in the descriptors have been removed to be consistent with BIP 380-387. |
|
Thanks for working on this! |
I'm not strongly opposed to this, but given the cost of scanning for each additional label I'm reluctant to add it - it may make a wallet synchronization trivially impractical in a non-obvious way simply by accidentally specifying a large range. |
|
Maybe with adding a note discouraging wallet to export a range? |
|
There has been a second request for label ranges, and as I've said I'm not strongly opposed to adding this. I suggest the following syntax - a hyphen between two integers represents an inclusive range:
I think this is intuitive, compact and allows mixing sparse labels with ranges. There is slightly more parsing logic required now, and ranges will need to be validated (start < end), but this seems to be a comprehensive solution. |
It makes sense |
Scanning cost is no longer the argument right? Do we still want arbitrary labels or do we want them to be consecutive? This would make it easier to parse in my opinion. See label set scanning vs bip scanning benchmarks in : bitcoin-core/secp256k1#1765 (comment) |
|
cc: @josibake, @RubenSomsen as authors of Silent Payments, @achow101 as author of most Descriptor BIPs. |
|
Label ranges in the style
I don't think the parsing is complex enough to justify limiting labels to be consecutive only. There is a scanning cost (for light clients at least) and restricting potential use cases doesn't seem like the right approach. |
I previously thought it would be better only to specify the max label in the descriptor to keep it simple, but |
Ye make sense if we can get enough to agree I guess we need to revert some changes for #2026 too and add label ranges to stay compatible (unless we want to force descriptors ofc). |
murchandamus
left a comment
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.
This looks pretty good from an editorial perspective. Perhaps some of the decisions around birthdate and labels could be collected into a Rationale section which currently doesn’t appear to be present. It would be good if more descriptor experts and silent payments experts could take a look at this, as I’m a bit of out of my depth regarding the nitty gritty details on those.
|
Sorry for the late review. I haven't really been following the current developments of silent payments, so it is possible that some of these thoughts are out of date. The originally discussed The birthday is metadata and should not be included in a descriptor. I'm unconvinced that labels belong in the descriptor too. Ultimately, that's very similar to ranged derivation paths which are explicitly unspecified in descriptors. It's up to the wallet loading the descriptor to choose the range. |
This builds on and provides an alternative to #2026, which references an earlier discussion. Instead of modifying BIP352, a new output descriptor format is proposed in the style of BIPs 381-387.
Similar to #2026 key expressions starting with
spscanandspspendare specified, but contain only version and key material information. The wallet birthday and additional labels are specified as optional additional arguments in the output descriptor format.BIP352 authors: @josibake @RubenSomsen
Mail list discussion: https://groups.google.com/g/bitcoindev/c/bP6ktUyCOJI