Add BIP352 `silentpayments` module #1519

josibake · 2024-04-19T14:21:15Z

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

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

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)
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)
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:

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
Generate an output (with k = 0)
Check if the output exists in the UTXO set (using their preferred light client protocol)
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:

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

src/modules/silentpayments/main_impl.h

include/secp256k1_silentpayments.h

src/modules/silentpayments/main_impl.h

src/modules/silentpayments/tests_impl.h

src/modules/silentpayments/main_impl.h

include/secp256k1_silentpayments.h

src/modules/silentpayments/main_impl.h

josibake · 2024-04-24T08:45:36Z

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

josibake · 2024-04-24T09:39:20Z

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.

src/modules/silentpayments/main_impl.h

include/secp256k1_silentpayments.h

src/modules/silentpayments/main_impl.h

theStack · 2024-04-28T01:44:35Z

examples/silentpayments.c

+                } else {
+                    printf("Failed to create keypair\n");
+                    return 1;
+                }


nit: i think the else-brach can be removed (or at least, the return 1; in it); if keypair creation fails, we want to continue in the loop and try with another secret key

Really done?

edit: Well, we should change the other examples and perhaps also the API docs. Creating keys in a while loop is not good practice. If you hit an invalid key, you're most probably not very lucky, but very unlucky because your randomness is broken. But hey, yeah, let's just yolo and try again. :)

So I guess either is fine for now: keep the loop for consistency with the other examples, or just return 1 here, but having a loop and the else branch is certainly a smell.

Whoops! I think I had a local commit for this but forgot to squash it in.

Regarding best practices, creating the keys in a loop seemed excessive to me but figured I'd just copy the existing examples in case there was something I wasn't understanding.

Considering this is new code, seems fine to me to break from the other examples and then I can open a separate PR to clean up the examples/API docs.

Actually fixed this time to take out the while loop and return if it fails to create the key.

josibake · 2024-05-03T08:25:39Z

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 · 2024-05-03T11:17:30Z

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

josibake · 2024-05-08T12:50:33Z

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.

include/secp256k1_silentpayments.h

src/modules/silentpayments/main_impl.h

include/secp256k1_silentpayments.h

src/bench.c

src/modules/silentpayments/main_impl.h

josibake · 2024-05-31T12:36:59Z

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

josibake · 2024-05-31T12:45:21Z

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 · 2024-05-31T13:45:02Z

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 · 2024-05-31T14:22:24Z

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 · 2024-05-31T16:23:47Z

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 · 2024-05-31T18:06:38Z

@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 · 2024-06-01T00:23:18Z

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

josibake · 2024-08-06T15:32:20Z

Update b2b904b -> 3165b6b (bip352-silentpayments-module-14 -> bip352-silentpayments-module-15, compare)

Add test for malformed secp256k1_pubkey_load
Handle _pubkey_load error missed in the last push

@theStack I spent some time trying to create a test where we create a secp256k1_pubkey object that is in an invalid state, but it doesn't seem possible, since errors are caught when calling _ec_pubkey_parse. This is why I decided not to make the internal create_shared_secret function handle an error when loading a public key because by that point the public key object has either been a) created by us, or b) validated by us.

In all other cases, I think it makes sense to check the error because the pubkey object would have been created by the caller and we have no way of knowing whether or not they created it correctly (e.g., using the ec_pubkey_parse function).

jlest01 · 2024-08-07T01:03:53Z

Given the following secp256k1_silentpayments_public_data :

0x00043e715c3499e40448bf23b53113f7218c5b9805b454a1003eb1009339a48ef69385272624f954ce1e87000eb8dc162e7952ede7d0d0d1d0e38dd7f0588101fe9506a48141afe37530269d77c297681e7efe6cb1d516f7ed50634570d3e0e0c54f

If serialized using secp256k1_silentpayments_recipient_public_data_serialize, the result will be:

0x039fefa1c745f46da57dc11376e80eb6c11cdd0e28733c50eae777cf5c4b0f95d7.

So, if this same light_client_data33 is parsed back to secp256k1_silentpayments_public_data using secp256k1_silentpayments_recipient_public_data_parse, the result will be:

0x01049fefa1c745f46da57dc11376e80eb6c11cdd0e28733c50eae777cf5c4b0f95d76df2de74016f234aef7957b66fa7a5eb8be025ded9122e32c50556f088efbbff0000000000000000000000000000000000000000000000000000000000000000

This is not clear to me. Shouldn't parsing the serialized data result in the same secp256k1_silentpayments_public_data?

josibake · 2024-08-07T07:53:49Z

Thanks for testing @jlest01 . The public data object contains a public key and 32 byte input_hash. This can be used directly in _scan_outputs or it can be serialised for later use (in an index or for sending to light clients). When the data is serialised , the public key is tweaked with the input_hash scalar, i.e., $A_{tweaked} = inputhash \cdot A_{sum}$.

