-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add BIP352 silentpayments
module
#1519
base: master
Are you sure you want to change the base?
Add BIP352 silentpayments
module
#1519
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
3d08027
to
8b48bf1
Compare
8b48bf1
to
f5585d4
Compare
Updated 8b48bf1 -> f5585d4 (bip352-silentpayments-module-rebase -> bip352-silentpayments-module-02, compare):
For the label scanning, I looked for an example of using an invalid public key but didn't see anything except for the I also used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second review round through, looks good so far! Left a bunch of nits, mostly about naming and missing ARG_CHECKS etc.
examples/silentpayments.c
Outdated
} else { | ||
printf("Failed to create keypair\n"); | ||
return 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually fixed this time to take out the while loop and return if it fails to create the key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #1570
9d75190
to
1a3a00b
Compare
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)
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 |
1a3a00b
to
92f5920
Compare
Rebased on #1518 1a3a00b -> 92f5920 (bip352-silentpayments-module-03 -> bip352-silentpayments-module-03-rebase, compare) |
92f5920
to
56ed901
Compare
Rebased on master (following #1518 merge) 92f5920 -> 56ed901 (bip352-silentpayments-module-03-rebase -> bip352-silentpayments-module-04-rebase, compare) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
56ed901
to
bd66eaa
Compare
Rebased on master to fix merge conflict 56ed901 -> bd66eaa (bip352-silentpayments-module-04-rebase -> bip352-silentpayments-module-05-rebase, compare) |
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?). |
bd66eaa
to
2dde8f1
Compare
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)
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 |
Indeed, see #1536 |
Some general notesOn error handling in generalError 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
Line 324 in 1791f6f
What does this mean for this discussion?
So let's take a look at the two sides: On the sender side: The secret keys sum up to zero (
|
@real-or-random thanks for the response, this is super helpful.
In hindsight, I think my preference for
If we imagine an index + light client scenario, the Thinking about this a bit more:
Most of the high-level functions in our API are calling multiple lower-level functions and so far the approach has been something like:
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. |
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 ( 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 |
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
3165b6b
to
f42e0dd
Compare
Update 3165b6b -> f42e0dd (bip352-silentpayments-module-15 -> bip352-silentpayments-module-16, compare)
Thanks again for the thorough review, @theStack ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn 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 |
/** 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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)
|
||
/* 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could also check the label and label tweak results here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, although the values are used in the next call, _create_labelled_spend_pubkey
and then the final result of that is checked against an exact expected value, so the extra checks don't seem worth the added verbosity?
* Given the public input data (secp256k1_silentpayments_public_data), | ||
* calculate the shared secret. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: it don't think it can fail because of overflow at this point (the overflow is ignored and t_k
has already been converted to a scalar in secp256k1_silentpayments_create_t_k
). we could move this comment up into secp256k1_silentpayments_create_t_k
and handle failure inside that function.
/* 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was meant to be
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)
The function . And the smallest outpoint: . These values represent this signet P2TR transaction: 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;
} |
Testing the same values using rust silentpayments crate, you get an error description: 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 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)
} |
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
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 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. |
const secp256k1_silentpayments_public_data *public_data, | ||
const secp256k1_pubkey *recipient_spend_pubkey, | ||
const secp256k1_silentpayments_label_lookup label_lookup, | ||
const void *label_context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could label_context (here and in the callback type) be made mutable? Is there a particular reason it is const
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually disallow (make undefined behavior) mutating the context from the callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the reason for making the pointer to label context mutable here? Making it const is meant to clearly communicate to the caller that this is meant to be a reference to some external data store. For your second question, I don't think making the pointer const prevents the lookup function itself from mutating the context, but I don't think there is a way we can prevent that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly (after searching online about this), the C compiler will assume that no mutation happens to the pointed to data using the pointer label_context
, because it is declared const
.
In Rust however, you can't prevent users from mutating captured variables of a closure. (Rust's shared references can be mutated through, still they are called immutable references.)
A fix on the Rust side would be to add a level of indirection, instead of passing a pointer to the context directly, a pointer to a pointer is passed. The pointed to data of the const void *
is never mutated (that would of type context_t *
then, in C terms, where context_t
is a struct defined by the user).
A fix on the C side would be to remove the const
keyword.
I am fine with both, but the extra level of indirection will be slightly less performant, but modern CPU caches can make this (almost) unobservable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the C compiler will assume that no mutation happens to the pointed to data using the pointer label_context, because it is declared const.
This is, as far as I understand, not correct. A const
pointer does not imply that the data pointed to cannot be mutated. It only means it cannot be mutated through that pointer.
What is true however, is that if all references to an object are const
, and the compiler can prove this, then no mutation to the object can happen to it at all. This in practice only applies to objects that are themselves declared const
.
If the object itself is non-const
, but you hold a const
pointer to it, then it is perfectly legal even to cast the const
ness of the pointer away and mutate it through that casted pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
If the object itself is non-
const
, but you hold aconst
pointer to it, then it is perfectly legal even to cast theconst
ness of the pointer away and mutate it through that casted pointer.
Especially this, this is new for me, thanks!
if (label_lookup != NULL) { | ||
ARG_CHECK(label_context != NULL); | ||
} else { | ||
ARG_CHECK(label_context == NULL); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the label_context ARG_CHECK'ed, shouldn't this be treated as opaque data for the callback alone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the label_lookup accepts a void* pointer for the context, I think it would be fine to make passing the context optional. Perhaps the better arg check would be to do this in reverse: complain if the caller passes a context and not a lookup function. Will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,297 @@ | |||
#!/usr/bin/env python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+'''
+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
+'''
if not last: | ||
out += "," | ||
out += "\n" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(sys.argv) != 2:
print("Usage: tests_silentpayments_generate.py vectors.json > vectors.h")
sys.exit(1)
#define SECP256K1_MODULE_SILENTPAYMENTS_TESTS_H | ||
|
||
#include "../../../include/secp256k1_silentpayments.h" | ||
#include "include/secp256k1.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
@@ -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]]), [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1c74941: can we just break the convention and make it --enable-module-silent-payments
? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha no :)
* recipient may passed multiple times to generate | ||
* multiple outputs for the same recipient | ||
* outpoint_smallest36: serialized smallest outpoint (lexicographically) | ||
* from the transaction inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a separate discussion on the Musig2 PR that I need to follow up on regarding this, where there might be a more clever way to enforce the array length without these suffix hints. For now, I think I will remove the 36 and add it to the docs, as you suggest.
Agree that we can't enforce they are passing the correct outpoint out of a list of outpoints here, but I do think we can make it clear in the header file that this is not handled by libsecp and implementations MUST ensure they are following the test vectors from the BIP. I'll add something as such to the header documentation.
/* 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? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offline discussion: key pair load should never fail, because key pair object has already been validated. So constant time is not an issue here.
/* 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could simplify the early return below by:
9d6769f: ret &=
} | ||
secp256k1_scalar_add(&a_sum_scalar, &a_sum_scalar, &addend); | ||
} | ||
/* If there are any failures in loading/summing up the secret keys, fail early */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josibake says to clear the a_sum_scalar
in early returns. Probably addend
too.
cmove
vs memset
: the former is constant time, but that's not always necessary.
In general the library is not always consistent when it comes to early returns. Those are often not constant time (invalid secret behaves different from valid secret).
cc @jonasnick please brainstorm a bit more about this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, we have this:
Lines 209 to 210 in f0868a9
/* Zero memory if flag == 1. Flag must be 0 or 1. Constant time. */ | |
static SECP256K1_INLINE void secp256k1_memczero(void *s, size_t len, int flag) { |
But AFAIU, this is "just" about clearing secrets from memory, and then memset
is the right answer. (The compiler will optimize those memsets, but this PR will address this: #1579) No need to be constant-time in an error branch.
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: there is no secp256k1_silentpayments_public_data_serialize
, did you mean secp256k1_silentpayments_recipient_public_data_serialize
? Did you also mean to drop "as a compressed public key" (since it contains more than that)?
I do think it's useful to briefly document what is in the opaque structure: the summed public key and input_hash.
Also, is there a light client use case that requires more than just tweak data? If not, then this struct could be reduced to 33 bytes. That's what the index uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this is thoroughly confusing, but I think I figured it out.
I rewrote the documentation to fix the method names and make it more clear that:
- 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];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the name of the struct to be silentpayments_recipient_public_data
and updated the documentation to have consistent naming.
On the doc rewrite, I'm not convinced this is better. It mentions input_hash
explicitly, which is an implementation detail internal to the module, and also mentions some of the low-level operations going on inside the functions, which I don't think we should be exposing/explaining to the caller here. To me, it seems sufficient to tell the caller: "hey, create a public data object using this function. If you're a light client and receive a previously created, serialised public data object, parse it with this function."
Going to leave the documentation as is for now, but would be interested to hear others thoughts on this.
* `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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: secp256k1_silentpayments_recipient_public_data_parse
/* 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, because I am a dum dum! I think the first line is left over from when I added the ser_ret check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* In: public_data: pointer to an initialized silentpayments_public_data | ||
* object | ||
*/ | ||
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_public_data_serialize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: the name of this function implies:
- 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];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue it does serialise everything in the struct: it takes a public_data object and returns the serialised format, which is 33 bytes. Also, this function does not do a hash. The input_hash
part of the object is created when the public data object is created.
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: BIP352 does not define the terms "label tweak" and "label value". I'm guessing "tweak" refers to hash(bscan || m)·G
?
Maybe just add "See secp256k1_silentpayments_recipient_create_label_tweak". From the formula there it seems that "value" is B1 - Bspend
. Could call that "tweak point".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the language to "label pubkey," as this is more consistent with where it is used elsewhere. Also added a reference to the secp256k1_silentpayments_recipient_create_label_tweak
function per your suggestion.
|
||
/** Scan for Silent Payment transaction outputs. | ||
* | ||
* Given a input public sum, an input_hash, a recipient's spend public key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: , a recipient's scan key b_scan and spend public key
* with the input_hash (see | ||
* `_recipient_public_data_create`) | ||
*/ | ||
SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_silentpayments_recipient_create_shared_secret( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: derive_
instead of create_
? (from the POV of the universe, the sender created it)
Ditto for recipient_create_output_pubkey
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly prefer create, as I want to avoid any confusion with bip32 style derivation. From the recipients perspective, they are also creating a shared secret per the spec. Its only after scanning with the shared secret that they can confirm this transaction was indeed a silent payment (i.e., the sender created a shared secret and outputs from that shared secret).
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: here the word "deriving" would reduce confusion imo. Otherwise it's sounds like this is a sender creating an output (of a transaction).
Maybe make it:
* .. deriving silent payment
* pubkeys for this recipient scan public key, which can be
* matched against transaction outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, derived is heavily associated with BIP32 style public key/private key derivation, so I want to avoid using it here as much as possible.
} else { | ||
ARG_CHECK(label_context == NULL); | ||
} | ||
/* TODO: do we need a _cmov call here to avoid leaking information about the scan key? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: leaking privacy still sucks, and afaik the _cmov
is super cheap. So just do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offline discussion: just do a memset
if (label_lookup != NULL) { | ||
ARG_CHECK(label_context != NULL); | ||
} else { | ||
ARG_CHECK(label_context == NULL); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (!ret) { | ||
return 0; | ||
} | ||
combined = (int)public_data->data[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is opaque to users of the library :) Not sure I see the value of creating a separate function that is not exposed in the public API and only used internally?
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++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: could be slightly optimised by skipping over outputs where you already found
something. But this loop seems reasonably cheap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had considered this! However, this is only relevant for large transactions with many outputs, where multiple outputs are created for the same recipient. For now, I think I'll leave this for a follow up.
* without any warranty. For the CC0 Public Domain Dedication, see * | ||
* EXAMPLES_COPYING or https://creativecommons.org/publicdomain/zero/1.0 * | ||
*************************************************************************/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
unsigned int m = 1; | ||
|
||
/* Load Bob's spend public key */ | ||
ret = secp256k1_ec_pubkey_parse(ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ret
is unused.
* and is using the resulting labelled spend pubkey to encode a | ||
* labelled silent payments address. | ||
*/ | ||
ret = secp256k1_silentpayments_recipient_create_label_tweak(ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ret
is only checked after ec_pubkey_serialize
.
|
||
/* Scan the transaction */ | ||
n_found_outputs = 0; | ||
ret = secp256k1_silentpayments_recipient_scan_outputs(ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ret
is unchecked.
ret = secp256k1_silentpayments_recipient_public_data_parse(ctx, | ||
&public_data, | ||
light_client_data33 | ||
); | ||
assert(ret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC public_data
typically comes from an untrusted source, so I don't think callers want to assert this. I think there are other places in the code where an assert
may mislead readers of the example. For example, when parsing the recipient address. It may also make sense to add a comment to assert(n_found_outputs == 1);
explaining that this is only true in this particular test. And btw, I noticed that the number of found outputs is not asserted for Carol.
/*** Sending ***/ | ||
{ | ||
secp256k1_keypair sender_seckeys[N_INPUTS]; | ||
const secp256k1_keypair *sender_seckey_ptrs[N_INPUTS]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called sender_seckeys
instead of sender_keypair
? Same in the silentpayments.h
.
} secp256k1_silentpayments_public_data; | ||
|
||
/** Compute Silent Payment public data from input public keys and transaction | ||
* inputs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* inputs. | |
* inputs. |
|
||
/** Scan for Silent Payment transaction outputs. | ||
* | ||
* Given a input public sum, an input_hash, a recipient's spend public key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just "public data" would be clearer than "input public sum and input_hash".
* public_data: pointer to the input public key sum | ||
* (optionally, with the `input_hash` multiplied | ||
* in, see `_recipient_public_data_create`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find anything in the _recipient_public_data_create
doc that would explain this optional multiplication.
|
||
/** Parse a 33-byte sequence into a silent_payments_public_data object. | ||
* | ||
* Returns: 1 if the data was able to be parsed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace
* a label value. This function takes a label | ||
* pubkey as an argument and returns a pointer to | ||
* the label tweak if the label exists, otherwise | ||
* returns a nullptr (NULL if labels are not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/nullptr/NULL pointer
would be more consistent.
* any further elliptic-curve operations from the wallet. | ||
*/ | ||
|
||
/* This struct serves as an In param for passing the silent payment address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* This struct serves as an In param for passing the silent payment address | |
/* This struct serves as an input argument for passing the silent payment address |
would be more consistent with secp256k1.h
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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 | |
* 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 |
last_recipient = *recipients[0]; | ||
k = 0; | ||
for (i = 0; i < n_recipients; i++) { | ||
if ((i == 0) || (secp256k1_ec_pubkey_cmp(ctx, &last_recipient.scan_pubkey, &recipients[i]->scan_pubkey) != 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit worried about that part, because it seems that if we have 3 recipients with 2 scan_pubkeys that are not contiguous, i.e. ABA instead of AAB, we would reset k to 0 when hitting A for the second time instead of correctly incrementing k to 1, resulting in generating the same pubkey twice (assuming identical spend_key of course).
I must be missing something though because I can't make the tests fail with this pattern, it seems that the same (correct) outputs are being generated regardless of the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that a few lines above before the loop, the recipients are sorted by scan pubkey (see secp256k1_silentpayments_recipient_sort
) in order to avoid the problem you described. The outputs are still returned in the original order, by taking use of the index
field of the recipient structure that a user has to set in ascending order (0, 1, 2, ...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, nothing to see here then
* Being a light client, Carol likely does not have access to the | ||
* transaction outputs. This means she will need to first generate | ||
* an output, check if it exists in the UTXO set (e.g. BIP158 or | ||
* some other means of querying) and only proceed to check the next | ||
* output (by incrementing `k`) if the first output exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this description of the light client workflow would only work for users that don't use labels at all. While we could assume that indeed most light client users wouldn't use labels, I think we'd rather make a slighter more general example here assuming that Carol also use labels, or if we don't want to make the example more complicated at the very least explain it here in comments. Here's a quick rewriting of this paragraph based on how we made it work for Dana:
/** Being a light client, Carol likely does not have access to the
* transaction outputs. This means she will need to first generate
* the first outputs she would get in a transaction, meaning the output
* from her plain spend_pubkey + one for each label she monitors
* with _k_ == 0 , and check if it exists in the UTXO set (e.g. BIP158 or
* some other means of querying).
*
* As soon as she finds an output she can request the whole transaction
* (or rather the whole block to not reveal which transactions
* she's interested in) and use the same function than Bob from there
* to find any other output that belongs to her.
*
* She would then repeat this operation for each `public_data` she received
*/
Alternatively I guess she could also increment k when she has a match and keep looking for outputs this way, but she needs to generate the k+1 output for each label also. Maybe that would indeed make more sense if we go for UTXO set scanning instead of scanning each block sequentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good call out. I added a sentence to indicate that, while its not recommended, Carol can still uses labels as a light client using the addition method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another reason I didn't include light client labels is because this would require a function for adding arbitrary public keys, which at this point we are trying to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree it's kind of an edge case and we want to be careful not to compromise too much because of it, but on the other hand we should acknowledge it because it will definitely happen I think. I'll try to complete the tests on that part and see how bad it makes things
Big thanks to all the review over the last few months and apologies for my slow response. I'm in the process of rebasing and incorporating review feedback, should have the PR updated shortly. |
* to identify the resulting transaction as a silent payments transaction and potentially link the transaction | ||
* back to the silent payment address | ||
*/ | ||
memset(hash_ser, 0, sizeof(hash_ser)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this will probably be a noop. See "notes" here: https://en.cppreference.com/w/c/string/byte/memset
secp has secure_erase
in examples. Maybe that could be brought out? Surely there are other places where a cleanse is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A memory cleanse function secp256k1_memclear
is available since the recently merged #1579 (using the same memory-barrier-approach as in Bitcoin Core) and should be used for that purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, perfect, thanks! I was grepping on an old branch and missed this.
* parameter pairs, depending on whether the seckeys correspond to x-only | ||
* outputs or not. | ||
* | ||
* Returns: 1 if creation of outputs was successful. 0 if an error occured. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9d6769f: typo nit: s/occured/occurred here and in few more places in the file
hash->bytes = 64; | ||
} | ||
|
||
static void secp256k1_silentpayments_create_t_k(secp256k1_scalar *t_k_scalar, const unsigned char *shared_secret33, unsigned int k) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9d6769f: micro nit: might make sense to add const qualifier to the int too. also for the int in other functions - secp256k1_silentpayments_recipient_create_output_pubkey
, secp256k1_silentpayments_recipient_create_label_tweak
, secp256k1_silentpayments_create_output_pubkey
/* Calculate the input hash and tweak a_sum, i.e., a_sum_tweaked = a_sum * input_hash */ | ||
secp256k1_silentpayments_calculate_input_hash(input_hash, outpoint_smallest36, &A_sum_ge); | ||
secp256k1_scalar_set_b32(&input_hash_scalar, input_hash, &overflow); | ||
ret &= !overflow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9d6769f: BIP sounds like it ignores overflow in https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki#creating-outputs. though everywhere in the code input_hash
overflow would lead to failure. It's a very unlikely scenario and not sure which option is preferable, but would be nice to keep BIP and code consistent.
secp256k1_write_be32(k_serialized, k); | ||
secp256k1_sha256_write(&hash, k_serialized, sizeof(k_serialized)); | ||
secp256k1_sha256_finalize(&hash, hash_ser); | ||
secp256k1_scalar_set_b32(t_k_scalar, hash_ser, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9d6769f: BIP says to fail If t_k is not valid tweak - https://github.com/bitcoin/bips/blob/master/bip-0352.mediawiki#creating-outputs
here, we're ignoring the overflow. we could keep it consistent with the BIP?
(have a similar comment in call site of secp256k1_silentpayments_create_t_k
)
ARG_CHECK(output33 != NULL); | ||
ARG_CHECK(public_data != NULL); | ||
/* Only allow public_data to be serialized if it has the hash and the summed public key | ||
* This helps protect against accidentally serialiazing just a the summed public key A |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: typo nit:
* This helps protect against accidentally serialiazing just a the summed public key A | |
* This helps protect against accidentally serializing just the summed public key A |
ret &= secp256k1_silentpayments_recipient_public_data_load_pubkey(ctx, &pubkey, public_data); | ||
secp256k1_silentpayments_recipient_public_data_load_input_hash(input_hash, public_data); | ||
ret &= secp256k1_ec_pubkey_tweak_mul(ctx, &pubkey, input_hash); | ||
secp256k1_ec_pubkey_serialize(ctx, output33, &pubkeylen, &pubkey, SECP256K1_EC_COMPRESSED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: micro nit: could add VERIFY_CHECK(pubkeylen == 33)
CHECK(secp256k1_silentpayments_recipient_public_data_parse(CTX, &pd, malformed) == 0); | ||
|
||
/* This public_data object was created with combined = 0, i.e., it has both the input hash and summed public keypair. | ||
* In instances where the caller has access the the full transaction, they should use `_scan_outputs` instead, so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* In instances where the caller has access the the full transaction, they should use `_scan_outputs` instead, so | |
* In instances where the caller has access to the full transaction, they should use `_scan_outputs` instead, so |
return 0; | ||
} | ||
combined = (int)public_data->data[0]; | ||
if (!combined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: I think comments here would be useful! (thanks @theStack for explaining the intuition behind this!)
maybe something like:
/**
* If combined is set, pubkey in `public_data` contains input_hash * pubkey. Save an extra point multiplication
* by only having to compute shared_secret = recipient_scan_key * (input_hash * pubkey).
*
* If combined is not set, update recipient_scan_key to contain recipient_scan_key * input_hash and then compute
* shared_secret = (recipient_scan_key * input_hash) * pubkey.
*/
EDIT: part I got confused with was I thought everyone could do (recipient_scan_key * input_hash) but light clients wouldn't be able to since they don't have access to summed pubkey, input_hash(computed from summed pubkey). would have liked to see that part in the comments but also think it doesn't fit here since it's bitcoin specific.
k = 0; | ||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* If we are on a different scan pubkey, its time to recreate the the shared secret and reset k to 0. | |
/* If we are on a different scan pubkey, its time to recreate the shared secret and reset k to 0. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: it don't think it can fail because of overflow at this point (the overflow is ignored and t_k
has already been converted to a scalar in secp256k1_silentpayments_create_t_k
). we could move this comment up into secp256k1_silentpayments_create_t_k
and handle failure inside that function.
|
||
/* Compute input private keys sum: a_sum = a_1 + a_2 + ... + a_n */ | ||
a_sum_scalar = secp256k1_scalar_zero; | ||
for (i = 0; i < n_plain_seckeys; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we handle secret keys that add up to SECP256k1_N
?
I tested this locally with ALICE_SECKKEY = 0xeadc78165ff1f8ea94ad7cfdc54990738a4c53f6e0507b42154201b8e5dff3b1
and another secret key I generated with SECP256K1_N - ALICE_SECKEY = 0x152387e9a00e07156b5283023ab66f8b306288efcef824f9aa905cd3ea564d90
. Passing these two plain secret keys causes secp256k1_silentpayments_sender_create_outputs
to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had an offline conversation with @josibake. The conclusion is that there is nothing to do. The sender has to try again with a different set of pubkeys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@josibake Here's a diff with the test case for this edge case,
https://github.com/josibake/secp256k1/compare/9d6769f4..144584b3
secp256k1_pubkey *label, | ||
unsigned char *label_tweak32, | ||
const unsigned char *recipient_scan_key, | ||
unsigned int m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9d6769f4
: m
could be const
return 0; | ||
} | ||
combined = (int)public_data->data[0]; | ||
if (!combined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94c6e1f: There's something I don't understand here. Why is secp256k1_silentpayments_public_data
98 bytes long? If we only kept the "combined" format input_hash . A_sum
like we do in secp256k1_silentpayments_recipient_public_data_serialize
, we would not need this if (!combined)
block. What are the merits of keeping A_sum || input_hash
in public data instead of input_hash . A_sum
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eunovo this explanation might help - #1519 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stratospher. I understand that we have to check if combined is set so we know how to handle the public data and I get that light clients need A_sum . Input_hash
, but what I'm confused about is why don't we do A_sum . Input_hash
in secp256k1_silentpayments_recipient_public_data_create
so we only need 32 bytes for public data and we don't need the secp256k1_silentpayments_recipient_public_data_serialize
function because the data is already presentable to light clients? I'm guessing there's some reason to not combine the A_sum
and input_hash
at the beginning?
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
taproot_seckeys
andplain_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_silentpayment_recipient
objectsThese 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 ordersilentpayments_sender_create_outputs
to generate the xonly public keys for the recipientsThis 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
taproot_pubkeys
andplain_pubeys
This avoids the caller needing to convert taproot public keys into compressed public keys (and vice versa)
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)
silentpayments_recipient_scan_outputs
to scan the transaction outputs and return the tweak data (and optionally label information) needed for spending laterIn 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: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
k = 0
)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:A_sum
andrecipient_spend_key
, which was impossible to catch withARG_CHECKS
and would result in the scanning process finishing without errors, but not finding any outputsinput_hash
from the caller, which makes for an overall simpler API IMOI 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