Add BIP352 module (take 3) #1698
Conversation
Add a routine for the entire sending flow which takes a set of private keys,
the smallest outpoint, and list of recipients and returns a list of
x-only public keys by performing the following steps:
1. Sum up the private keys
2. Calculate the input_hash
3. For each recipient group:
3a. Calculate a shared secret
3b. Create the requested number of outputs
This function assumes a single sender context in that it requires the
sender to have access to all of the private keys. In the future, this
API may be expanded to allow for a multiple senders or for a single
sender who does not have access to all private keys at any given time,
but for now these modes are considered out of scope / unsafe.
Internal to the library, add:
1. A function for creating shared secrets (i.e., a*B or b*A)
2. A function for generating the "SharedSecret" tagged hash
3. A function for creating a single output public key
Add function for creating a label tweak. This requires a tagged hash function for labels. This function is used by the receiver for creating labels to be used for a) creating labeled addresses and b) to populate a labels cache when scanning. Add function for creating a labeled spend pubkey. This involves taking a label tweak, turning it into a public key and adding it to the spend public key. This function is used by the receiver to create a labeled silent payment address. Add tests for the label API.
| * label_lookup: pointer to a callback function for looking up | ||
| * a label value. This function takes a label | ||
| * pubkey as an argument and returns a pointer to | ||
| * the label tweak if the label exists, otherwise | ||
| * returns a NULL pointer (NULL if labels are not | ||
| * used) |
There was a problem hiding this comment.
I think it should be documented for how long the returned pointer from label_lookup should be valid. I think it is obvious it should be valid until the next call of label_lookup, but it is currently not clear (from the docs) whether it should remain valid until secp256k1_silentpayments_recipient_scan_outputs returns.
The current implementation of secp256k1_silentpayments_recipient_scan_outputs does not need this (from looking at the code) and in a safe Rust abstraction for this function (code, does contain some outdated comments) suggested for rust-bitcoin/rust-secp256k1#721 (WIP bindings to this pull request's code) I relied on this not being a requirement (though that can be changed).
There was a problem hiding this comment.
Hi @antonilol , thanks for the review and bindings code! Agreed that this should be documented. How about something like:
diff --git a/include/secp256k1_silentpayments.h b/include/secp256k1_silentpayments.h
index 2e71405..11fa596 100644
--- a/include/secp256k1_silentpayments.h
+++ b/include/secp256k1_silentpayments.h
@@ -311,6 +311,11 @@ typedef struct secp256k1_silentpayments_found_output {
* the recipient uses labels. This allows for checking if a label exists in
* the recipients label cache and retrieving the label tweak during scanning.
*
+ * If used, the `label_lookup` function must return a pointer to a 32-byte label
+ * tweak if the label is found, or NULL otherwise. The returned pointer must remain
+ * valid until the next call to `label_lookup` or until the function returns,
+ * whichever comes first. It is not retained beyond that.
+ *
* For the labels cache, `secp256k1_silentpayments_recipient_create_label`
* can be used.
*I think the above is the most precise requirement, but perhaps a simpler alternative would be to require that the returned pointer (and labels cache, if used) be valid until _scan_outputs returns? You mention you rely on this not being the case today, but does this make a material difference if we do require that the returned pointer and labels cache be valid for the lifetime of the _scan_outputs function?
There was a problem hiding this comment.
Looks good!
I think the above is the most precise requirement, but perhaps a simpler alternative would be to require that the returned pointer (and labels cache, if used) be valid until
_scan_outputsreturns? You mention you rely on this not being the case today, but does this make a material difference if we do require that the returned pointer and labels cache be valid for the lifetime of the_scan_outputsfunction?
This depends on the function signature on the Rust side, I have considered 2 cases. The difference is in what the user has to return from label_lookup.
First, there is the one in the proof of concept binding I mentioned here, the user has to return any byte array of length 32 or nothing (translates to NULL pointer).
The C side accepts a pointer so the Rust side stores this byte array on the stack and gives the C side a pointer to that. This place on the stack is reused, so requiring that the pointer lives for longer than until the next call of label_lookup means that previous return values need to be kept and some dynamic allocation would be needed.
I see this as the easiest for user to use. As far as I can tell no borrow checker issues can arise from this design.
Second, the user has to return a reference to a byte array of length 32 or nothing. The reference is required to live for as long as the function runs. This is because in Rust it is not (yet) possible to declare the reference is only required to live until the next call of label_lookup.
This is simple to implement because it is basically a pointer cast and is compatible with _scan_outputs needing pointers to be valid for the lifetime of the function.
This one is harder to use, especially for label_lookups that are not simply looking up in a data structure. Returning a reference to any byte array that is not stored outside the function's stack frame is not allowed and storing it on the fly like the first case here probably requires the user to use unsafe code (needs a data structure that allows adding elements while there exist references to its existing elements because the reference is required to live for as long as the function runs) and has the same need for dynamic allocation like the first case can have.
The validity of the context pointer should be completely up to the users of the C library (I consider the Rust binding to also be a user of the C library).
I will also link this discussion in the Rust bindings pull request.
There was a problem hiding this comment.
Thanks for the detailed explanation! As I was thinking through this, it also occurred to me requiring the pointers to be valid for the lifetime of _scan_outputs would require keeping around pointers to every label found, even after they had been used by _scan_outputs. This seems undesirable, e.g., imagine a transaction with ~15k labeled outputs all paying to a single recipient. Your explanation seems to confirm my understanding.
Add routine for scanning a transaction and returning the necessary spending data for any found outputs. This function works with labels via a lookup callback and requires access to the transaction outputs. Requiring access to the transaction outputs is not suitable for light clients, but light client support is enabled by exposing the `_create_shared_secret` and `_create_output_pubkey` functions in the API. This means the light client will need to manage their own scanning state, so wherever possible it is preferrable to use the `_recipient_scan_ouputs` function. Add an opaque data type for passing around the summed input public key (A_sum) and the input hash tweak (input_hash). This data is passed to the scanner before the ECDH step as two separate elements so that the scanner can multiply b_scan * input_hash before doing ECDH. Add functions for deserializing / serializing a public_data object to and from a public key. When serializing a public_data object, the input_hash is multplied into A_sum. This is so the object can be stored as public key for wallet rescanning later, or to vend to light clients. For the light client, a `_parse` function is added which parses the compressed public key serialization into a `public_data` object. Finally, add test coverage for the receiving API.
Demonstrate sending, scanning, and light client scanning.
Add a benchmark for a full transaction scan and for scanning a single output. Only benchmarks for scanning are added as this is the most performance critical portion of the protocol.
Add the BIP-352 test vectors. The vectors are generated with a Python script that converts the .json file from the BIP to C code: $ ./tools/tests_silentpayments_generate.py test_vectors.json > ./src/modules/silentpayments/vectors.h
Co-authored-by: Jonas Nick <2582071+jonasnick@users.noreply.github.com> Co-authored-by: Sebastian Falbesoner <91535+thestack@users.noreply.github.com>
josibake
force-pushed
the
bip352-silentpayments-module-2025
branch
from
6264c3d to
9e85256
Compare
|
Updated 6264c3d -> 9e85256 (2025_00 -> 2025_01, compare)
|
This PR implements BIP352 - Silent payments. It is recommended to read through the BIP before reviewing this PR.
This is a continuation of the work in #1519 and only opened as a new PR due to the comment history on #1519 becoming quite long and difficult to sift through. It is recommended reviewers go through #1519 for background context, if interested.