$A_{tweaked}$ is what gets serialised, so when $A_{tweaked}$ is deserialised the public data object now only contains a public key. In your example, you can see that the compressed serialisation of the first public data object has the x coordinate 9fefa1c745f46da57dc11376e80eb6c11cdd0e28733c50eae777cf5c4b0f95d7, which is the same x-coordinate in the de-serialised public data at the end. This isn't really explained in the API docs, by design, since this is supposed to be an opaque data type and these details aren't really relevant for the caller.

theStack · 2024-08-08T15:17:41Z

@theStack I spent some time trying to create a test where we create a secp256k1_pubkey object that is in an invalid state, but it doesn't seem possible, since errors are caught when calling _ec_pubkey_parse. This is why I decided not to make the internal create_shared_secret function handle an error when loading a public key because by that point the public key object has either been a) created by us, or b) validated by us.

Thanks for following up! It seems that for the sender API function, the public component (i.e. a recipient's scan public key) is never validated though? With the following change in the send API test:

diff --git a/src/modules/silentpayments/tests_impl.h b/src/modules/silentpayments/tests_impl.h
index fdca422..bb4e4b9 100644
--- a/src/modules/silentpayments/tests_impl.h
+++ b/src/modules/silentpayments/tests_impl.h
@@ -254,7 +254,7 @@ static void test_send_api(void) {
      * trying to parse the public key with _ec_pubkey_parse
      */
     p[0] = ALICE_SECKEY;
-    memset(&r[0].spend_pubkey.data, 0, sizeof(secp256k1_pubkey));
+    memset(&r[0].scan_pubkey.data, 0, sizeof(secp256k1_pubkey));
     CHECK_ILLEGAL(CTX, secp256k1_silentpayments_sender_create_outputs(CTX, op, rp, 2, SMALLEST_OUTPOINT, NULL, 0, p, 1));
 }

the test fails on that pubkey's serialization in the shared secret creation helper:

$ ./tests
test count = 64
random seed = de0af255fa70eafa2f1bb5b09fdfd112
src/modules/silentpayments/main_impl.h:86: test condition failed: ret && len == 33
Aborted (core dumped)

theStack

Did another review round, left a few nitty-nits below. Overall I think the module is in a pretty good state. Would be nice if the "friendly neighborhood cryptographers" could give an opinion on the remaining few TODOs regarding _cmov calls following _set_b32-seckey :-) I think the answer to most of them is just "no":

https://github.com/josibake/secp256k1/blob/3165b6b091a30a4ace948d67d55142c61a12929d/src/modules/silentpayments/main_impl.h#L193
https://github.com/josibake/secp256k1/blob/3165b6b091a30a4ace948d67d55142c61a12929d/src/modules/silentpayments/main_impl.h#L202
https://github.com/josibake/secp256k1/blob/3165b6b091a30a4ace948d67d55142c61a12929d/src/modules/silentpayments/main_impl.h#L455
https://github.com/josibake/secp256k1/blob/3165b6b091a30a4ace948d67d55142c61a12929d/src/modules/silentpayments/main_impl.h#L585

theStack · 2024-08-08T14:00:55Z

src/modules/silentpayments/main_impl.h

+    secp256k1_ge ss, pk;
+    size_t len;
+    int ret;
+    memset(shared_secret33, 0, 33);


nit: I think this memset can be removed, as there is no early return in this function and it's guaranteed that shared_secret33 will be filled with the _pubkey_serialize call below

theStack · 2024-08-08T14:04:24Z

src/modules/silentpayments/main_impl.h

+    (void)ret;
+}


could clear out the ge/gej shared secret representations at the end (with the same "while not technically 'secret' data..." argumentation that's already used in other functions)

Suggested change

(void)ret;

}

(void)ret;

secp256k1_ge_clear(&ss);

secp256k1_gej_clear(&ss_j);

}

theStack · 2024-08-08T15:53:34Z

src/modules/silentpayments/main_impl.h

+        return ret;
+    }
+    secp256k1_gej_set_ge(&result_gej, &B_m);
+    secp256k1_gej_add_ge_var(&result_gej, &result_gej, &label_addend, NULL);


should we add a check here if the result is the point at infinity and return 0?

theStack · 2024-08-08T16:43:29Z

include/secp256k1_silentpayments.h

+ *       recipient_spend_pubkey: pointer to the recipient's spend pubkey
+  *                label_lookup: pointer to a callback function for looking up


whitespace nit

Suggested change

* recipient_spend_pubkey: pointer to the recipient's spend pubkey

* label_lookup: pointer to a callback function for looking up

* recipient_spend_pubkey: pointer to the recipient's spend pubkey

* label_lookup: pointer to a callback function for looking up

theStack · 2024-08-08T16:47:07Z

src/modules/silentpayments/main_impl.h

+    secp256k1_silentpayments_calculate_input_hash(input_hash_local, outpoint_smallest36, &A_sum_ge);
+    /* serialize the public_data struct */
+    public_data->data[0] = 0;
+    secp256k1_eckey_pubkey_serialize(&A_sum_ge, &public_data->data[1], &pubkeylen, 0);


could check the return value and pubkeylen here if VERIFY is set (like also done in e.g. _silentpayments_calculate_input_hash), e.g.

Suggested change

secp256k1_eckey_pubkey_serialize(&A_sum_ge, &public_data->data[1], &pubkeylen, 0);

ser_ret = secp256k1_eckey_pubkey_serialize(&A_sum_ge, &public_data->data[1], &pubkeylen, 0);

VERIFY_CHECK(ser_ret && pubkeylen == 65);

(void)ser_ret;

theStack · 2024-08-08T17:05:16Z

examples/silentpayments.c

+    /* Randomizing the context is recommended to protect against side-channel
+     * leakage See `secp256k1_context_randomize` in secp256k1.h for more


missing dot nit

Suggested change

/* Randomizing the context is recommended to protect against side-channel

* leakage See `secp256k1_context_randomize` in secp256k1.h for more

/* Randomizing the context is recommended to protect against side-channel

* leakage. See `secp256k1_context_randomize` in secp256k1.h for more

theStack · 2024-08-08T17:11:30Z

include/secp256k1_silentpayments.h

-/* This module provides an implementation for Silent Payments, as specified in BIP352.
- * This particularly involves the creation of input tweak data by summing up private
- * or public keys and the derivation of a shared secret using Elliptic Curve Diffie-Hellman.
- * Combined are either:
- *   - spender's private keys and recipient's public key (a * B, sender side)
- *   - spender's public keys and recipient's private key (A * b, recipient side)
- * With this result, the necessary key material for ultimately creating/scanning
- * or spending Silent Payment outputs can be determined.
+/* This module provides an implementation for Silent Payments, as specified in
+ * BIP352. This particularly involves the creation of input tweak data by
+ * summing up private or public keys and the derivation of a shared secret
+ * using Elliptic Curve Diffie-Hellman. Combined are either: - spender's
+ * private keys and recipient's public key (a * B, sender side) - spender's
+ * public keys and recipient's private key (A * b, recipient side) With this
+ * result, the necessary key material for ultimately creating/scanning or
+ * spending Silent Payment outputs can be determined.


here and below: seems like these format changes should be already part of the first commit

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 Finally, add tests for the sender API.

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 labelled addresses and b) to populate a labels cache when scanning. Add function for creating a labelled 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 labelled silent payment address. Add tests for the label API.

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 recieiving 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

josibake · 2024-08-15T10:57:22Z

Update 3165b6b -> f42e0dd (bip352-silentpayments-module-15 -> bip352-silentpayments-module-16, compare)

Added test for malformed scan_pubkey and moved _pubkey_load out of _create_shared_secret. Overall, I think this makes the error handling more clear to validate the public keys before calling create_shared_secret
Added VERIFY and infinity checks (h/t @theStack)
Spelling and formatting fix-ups (h/t @theStack)

Thanks again for the thorough review, @theStack !

theStack

Light ACK modulo some more smaller doc and test nits I found

As an additional testing idea, could add some checks that the initialized "BIP0352/..." tagged hashes have the expected state (inspired by the MuSig2 PR), but that seems fine to also do in a follow-up.

theStack · 2024-08-19T21:52:03Z

src/modules/silentpayments/main_impl.h

+    for (i = 0; i < n_recipients; i++) {
+        if ((i == 0) || (secp256k1_ec_pubkey_cmp(ctx, &last_recipient.scan_pubkey, &recipients[i]->scan_pubkey) != 0)) {
+            /* If we are on a different scan pubkey, its time to recreate the the shared secret and reset k to 0.
+             * It's very unlikely tha the scan public key is invalid by this point, since this means the caller would


Suggested change

* It's very unlikely tha the scan public key is invalid by this point, since this means the caller would

* It's very unlikely that the scan public key is invalid by this point, since this means the caller would

theStack · 2024-08-19T22:22:44Z

include/secp256k1_silentpayments.h

+/** Create Silent Payment labelled spend public key.
+ *
+ *  Given a recipient's spend public key B_spend and a label, calculate the
+ *  corresponding serialized labelled spend public key:


Suggested change

* corresponding serialized labelled spend public key:

* corresponding labelled spend public key:

(I assume that's a leftover from an earlier version of the API where the resulting label was indeed serialized)

theStack · 2024-08-19T22:33:43Z

src/modules/silentpayments/tests_impl.h

+
+    /* Create a label and labelled spend public key, verify we get the expected result */
+    CHECK(secp256k1_ec_pubkey_parse(CTX, &s, BOB_ADDRESS[1], 33));
+    CHECK(secp256k1_silentpayments_recipient_create_label_tweak(CTX, &l, lt, ALICE_SECKEY, 1));


nit: could also check the label and label tweak results here

theStack · 2024-08-19T22:57:33Z

include/secp256k1_silentpayments.h

+ *  Given the public input data (secp256k1_silentpayments_public_data),
+ *  calculate the shared secret.


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.

theStack · 2024-08-19T23:18:09Z

src/modules/silentpayments/main_impl.h

