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 352: Silent Payments #1458

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

josibake
Copy link
Member

@josibake josibake commented Jun 5, 2023

Silent payments is a static address protocol for Bitcoin, originally proposed on the mailing list here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-March/020180.html

Since then, the proposal has received several rounds of review and has a WIP implementation here: bitcoin/bitcoin#27827 . The proposal has also been sent to the mailing list for review here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-June/021750.html

Proposing this as an informational BIP to ensure wallets across the ecosystem can standardize and correctly implement the protocol.

bip-0000.mediawiki Outdated Show resolved Hide resolved
Copy link
Contributor

@brandonblack brandonblack left a comment

Choose a reason for hiding this comment

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

Glad to see this proposal advancing! I think with some minor changes it can be compatible with MuSig2 Taproot Keypaths (and potentially other off chain key aggregation schemes), which make it a powerful way of transacting privately even between users with advanced keys and scripts on Taproot.

bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
Copy link
Contributor

@vostrnad vostrnad left a comment

Choose a reason for hiding this comment

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

Some light review so far, but I'm really liking the proposal. Being able to easily implement just the sending part will no doubt greatly accelerate adoption by wallets.

Apart from the inline comments I also have a few style nits:

  • "e.g" is used instead of "e.g." in a few places.
  • Mixed usage of curly () and straight (') single quotes.
  • Some footnotes ("Rationale and References") are missing a period at the end.
  • In Creating outputs, "Let a = a0 + a1 + … ai, where each ai" should have an be the last member of the sum, like in the rest of the document. Same goes for the sum in Scanning where the last member should be An.

bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
@josibake
Copy link
Member Author

josibake commented Jun 8, 2023

Thanks for the review, @vostrnad !

Apart from the inline comments I also have a few style nits:

These should all be fixed now in 1ae1b4b

bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
@josibake josibake force-pushed the silent-payments-bip branch 3 times, most recently from 98f37a9 to d9e5a1b Compare June 14, 2023 12:37
Copy link
Contributor

@vostrnad vostrnad left a comment

Choose a reason for hiding this comment

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

Second round of review. To complement this I wrote a quick and dirty implementation covering most of both sending and receiving. It was quite easy and seems to work, can't wait for test vectors to see how many bugs it has! 😅

In addition to the inline comments I have one general comment and one style nit:

  • There are no specifics on how to deal with situations where EC operations fail to produce a valid point. I'm not sure if this is necessary, just mentioning this because my implementation has a lot of assertions.
  • Tweak expressions are not consistent in whether the G part comes first or second. For example, in BIP341 it always comes second.

bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
@josibake josibake force-pushed the silent-payments-bip branch 3 times, most recently from 889a0b6 to be18f6f Compare June 15, 2023 10:35
@josibake
Copy link
Member Author

Thanks for the continued review, @vostrnad !

There are no specifics on how to deal with situations where EC operations fail to produce a valid point. I'm not sure if this is necessary, just mentioning this because my implementation has a lot of assertions.

Do you have a specific scenario in mind? I'm certainly not an expert in this area, but I don't think we should get a failure by doing additions and multiplications (summing priv keys, pub keys, ecdh). If we can have a failure when summing up the private keys, public keys, or during the ECDH step, our only recourse would be to restart the coin selection process and ensure we get different inputs.

@josibake josibake force-pushed the silent-payments-bip branch 2 times, most recently from ad7fa34 to d342de6 Compare June 15, 2023 12:41
@vostrnad
Copy link
Contributor

I don't think we should get a failure by doing additions and multiplications (summing priv keys, pub keys, ecdh)

One reason EC operations can fail is when a pseudorandom scalar value exceeds the curve order, which should only happen with negligible probability but other BIPs still have special cases for when it happens (e.g. BIP32, BIP340 and BIP341). I suppose it could be fine to ignore this, but I'm certainly no expert either.

bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
bip-0000.mediawiki Outdated Show resolved Hide resolved
@josibake
Copy link
Member Author

I don't think we should get a failure by doing additions and multiplications (summing priv keys, pub keys, ecdh)

One reason EC operations can fail is when a pseudorandom scalar value exceeds the curve order, which should only happen with negligible probability but other BIPs still have special cases for when it happens (e.g. BIP32, BIP340 and BIP341). I suppose it could be fine to ignore this, but I'm certainly no expert either.

I'll take a look to see how it's being handled in those BIPs. Ideally, we can reuse the same solution here. The main thing is ensuring both the sender and receiver can deterministically handle these low-probability events.

bip-0000.mediawiki Outdated Show resolved Hide resolved
@kallewoof
Copy link
Member

@luke-jr Unless you see anything objectionable, please assign BIP number.

@josibake
Copy link
Member Author

@vostrnad

One reason EC operations can fail is when a pseudorandom scalar value exceeds the curve order, which should only happen with negligible probability but other BIPs still have special cases for when it happens (e.g. BIP32, BIP340 and BIP341). I suppose it could be fine to ignore this, but I'm certainly no expert either.

I did some more reading on this and from what I can tell this applies to choosing a scalar which results in a valid point on the curve, which is not true for every scalar. In our case, we are already using existing valid private keys or, in the case of the silent payment address, are using BIP32 to generate them. Adding these keys together (using the elliptic curve group operation) or multiplying a valid scalar with a point (ECDH) gives us a result that is guaranteed to be a valid point on the curve.

@TheBlueMatt
Copy link
Contributor

This goes against how we've seen BIP21 being implemented in practice (i.e. as bitcoin:[any address]). Even if we don't specify bitcoin:[SP address], doesn't mean we won't be seeing it.

I mean its possible sure, its also possible people try to byte-swap your public keys and send to an invalid address. People can creatively mis-interpret specs in all kinds of fun ways, but if we tell them what to do and have a few wallets doing it "right" and not supporting anything "wrong", then new developers will figure out that they're doing it wrong real fast when they go to test it once.

Past address formats have already failed to do this, so clearly it hasn't been working well. And the very fact that we're currently having this discussion further shows that this is not exactly a smooth process.

Exactly! So lets improve the process and learn from past mistakes. Instead of not bothering to define anything remotely backwards compatible, lets use silent payments to show how to do it right in a way that lets people use new features without waiting five years for the whole ecosystem to adopt it. (or, honestly, more, taproot probably wont be usable until more than five years after activation...).

More broadly, Bitcoin has grown up - the utter failure that is taproot address support should be an indication that we need to totally rethink this. Bitcoin is a much bigger world than it used to be, and just throwing things over the wall doesn't work anymore - there will probably always exist senders somewhere that don't support taproot addresses.

BIP21 already severely needs updating to be more in line with the current reality anyway, this seems like a logical addition.

Indeed, we need to do something, and if I had to rewrite BIP21 today I'd write explicitly that "new address formats MUST define a key and be placed in the parameters section." I don't see a reason why we'd prefer some kind of generic optionN vs just saying "this is type Y" - the second makes it much clearer to the reader what they're dealing with (eg new address formats could elide the HRP if they wanted to save a bit on space in QR codes, which is often worth it), whereas the first is just an opaque "here's a thing", while wasting QR code space.

@josibake
Copy link
Member Author

josibake commented Mar 1, 2024

@TheBlueMatt @RubenSomsen @kristapsk this discussion is broader than just BIP352 and is starting to feel off-topic for this BIP. I've opened a delving bitcoin topic where we can continue the discussion: https://delvingbitcoin.org/t/revisiting-bip21/630

Would love to have your input over there!

@TheBlueMatt
Copy link
Contributor

I'm really not sure why this ended up being a whole discussion - a simple one-line "Silent payment instructions in bitcoin: URIs MUST be placed in the sp parameter value. Wallets supporting silent payments MUST handle bitcoin: URIs with no body section (i.e. bitcoin:?sp=...) as well as URIs with a body section containing a fallback on-chain address" would suffice :). If you really feel strongly about the option thing you're welcome to replace sp here with option1, though I'm not at all convinced that's a useful change.

@TheBlueMatt
Copy link
Contributor

I went ahead and suggested concrete updates to BIP 21 at #1555

bip-0352.mediawiki Outdated Show resolved Hide resolved
bip-0352.mediawiki Outdated Show resolved Hide resolved
@josibake
Copy link
Member Author

Updated 359fa75 -> 92b12ab (compare):

  • Fixed typo with taproot scriptPubKey templates (wrong witness version)

@josibake
Copy link
Member Author

Updated 92b12ab -> f6dd067 (compare):

  • Removed amount from the test vectors
  • Removed sorting from the test vectors

Amount was not being used and the sorting step was causing unnecessary complications.

@mplsgrant
Copy link

mplsgrant commented Mar 28, 2024

When calculating input_hash, should we select the smallest outpoint from the set of all outpoints in the transaction, or should we select it from the set of associated outpoints related to the inputs we used for shared secret derivation?

While trying to reason about this, I looked at the test vectors. Two of the tests deal with excluding inputs for shared secret derivation; however, they both exclude the lexicographically largest outpoints. That means if I believed in either "set of all" or "set of associated", then the tests would pass, but my logic would be faulty.

The BIP's test vectors related to my question:

  • Skip invalid P2SH inputs
  • P2PKH and P2WPKH Uncompressed Keys are skipped

@Eunovo
Copy link

Eunovo commented Mar 29, 2024

While trying to reason about this, I looked at the test vectors. Two of the tests deal with excluding inputs for shared secret derivation; however, they both exclude the lexicographically largest outpoints. That means if I believed in either "set of all" or "set of associated", then the tests would pass, but my logic would be faulty.

@mplsgrant Not all outpoints can be used for shared secret derivation. The excluded inputs were not excluded because of their lexicographic positions, they were excluded because they did not meet the criteria defined in https://github.com/bitcoin/bips/blob/f6dd0672d81aa22368024c8ce7fe7e763a77a9f5/bip-0352.mediawiki#inputs-for-shared-secret-derivation

Suppose we have a Transaction T,
A is the set of inputs in T that meet the shared secret derivation criteria
U is the set of all inputs in T
OUTPOINT(L) is the smallest outpoint lexicographically by txid and vout in U
input_hash = HASHbip0352/inputs(OUTPOINT(L) || Sum(A)),
See https://github.com/bitcoin/bips/blob/f6dd0672d81aa22368024c8ce7fe7e763a77a9f5/bip-0352.mediawiki#input-hash

That said, it might be a good idea to add vectors that use an OUTPOINT(L) that is not in Set A.
WDYT? @josibake

EDIT
Sum(A) sums up the pubkeys of inputs in Set A

@josibake
Copy link
Member Author

josibake commented Apr 2, 2024

Updated f6dd067 -> 9c82669 (compare):

  • Added all possible permutations to test outputs (i.e. remove order dependence for sender tests)
  • Made smallest outpoint an excluded outpoint in "Exclude uncompressed" tests

For implementing sending, the tests give a set of inputs and outputs to match against. However, due to hashing with the counter k, it is possible to generate different output sets for the recipient depending on which order the recipient addresses are evaluated. All of these output sets are valid, but this is particularly annoying when evaluating a sending implementation against the test vectors. To make this less cumbersome, I've included all possible sets.

I also included a small change based on @mplsgrant feedback which makes it more clear that the smallest_outpoint is picked from the total set of outpoints and not from the set of "Inputs for shared secret derivation." h/t @Eunovo for the concise explanation.

bip-0352.mediawiki Outdated Show resolved Hide resolved
@josibake
Copy link
Member Author

josibake commented Apr 4, 2024

Updated 9c82669 -> 57c89ae (compare)

  • Fixed typo in overview section

@CakeWallet
Copy link

@TheBlueMatt @RubenSomsen @kristapsk this discussion is broader than just BIP352 and is starting to feel off-topic for this BIP. I've opened a delving bitcoin topic where we can continue the discussion: https://delvingbitcoin.org/t/revisiting-bip21/630

Would love to have your input over there!

We have implemented Silent Payments in Cake Wallet for iOS, Android, Mac, and Linux. Would love for you guys to test it and give some feedback back.

@CakeWallet
Copy link

We have implemented Silent Payments in Cake Wallet for iOS, Android, Mac, and Linux. Would love for you guys to test it and give some feedback back.

@RubenSomsen
Copy link

Thanks everyone for all the precious feedback. It has been humbling to see the amount of support this BIP has received. @josibake and I feel the BIP is now mature and will no longer be making breaking changes. With various implementations well underway, we propose moving the BIP status to final.

@jirijakes
Copy link

jirijakes commented Apr 9, 2024

Versions are specified as:

If the receiver's silent payment address version is:

  • v0: check that the data part is exactly 66-bytes. Otherwise, fail
  • v1 through v30: read the first 66-bytes of the data part and discard the remaining bytes
  • v31: fail

In the same Versions section:

Version is communicated through the address in the same way as Segwit addresses.

and later in Address encoding section:

The final address is a Bech32m [refers to BIP 350] encoding of[…]

However, both BIP 350 and BIP 173 restrict the version numbers to range 0..16.

BIP 350:

The following code demonstrates the checks that need to be performed.

def decode(hrp, addr):
   […]
   # Witness versions are in range 0..16.
   if data[0] > 16:
       return (None, None)

So while technically the encoding format could handle whole 0..31 range, the BIPs restrict it. The existing Bech32 decoders might, therefore, fail with versions higher than 16, for example, rust-bech32.

Not sure what would be the best way to deal with it – either allow the same version range as BIP 350 or make it clear that the version range differs from BIP 350.

input_hash = get_input_hash([vin.outpoint for vin in vins], A_sum)
pre_computed_labels = {
(generate_label(b_scan, label) * G).get_bytes(False).hex(): generate_label(b_scan, label).hex()
for label in given["labels"]

Choose a reason for hiding this comment

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

I have a suggestion to reflect “always check for the change label” in reference implementation and test vector. I believe the following changes should be enough.

Given this, the address for change would have to be removed from the relevant test case but I believe that it would be consistent with “It is important that the wallet never hands out the label with m = 0.”

Reference (always check for m = 0):

Suggested change
for label in given["labels"]
for label in given["labels"] + [0]

And then in test data (don't explicitly refer to label 0):

diff --git a/bip-0352/send_and_receive_test_vectors.json b/bip-0352/send_and_receive_test_vectors.json
index 5d329f7..99cc52c 100644
--- a/bip-0352/send_and_receive_test_vectors.json
+++ b/bip-0352/send_and_receive_test_vectors.json
@@ -1829,14 +1829,11 @@
                         "spend_priv_key": "b8f87388cbb41934c50daca018901b00070a5ff6cc25a7e9e716a9d5b9e4d664",
                         "scan_priv_key": "11b7a82e06ca2648d5fded2366478078ec4fc9dc1d8ff487518226f229d768fd"
                     },
-                    "labels": [
-                        0
-                    ]
+                    "labels": []
                 },
                 "expected": {
                     "addresses": [
-                        "sp1qqw6vczcfpdh5nf5y2ky99kmqae0tr30hgdfg88parz50cp80wd2wqqauj52ymtc4xdkmx3tgyhrsemg2g3303xk2gtzfy8h8ejet8fz8jcw23zua",
-                        "sp1qqw6vczcfpdh5nf5y2ky99kmqae0tr30hgdfg88parz50cp80wd2wqqlv6saelkk5snl4wfutyxrchpzzwm8rjp3z6q7apna59z9huq4x754e5atr"
+                        "sp1qqw6vczcfpdh5nf5y2ky99kmqae0tr30hgdfg88parz50cp80wd2wqqauj52ymtc4xdkmx3tgyhrsemg2g3303xk2gtzfy8h8ejet8fz8jcw23zua"
                     ],
                     "outputs": [
                         {

@josibake
Copy link
Member Author

@jirijakes thanks for the thorough review!

So while technically the encoding format could handle whole 0..31 range, the BIPs restrict it. The existing Bech32 decoders might, therefore, fail with versions higher than 16, for example, rust-bech32.

Not sure what would be the best way to deal with it – either allow the same version range as BIP 350 or make it clear that the version range differs from BIP 350.

Great point, I'm leaning towards having it match the versioning restrictions of BIP350 to avoid unnecessary headache for implementers. Regardless, I think we also need to include sending to a higher version address in our test vectors.

I have a suggestion to reflect “always check for the change label” in reference implementation and test vector. I believe the following changes should be enough.

Given this, the address for change would have to be removed from the relevant test case but I believe that it would be consistent with “It is important that the wallet never hands out the label with m = 0.”

Great suggestion, will add!

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Began an initial review a fortnight ago -- a couple comments I had at the time before GitHub loses them, that may or may not be relevant.

@@ -0,0 +1,162 @@
import hashlib
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add at the top of this file and in secp256k1.py to allow invoking these files directly.

+#!/usr/bin/env python3
+
 import hashlib
 import struct
 from io import BytesIO

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, these are library files and shouldn't be run directly. Not sure if there is a standard way in python to communicate that, but I think the only file that should have a shebang is reference.py?

bip-0352/reference.py Show resolved Hide resolved
bip-0352/bitcoin_utils.py Outdated Show resolved Hide resolved
Co-Authored-By: Ruben Somsen <rsomsen@gmail.com>
josibake and others added 2 commits April 27, 2024 15:04
* reference.py contains the silent payment specific code
* secp256k1.py for doing the EC operations
* bech32m.py contains code for encoding/decoding bech32(m) addresses
* bitcoin_utils.py contains some helper code, not specific to silent
  payments
* send_and_receive_test_vectors.json contains the wallet unit test
  vectors

Co-Authored-By: S3RK <1466284+S3RK@users.noreply.github.com>
Co-Authored-By: Oghenovo Usiwoma <37949128+Eunovo@users.noreply.github.com>
Co-authored-by: S.Blagogee <34041358+setavenger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet