This PR is the first step towards implementing BIP352: Silent Payments in the Bitcoin Core wallet. BIP352 is “a protocol for static payment addresses in Bitcoin without on-chain linkability of payments or a need for on-chain notifications.”
CPubKey and CKey are Bitcoin Core’s classes for public and private keys. These classes handle serializing and deserializing key data and encapsulate libsecp256k1 cryptographic operations. This PR adds new methods to these classes to support the cryptographic operations needed for BIP352.
CTxDestination is a std::variant class used to represent a transaction output (or “destination”). This variant represents standard scriptPubKey templates and handles encoding and decoding outputs via the DestinationEncoder class and DecodeDestination function. BIP352 introduces a new output type which does not represent a single scriptPubKey, but instructions for generating a scriptPubKey using ECDH. This PR introduces new methods for encoding and decoding a silent payment address.
BIP352 has a sending section and a receiving section. This allows wallets to implement either half of the protocol without needing to implement the other. The logic for sending is implemented in the Sender class, with the bulk of the logic in the GenerateRecipientScriptPubKeys() method. Receiving is implented in the Recipient class, with the bulk of the logic in the ScanTxOutputs method. This PR only implements the BIP352 primitives and leaves the wallet implementation for a follow-up PR. As such, the interface for these classes is abstracted away from transactions and descriptors and instead works directly with CPubKeys and CKeys.
Questions
Did you read the BIP? In a few sentences, can you summarize it in your own words?
In 5688262, why do we need a custom hash function? Why not just use the default hash function from libsecp256k1? (hint: see this discussion on the BIP)
ae6019b and 439e57a add functions for decoding and encoding silent payment addresses. Why can’t we just add silent payment addresses as a new CTxDestination variant and use the existing encoder class and decoder function?
BIP352 talks about about versioning and forward compatibility. Why is this important? Do you see any issues with the way the PR is written with respect to forwards compatibility (hint: look at the decoding logic in ae6019b)
In DecodeSilentAddress there is a check on the version and data size. In your own words, can you explain what this check is doing? Why is it important?
The new Silent Payments code is under the wallet directory in src/wallet/silentpayments.cpp. Is this a good place for the code to live? Why or why not? Can you think of use case where it would be better to have the code outside the wallet?
Recipient is a class which is initialized with two private keys, the scan key and the spend key. Are both of these keys necessary for scanning? Can you propose a better implementation? Which functions would you need to re-write? (hint: take a look at Recipient::ScanTxOutputs
Based on your answer above, what’s one benefit for how the Recipient class is currently written?
In Sender::CreateOutput, we hash the ECDH shared secret with an integer. Why do we need the integer counter?
Bitcoin Core has a HashWriter class, which we use. Can you describe what this class does? For our usecase, we need to hash a public key (the shared secret) and an integer. The HashWriter provides a template function for hashing two objects: why not use this? Even better, why not hash the pubkey with the << operator? (hint: look at how a CPubKey is serialized)
For the unit tests, we define an equality comparator as a “friendly” function. What is a “friendly” function? Does it make sense to use one here?
<josie> lets get started! did you get a chance to read the BIP? if so, can you summarize it in a sentence or two, or mention a part that stood out to you?
<Murch> Silent Payments describes a static address scheme where the transaction itself provides sufficient information for the recipient to recognize that they got paid without revealing to any uninvolved parties that a silent payment has occurred. This is achieved by creating a unique recipient output script from the recipient’s static address and the public keys used in the spending inputs.
<BrandonOdiwuor> BIP 352 Provides a solution for making private payments using static addresses eliminating the need for interaction or on-chain overheads i.e notifications while preserving the privacy of both the sender and the receiver
<larryruane> I loved how the BIP adds complexity in stages, first simple, then creating > 1 output, etc. (I got a bit lost on the scan versus spend, and the labels)
<sosthene> Hi, just wanted to mention that the BIP made a lot of progress as far as I'm concern, and that the way labels work today lifted the most serious worry I had about the first iteration
<stickies-v> larryruane: the scan vs send means (I think) that, similar to the xpub setup, you want to delegate scanning for received payments to a hot machine that only has your spending pubkey and your scanning private key, which is enough to recognize payments, but not to spend them (for which you need the spending privkey too)
<Murch> Does the wallet keep track of the output scripts after it has found an output corresponding to one? Otherwise, it would not match the protocol, and the search process would not find it
<josie> for those that read the BIP, did you also look at the python reference implementation and test vectors? Can you think of any additional test cases?
<sosthene> it means you received some funds on a regular sp scriptpubkey, and the sender keep it and send to the same script pubkey again in another transaction
<josie> sosthene, Murch: since we are using a sha256 to ensure a unique address is created, address reuse will not happen if the sender is following the protocol. of course, nothing stops a sender from maliciously sending to an output that they sent to previously, but this is true of bitcoin in general
<josie> sosthene, ottosch: the receiving wallet would find the initial payment when scanning. continuing to monitor that output script for additional payments can be done with any other wallet protocol, because at that point the scriptPubKey is known
<larryruane> A basic question, `P = B + hash(a·B)·G` ... that addition, does this depend on Schnorr signatures being used? I'm unclear about how Bob creates the unlock script to spend a SP later (since the pubkey is this weird thing)
<sosthene> larryruane that's just adding points on the curve, `a*B` is your ECDH, hashing it gives you something you can use as a Scalar, multiplying by G gives you another point on the curve, nothing to do with Schnorr
<Murch> @josie: sorry, got called away for a moment. sosthene already explained, but I meant that tx_A sent a regular SP to Bob, and then Mallory sends to the same output script as the SP to Bob went to. Would Bob’s wallet even notice that it got paid again to a prior SP output script, when in this second transaction the output script is not a valid SP
<larryruane> sosthene: josie: thanks .. I need to study more, i don't understand how Bob (the receipient) later spends these silent payments, but that's okay
<Murch> @ottosch: If it’s the same sender, they should just make a new SP if they want to pay me again. If it’s someone sending a tiny amount to force address reuse, I’d rather not even see it so I don’t have to think about it
<josie> Murch: if the question is "can a silent payments wallet detect payments not sent via the silent payments protocol" I think the correct answer is "it depends" :)
<stickies-v> larryruane: bob would just do a keypath spend, and know how to construct the private key that's needed for the (schnorr, because we're in taproot world, but could be ECDSA in theory) signature based on his (scan, spend) private key as well as the public key of the input used in the tx
<Murch> larryruane: The second part `hash(a·B)·G` is derived from the shared secret of Alice and Bob: Alice’s private key and Bob’s public key multiplied result in the same value as Bob’s private key multiplied with Alice’s public key. This is just Diffie-Hellman (a·B = b·A)
<Murch> Then Alice adds Bob’s public key to it the key. This means that only Bob will be able to spend it, because he needs the corresponding private key to sign
<josie> stickies-v, larryruane: that's correct! basically, the final private key ends up being Bob's spend secret key + the shared secret from Alice doing ECDH with Bob's scan key
<Murch> josie: I just skimmed it. Huge concept ACK on Silent Payments, but don’t have an opinion on the implementation, yet. Hope to stare at it more in the coming ten days or so
<stickies-v> yeah concept ACK on SP too, and approach-wise i like that you're implementing this first bit without the wallet, that seems to make sense at first sight
<sosthene> I didn't reviewed it in details, I just built it and ran the tests. However I would like to ask why chose to keep the send/receive for another PR?
<josie> sosthene: great question! my main motivation was to break the original PR into smaller chunks to make it easier to review. Having a PR just for the BIP352 logic (independent of send and receive) made unit testing much easier
<josie> sosthene: also, sending and receiving can be implemented separately. In other words, a wallet can send without needing to be able to receive. It seemed logical to then open the PRs as separate since the code allows it
<stickies-v> ottosch: if wallets want to implement scanning the full blockchain for transaction history, i don't think anything's stopping them, scanning the UTXO set is just the minimum requirement (and probably more than enough for most use cases?)
<Murch> ottosch: If you only care to find spendable UTXOs, you can restrict your scanning to transactions with unspent P2TR outputs since you last scanned. If you want the full history, you have to scan all transactions with P2TR outputs since your wallet birthdate
<josie> emjshrx: yeah, its a good point. definitely in favor of keeping the new code contained to just silent payments, where it makes sense. for some of the cryptographic operations, though, I think it makes sense to implement on the CKey and CPubKey objects only because we don't want to pull all that cryptography code into the wallet
<josie> ottosch, BrandonOdiwour: scanning the UTXO set is an optimization for mobile clients to limit the data they need to download. transaction history can always be recovered by scanning the full chain
<josie> ottosch, larryruane: correct! and also "stealth addresses", the original static address proposal for bitcoin, used ECDH. unrelated to wallets, BIP324 uses ECDH, but a modified XOnlyECDH iirc
<josie> great segue to the next question! for ECDH, why does the PR define a new custom hash function? why not use the default hash function from libsecp256k1?
<BrandonOdiwuor> This is due to the need for un-hashed ECDH result, the ‘custom_ecdh_hash’ function returns the serialized public key without hashing it
<josie> Murch, stickies-v: because I'm bad at naming things :P jokes aside, I was trying to match the language in libsecp, where they say you can use the "default" hash function or pass a "custom" hash function
<sosthene> but frankly when I noticed it I thought we could just hash it anyway, add the counter and hash it again, so that we don't need the `this_is_not_a_hash_function` trick
<josie> Murch: less specific to Musig2, but more generally any time the creator of the transaction does not control all the inputs. By not hashing the result during ECDH, this allows individual participants to do ECDH with just their private key, and then pass the partial ECDH along. the partial ECDH results can then be summed up , and the rest of the protocol performed (hashing with the
<Murch> which would very likely leak the private keys to the other participants (unless there is a really nifty way of aggregating them without revealing them or smth, but just not hashing is way easier)
<josie> so we've got 4 minutes left, so I think I'll wait til tomorrow to dive into the next question, because it's a fairly big one. As a reminder, we are meeting tomorrow at the same time and are going to jump right in with question 5
<josie> emjshrx: the spend and scan key is so that the receiver can always keep one key in cold storage. in fact, the spend key itself can be an aggregate key (musig2, FROST)
<josie> these commits add functions for decoding and encoding a silent payment address. why can't we just reuse the existing encoder and decoder functions and add silent payments as a new `CTxDestination`?
<josie> abubakarsadiq: well, the silent payments protocol dictates that the outputs need to be `WitnessV1Taproot`, but that's different than the silent payment address itself
<josie> emjshrx: it is longer, yes. in the payload, it contains two pubkeys. but we could easily handle this in the existing Encoder and Decoder functions. any ideas as to why we are writing new functions just for a silent payment address?
<vostrnad> abubakarsadiq: The silent payment address encodes the public keys needed to derive the actual output script, which is also dependent on the inputs of your transaction, so the output script is different every time.
<josie> yep! instead of giving you a scriptPubKey to send to (which is what a traditional address does), a silent payment address gives you pubkeys to do ECDH with, and then the protocol dictates how to turn that shared secret into a scriptPubKey for the receiver
<josie> on the subject of address, let's go to question 6: BIP352 talks about versioning and forward compatibility. why is forward compatibility important?
<vostrnad> Forward compatibility in this context means that wallets that only implement Silent Payments v0 can still send to future SP version addresses. (I'm still a bit unsure what kinds of changes could be useful to add to new versions without breaking compatibility with v0.)
<josie> abubakarsadiq: that's correct! even if someone gives you a newer version, your wallet should be able to decode it and send to it, even if you haven't upgraded
<vostrnad> josie: What if a new version wants to intentionally break compatibility? Is that what v31 is for? If so, shouldn't more than one version be allocated for this?
<josie> vostrnad: great question. v31 is reserved for an upgrade which would break compatibility. allocating more "breaking change" versions ahead of time is one approach, but can you think of why its okay to just allocate one?
<abubakarsadiq> but I think `DecodeSilentAddress` and `DecodeSilentData` should be `DecodeV0SilentAddress` and `DecodeSilentData` because thats what they do actually
<josie> abubakarsadiq: its true, that's what they do now, but these same functions could be added to in the future to handle v1 data, v2 data, etc without us needing to create new functions
<josie> (also, in a previous question I asked if there were any issues with forward compatibility with the way the PR is currently written. there is ;) and it might be more obvious after looking at `DecodeSilentAddress`)
<josie> Murch: definitely! lets say v1 silent payments adds a third pubkey (just for the sake of example). what would happen with how the code is currently written? (hint: look at the change in src/bech32.cpp)
<josie> Murch: bingo. right now, it just extends the length to 117. but it should really just remove the limit for a silent payment address (or make it something much higher, like 1023)
<josie> vostrnad: I actually ran into this working on the receiving PR for silent payments :) some of the RPCs for creating transactions don't need a wallet at all
<josie> glozow: exactly! one can imagine a use case where a full node indexes the tweak data for transactions and stores it in an index for light clients to query, or serves data via some bip158 like filter
<josie> glozow: yeah, better code organization and much easier to spot code smell. in the original version of the PR, the silent payments code was outside the wallet and was calling functions in the script interpreter during scanning. moving it into the wallet made me rethink some of those boundaries
<josie> Murch, glozow: definitely. the current use case is most clearly a node scanning for its own wallet. so it wouldn't make sense to compile this code for nodes that explicitly don't want a wallet.
<josie> in the future, we might lift some of this code out to be available for more general scanning, but it seems better to only lift out what we need at that point
<josie> Guest35: yep! BIP352 defines a spend key and a scan key. the silent payment output is created in such a way where the output can be found with just the scan key, but can only be spent with the spend key + the scan key
<josie> we are almost at time and there are still a few questions left, so I'd encourage everyone to go through the remaining questions and leave your feedback on the PR!