+        secp256k1_silentpayments_create_t_k(&t_k_scalar, shared_secret, k);
+
+        /* Calculate P_output = B_spend + t_k * G
+         * This can fail if t_k overflows the curver order, but this is statistically improbable


Suggested change

* This can fail if t_k overflows the curver order, but this is statistically improbable

* This can fail if t_k overflows the curve order, but this is statistically improbable

theStack · 2024-08-20T16:27:36Z

src/modules/silentpayments/tests_impl.h

+    /* Try to create a shared secret with a malformed recipient scan key (all zeros) */
+    CHECK(secp256k1_silentpayments_recipient_create_shared_secret(CTX, o, MALFORMED_SECKEY, &pd) == 0);
+    /* Try to create a shared secret with a malformed public key (all zeros) */
+    memset(&pd.data[1], 0, sizeof(&pd.data - 1));


I think this was meant to be

Suggested change

memset(&pd.data[1], 0, sizeof(&pd.data - 1));

memset(&pd.data[1], 0, sizeof(pd.data) - 1));

(resulting in 97, rather than 8 due to being the sizeof a pointer)

jlest01 · 2024-08-24T15:04:37Z

The function secp256k1_silentpayments_recipient_public_data_create fails for the public keys:

. 02aba9d21ae5bb3832e845eeca8c0b58e07b27b501debc759cdccb9d86beef3d48
. 03aba9d21ae5bb3832e845eeca8c0b58e07b27b501debc759cdccb9d86beef3d48

And the smallest outpoint:

. 66bd2917be75420a789924737f3e47500bf134cea57a10ce67bf33d8a256e7b000000000

These values represent this signet P2TR transaction:
https://mempool.space/signet/tx/d73f4a19f3973e90af6df62e735bb7b31f3d5ab8e7e26e7950651b436d093313

Code below for reproduction:

static unsigned char smallest_outpoint[36] = {
    0x66, 0xbd, 0x29, 0x17, 0xbe, 0x75, 0x42, 0x0a, 0x78, 
    0x99, 0x24, 0x73, 0x7f, 0x3e, 0x47, 0x50, 0x0b, 0xf1, 
    0x34, 0xce, 0xa5, 0x7a, 0x10, 0xce, 0x67, 0xbf, 0x33, 
    0xd8, 0xa2, 0x56, 0xe7, 0xb0, 0x00, 0x00, 0x00, 0x00
};

static unsigned char first_pubkey_bytes[33] = {
    0x02, 0xab, 0xa9, 0xd2, 0x1a, 0xe5, 0xbb, 0x38, 
    0x32, 0xe8, 0x45, 0xee, 0xca, 0x8c, 0x0b, 0x58,
    0xe0, 0x7b, 0x27, 0xb5, 0x01, 0xde, 0xbc, 0x75, 
    0x9c, 0xdc, 0xcb, 0x9d, 0x86, 0xbe, 0xef, 0x3d, 0x48
};

static unsigned char second_pubkey_bytes[33] = {
    0x03, 0xab, 0xa9, 0xd2, 0x1a, 0xe5, 0xbb, 0x38, 
    0x32, 0xe8, 0x45, 0xee, 0xca, 0x8c, 0x0b, 0x58,
    0xe0, 0x7b, 0x27, 0xb5, 0x01, 0xde, 0xbc, 0x75, 
    0x9c, 0xdc, 0xcb, 0x9d, 0x86, 0xbe, 0xef, 0x3d, 0x48
};

int main(void) {

    enum { N_INPUTS = 2 };

    unsigned char randomize[32];

    secp256k1_pubkey first_pubkey;
    secp256k1_pubkey second_pubkey;
    const secp256k1_pubkey *tx_input_ptrs[N_INPUTS];
    secp256k1_silentpayments_public_data public_data;

    int ret;

    secp256k1_context* ctx = secp256k1_context_create(SECP256K1_CONTEXT_NONE);
    if (!fill_random(randomize, sizeof(randomize))) {
        printf("Failed to generate randomness\n");
        return 1;
    }

    ret = secp256k1_context_randomize(ctx, randomize);
    assert(ret);

    ret = secp256k1_ec_pubkey_parse(ctx, &first_pubkey, first_pubkey_bytes, 33);
    ret = secp256k1_ec_pubkey_parse(ctx, &second_pubkey, second_pubkey_bytes, 33);

    tx_input_ptrs[0] = &first_pubkey;
    tx_input_ptrs[1] = &second_pubkey;

    ret = secp256k1_silentpayments_recipient_public_data_create(ctx,
                &public_data,
                smallest_outpoint,
                NULL, 0, 
                tx_input_ptrs, N_INPUTS
            );

    assert(ret); // main: Assertion `ret' failed.

    return 0;

}

jlest01 · 2024-08-24T15:48:51Z

Testing the same values using rust silentpayments crate, you get an error description: Secp256k1Error(InvalidPublicKeySum) (code below).

I assume the problem is that adding these two public keys results in a point at infinity (since they have opposite parity).

