bips icon indicating copy to clipboard operation
bips copied to clipboard

BIP 352: Silent Payments

Open josibake opened this issue 2 years ago • 69 comments
trafficstars

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: https://github.com/bitcoin/bitcoin/pull/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.

josibake avatar Jun 05 '23 16:06 josibake

Thanks for the review, @vostrnad !

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

These should all be fixed now in https://github.com/bitcoin/bips/pull/1458/commits/1ae1b4bf80bc21911fbd90033edd8006d5e6b592

josibake avatar Jun 08 '23 09:06 josibake

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 avatar Jun 15 '23 10:06 josibake

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.

vostrnad avatar Jun 15 '23 17:06 vostrnad

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.

josibake avatar Jun 16 '23 11:06 josibake

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

kallewoof avatar Jun 20 '23 00:06 kallewoof

@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.

josibake avatar Jun 20 '23 13:06 josibake

we are already using existing valid private keys

@vostrnad Actually, what I said above is not true and your point stands. When we do tn = hash(ecdh_shared_secret || n) and then multiply by G, we might get an invalid point, so we should include a step on how to handle this. Will update.

josibake avatar Jun 22 '23 14:06 josibake

Assigned BIP number 352

luke-jr avatar Jun 29 '23 16:06 luke-jr

I've added a reference Python implementation along with test vectors for sending and receiving. I'll be updating the "Appendix B: Test Vectors" section shortly.

josibake avatar Jul 03 '23 17:07 josibake

There was a suggestion to use BIP21 for silent payments instead of introducing a new address format, the reasoning being introducing yet another address format is a burden on wallet developers. I looked into this a bit, and here's my thinking:

Case 1: bitcoin:bc1pspendpublickey?sp=0x02scanpublickey

The "address" is the spend public key encoded as a taproot address and an sp= extension is added which contains the scan public key. Wallets that understand the sp= extension make a silent payment using both keys, wallets that don't skip the extension and send the funds to the spend public key. You can argue that this is no worse than people posting a static address today, with the option to receive funds more privately if the sender understands silent payments. I disagree for the following reasons:

  • Encourages address reuse: a receiver who did not previously have a static address posted will now post a static address as a fallback to receive silent payments. Encouraging users to post a static address is an anti-pattern, as increases the amount of address reuse, which is exactly what we want to avoid
  • Damages the privacy of users who do support sending silent payments: Bob receives a payment from Alice, where Alice's wallet doesn't understand the sp= extension, so she sends it to his spend public key. Everyone can see that Bob received a payment to his silent payment address. Carol sends a payment to Bob as a silent payment. Bob then spends the UTXO from Alice and the UTXO from Carol together, effectively linking Carol's transaction to Bob's silent payment address.

Case 2: bitcoin:bc1pxxx?sp=0x02spendpublickey0x03scanpublickey

The address is a static bitcoin address and the sp= param contains the spend and scan public key. The same critiques of case 1 apply in this case, as well.

Case 3: bitcoin:bc1pxxx?r-sp=0x02spendpublickey0x03scanpublickey

By adding r- to the param, we make the extension required. This is the same as introducing a new address format: either wallets support sending to it, or the payment fails. I don't see any benefits of using BIP21 over introducing a new address in this case.

Proposed solution

Keep the sp1qxxx bech32m encoding and add it to BIP21 once there is widespread support for sending to silent payment addresses and something like BOLT12. This would allow us to construct a static payment code URI like bitcoin:sp1qxxx?lightning=lno1pg257enxv4ezqcneype82um50ynhxgrwdajx283qfwdpl28qqmc78ymlvhmxcsywdk5wrjnj36jryg488qwlrnzyjczs, where the wallet first uses the bolt12 offer and falls back to a silent payment address. While I agree that introducing a new address format isn't ideal, I don't see a way to introduce a reusable payment code protocol without specifying a new address type. This was also mentioned in the design of BIP173. Reusing bech32m does a lot to alleviate the address format fatigue in that any wallet wanting to send to a silent payment address needs to be able to create taproot outputs, so they will already have bech32m encoding and decoding logic. We've also made the silent payment address forward compatible with new silent payment versions, so ideally this format is the one-and-done reusable payment code format.

