Add BIP352 silentpayments module

[josibake]

This PR adds a new Silent Payments (BIP352) module to secp256k1. It is a continuation of the work started in #1471.

The module implements the full protocol, except for transaction input filtering and silent payment address encoding / decoding as those will be the responsibility of the wallet software. It is organized with functions for sending (prefixed with _sender) and receiving (prefixed by _recipient).

For sending

1. Collect private keys into two lists: taproot_seckeys and plain_seckeys
   Two lists are used since the taproot_seckeys may need negation. taproot_seckeys are passed as keypairs to avoid the function needing to compute the public key to determine parity. plain_seckeys are passed as just secret keys
2. Create the _silentpayment_recipient objects
   These structs hold the scan and spend public key and an index for remembering the original ordering. It is expected that a caller will start with a list of silent payment addresses (with the desired amounts), convert these into an array of recipients and then match the generated outputs back to the original silent payment addresses. The index is used to return the generated outputs in the original order
3. Call silentpayments_sender_create_outputs to generate the xonly public keys for the recipients
   This function can be called with one or more recipients. The same recipient may be repeated to generate multiple outputs for the same recipient

For scanning

1. Collect the public keys into two lists taproot_pubkeys and plain_pubeys
   This avoids the caller needing to convert taproot public keys into compressed public keys (and vice versa)
2. Compute the input data needed, i.e. sum the public keys and compute the input_hash
   This is done as a separate step to allow the caller to reuse this output if scanning for multiple scan keys. It also allows a caller to use this function for aggregating the transaction inputs and storing them in an index to vend to light clients later (or for faster rescans when recovering a wallet)
3. Call silentpayments_recipient_scan_outputs to scan the transaction outputs and return the tweak data (and optionally label information) needed for spending later

In addition, a few utility functions for labels are provided for the recipient for creating a label tweak and tweaked spend public key for their address. Finally, two functions are exposed in the API for supporting light clients, _recipient_created_shared_secret and _recipient_create_output_pubkey. These functions enable incremental scanning for scenarios where the caller does not have access to the transaction outputs:

1. Calculating a shared secret
   This is done as a separate step to allow the caller to reuse the shared secret result when creating outputs and avoid needing to do a costly ECDH every time they need to check for an additional output
2. Generate an output (with k = 0)
3. Check if the output exists in the UTXO set (using their preferred light client protocol)
4. If the output exists, proceed by generating a new output from the shared secret with k++

See examples/silentpayments.c for a demonstration of how the API is expected to be used.

Note for reviewers

My immediate goal is to get feedback on the API so that I can pull this module into bitcoin/bitcoin#28122 (silent payments in the bitcoin core wallet). That unblocks from finishing the bitcoin core PRs while work continues on this module.

Notable differences between this PR and the previous version

See #1427 and #1471 for discussions on the API design. This iteration of the module attempts to be much more high level and incorporate the feedback from #1471. I also added a secp256k1_silentpayments_public_data opaque data type, which contains the summed public key and the input_hash. My motivation here was:

1. I caught myself mixing up the order of arguments between A_sum and recipient_spend_key, which was impossible to catch with ARG_CHECKS and would result in the scanning process finishing without errors, but not finding any outputs
2. Combining public key and input_hash into the same data type allows for completely hiding input_hash from the caller, which makes for an overall simpler API IMO

I also removed the need for the recipient to generate a shared secret before using the secp256k1_silentpayments_recipient_scan_outputs function and instead create the shared secret inside the function.

Outstanding work

• clean up the testing code
• improve test coverage (currently only using the BIP352 test vectors)
• optimize the implementation, where possible

[theStack]

Concept ACK

Left some initial feedback, especially around the scanning routine, will do an in-depth review round soon. Didn't look closer at the public_data type routines and the examples yet.

[josibake]

Rebased on #1518 (3d08027 -> 8b48bf1, compare)

[josibake]

Updated 8b48bf1 -> f5585d4 (bip352-silentpayments-module-rebase -> bip352-silentpayments-module-02, compare):