Would it be the caller's responsibility to do this check before calling secp256k1_silentpayments_recipient_public_data_create ?

use silentpayments::{secp256k1::PublicKey, utils::receiving::calculate_tweak_data};

fn main() {

    let mut pubkeys: Vec<PublicKey> = Vec::with_capacity(2);

    let first_pubkey_bytes = hex::decode("02aba9d21ae5bb3832e845eeca8c0b58e07b27b501debc759cdccb9d86beef3d48").unwrap();
    let first_pubkey = PublicKey::from_slice(first_pubkey_bytes.as_slice()).unwrap();
    pubkeys.push(first_pubkey);

    let second_pubkey_bytes = hex::decode("03aba9d21ae5bb3832e845eeca8c0b58e07b27b501debc759cdccb9d86beef3d48").unwrap();
    let second_pubkey = PublicKey::from_slice(second_pubkey_bytes.as_slice()).unwrap();
    pubkeys.push(second_pubkey);

    let input_pub_keys: Vec<&PublicKey> = pubkeys.iter().collect();
    
    let mut outpoints_data: Vec<(String, u32)> = Vec::with_capacity(2);

    outpoints_data.push(("b0e756a2d833bf67ce107aa5ce34f10b50473e7f732499780a4275be1729bd66".to_string(), 0));
    outpoints_data.push(("b0e756a2d833bf67ce107aa5ce34f10b50473e7f732499780a4275be1729bd66".to_string(), 1));

    let tweak_data = calculate_tweak_data(&input_pub_keys, &outpoints_data).unwrap();
    // panicked! called `Result::unwrap()` on an `Err` value: Secp256k1Error(InvalidPublicKeySum)
}

josibake · 2024-08-26T13:00:31Z

Hey @jlest01 , thanks for the thorough testing! The error you're encountering is from here:

secp256k1/src/modules/silentpayments/main_impl.h

Lines 369 to 374 in f42e0dd

    
               /* Since an attacker can maliciously craft transactions where the public keys sum to zero, fail early here 
        
                * to avoid making the caller do extra work, e.g., when building an index or scanning many malicious transactions 
        
                */ 
        
               if (secp256k1_gej_is_infinity(&A_sum_gej)) { 
        
                   return 0; 
        
               }

As you mention, this is because the public keys sum to the point at infinity, i.e., the second pubkey is a negation of the first one. It is expected that callers should check the return value from _public_data_create and proceed to the next transaction if they get an error. Earlier in this PR we did discuss having more specific error codes but ultimately decided it's best to try and make it such that if a user encounters an error, there's likely nothing they can do but skip the transaction. From a user's perspective, if they can't do anything to fix the error, they don't really gain anything by knowing why it failed.

I think we have a case for this in the test vectors, but if not I'll definitely add this to the BIP test vectors since scanning implementations should be checking for this.

antonilol · 2024-09-07T14:04:01Z

src/modules/silentpayments/main_impl.h

+    const secp256k1_silentpayments_public_data *public_data,
+    const secp256k1_pubkey *recipient_spend_pubkey,
+    const secp256k1_silentpayments_label_lookup label_lookup,
+    const void *label_context


Could label_context (here and in the callback type) be made mutable? Is there a particular reason it is const?

Does this actually disallow (make undefined behavior) mutating the context from the callback?

antonilol · 2024-09-07T16:23:13Z

src/modules/silentpayments/main_impl.h

+    if (label_lookup != NULL) {
+        ARG_CHECK(label_context != NULL);
+    } else {
+        ARG_CHECK(label_context == NULL);
+    }


Why is the label_context ARG_CHECK'ed, shouldn't this be treated as opaque data for the callback alone?

That means the (wallet) implementer has to make sure to perform these checks in their implementation of secp256k1_silentpayments_label_lookup. Seems better to do it here.

That seems unnecessary, as both the callback and the context pointer are passed by the caller. If the caller wants to use global state or some other way that does not require label_context it should be allowed to set it to NULL.

Sjors

Reviewed the scaffold, send, labels and part of the receive commit (94c6e1f). Did not study the tests.

You could split the light client / indexer functions out of the receive commit 94c6e1f.

Sjors · 2024-09-09T08:18:38Z

tools/tests_silentpayments_generate.py

@@ -0,0 +1,297 @@
+#!/usr/bin/env python3


5ce0db1

+''' +A script to convert BIP352 test vectors from JSON to a C header. + +Usage: + + ./tools/tests_silentpayments_generate.py src/modules/silentpayments/bip352_send_and_receive_test_vectors.json > ./src/modules/silentpayments/vectors.h +'''

Sjors · 2024-09-09T08:19:43Z

tools/tests_silentpayments_generate.py

+    if not last:
+        out += ","
+    out += "\n"
+


5ce0db1

if len(sys.argv) != 2: print("Usage: tests_silentpayments_generate.py vectors.json > vectors.h") sys.exit(1)

Sjors · 2024-09-09T08:35:17Z

src/modules/silentpayments/tests_impl.h