cc @TheBlueMatt, in case I've missed something or misrepresented the arguments.

josibake avatar Jul 11 '23 09:07 josibake

NACK without fixing the lack of an expiration time.

Rational: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-August/021849.html

IMO better to discuss this issue on the bitcoin-dev mailing list, as it's relevant to all potential new address formats.

petertodd avatar Aug 04 '23 17:08 petertodd

Regarding the test vectors: I think it is best to remove the 'amount' values from the sending tests.

Payment amounts don't test any logic about silent payments directly. Rather, they are indirectly used for the following two purposes:

  1. to match generated taproot outputs with a specific recipient (because all the amounts are unique)
  2. to ensure determinism in testing (sorting on amounts for every silent payment group)

In other words, they are used as a sort-able unique identifier for each recipient.

Both of these goals can be achieved in another way:

  1. For the sending outputs, expect a dictionary object instead, where the key is a recipient (bech32m string representing a silent payment address), and the value is a list of taproot outputs (hex encoding of 32-byte x-only public key) generated for this recipient.
  2. In the tests, sort by Bm where we currently sort by amount. This shouldn't reveal any extra information in the generated outputs, since these outputs are calculated as Pmn = Bm + tn·G and tn is a sha256 digest.

Ordering of a ECPubKey does not seem to be implemented in the simplified secp256k1.py test library, but it is defined in the secp256k1 library itself. I have access to it in the rust-secp256k1 library via the Ord and PartialOrd trait implementations of the PublicKey struct.

I think doing (1) at least makes a lot of sense. SP implementations will always need to be able to map outputs with specific recipients, so the tests should reflect that.

For (2) I am less certain. Regardless, if the only point of 'amounts' is to create deterministic test results, it would be better to rename amount to order or something.

cygnet3 avatar Aug 08 '23 17:08 cygnet3

Regarding the test vectors: I think it is best to remove the 'amount' values from the sending tests.

Payment amounts don't test any logic about silent payments directly. Rather, they are indirectly used for the following two purposes:

1. to match generated taproot outputs with a specific recipient (because all the amounts are unique)

2. to ensure determinism in testing (sorting on amounts for every silent payment group)

In other words, they are used as a sort-able unique identifier for each recipient.

Both of these goals can be achieved in another way:

1. For the sending outputs, expect a dictionary object instead, where the key is a recipient (bech32m string representing a silent payment address), and the value is a list of taproot outputs  (hex encoding of 32-byte x-only public key) generated for this recipient.

2. In the tests, sort by Bm where we currently sort by amount. This shouldn't reveal any extra information in the generated outputs, since these outputs are calculated as Pmn = Bm + tn·G and tn is a sha256 digest.

Ordering of a ECPubKey does not seem to be implemented in the simplified secp256k1.py test library, but it is defined in the secp256k1 library itself. I have access to it in the rust-secp256k1 library via the Ord and PartialOrd trait implementations of the PublicKey struct.

I think doing (1) at least makes a lot of sense. SP implementations will always need to be able to map outputs with specific recipients, so the tests should reflect that.

For (2) I am less certain. Regardless, if the only point of 'amounts' is to create deterministic test results, it would be better to rename amount to order or something.

I totally support this, and even for determinism frankly I already got rid of the amounts in my own test and it doesn't seem to affect me.

I would change some other details about the tests and test vectors, I'll make a comment about that specifically soon.

Sosthene00 avatar Aug 08 '23 17:08 Sosthene00

@Sosthene00 @cygnet3 Thanks for the suggestions! I think just sorting the labeled spend pub keys is likely the best solution. For sorting, we can just lexicographically sort the compressed format (which is what secp2561 does).