• Fix function documentation for _recipient_scan_outputs
• Replace VERIFY_CHECK with return 0; in _sender_create_outputs
• Remove unneeded declassify code from _sender_create_outputs
• Change _gej_add_ge to _gej_add_var in _recipient_public_data_create
• Fix label scanning in _recipient_scan_outputs
• Remove unneeded prints from the tests

For the label scanning, I looked for an example of using an invalid public key but didn't see anything except for the invalid_pubkey_bytes in the tests. For now, if the output is found without a label, I'm setting found_with_label = 0 and saving the found output in both the output and label field. Happy to change this if there is a better suggestion for communicating an invalid public key.

I also used secp256k1_pubkey_save instead of output = *tx_outputs, as I think this makes the code more clear.

[theStack]

Second review round through, looks good so far! Left a bunch of nits, mostly about naming and missing ARG_CHECKS etc.

[josibake]

Thanks for the thorough review, @theStack ! I've addressed your feedback, along with some other changes.

---

Update f5585d4 -> 1a3a00b (bip352-silentpayments-module-02 -> bip352-silentpayments-module-03, compare)

• Spelling and wording cleanups, notably:
  • s/receiver/recipient/, s/labeled/labelled/
  • s/scan_seckey/scan_key/
• Reduce duplicate code in scan_outputs
• Add ARG_CHECKs
• Update tests
• Add benchmark for scan_outputs

The sending tests now check that the generated outputs match exactly one of the possible expected output sets. Previously, the sending tests were checking that the generated outputs exist in the array of all possible outputs, but this wouldn't catch a bug where k is not being set correctly e.g. [A_k=0, B_k=0] would (incorrectly) pass [A_k=0, B_k=1, A_k=1, B_k=0] but will now (correctly) fail [[A_k=0, B_k=1], [A_k=1, B_k=0]]

[josibake]

Rebased on #1518 1a3a00b -> 92f5920 (bip352-silentpayments-module-03 -> bip352-silentpayments-module-03-rebase, compare)

[josibake]