+#define SECP256K1_MODULE_SILENTPAYMENTS_TESTS_H
+
+#include "../../../include/secp256k1_silentpayments.h"
+#include "include/secp256k1.h"


9d6769f: You don't need this include and it breaks the cmake build.

cd build cmake .. -DSECP256K1_ENABLE_MODULE_SILENT_PAYMENTS=O cmake --build . ... .../secp256k1/src/modules/silentpayments/tests_impl.h:11:10: fatal error: 'include/secp256k1.h' file not found #include "include/secp256k1.h"

Sjors · 2024-09-09T08:39:55Z

configure.ac

@@ -188,6 +188,10 @@ AC_ARG_ENABLE(module_ellswift,
    AS_HELP_STRING([--enable-module-ellswift],[enable ElligatorSwift module [default=yes]]), [],
    [SECP_SET_DEFAULT([enable_module_ellswift], [yes], [yes])])

+AC_ARG_ENABLE(module_silentpayments,
+    AS_HELP_STRING([--enable-module-silentpayments],[enable Silent Payments module [default=no]]), [],


1c74941: can we just break the convention and make it --enable-module-silent-payments? :-)

Sjors · 2024-09-09T09:52:08Z

include/secp256k1_silentpayments.h

+ *                            recipient may passed multiple times to generate
+ *                            multiple outputs for the same recipient
+ *       outpoint_smallest36: serialized smallest outpoint (lexicographically)
+ *                            from the transaction inputs


9d6769f: the 36 suffix is weird. outpoint36_smallest would be slightly more clear.

Suggest adding to the doc (instead): (36 bytes)

Given how many times this has gone wrong, the following warning seems justified:

Lexicographical sorting applies to the concatenated outpoint hash and index (vout). The latter is little-endian encoded, so must not be sorted in its integer representation.

The test vector in this PR doesn't prevent this, because sorting is left to the implementer.

Test vectors in the BIP don't necessary prevent this either, because implementers will assume it's all covered in libsecp.

Sjors · 2024-09-09T13:08:30Z

src/modules/silentpayments/main_impl.h

+    /* private keys used for taproot outputs have to be negated if they resulted in an odd point */
+    for (i = 0; i < n_taproot_seckeys; i++) {
+        secp256k1_ge addend_point;
+        /* TODO: why don't we need _cmov here after calling keypair_load? Because the ret is declassified? */


9d6769f: My impression is that _cmov is used to clear addend in case _load fails. But since we don't have an early return inside the loop, might as well do it at the end of the loop.

Sjors · 2024-09-09T13:15:25Z

src/modules/silentpayments/main_impl.h

+        /* TODO: why don't we need _cmov here after calling keypair_load? Because the ret is declassified? */
+        ret &= secp256k1_keypair_load(ctx, &addend, &addend_point, taproot_seckeys[i]);
+        if (secp256k1_fe_is_odd(&addend_point.y)) {
+            secp256k1_scalar_negate(&addend, &addend);


Could simplify the early return below by:

9d6769f: ret &=

Sjors · 2024-09-09T13:15:50Z

src/modules/silentpayments/main_impl.h

+        }
+        secp256k1_scalar_add(&a_sum_scalar, &a_sum_scalar, &addend);
+    }
+    /* If there are any failures in loading/summing up the secret keys, fail early */


9d6769f

const int zero = 0; secp256k1_int_cmov(addend, &zero, !ret); secp256k1_int_cmov(a_sum_scalar, &zero, !ret); /* If there are any failures ... */ if (!ret) return 0;

Sjors · 2024-09-09T14:16:45Z

include/secp256k1_silentpayments.h

+ *  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


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.

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:

this struct can be created in two ways

the struct itself is not intended to be sent to light clients

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];

Sjors · 2024-09-09T14:17:13Z

include/secp256k1_silentpayments.h

+ *  `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`.


94c6e1f: secp256k1_silentpayments_recipient_public_data_parse

Sjors · 2024-09-10T10:05:42Z

src/modules/silentpayments/main_impl.h

+    /* serialize the public_data struct */
+    public_data->data[0] = 0;
+    secp256k1_eckey_pubkey_serialize(&A_sum_ge, &public_data->data[1], &pubkeylen, 0);
+    ser_ret = secp256k1_eckey_pubkey_serialize(&A_sum_ge, &public_data->data[1], &pubkeylen, 0);


94c6e1f: why are you doing this twice?

Maybe use a compressed key? I know the struct doesn't need to be particularly small, but it seems better to use compressed keys by default unless there's a reason not to.

Sjors

Reviewed the rest. This PR @ f42e0dd seems to be in pretty good shape, but I'm not a low level C guru.

I didn't check the changes since my last review of the example, but its usage of labels is now clear to me.

I didn't review the tests and benchmarks.

Sjors · 2024-09-10T11:24:51Z

include/secp256k1_silentpayments.h

+ *  In:   public_data: pointer to an initialized silentpayments_public_data
+ *                     object
+ */
+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_public_data_serialize(


94c6e1f: the name of this function implies:

That it serializes everything in the struct, which it doesn't

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]; }

