[WIP] Add bip352 silentpayments module #721

jlest01 · 2024-08-07T22:58:40Z

This PR adds bip352 silentpayments module based on bitcoin-core/secp256k1#1519.

Currently, it creates bindings for all silent payment structs and functions and also adds the high-level API to access them.

examples/silentpayments.rs implements the same example as examples/silentpayments.c from the original repository and shows how to use all functions.
The example can be run with cargo run --example silentpayments --features="rand std".

Although it implements all the features of the silentpayments module, this is still a work in progress, as the current module only works with the standard library.

For review purposes, only the last three commits matter. The others may be discarded after the module is merged into the original repository and the depend folder updated.

This modification allows running the command `./vendor-libsecp.sh -f pull/1519/head` to obtain MuSig2 components from `bitcoin-core/secp256k1`. This commit may be discarded after merging PR #1519.

Kixunil

Cool! Concept ACK.

src/silentpayments.rs

Cargo.toml

josibake

Thanks for picking this up!

josibake · 2024-08-15T08:49:26Z

src/constants.rs

@@ -33,6 +33,9 @@ pub const KEY_PAIR_SIZE: usize = 96;
 /// The size of a full ElligatorSwift encoding.
 pub const ELLSWIFT_ENCODING_SIZE: usize = 64;

+/// The size of a silent payment public data.


in 0e7e062 ("Add Silent Payments module"):

nit: "silent payments"

Suggested change

/// The size of a silent payment public data.

/// The size of a silent payments public data object.

Thanks for the review. Done !

josibake · 2024-08-15T08:59:41Z

src/silentpayments.rs