Rebased on master (following #1518 merge) 92f5920 -> 56ed901 (bip352-silentpayments-module-03-rebase -> bip352-silentpayments-module-04-rebase, compare)

[theStack]

Went through another round. To the best of my knowledge, this PR matches the BIP352 specification and I'm close to non-cryptographer-light-ACKing it :-)

Found some nits an one open TODO that should probably be discussed though.

[josibake]

Rebased on master to fix merge conflict 56ed901 -> bd66eaa (bip352-silentpayments-module-04-rebase -> bip352-silentpayments-module-05-rebase, compare)

[josibake]

CI failure seems related to not being able to install valgrind via homebrew and unrelated to my change so ignoring for now (cc @real-or-random for confirmation?).

[josibake]

Thanks for the review @theStack ! Sorry for the slow response, I somehow missed the notification for your review 😅

---

Update bd66eaa -> 2dde8f1 (bip352-silentpayments-module-05-rebase -> bip352-silentpayments-module-06, compare)

• spelling, grammar, and fixups per @theStack 's review
• Added ARG_CHECKs to check for the sum of the private keys / public keys being zero

Per #1519 (comment), I agree returning 0 is not the right thing to do, but having multiple error codes also seemed gross. I think an ARG_CHECK makes sense here because if the caller passed all valid seckeys / pubkeys and then they sum to zero, in principle its the caller passing incorrect arguments. The only thing the caller can do at this point is try again with different arguments. For the sender, this would mean repeating coin selection to get a different input set, and for the recipient this would mean skipping the transaction and moving on to the next one. Also happy to change if there is a better suggestion!

[real-or-random]

CI failure seems related to not being able to install valgrind via homebrew and unrelated to my change so ignoring for now (cc @real-or-random for confirmation?).

Indeed, see #1536

[real-or-random]

Some general notes

On error handling in general

Error handling is hard, and the caller usually can't really recover from an error anyway. This is in particular true on malicious inputs: there's no reason to try to continue dealing with the attacker, and you simply want to abort. That's why, as a general rule, we try to avoid error paths as much as possible. This usually boils down to merging all errors into a single one, i.e., a) have just a single error "code" for all possible errors, b) and in the case of a multi-stage thing involving multiple function calls, have just a single place where errors are returned.

Signature verification is a good example. A (signature, message, pubkey) triple is either valid or not. The caller should not care why exactly a signature fails to verify, so we don't even want to expose this to the caller.

However, signature verification this is also a nice example of a case in which we stretch the rules a bit. Signature verification is implemented as two-stage process: 1. Parse the public key (which can fail). 2. Check the signature (which can fail). Purely from a "safe" API point of view, this is not great because we give the user two functions and two error paths instead of one. Ideally, there could just be one verification function which also takes care of parsing (this is how it's defined BIP340). The primary reason why we want to have a separate parsing function in this case is performance: if you check several signatures under the same key, you don't want to parse, which involves computing the y-coordinate, every time.

ARG_CHECK

ARG_CHECK will call the "illegal (argument) callback", which, by default, crashes. See the docs here:

secp256k1/include/secp256k1.h

Line 324 in 1791f6f

/** Set a callback function to be called when an illegal argument is passed to

The callback/crash indicates to the caller that there's a bug in the caller's code.

What does this mean for this discussion?

• Added ARG_CHECKs to check for the sum of the private keys / public keys being zero

Per #1519 (comment), I agree returning 0 is not the right thing to do, but having multiple error codes also seemed gross. I think an ARG_CHECK makes sense here because if the caller passed all valid seckeys / pubkeys and then they sum to zero, in principle its the caller passing incorrect arguments. The only thing the caller can do at this point is try again with different arguments. For the sender, this would mean repeating coin selection to get a different input set, and for the recipient this would mean skipping the transaction and moving on to the next one. Also happy to change if there is a better suggestion!

So let's take a look at the two sides:

On the sender side: The secret keys sum up to zero (sum_i a_i = 0)

This will happen only with negligible probability for honestly generated (=random) secret keys. That is, this will in practice only happen if the caller has a bug, or the caller has been tricked into using these secret keys, e.g., if someone else has crafted malicious secret keys for the caller. Since the latter is not a crazy scenario, we should not use ARG_CHECK here.

We can just return 0 here to indicate to the caller that we can't continue with these function inputs. And even if there are other error cases, I don't see a reason why the caller code should care much about why the function fails. As long as you call the function with honestly generated inputs, everything will work out. (Devs will be interested in the exact failure case when debugging the caller's code, but I think they can figure out during debugging then. "Normal" caller code should get just a single error code.)

On the recipient side: The public keys sum up to infinity (sum_i A_i = 0) [1]

Again, this can only happen if the sender is malicious. But since we're not the sender, it's entirely possible that the sender is malicious. And then these inputs are certainly legal, they're just not valid. (In the same sense as it's perfectly legal to use the signature verification algorithm on an invalid signature.) So an ARG_CHECK will not be appropriate at all: a malicious sender could trigger it and crash the scanning process.

We should also simply return 0 to indicate that this transaction is not well-formed/not eligible for SP. And again, even if there are other error cases, I don't see a reason why the caller should care why this transaction is not eligible.

Alternatively, we could even return 1, store infinity in the public_data, and simply make sure that scanning won't find any payments in that case. This would avoid the error path for this function entirely. But if the caller then calls secp256k1_silentpayments_recipient_create_shared_secret, I think we'd just postpone the error to this function, and for this function, I don't see another way than returning an error. So I'm not convinced that this is better.

[1] We should perhaps rename "infinity" to "zero"... ;)

[josibake]

@real-or-random thanks for the response, this is super helpful.

Devs will be interested in the exact failure case when debugging the caller's code, but I think they can figure out during debugging then

In hindsight, I think my preference for ARG_CHECK was "better error messages as to what went wrong," but I now realize it was because I was thinking as a dev ;). Also an oversight on my part: I didn't realize/forgot that ARG_CHECK is actually crashing the program by default. I certainly agree that we don't want this in either failure case.

Alternatively, we could even return 1, store infinity in the public_data, and simply make sure that scanning won't find any payments in that case. This would avoid the error path for this function entirely. But if the caller then calls secp256k1_silentpayments_recipient_create_shared_secret, I think we'd just postpone the error to this function, and for this function, I don't see another way than returning an error. So I'm not convinced that this is better.

If we imagine an index + light client scenario, the public_data would be created by the index and then sent to the light client, where the light client would call secp256k1_silentpayments_recipient_create_shared_secret (and then get the error). Given this, I think it would be better to have the error path so that the index ends up not storing any data at all for the malicious crafted transaction, which saves space for the index and bandwidth for the light client.

---

Thinking about this a bit more:

That's why, as a general rule, we try to avoid error paths as much as possible. This usually boils down to merging all errors into a single one, i.e., a) have just a single error "code" for all possible errors, b) and in the case of a multi-stage thing involving multiple function calls, have just a single place where errors are returned.

Most of the high-level functions in our API are calling multiple lower-level functions and so far the approach has been something like:

if (!secp256k1_func_that_returns_0_on_error(args)) {
    return 0;
}
...
if (!secp256k1_another_func_that_returns_0_on_error(args)) {
    return 0;
}

Perhaps its worth looking to consolidate and try and only return an error at the end of a multi-stage process? This would mean ignoring the return values for a lot of the lower level function calls, which initially made me feel a bit weird. But in light of your recent comment, feels like this might be the preferred approach?

EDIT: reading your comment again, I realize "error paths" is not really talking about branches in the code and more error paths for the user.

[theStack]

We should also simply return 0 to indicate that this transaction is not well-formed/not eligible for SP. And again, even if there are other error cases, I don't see a reason why the caller should care why this transaction is not eligible.

Makes sense. My worry was that without an explicit error-code for this corner case, some users wouldn't even be aware of an indirect "not eligible" case and more likely interpret a return value of 0 as "only possible if there's a logic error on our side, so let's assert for success" (given the passed in data is public and already verified for consensus-validity). But in the end that's more a matter of good API documentation I guess.

An example for the "input public keys sum up to point of infinity" case ($\sum_i A_i = 0$) is now available on the Signet chain via tx d73f4a19f3973e90af6df62e735bb7b31f3d5ab8e7e26e7950651b436d093313 [1], mined in block 198023. It consists of two inputs spending P2WPKH prevouts with negated pubkeys $(x,y)$ and $(x,-y)$ (easy to verify by looking at the second item of the witness stack each, where only the first byte for encoding the sign bit differs), and one dummy P2TR output. It hopefully helps SP implementations to identify potential problems with this corner case early. As first example and proof that it triggers the discussed code path, it makes the Silent Payment Index PR #28241 crash, which asserts on a return value of 1 for _recipient_public_data_create.

I think it would be also a good idea to add this scenario to the BIP352 test vectors, or at least a unit test in this PR?

[1] created with the following Python script: https://github.com/theStack/bitcoin/blob/202405-contrib-bip352_input_pubkeys_cancelled/contrib/silentpayments/submit_input_pubkeys_infinity_tx.py

[jonasnick]

Suggested change

	/** This struct serves as an input parameter for passing the silent payment
	* address data.
	*
	* The index field is for when more than one address is being sent to in
	* a transaction. Index is set based on the original ordering of the addresses
	* and used to return the generated outputs matching the original ordering.
	* When more than one recipient is used, the recipient array will be sorted in
	* place as part of generating the outputs, but the generated outputs will be
	* returned in the original ordering specified by the index to ensure the
	* caller is able to match up the generated outputs to the correct silent
	* payment address (e.g., to be able to assign the correct amounts to the
	* correct generated outputs in the final transaction).
	/** This struct serves as an input parameter for passing the silent payment
	* address data to `silentpayments_sender_create_outputs`.
	*
	* The index field is for when more than one address is being sent to in
	* a transaction. Index is set to the position of this recipient in the `recipients` array passed to `silentpayments_sender_create_outputs`
	* and used to return the generated outputs matching the original ordering.

nit: this is more clear about how to set the index and doesn't repeat what is stated in the silentpayments_sender_create_outputs API doc.

[jonasnick]

The type of k is a bit strange. In scan_outputs it's a size_t. When it gets passed into this function it's a unsigned int, which is only guaranteed to be 16 bits. Then in write_be32 it gets casted to a uint32_t. This means that when size_t is larger than unsigned int or size_t is larger than uint32_t the function will return wrong results.

I suggest to change the type of k in this function from unsigned int to uint32_t and add a branch to scan_outputs that returns 0 if k exceeds UINT32_MAX. Does the BIP 352 spec have a different approach to this?

[jonasnick]

add a branch to scan_outputs that returns 0 if k exceeds UINT32_MAX.

Alternatively, we can change the type of k in the API to uint32_t (and perhaps do the same to m in secp256k1_silentpayments_recipient_create_label.

[josibake]

Good catch - k is underspecified in the BIP. IIRC, we did have it specified as a uint32 in an earlier version of the BIP. I'll update it here to be uint32 and make a note to update the BIP, as well.

[jonasnick]

How about

secp256k1_scalar_set_b32(t_k_scalar, hash_ser, overflow);
VERIFY_CHECK(!overflow);
VERIFY_CHECK(!secp256k1_scalar_is_zero(&t_k_scalar));

[jonasnick]

This comment doesn't seem to be true. This only fails if t_k = -b_spend where b_spend*G = B_spend.

Suggested change

	/* Calculate P_output = B_spend + t_k * G
	* This can fail if t_k overflows the curve order, but this is statistically improbable
	*/
	/* Calculate P_output = B_spend + t_k * G
	* This can fail if t_k equals the negation of the DLog of P_output, but t_k
	* is the output of a hash function.
	*/
	ret = secp256k1_eckey_pubkey_tweak_add(&P_output_ge, &t_k_scalar);

Also, aren't we calculating B_spend and not P_output?

[josibake]

The comment is indeed incorrect, since we aren't ever checking if the output of the hash function (t_k) overflows the curve order.

I can have _create_t_k fail if t_k_scalar overflows, although this introduces a branch that we are unable to test (and is statistically improbable). This would, however, make the function more strictly adhere to the specification

[LLFourn]

Has the topic of using x-only ECDH come up on silent payments before like is used in BIP324? This avoids a square root which I think a small but measurable speed up on an ecmult. Perhaps someone more familiar with the x-only impl in BIP324 can comment on what the exact speed up would be.
Note that in terms of code all that has to change is to make the shared secret just the x coordinate (change it to shared_secret32) and the actual optimization could be implemented later. It seems like it could be worth changing the spec slightly even at this stage to at least leave the door open to taking advantage of this so I thought it'd be worth mentioning.

cc @sipa @real-or-random @jonasnick

[real-or-random]

(@sipa Replying here in the thread to keep things organized in this huge PR.)

@LLFourn I've included the possibility of x-only ECDH (and x-only B_scan) in my review document of the BIP: gist.github.com/sipa/c9299811fb1f56abdcd2451a8a078d20

Note that in terms of code all that has to change is to make the shared secret just the x coordinate (change it to shared_secret32) and the actual optimization could be implemented later. It seems like it could be worth changing the spec slightly even at this stage

Yeah, I think x-only ECDH has fallen between the cracks. For all other protocols, there was not enough pressure to optimize our existing ECDH because it's fast enough (BIP324 is an exception in that we had to redo it anyway from scratch due to ellswift).

But I agree that the situation is different here. Scanning is the bottleneck, and it will be nice to save a few percent.

We even have draft implementations:

• input=xonly, shared_secret=xonly: Add x-only ECDH support to ecdh module #1198 (this one is in a good shape)
• input=compressed, shared_secret=xonly: x-only ECDH without sqrt #262

The big question is whether the spec can be changed at this point, and I'm afraid the answer is no. If I recall correctly, others have picked it up already? @josibake @RubenSomsen

[sipa]

@real-or-random No implementation work is really needed, since the silentpayment module does ECDH internally; it doesn't expect users to use the ecdh module interface, and the internal side of things, the secp256k1_ecmult_const_xonly function, already exists since #1118.

The big question is indeed whether breaking spec changes are still possible. Please have a look at my other questions/suggestions in the doc too, if it is.

[josibake]

Out of curiosity, I tried implementing x-only ecdh using secp256k1_ecmult_const_xonly here: josibake@e7ec39e

When I ran the benchmarks between the old and the new, I didn't see any speedup. This means:

1. I didn't implement the x-only ECDH correctly
2. The benchmark is not accurate
3. The speedup doesn't make a noticeable difference in the context of scanning

@real-or-random @sipa others have indeed picked it up, but it's a small enough group that I think we could sneak in a breaking change and coordinate an update if there is a compelling enough reason to do so. Assuming 3) is correct, this doesn't seem like a compelling enough speedup to warrant a breaking change. However, it would be great if someone could double check my work as I'm a bit skeptical of my results.

My results:

❯ ./new/bin/bench silentpayments
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

silentpayments_full_tx_scan   ,    98.1       ,    98.2       ,    98.7
silentpayments_output_scan    ,    74.5       ,    74.6       ,    74.8

❯ ./old/bin/bench silentpayments
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

silentpayments_full_tx_scan   ,    98.3       ,    98.3       ,    98.3
silentpayments_output_scan    ,    74.5       ,    74.5       ,    74.6

[jonasnick]

@josibake

@real-or-random , @jonasnick I left a TODO comment in the tests where I have two test cases for ensuring the _create_outputs function can handle malformed public keys passed in via the recipients arguments. Since this argument is provided by the user, I think it's reasonable to test for invalid public keys but I couldn't figure out how to do this with the way the CHECK_ILLEGAL macro works. I left a more detailed explanation in the TODO comment, would appreciate suggestions here!

How about this: jonasnick@7c4ecb8. It matches how we test multiple illegal callbacks in test_sort_api(void).

[jonasnick]

#1659

[josibake]

Not sure I follow. The linked PR implies I should remove the SECP256K1_WARN_UNUSED_RESULT if the function always returns 1, but in this case _public_data_serialize does not always return 1?

[jonasnick]

The PR removes SECP256K1_WARN_UNUSED_RESULT for functions that always return 1. The doc of _public_data_serialize indicates that this is such a function.

Looking into _public_data_serialize, it seems like it cannot fail unless the API is used incorrectly or if input_hash is out of range. I think that public data create should only succeed wheninput_hash is in range.

[real-or-random]

VERIFY_CHECK/handle return value secp256k1_ec_pubkey_serialize

[real-or-random]

VERIFY_CHECK/handle return value secp256k1_ec_pubkey_serialize

[jonasnick]

This seems unsafe because we're casting const away for a function that is definitely violating the const. If we only want to use one array for creating and scanning, then we should just remove const from the declaration of tx_output_ptrs.

[real-or-random]

If we only want to use one array for creating and scanning, then we should just remove const from the declaration of tx_output_ptrs.

Strongly agree with this.

For the sake of education, it is safe because the pointers in tx_out_ptrs point to the elements of tx_outputs, which is not declared const. So the "object" we're modifying is not declared const. And only this would be UB: "If an attempt is made to modify an object defined with a const-qualified type through use of an lvalue with non-const-qualified type, the behavior is undefined." (https://port70.net/~nsz/c/c99/n1256.html#6.7.3p5)

But yep, relying on this is probably not the kind of programming style we should encourage in an example. ^^

[josibake]

Good point re: this being an example. I changed this to remove const from the declaration of tx_output_ptrs. This required adding a cast later on when tx_output_ptrs is used as the input to scan_outputs, but since this cast is adding the const declaration, I think this is fine?

[real-or-random]

I haven't checked the code, but is the new cast really necessary? An explicit cast shouldn't be necessary if you're casting to a more "restricted" type.

[josibake]

I had thought the same, but this is the warning I get when I remove the cast:

/root/secp256k1/examples/silentpayments.c: In function ‘main’:
/root/secp256k1/examples/silentpayments.c:421:17: warning: passing argument 4 of ‘secp256k1_silentpayments_recipient_scan_outputs’ from incompatible pointer type [-Wincompatible-pointer-types]
  421 |                 tx_output_ptrs, N_OUTPUTS,
      |                 ^~~~~~~~~~~~~~
      |                 |
      |                 secp256k1_xonly_pubkey **
In file included from /root/secp256k1/examples/silentpayments.c:14:
/root/secp256k1/include/secp256k1_silentpayments.h:342:43: note: expected ‘const secp256k1_xonly_pubkey * const*’ but argument is of type ‘secp256k1_xonly_pubkey **’
  342 |     const secp256k1_xonly_pubkey * const *tx_outputs,
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
[100%] Linking C executable ../bin/silentpayments_example
[100%] Built target silentpayments_example

[jonasnick]

Ideally, the documentation in the include file would indicate whether this can be asserted or not. In the case of create_outputs and potentially others, it's not.

[josibake]

Doing a pass on the full header to make sure the documentation is consistent and up-to-date for all of the functions, will leave this comment open for now.

[jonasnick]

If we're consistent, then I think the return value can be asserted because it fails only if the API is used in a wrong way.

[jonasnick]

add a branch to scan_outputs that returns 0 if k exceeds UINT32_MAX.

Alternatively, we can change the type of k in the API to uint32_t (and perhaps do the same to m in secp256k1_silentpayments_recipient_create_label.

[jonasnick]

nit: The terminology could be more clear. The naming in this struct appears to suggest that a spend_pubkey can either be the "actual" spend pubkey or a labelled spend pubkey. However, everywhere else in the API spend_pubkey is the actual spend pubkey and not the labelled spend pubkey.

[josibake]

Agree, this is pretty confusing. Perhaps spend_pubkey can be renamed to labelled_spend_pubkey, with a comment mentioning that the label is optional? Something like:

/**
 * ...
 * `lablled_spend_pubkey` is the spend public key with an (optional) label tweak applied. 
 * If no label tweak is applied, `labelled_spend_pubkey` is the recipients spend public key.
 * ...
 */
typedef struct {
    secp256k1_pubkey scan_pubkey;
    secp256k1_pubkey labelled_spend_pubkey;
...

[jonasnick]

Suggested change

	* In: recipient_scan_key: pointer to the recipient's scan key
	* In: recipient_scan_key: pointer to the recipient's 32 byte scan key

Maybe also consider calling this argument recipient_scan_key32. Same in scan_outputs and create_shared_secrets.

[jonasnick]

Maybe best to explain the rationale of the combined flag (and tradeoffs) when the public data data structure is defined because it affects multiple functions.

[jonasnick]

I think it would be best if a properly created public_data object would only contain valid data. Then, the overflow check here would be unnecessary.

To detect uninitialized data structure from being passed to functions, we started using a 4 byte magic sequence in the musig module. See for example here,

secp256k1/src/modules/musig/session_impl.h

Line 55 in 70f149b

static const unsigned char secp256k1_musig_secnonce_magic[4] = { 0x22, 0x0e, 0xdc, 0xf1 };

. As a result, the _load functions ARG_CHECK for the correct magic.

[jonasnick]

The BIP does not specify the conversion from the input hash byte array to a scalar. In particular, it doesn't specify whether to fail on overflow or not.

[jonasnick]

Wouldn't the benchmarks be more helpful if the full tx scan also included public data creation because this is effectively what wallets do when encountering a transaction?

[josibake]

Good point, will update.

[josibake]

Rebased 71df073 -> c31114a (bip352-silentpayments-module-18 -> bip352-silentpayments-module-rebased, compare)

• Rebased on master
• Also included a fix for the Valgrind false positive in the example (h/t @real-or-random for helping me troubleshoot this!)

Separating the rebase from addressing feedback for my own sanity and to help make sure I haven't missed any feedback.

[josibake]

Updated c31114a -> 592f251 (bip352-silentpayments-module-rebased -> bip352-silentpayments-module-19, compare)

A few smaller changes, namely:

• Spelling and wording fixup's
• Change k and m to be uint32_t
• Improving test coverage for malformed inputs (h/t @jonasnick)

Bigger changes:

• Introducing magic bytes to ensure public_data is initialised correctly and only contains valid data - this is inspired by the approach in the new musig2 module. Looking for feedback on how I implemented this and on the choice of magic bytes (easy enough to change). Not fully satisfied with how this is implemented but figured I'd push what I had and see what others think
• Reworked the example - notably, showed how to use the spend key + tweak for spending the output and added a check that demonstrates usage of the create_labelled_spend_pubkey function

I'm still working through some of the feedback, namely improving the API documentation in the header and adding some comments in a few places.