Sjors · 2024-09-10T11:37:05Z

include/secp256k1_silentpayments.h

+ *  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


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

Sjors · 2024-09-10T11:53:45Z

include/secp256k1_silentpayments.h

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


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

Sjors · 2024-09-10T12:05:04Z

include/secp256k1_silentpayments.h

+ *                              with the input_hash (see
+ *                              `_recipient_public_data_create`)
+ */
+SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_create_shared_secret(


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

Ditto for recipient_create_output_pubkey below.

Sjors · 2024-09-10T12:09:52Z

include/secp256k1_silentpayments.h

+ *  Given the public input data (secp256k1_silentpayments_public_data),
+ *  calculate the shared secret.
+ *
+ *  The resulting shared secret is needed as input for creating silent payments


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.

Sjors · 2024-09-10T13:12:19Z

src/modules/silentpayments/main_impl.h

+    } else {
+        ARG_CHECK(label_context == NULL);
+    }
+    /* TODO: do we need a _cmov call here to avoid leaking information about the scan key?


94c6e1f: leaking privacy still sucks, and afaik the _cmov is super cheap. So just do it?

Sjors · 2024-09-10T13:13:41Z

src/modules/silentpayments/main_impl.h

+    if (label_lookup != NULL) {
+        ARG_CHECK(label_context != NULL);
+    } else {
+        ARG_CHECK(label_context == NULL);
+    }


That means the (wallet) implementer has to make sure to perform these checks in their implementation of secp256k1_silentpayments_label_lookup. Seems better to do it here.

Sjors · 2024-09-10T13:26:10Z

src/modules/silentpayments/main_impl.h

+    if (!ret) {
+        return 0;
+    }
+    combined = (int)public_data->data[0];


94c6e1f: so much for opaque datatype :-)

Maybe add a helper function secp256k1_silentpayments_recipient_public_data_get_tweak_data_scalar(&rsk_scalar, recipient_scan_key, public_data)

Sjors · 2024-09-10T13:45:55Z

src/modules/silentpayments/main_impl.h

+        ret &= secp256k1_eckey_pubkey_tweak_add(&P_output_ge, &t_k_scalar);
+        found = 0;
+        secp256k1_xonly_pubkey_save(&P_output_xonly, &P_output_ge);
+        for (i = 0; i < n_tx_outputs; i++) {


94c6e1f: could be slightly optimised by skipping over outputs where you already found something. But this loop seems reasonably cheap.

Sjors · 2024-09-10T14:32:35Z

examples/silentpayments.c

+ * without any warranty. For the CC0 Public Domain Dedication, see       *
+ * EXAMPLES_COPYING or https://creativecommons.org/publicdomain/zero/1.0 *
+ *************************************************************************/
+


5c546e2 for cmake:

