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

Add BIP352 silentpayments module #1519

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
241 changes: 241 additions & 0 deletions include/secp256k1_silentpayments.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,247 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipien
const secp256k1_pubkey *label
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);

/** Opaque data structure that holds silent payments public input data.
*
* This structure does not contain secret data. Guaranteed to be 98 bytes in
* size. It can be safely copied/moved. Created with
* `secp256k1_silentpayments_public_data_create`. Can be serialized as a
* compressed public key using
* `secp256k1_silentpayments_public_data_serialize`. The serialization is
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: there is no secp256k1_silentpayments_public_data_serialize, did you mean secp256k1_silentpayments_recipient_public_data_serialize? Did you also mean to drop "as a compressed public key" (since it contains more than that)?

I do think it's useful to briefly document what is in the opaque structure: the summed public key and input_hash.

Also, is there a light client use case that requires more than just tweak data? If not, then this struct could be reduced to 33 bytes. That's what the index uses.

Copy link
Member

@Sjors Sjors Sep 10, 2024

Choose a reason for hiding this comment

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

Alright, this is thoroughly confusing, but I think I figured it out.

I rewrote the documentation to fix the method names and make it more clear that:

  1. this struct can be created in two ways
  2. the struct itself is not intended to be sent to light clients
  3. use terminology from the BIP ("public tweak data")
diff --git a/include/secp256k1_silentpayments.h b/include/secp256k1_silentpayments.h
index 05cabb8..11164e1 100644
--- a/include/secp256k1_silentpayments.h
+++ b/include/secp256k1_silentpayments.h
@@ -159,17 +159,20 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipien
 ) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);
 
 /** Opaque data structure that holds silent payments public input data.
  *
  *  This structure does not contain secret data. Guaranteed to be 98 bytes in
- *  size. It can be safely copied/moved. Created with
- *  `secp256k1_silentpayments_public_data_create`. Can be serialized as a
- *  compressed public key using
- *  `secp256k1_silentpayments_public_data_serialize`. The serialization is
- *  intended for sending the public input data to light clients. Light clients
- *  can use this serialization with
- *  `secp256k1_silentpayments_public_data_parse`.
+ *  size. It can be safely copied/moved.
+ *
+ *  When the summed public key and input_hash are available, it's created using
+ *  `secp256k1_silentpayments_recipient_public_data_create`. Otherwise it can be
+ *  constructed from a compressed public key with the `input_hash` multiplied in
+ *  (public tweak data) using secp256k1_silentpayments_recipient_public_data_parse.
+ *
+ *  To serve light clients, the public tweak data can generated and serialized to
+ *  33 bytes using `secp256k1_silentpayments_recipient_public_data_serialize`,
+ *  which they can parse with `secp256k1_silentpayments_recipient_public_data_parse`.
  */
 typedef struct {
     unsigned char data[98];

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the name of the struct to be silentpayments_recipient_public_data and updated the documentation to have consistent naming.

On the doc rewrite, I'm not convinced this is better. It mentions input_hash explicitly, which is an implementation detail internal to the module, and also mentions some of the low-level operations going on inside the functions, which I don't think we should be exposing/explaining to the caller here. To me, it seems sufficient to tell the caller: "hey, create a public data object using this function. If you're a light client and receive a previously created, serialised public data object, parse it with this function."

Going to leave the documentation as is for now, but would be interested to hear others thoughts on this.

* intended for sending the public input data to light clients. Light clients
* can use this serialization with
* `secp256k1_silentpayments_public_data_parse`.
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: secp256k1_silentpayments_recipient_public_data_parse

*/
typedef struct {
unsigned char data[98];
} secp256k1_silentpayments_public_data;

/** Compute Silent Payment public data from input public keys and transaction
* inputs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* inputs.
* inputs.

*
* Given a list of n public keys A_1...A_n (one for each silent payment
* eligible input to spend) and a serialized outpoint_smallest, create a
* `public_data` object. This object summarizes the public data from the
* transaction inputs needed for scanning.
*
* `outpoint_smallest36` refers to the smallest outpoint lexicographically
* from the transaction inputs (both silent payments eligible and non-eligible
* inputs). This value MUST be the smallest outpoint out of all of the
* transaction inputs, otherwise the recipient will be unable to find the the
* payment.
*
* The public keys have to be passed in via two different parameter pairs, one
* for regular and one for x-only public keys, in order to avoid the need of
* users converting to a common pubkey format before calling this function.
* The resulting data can be used for scanning on the recipient side, or
* stored in an index for later use (e.g. wallet rescanning, vending data to
* light clients).
*
* If calling this function for simply aggregating the public transaction data
* for later use, the caller can save the result with
* `silentpayments_public_data_serialize`.
*
* Returns: 1 if tweak data creation was successful. 0 if an error occured.
* Args: ctx: pointer to a context object
* Out: public_data: pointer to public_data object containing the
* summed public key and input_hash.
* In: outpoint_smallest36: serialized smallest outpoint (lexicographically)
* from the transaction inputs
* xonly_pubkeys: pointer to an array of pointers to taproot
* x-only public keys (can be NULL if no taproot
* inputs are used)
* n_xonly_pubkeys: the number of taproot input public keys
* plain_pubkeys: pointer to an array of pointers to non-taproot
* public keys (can be NULL if no non-taproot
* inputs are used)
* n_plain_pubkeys: the number of non-taproot input public keys
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_public_data_create(
josibake marked this conversation as resolved.
Show resolved Hide resolved
const secp256k1_context *ctx,
secp256k1_silentpayments_public_data *public_data,
const unsigned char *outpoint_smallest36,
const secp256k1_xonly_pubkey * const *xonly_pubkeys,
size_t n_xonly_pubkeys,
const secp256k1_pubkey * const *plain_pubkeys,
size_t n_plain_pubkeys
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

/** Serialize a silentpayments_public_data object into a 33-byte sequence.
*
* Returns: 1 always.
*
* Args: ctx: pointer to a context object
* Out: output33: pointer to a 33-byte array to place the serialized
* `silentpayments_public_data` in
* In: public_data: pointer to an initialized silentpayments_public_data
* object
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_public_data_serialize(
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: the name of this function implies:

  1. That it serializes everything in the struct, which it doesn't
  2. That it merely serializes data, which it doesn't - it performs a hash

Maybe call it secp256k1_silentpayments_get_serialized_public_tweak_data?

And to make it more clear that this thing can go in the function below, you could make a:

/** Can be sent to light clients */
typedef struct {
     unsigned char public_tweak_data[33];
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I would argue it does serialise everything in the struct: it takes a public_data object and returns the serialised format, which is 33 bytes. Also, this function does not do a hash. The input_hash part of the object is created when the public data object is created.

const secp256k1_context *ctx,
unsigned char *output33,
const secp256k1_silentpayments_public_data *public_data
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

/** Parse a 33-byte sequence into a silent_payments_public_data object.
*
* Returns: 1 if the data was able to be parsed.
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing whitespace

* 0 if the sequence is invalid (e.g. does not represent a valid
* public key).
*
* Args: ctx: pointer to a context object.
* Out: public_data: pointer to a silentpayments_public_data object. If 1 is
* returned, it is set to a parsed version of input33.
* In: input33: pointer to a serialized silentpayments_public_data.
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_public_data_parse(
const secp256k1_context *ctx,
secp256k1_silentpayments_public_data *public_data,
const unsigned char *input33
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3);

/** Callback function for label lookups
*
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: Lookup the tweak corresponding to a tweak point.

(or something to that effect, to make it more clear why you need a lookup table)

* This function is implemented by the recipient to check if a value exists in
* the recipients label cache during scanning.
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: the term "cache" is confusing. It implies data that you can derive yourself, but is quicker to lookup. But the function that uses this callback can't do that derivation.

(from a higher level perspective it is a cache, since a wallet will do the necessarily derivations just once)

Better to say "recipients label tweak lookup table" (as the function name does)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think cache is appropriate here? As you mention, the wallet could derive this itself for each scan, but better to derive once and cache the results. "Lookup table" implies a specific data structure to me, which is something I'd like to avoid.

*
* Returns: pointer to the 32-byte label tweak if there is a match.
* NULL pointer if there is no match.
*
* In: label: pointer to the label value to check (computed during
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: BIP352 does not define the terms "label tweak" and "label value". I'm guessing "tweak" refers to hash(bscan || m)·G?

Maybe just add "See secp256k1_silentpayments_recipient_create_label_tweak". From the formula there it seems that "value" is B1 - Bspend. Could call that "tweak point".

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the language to "label pubkey," as this is more consistent with where it is used elsewhere. Also added a reference to the secp256k1_silentpayments_recipient_create_label_tweak function per your suggestion.

* scanning)
* label_context: pointer to the recipients label cache.
*/
typedef const unsigned char* (*secp256k1_silentpayments_label_lookup)(const unsigned char* label33, const void* label_context);

/** Found outputs struct
*
* Struct for holding a found output along with data needed to spend it later.
*
* output: the x-only public key for the taproot output
* tweak: the 32-byte tweak needed to spend the output
* found_with_label: boolean value to indicate if the output was sent to a
* labelled address. If true, label will be set with a valid
* public key.
* label: public key representing the label used.
* If found_with_label = false, this is set to an invalid
* public key.
*/
typedef struct {
secp256k1_xonly_pubkey output;
unsigned char tweak[32];
int found_with_label;
secp256k1_pubkey label;
} secp256k1_silentpayments_found_output;

/** Scan for Silent Payment transaction outputs.
*
* Given a input public sum, an input_hash, a recipient's spend public key
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: , a recipient's scan key b_scan and spend public key

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just "public data" would be clearer than "input public sum and input_hash".

* B_spend, and the relevant transaction outputs, scan for outputs belonging to
* the recipient and return the tweak(s) needed for spending the output(s). An
* optional label_lookup callback function and label_context can be passed if
* the recipient uses labels. This allows for checking if a label exists in
* the recipients label cache and retrieving the label tweak during scanning.
*
* Returns: 1 if output scanning was successful.
* 0 if an error occured.
*
* Args: ctx: pointer to a context object
* Out: found_outputs: pointer to an array of pointers to found
* output objects. The found outputs array MUST
* be initialized to be the same length as the
* tx_outputs array
* n_found_outputs: pointer to an integer indicating the final
* size of the found outputs array. This number
* represents the number of outputs found while
* scanning (0 if none are found)
* In: tx_outputs: pointer to the tx's x-only public key outputs
* n_tx_outputs: the number of tx_outputs being scanned
* recipient_scan_key: pointer to the recipient's scan key
* public_data: pointer to the input public key sum
* (optionally, with the `input_hash` multiplied
* in, see `_recipient_public_data_create`).
Comment on lines +317 to +319
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find anything in the _recipient_public_data_create doc that would explain this optional multiplication.

* recipient_spend_pubkey: pointer to the recipient's spend pubkey
* 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 nullptr (NULL if labels are not
Copy link
Contributor

Choose a reason for hiding this comment

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

s/nullptr/NULL pointer would be more consistent.

* used)
* label_context: pointer to a label context object (NULL if
* labels are not used)
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_scan_outputs(
const secp256k1_context *ctx,
secp256k1_silentpayments_found_output **found_outputs,
size_t *n_found_outputs,
const secp256k1_xonly_pubkey * const *tx_outputs,
size_t n_tx_outputs,
const unsigned char *recipient_scan_key,
const secp256k1_silentpayments_public_data *public_data,
const secp256k1_pubkey *recipient_spend_pubkey,
const secp256k1_silentpayments_label_lookup label_lookup,
const void *label_context
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4)
SECP256K1_ARG_NONNULL(6) SECP256K1_ARG_NONNULL(7) SECP256K1_ARG_NONNULL(8);

/** Create Silent Payment shared secret.
*
* Given the public input data (secp256k1_silentpayments_public_data),
* calculate the shared secret.
Comment on lines +346 to +347
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Given the public input data (secp256k1_silentpayments_public_data),
* calculate the shared secret.
* Given the public input data (secp256k1_silentpayments_public_data)
* and a recipient's scan key, calculate the shared secret.

*
* The resulting shared secret is needed as input for creating silent payments
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: here the word "deriving" would reduce confusion imo. Otherwise it's sounds like this is a sender creating an output (of a transaction).

Maybe make it:

* .. deriving silent payment
* pubkeys for this recipient scan public key, which can be
* matched against transaction outputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same comment as above, derived is heavily associated with BIP32 style public key/private key derivation, so I want to avoid using it here as much as possible.

* outputs belonging to the same recipient scan public key. This function is
* intended for light clients, i.e., scenarios where the caller does not have
* access to the full transaction. If the caller does have access to the full
* transaction, `secp256k1_silentpayments_recipient_scan_outputs` should be
* used instead.
*
* Returns: 1 if shared secret creation was successful. 0 if an error occured.
* Args: ctx: pointer to a context object
* Out: shared_secret33: pointer to the resulting 33-byte shared secret
* In: recipient_scan_key: pointer to the recipient's scan key
* public_data: pointer to the input public key sum, tweaked
* with the input_hash (see
* `_recipient_public_data_create`)
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_create_shared_secret(
Copy link
Member

Choose a reason for hiding this comment

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

94c6e1f: derive_ instead of create_? (from the POV of the universe, the sender created it)

Ditto for recipient_create_output_pubkey below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Slightly prefer create, as I want to avoid any confusion with bip32 style derivation. From the recipients perspective, they are also creating a shared secret per the spec. Its only after scanning with the shared secret that they can confirm this transaction was indeed a silent payment (i.e., the sender created a shared secret and outputs from that shared secret).

const secp256k1_context *ctx,
unsigned char *shared_secret33,
const unsigned char *recipient_scan_key,
const secp256k1_silentpayments_public_data *public_data
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);

/** Create Silent Payment output public key.
*
* Given a shared_secret, a public key B_spend, and an output counter k,
* create an output public key.
*
* This function is used by the recipient when scanning for outputs without
* access to the transaction outputs (e.g. using BIP158 block filters). When
* scanning with this function, it is the scanners responsibility to determine
* if the generated output exists in a block before proceeding to the next
* value of `k`.
*
* Returns: 1 if output creation was successful. 0 if an error occured.
* Args: ctx: pointer to a context object
* Out: P_output_xonly: pointer to the resulting output x-only pubkey
* In: shared_secret33: shared secret, derived from either sender's
* or recipient's perspective with routines from
* above
* recipient_spend_pubkey: pointer to the recipient's spend pubkey
* (labelled or unlabelled)
* k: output counter (initially set to 0, must be
* incremented for each additional output created
* or after each output found when scanning)
*/
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_create_output_pubkey(
const secp256k1_context *ctx,
secp256k1_xonly_pubkey *P_output_xonly,
const unsigned char *shared_secret33,
const secp256k1_pubkey *recipient_spend_pubkey,
unsigned int k
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4);

#ifdef __cplusplus
}
#endif
Expand Down
Loading