Also fine with removing amounts, so long as we can be sure the correct amounts are being paid by the generated taproot outputs.

@Sosthene00 I'll wait for the rest of your feedback before updating the tests

josibake avatar Aug 09 '23 09:08 josibake

What do you mean by "without requiring any interaction or on-chain overhead"?

You still have to record a new transaction in the ledger with this new UTXO targeting the "silent tweaked address" (X' = hash(i*X)*G + X). Thus I don't understand where don't you have on-chain overhead or what you mean by it.

Thanks

FeatureSpitter avatar Sep 02 '23 07:09 FeatureSpitter

What do you mean by "without requiring any interaction or on-chain overhead"?

You still have to record a new transaction in the ledger with this new UTXO targeting the "silent tweaked address" (X' = hash(i*X)*G + X). Thus I don't understand where don't you have on-chain overhead or what you mean by it.

Thanks

Hi, I think it just means that we don't need extra transactions beside the payment and don't add data to transactions either, like BIP47 or Private Payment that both need a notification transaction with some data in an OP_RETURN.

Sosthene00 avatar Sep 04 '23 10:09 Sosthene00

Thanks for the feedback @stevenroose

I would prefer to just have a description of the final construction

In the opening paragraph of the Overview we explicitly mention this:

We first present an informal overview of the protocol. [...] Each section of the overview is incomplete on its own and is meant to build on the previous section in order to introduce and briefly explain each aspect of the protocol. For the full protocol specification, see Specification.

While I see how it can also lead to confusion in some cases, we've also received a lot of feedback on how this made it easier to comprehend the protocol. There are a lot of steps involved, and this gives us a good framework of introducing them to the reader one by one.

RubenSomsen avatar Sep 12 '23 15:09 RubenSomsen

Thanks for the review @naumenkogs !

most of my comments are addressed and can be closed

I went through the open comments and resolved most of them. Let me know if there any I resolved that you feel are still unaddressed

josibake avatar Dec 06 '23 13:12 josibake

Thanks again to everyone for the thorough review! I've tried to address all the unaddressed comments, but please let me know if I've missed anything.

Major changes:

  • Hashing has been updated to hash the smallest output || sum of the silent payment eligible input pubkeys
  • Labels are generated using the hash of the scan private key || an integer (updated in the reference python, as well)
  • Removed P2PK, uncompressed pubkeys, hybrid keys
  • Updated the inputs for shared secret derivation to include script templates
  • Wording / notation fixups

I think the only unaddressed comment still on my todo list is to add tagged hashes

josibake avatar Jan 10 '24 22:01 josibake

One overall comment is that the real world applicability of this is currently going to be limited to single signature taproot wallets, which is understandable, but also disappointing. This limitation arises in part from the lack of a security proof (or intuition) for FROST/MuSig2 keys, from the specific derivation description for the scan and spend keys, and lack of description of how a tapscript wallet would publish their tweaked internal key in a compatible way (obviously assuming the existence of an internal key for which ECDH is implemented).

I'm not sure I understand some of the specific points here. I agree that FROST/Musig and collaborative usage (e.g. CoinJoin) are areas of open research, but we've also tried to make sure they are supported usecases as the BIP is currently written one we've had more time to evaluate the security.

Regarding the spend and scan key derivations, I think we do need to make some sort of recommendation here to ensure compatibility between existing wallets, but I fully expect we will need to updated this recommendation or propose a new wallet BIP for defining standards around FROST/Musig, exporting the scan key, etc.

josibake avatar Jan 10 '24 22:01 josibake

Updated reference.py and labels format in test vectors

josibake avatar Jan 11 '24 15:01 josibake

Hey @josibake, why doesn't the scanning algorithm just keep looking for labels until it hits a point where there are no more payments? Like going through from m = 0 to n, and stopping when it doesn't find any payments at m = n. The same method is already used for scanning P(k) but not labels.

Eunovo avatar Jan 14 '24 21:01 Eunovo

@Eunovo I'm not sure if I completely understood the question, but there is a negligible computational cost to checking for more than one label, because we essentially calculate the label for each output and match it with a label database (if it's not a payment to you then there won't be a match).

If we scanned from 0 to n and then half the time be forced to go back and scan all over again from n+1 to p, this would be significantly slower.

Now I do think this is reasonable if we set n so high that 99.9% of all users never hit it, but then it's probably also reasonable to expect that those 0.01% power users know to manually set n to a higher value when scanning.

RubenSomsen avatar Jan 15 '24 00:01 RubenSomsen

Hey @josibake, why doesn't the scanning algorithm just keep looking for labels until it hits a point where there are no more payments? Like going through from m = 0 to n, and stopping when it doesn't find any payments at m = n. The same method is already used for scanning P(k) but not labels.

What does n represent here? The number of outputs in the transaction? Or just some large number defined by the scanner?

josibake avatar Jan 15 '24 08:01 josibake

Updated https://github.com/bitcoin/bips/commit/f0625f73c08030293da925090cfbe9a927980dd5 -> https://github.com/bitcoin/bips/commit/901d41fc10f66d73a17581344b4646e981a52910 (compare)

  • Split BIP text and testing vectors into two commits. I expect fewer changes to the BIP text going forward and most changes (if any) to be around updating the test vectors with new test case
  • Updated the BIP type, per @luke-jr feedback
  • Updated the notation to use 1 .. n, per @theStack , @vostrnad feedback
  • Updated the BIP/test vectors to use tagged hashes, per @real-or-random feedback
  • Updated the wording for BIP32 derivation to imply support for FROST/Musig/etc, per @brandonblack feedback

josibake avatar Jan 15 '24 12:01 josibake

Updated https://github.com/bitcoin/bips/commit/901d41fc10f66d73a17581344b4646e981a52910 -> https://github.com/bitcoin/bips/commit/b3495171054784d100b1a90addef463b61c9149c (compare)

  • Updated "Test vectors" section
  • Updated test vectors, reference.py to match vin structured is in bitcoin core rpcs

josibake avatar Jan 15 '24 14:01 josibake

Updated https://github.com/bitcoin/bips/commit/b3495171054784d100b1a90addef463b61c9149c -> https://github.com/bitcoin/bips/commit/d73e924a5b1e43f0e5736d568097aa9a0b8cb210 (compare)

  • Updated reference.py to check for compressed keys (per the updates to the BIP)
  • Code cleanup for reference.py (mainly adding a bitcoin_utils.py for non-silent payment specific stuff)
  • Added deterministic signatures (RFC6979) to minimize diffs as we update the unit tests in send_and_receive.json

josibake avatar Jan 16 '24 17:01 josibake

Updated https://github.com/bitcoin/bips/commit/d73e924a5b1e43f0e5736d568097aa9a0b8cb210 -> https://github.com/bitcoin/bips/commit/55b05c68b04d486868c90d6b372a2ac8ff8f3f79 (compare)

  • Small fixups (wording/spelling)
  • Removed unused function from reference.py

josibake avatar Jan 25 '24 14:01 josibake

Addressed all the outstanding feedback (let me know if I missed anything!). @RubenSomsen and I have also done a final round of review and are considering the protocol final.

@luke-jr RFM, unless there are any objections?

josibake avatar Jan 25 '24 14:01 josibake

Updated https://github.com/bitcoin/bips/pull/1458/commits/55b05c68b04d486868c90d6b372a2ac8ff8f3f79 -> https://github.com/bitcoin/bips/pull/1458/commits/a2b52fea2538f106d13098c4efde016baf72052a (compare)

  • Fixed spelling errors / improved wording (h/t @vostrnad)
  • Removed unused import from reference.py (h/t @theStack)

Thanks for the thorough @vostrnad and @theStack, much appreciated!

josibake avatar Jan 26 '24 16:01 josibake