+pub fn silentpayments_sender_create_outputs<C: Verification>(
+    secp: &Secp256k1<C>,
+    recipients: &[SilentpaymentsRecipient],
+    smallest_outpoint: &[u8; 36],


in 0e7e062 ("lib: Add Silent Payments module"):

Would it make sense to use an Outpoint type here (assuming one exists)? On the one hand, it makes things more clear and less foot gunny for the caller, on the other hand I can see an argument for having the rust-secp256k1 crate be unaware of Bitcoin protocol specifics.

Sounds like we should wait for bitcoin-primitives to be released and gate this behind an optional dependency on it.

I had a massive brainfart here. We want to optionally use crypto in primitives which will optionally depend on secp256k1, so secp256k1 cannot (optionally) depend on primitives as it'd create a cycle if all features are on. I'm not sure what the best solution is but having silent payments implemented outside secp256k1 sounds most appealing at first thought.

josibake · 2024-08-15T09:09:03Z

src/silentpayments.rs

+    /// Creates an `SilentpaymentsPublicData` object from a 98-byte array.
+    pub fn from_array(data: [u8; 98]) -> SilentpaymentsPublicData {
+        SilentpaymentsPublicData(ffi::SilentpaymentsPublicData::from_array(data))
+    }
+
+    /// Returns the 64-byte array representation of this `SilentpaymentsPublicData` object.
+    pub fn to_array(&self) -> [u8; 98] { self.0.to_array() }


in 0e7e062 ("lib: Add Silent Payments module"):

How do you foresee the to_array/from_array functions being used? public_data is meant to be an opaque data type, so it seems better to me to not have these methods and instead have callers always use the create and serialize functions, but perhaps I'm missing something.

Good catch ! Done.

jlest01 · 2024-09-07T00:19:39Z

Removed dynamically sized vector in most functions and simplified the code.
There are still two functions with vectors: silentpayments_recipient_scan_outputs() and silentpayments_sender_create_outputs().

silentpayments_sender_create_outputs() returns a vector of XOnlyPublicKey with the same number of recipients.
For the function to not dynamically allocate memory for this, the caller would have to pass in an already allocated mutable XOnlyPublicKey slice reference .
But the XOnlyPublicKey struct does not allow instantiation with an empty key, only ffi::XOnlyPublicKey::new() does.
Would the solution be a new function for XOnlyPublicKey to allow an empty key?

Cargo.toml

antonilol · 2024-09-07T09:51:56Z

src/silentpayments.rs

+    unsafe {
+        // Get a pointer to the inner ffi::PublicKey
+        let ffi_pubkey_ptr: *const ffi::PublicKey = pubkey.as_c_ptr();
+
+        // Dereference the pointer to get the ffi::PublicKey
+        // Then create a copy of it
+        (*ffi_pubkey_ptr).clone()
+    }


Unnecessary unsafe and clone, PublicKey wraps ffi::PublicKey and they are both Copy.

Thanks. Done in 6fbb059

antonilol · 2024-09-07T10:09:40Z

src/silentpayments.rs

+        for (i, pubkey) in out_pubkeys.iter_mut().enumerate() {
+            out_pubkeys_ptrs[i] = pubkey as *mut _;
+        }


Looks redundant, doesn't out_pubkeys.iter_mut().map(|k| k as *mut _).collect() (line 81) do the same?

Thanks. Done in 6fbb059

antonilol · 2024-09-07T10:13:49Z

src/silentpayments.rs

+
+        let (ffi_taproot_seckeys, n_taproot_seckeys) = match taproot_seckeys {
+            Some(keys) => {
+                let ffi_keys: &[*const ffi::Keypair] = transmute::<&[&Keypair], &[*const ffi::Keypair]>(taproot_seckeys.unwrap());


Pointer casting is recommended over transmuting pointers and references, but both approaches are unsound if Keypair is not guaranteed to have the same layout as ffi::Keypair. Adding #[repr(transparent)] to Keypair fixes this.

Same comment for SecretKey a few lines down, and some more in this file, please check all uses of transmute and use pointer casting where possible, and review if the source and destination type are layout-compatible.

Thanks. Done in ac90bf3 and 6fbb059

antonilol · 2024-09-07T10:18:41Z

src/silentpayments.rs

+        );
+
+        if res == 1 {
+            Ok(out_pubkeys.into_iter().map(XOnlyPublicKey::from).collect())


This can be optimized if XOnlyPublicKey is #[repr(transparent)]. (using Vec::from_raw_parts)

Thanks. Done in ac90bf3 and 6fbb059

antonilol · 2024-09-07T10:21:33Z

src/silentpayments.rs

+            return Ok(LabelTweakResult { pubkey, label_tweak });
+        } else {
+            return Err(LabelTweakError::Failure);
+        }


Syntax could be more "rusty" by removing the 'return' keywords (this is a clippy warning, see other clippy warnings for more simple improvements like this)

Thanks. Done in 6fbb059

antonilol · 2024-09-07T10:25:35Z

src/silentpayments.rs

+        unsafe {
+
+            let empty_data = [0u8; constants::SILENT_PAYMENTS_PUBLIC_DATA_SIZE];
+            let mut silentpayments_public_data = ffi::SilentpaymentsPublicData::from_array(empty_data);


This can use Self::new(), and as_mut_c_ptr() for the c function. It then does not need to be wrapped before returning.

Same comment for line 308.

Thanks. Done in 6fbb059

antonilol · 2024-09-07T10:30:54Z

src/silentpayments.rs

+        let mut result = vec![SilentpaymentsFoundOutput::empty(); n_found_outputs];
+
+        for i in 0..n_found_outputs {
+            result[i] = SilentpaymentsFoundOutput(out_found_output[i]);
+
+        }


Can be optimized to use Vec::from_raw_parts if SilentpaymentsFoundOutput is made #[repr(transparent)].

Thanks. Done in 6fbb059

antonilol · 2024-09-07T10:33:04Z

src/silentpayments.rs

+    label_lookup: SilentpaymentsLabelLookupFunction,
+    label_context: L,


This requires users to use unsafe, and is maybe even unsound to accept a function that can return pointers to a "safe" (not annotated unsafe) function.

This is also unsound on Rust < 1.81, see https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html#abort-on-uncaught-panics-in-extern-c-functions. To fix that you have to catch_unwind and abort manually when using older versions.

I think I managed to create a safe abstraction for this callback (given the current restrictions, which might change as bitcoin-core/secp256k1#1519 is not merged yet). I added it (2b3f633) to a branch on my fork, feel free to add (may be modified) it here.

Thanks @antonilol for all your contributions, here and to other projects like electrs! They are very helpful.

Thanks for working on this and proposing a better approach.
But I haven't added the commit yet because it still has some TODO and the example file is not using the modified function.
I would like to better understand your proposal and how we can implement the TODOs and call the function from the example file.

First TODO: the C library allows you to pass a null pointer for the label lookup function, but the function signature on the Rust side does not, this type definition must be wrapped in Option<...> to allow null pointers.
Second TODO: the C library checks that the context pointer is non null when the function pointer is non null, in my opinion the C library should treat it as opaque and 100% managed by the user so should not check it, just pass it to the callback (comment thread, same link as in the code). Note this will not be an issue for the Rust side anymore when TODO 1 is fixed, but at first I thought that could be harder because bindings are normally generated.

Function pointer + context pointer is a C thing, in Rust a closure automatically captures the variables it needs. To allow users to pass closures, the closure needs to be "converted" to a function pointer and context pointer for the C side to use.
This safe wrapper stores the state part of the closure (a Rust closure can be seen as a combination of a function and state) on the stack and passes the pointer to that as the context pointer. In the callback the function and the state are but "back together" and called.
To add to this, the C side expects a pointer to the return value. The C function uses this after the callback returned (obviously), so this pointer needs to live for longer than the callback, but as long as the safe wrapper is enough, so that return value is stored on the stack as well. This storage (the variable named storage) is passed with the closure state to the C side as the context pointer.

Calling from the example (or from any user code) should be straightforward, these should (roughly) be the changes needed to the example:

make the cache a Vec for simplicity

change the callback function (in the argument list) to

|label| cache.iter().find(|cache_entry| cache_entry.label == label).map(|cache_entry| cache_entry.label_tweak)

cache will be a captured variable and the safe wrapper will take care of getting it to the C side.

Sosthene00 · 2024-10-26T10:38:45Z

src/silentpayments.rs

+    }
+}
+
+impl SilentpaymentsFoundOutput {


Maybe we should implement some getter functions too?

What would be the use cases for the getter functions?

I may be missing something, but in the tests we're just printing the SilentpaymentsFoundOutput returned by silentpayments_recipient_scan_outputs, and I can't figure out how I would take the x only pubkey to update my wallet state, let alone use the tweak to actually create the spending key for that output.

I made a quick commit just to show you what I mean https://github.com/Sosthene00/rust-secp256k1/tree/bip352-add-getters

…Key`

jlest01 added 4 commits August 7, 2024 19:13

Modify the vendor-libsecp file to enable checkout of a pull request

5d4e2aa

This modification allows running the command `./vendor-libsecp.sh -f pull/1519/head` to obtain MuSig2 components from `bitcoin-core/secp256k1`. This commit may be discarded after merging PR #1519.

Enable silent payment module in the secp256k1-sys/build.rs

a9bbf74

Update codebase to 0_10_0 to include secp256k1 silent payment module

cde8c8a

secp256k1-sys: Add Rust FFI for Silent Payments module

337e58f

Kixunil reviewed Aug 8, 2024

View reviewed changes

src/silentpayments.rs Show resolved Hide resolved

src/silentpayments.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

jlest01 force-pushed the bip352-silentpayments-module branch from ec34fda to c217147 Compare August 9, 2024 20:43

josibake reviewed Aug 15, 2024

View reviewed changes

jlest01 added a commit to jlest01/rust-bitcoin that referenced this pull request Aug 20, 2024

Update secp256k1 dependency to use PR rust-bitcoin/rust-secp256k1#721

9bf8dfc

jlest01 added a commit to jlest01/rust-bitcoin that referenced this pull request Aug 21, 2024

Update secp256k1 dependency to use PR rust-bitcoin/rust-secp256k1#721

5ae56d1

jlest01 added a commit to jlest01/bitcoin_slices that referenced this pull request Aug 21, 2024

Update secp256k1 dependency to use PR rust-bitcoin/rust-secp256k1#721

ee774c1

jlest01 added a commit to jlest01/rust-bitcoincore-rpc that referenced this pull request Aug 21, 2024

Update secp256k1 dependency to use PR rust-bitcoin/rust-secp256k1#721

0a0db43

jlest01 added a commit to jlest01/electrs that referenced this pull request Aug 21, 2024

Update secp256k1 dependency to use PR rust-bitcoin/rust-secp256k1#721

bdf26d1

jlest01 mentioned this pull request Aug 25, 2024

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

Open

jlest01 force-pushed the bip352-silentpayments-module branch from c217147 to 36b2c6b Compare September 6, 2024 20:05

jlest01 added a commit to jlest01/electrs that referenced this pull request Sep 6, 2024

Update secp256k1 dependency to use PR rust-bitcoin/rust-secp256k1#721

0e7a4fa

jlest01 added a commit to jlest01/electrs that referenced this pull request Sep 6, 2024

Update secp256k1 dependency to use PR rust-bitcoin/rust-secp256k1#721

a7a962e

antonilol reviewed Sep 7, 2024

View reviewed changes

Sosthene00 reviewed Oct 26, 2024

View reviewed changes

jlest01 added 3 commits October 29, 2024 23:28

Add #[repr(transparent)] to Keypair, SecretKey and `XOnlyPublic…

ac90bf3

…Key`

lib: Add Silent Payments module

6fbb059

Add example file

9566ad2

jlest01 force-pushed the bip352-silentpayments-module branch from 36b2c6b to 9566ad2 Compare October 30, 2024 02:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add bip352 silentpayments module #721

[WIP] Add bip352 silentpayments module #721

jlest01 commented Aug 7, 2024 •

edited

Loading

Kixunil left a comment

josibake left a comment

josibake Aug 15, 2024

jlest01 Sep 7, 2024

josibake Aug 15, 2024

Kixunil Aug 15, 2024

Kixunil Sep 7, 2024

josibake Aug 15, 2024

jlest01 Sep 7, 2024

jlest01 commented Sep 7, 2024

antonilol Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

Kixunil Sep 7, 2024

jlest01 Oct 30, 2024

antonilol Oct 30, 2024

Sosthene00 Oct 26, 2024

jlest01 Oct 30, 2024

Sosthene00 Oct 30, 2024

Sosthene00 Oct 30, 2024

	/// The size of a silent payment public data.
	/// The size of a silent payments public data object.

		label_lookup: SilentpaymentsLabelLookupFunction,
		label_context: L,

[WIP] Add bip352 silentpayments module #721

Are you sure you want to change the base?

[WIP] Add bip352 silentpayments module #721

Conversation

jlest01 commented Aug 7, 2024 • edited Loading

Kixunil left a comment

Choose a reason for hiding this comment

josibake 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

jlest01 commented Sep 7, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlest01 commented Aug 7, 2024 •

edited

Loading