[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 libc cc".

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?

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

Cargo.toml

+# only used in the example of silent payments
+libc = { version = "0.2", optional = true }
+cc = { version = "1.0", optional = true }


Dev dependencies may be used in examples, so these can be put there. (https://doc.rust-lang.org/rust-by-example/testing/dev_dependencies.html)

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.

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?

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.

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)

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)

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.

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

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.

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 added 2 commits September 6, 2024 17:04

lib: Add Silent Payments module

f756622

Add example file

36b2c6b

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

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

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

antonilol Sep 7, 2024

Kixunil Sep 7, 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

jlest01 commented Aug 7, 2024 •

edited

Loading