diff --git a/examples/CMakeLists.txt b/examples/CMakeLists.txt index fd1ebce..66e989e 100644 --- a/examples/CMakeLists.txt +++ b/examples/CMakeLists.txt @@ -3,6 +3,7 @@ function(add_example name) add_executable(${target_name} ${name}.c) target_include_directories(${target_name} PRIVATE ${PROJECT_SOURCE_DIR}/include + ${PROJECT_SOURCE_DIR} ) target_link_libraries(${target_name} secp256k1 @@ -32,3 +33,7 @@ endif() if(SECP256K1_ENABLE_MODULE_ELLSWIFT) add_example(ellswift) endif() + +if(SECP256K1_ENABLE_MODULE_SILENTPAYMENTS) + add_example(silentpayments) +endif()

cmake .. -DSECP256K1_BUILD_EXAMPLES=1 -DSECP256K1_ENABLE_MODULE_SILENTPAYMENTS=1

josibake mentioned this pull request Apr 19, 2024

Add silentpayments (BIP352) module #1471

Closed

real-or-random added the feature label Apr 19, 2024

josibake mentioned this pull request Apr 22, 2024

BIP352 tracking issue bitcoin/bitcoin#28536

Open

15 tasks

theStack reviewed Apr 22, 2024

View reviewed changes

josibake force-pushed the bip352-silentpayments-module branch from 3d08027 to 8b48bf1 Compare April 24, 2024 08:38

josibake force-pushed the bip352-silentpayments-module branch from 8b48bf1 to f5585d4 Compare April 24, 2024 09:38

theStack reviewed Apr 28, 2024

View reviewed changes

Sjors mentioned this pull request May 2, 2024

Silent Payments: Implement BIP352 bitcoin/bitcoin#28122

Draft

josibake force-pushed the bip352-silentpayments-module branch 2 times, most recently from 9d75190 to 1a3a00b Compare May 3, 2024 08:21

josibake force-pushed the bip352-silentpayments-module branch from 1a3a00b to 92f5920 Compare May 3, 2024 11:11

josibake mentioned this pull request May 3, 2024

Add secp256k1_pubkey_sort #1518

Merged

josibake force-pushed the bip352-silentpayments-module branch from 92f5920 to 56ed901 Compare May 8, 2024 12:48

edwargix mentioned this pull request May 16, 2024

[ Feature Request ] Implement BIP352 sending (send to silent payment address) spesmilo/electrum#8847

Open

apoelstra mentioned this pull request May 18, 2024

Silent payments addresses (BIP352) rust-bitcoin/rust-bitcoin#2778

Open

theStack reviewed May 28, 2024

View reviewed changes

josibake force-pushed the bip352-silentpayments-module branch from 56ed901 to bd66eaa Compare May 31, 2024 12:34

josibake force-pushed the bip352-silentpayments-module branch from bd66eaa to 2dde8f1 Compare May 31, 2024 13:37

josibake force-pushed the bip352-silentpayments-module branch from b2b904b to 3165b6b Compare August 6, 2024 15:24

jlest01 mentioned this pull request Aug 7, 2024

[WIP] Add bip352 silentpayments module rust-bitcoin/rust-secp256k1#721

Open

theStack reviewed Aug 8, 2024

View reviewed changes

TheCharlatan mentioned this pull request Aug 13, 2024

kernel: Introduce initial C header API bitcoin/bitcoin#30595

Open

theStack and others added 9 commits August 15, 2024 10:24

build: add skeleton for new silentpayments (BIP352) module

1c74941

silentpayments: add examples/silentpayments.c

5c546e2

Demonstrate sending, scanning, and light client scanning.

silentpayments: add benchmarks for scanning

566b5b8

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.

tests: add BIP-352 test vectors

5ce0db1

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

ci: enable silentpayments module

5b9714f

docs: update README

f42e0dd

josibake force-pushed the bip352-silentpayments-module branch from 3165b6b to f42e0dd Compare August 15, 2024 10:52

theStack reviewed Aug 20, 2024

View reviewed changes

jlest01 mentioned this pull request Aug 25, 2024

[POC/WIP] Add optional Bip352 silentpayments index romanz/electrs#1075

Open

Cheapshot003 mentioned this pull request Sep 3, 2024

#1570 improve examples: remove key generation loop #1599

Open

antonilol reviewed Sep 7, 2024

View reviewed changes

Sjors reviewed Sep 9, 2024

View reviewed changes

Sjors reviewed Sep 10, 2024

View reviewed changes

		* recipient_spend_pubkey: pointer to the recipient's spend pubkey
		* label_lookup: pointer to a callback function for looking up

		/* Randomizing the context is recommended to protect against side-channel
		* leakage See `secp256k1_context_randomize` in secp256k1.h for more

	* It's very unlikely tha the scan public key is invalid by this point, since this means the caller would
	* It's very unlikely that the scan public key is invalid by this point, since this means the caller would

	* corresponding serialized labelled spend public key:
	* corresponding labelled spend public key:

		* Given the public input data (secp256k1_silentpayments_public_data),
		* calculate the shared secret.

	* This can fail if t_k overflows the curver order, but this is statistically improbable
	* This can fail if t_k overflows the curve order, but this is statistically improbable

	memset(&pd.data[1], 0, sizeof(&pd.data - 1));
	memset(&pd.data[1], 0, sizeof(pd.data) - 1));

Add BIP352 silentpayments module #1519

Are you sure you want to change the base?

Add BIP352 silentpayments module #1519

Conversation

josibake commented Apr 19, 2024 • edited Loading

For sending

For scanning

Note for reviewers

Notable differences between this PR and the previous version

Outstanding work

theStack left a comment

Choose a reason for hiding this comment

josibake commented Apr 24, 2024

josibake commented Apr 24, 2024

theStack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

josibake commented May 3, 2024

josibake commented May 3, 2024

josibake commented May 8, 2024

theStack left a comment

Choose a reason for hiding this comment

josibake commented May 31, 2024

josibake commented May 31, 2024

josibake commented May 31, 2024

real-or-random commented May 31, 2024

real-or-random commented May 31, 2024 • edited Loading

Some general notes

On error handling in general

ARG_CHECK

What does this mean for this discussion?

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

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

josibake commented May 31, 2024 • edited Loading

theStack commented Jun 1, 2024

josibake commented Aug 6, 2024

jlest01 commented Aug 7, 2024

josibake commented Aug 7, 2024

theStack commented Aug 8, 2024

theStack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josibake commented Aug 15, 2024

theStack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlest01 commented Aug 24, 2024 • edited Loading

jlest01 commented Aug 24, 2024

josibake commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add BIP352 `silentpayments` module #1519

Add BIP352 `silentpayments` module #1519

josibake commented Apr 19, 2024 •

edited

Loading

real-or-random Jul 12, 2024 •

edited

Loading

real-or-random commented May 31, 2024 •

edited

Loading

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

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

josibake commented May 31, 2024 •

edited

Loading

jlest01 commented Aug 24, 2024 •

edited

Loading

Sjors left a comment •

edited

Loading

Sjors Sep 10, 2024 •

edited

Loading