Add BIP 352 bindings (full node scanning only)

[nymius]

This PR adds a silentpayments module based on bitcoin-core/secp256k1#1765.

I've agreed with @jlest01 on opening this PR replacing the old #721.

Commit 7888c66 contains all the changes from bitcoin-core/secp256k1#1765. I'm referencing my own version of the branch to be free of rebasing it in the future. These commits should refer back to master branch once bitcoin-core/secp256k1#1765 is merged.

049e07f is a workaround for a compilation issue I had. I cannot provide a confident answer about the reasons, but the old configuration was breaking the symbols of the library making linking impossible.

Commit 1889342 to 776653d contain all the main changes related to the BIP 352 module.

I've collected the feedback from #721 and applied it in this PR.

Beside the changes related to changes in bitcoin-core/secp256k1#1765 or the feedback provided in #721, I included the following:

• Scoped sender and recipient logic in independent modules.
• Scoped silentpayments in its own module in secp256k1-sys.
• Adapted docs from libsecp256k1 PR, and added extra notes depending of the case.
• Added a feature gate for silentpayments.
• Transformed the original example into an integration test, as it is deterministic.
• Based the changes on the latest rust-secp256k1 master, so this PR uses a global secp context.

I changed the old #721 manually implemented cache for a rust-like one, using HashMaps. Now, thanks to the suggestion of @antonilol, you can pass a rust closure to the bindings, avoiding the implementation of unsafe extern C code.

Note to the reviewers:
After looking at the different context related issues (#813), and mainly #844 (comment), I decided to apply this re-randomization step only where I had secrets related to UTXO unlocking. For context: in BIP 352 you have the scanning secret key, which is not directly associated with UTXO unlocking, that's what the spending key is for.
So I decided to implement something similar to musig::new_nonce_pair for silentpayments::sender::create_outputs, using the combination of secret as random seed.
On the contrary, I didn't implement randomization for silentpayments::sender::scan_outputs for the reason explained above.
I would like to know if the criteria applied here is correct or not.

To execute the test:

cargo test --features="silentpayments" -- full_silentpayment_flow_one_sender_two_receivers

To execute the example:

cargo run --example silentpayments --features="rand silentpayments"

[apoelstra]

In 1889342:

Here you make various rust-secp types repr(transparent) making it an API guarantee that they will be castable to secp256k-sys types. Unfortunately we want to stabillize rust-secp while secp-sys is permanently unstable (since it contains bindings to a C library that will never be "done"). So we can't make this guarantee.

It looks like you do this so that you can cast vectors around. I'd suggest not doing this and just copying the vectors, manually converting the ffi objects into rust-secp ones. You can file a followup issue to attempt to eliminate this copying, though I'm not optimistic we'll find a solution.

In 049e07f:

How is SECP256k1_API causing you trouble? You can PR separately to remove it; it doesn't do anything for us since we don't bind to any files that use the symbol. But I'm surprised to hear that it's actually harmful.

[antonilol]

Here you make various rust-secp types repr(transparent) making it an API guarantee that they will be castable to secp256k-sys types.

I am not sure about this, is repr(transparent) part of the public API? If this is the case can this be avoided by documenting that it should not be relied on?

I'd suggest not doing this and just copying the vectors, manually converting the ffi objects into rust-secp ones.

Just wanted to add that this would not be a big deal, the Rust compiler often "sees through" this and makes it a pointer cast. Example: Converting between Vecs of 2 types that happen to have the same layout: https://rust.godbolt.org/z/9ahbsdr4x. Explicitly pointer casting is still more clear, not many people know that Rust has specialization behind the collect function (among other things) that makes this optimization work.

[apoelstra]

I am not sure about this, is repr(transparent) part of the public API?

Yes.

If this is the case can this be avoided by documenting that it should not be relied on?

No.

[nymius]

Here you make various rust-secp types repr(transparent) making it an API guarantee that they will be castable to secp256k-sys types. Unfortunately we want to stabillize rust-secp while secp-sys is permanently unstable (since it contains bindings to a C library that will never be "done"). So we can't make this guarantee.

Does this mean that other types that were already marked with #[repr(transparent)] make this guarantee, or the marker is going to be removed from all of them?

[apoelstra]

Does this mean that other types that were already marked with #[repr(transparent)] make this guarantee,

Yep.

or the marker is going to be removed from all of them?

I think some of them are #[repr(transparent)] but wrap a u8 array, which is fine. But the ones that wrap a secp256k1_sys type, yes, we'll have to remove that repr.

[nymius]

In 1889342:

Here you make various rust-secp types repr(transparent) making it an API guarantee that they will be castable to secp256k-sys types. Unfortunately we want to stabillize rust-secp while secp-sys is permanently unstable (since it contains bindings to a C library that will never be "done"). So we can't make this guarantee.

I've addressed this in the last changes, removing the transparent marker and the casts that relied in this assumption.

In 049e07f:

How is SECP256k1_API causing you trouble? You can PR separately to remove it; it doesn't do anything for us since we don't bind to any files that use the symbol. But I'm surprised to hear that it's actually harmful.

I have expanded on this in #878, proposing a fix in #879.

To avoid mixing the base changes with requested changes, I didn't squash nor force pushed the changes, but I can do that once concerns are settled.

Also, 049e07f is going to probably disappear once #879 is merged.

[nymius]

I have updated to the latest state of libsecp256k1 PR so far, and adapted the rust code to the API changes.

I've included these changes as separate commits to ease the review process.

[nymius]

• Changed PrevoutsSummary and Label from single field structs to tuple structs with single anonymous field.
• Added FromStr method for Label.
• Added serialization and de-serialization for Label, needed by downstream crates.

[tcharding]

Need to update the lock files mate. You can do so by running just ulf. Needs to be in the same patch that breaks them.

[nymius]

Need to update the lock files mate. You can do so by running just ulf. Needs to be in the same patch that breaks them.

Ouch, I always forgot this, thanks for the heads up!

[nymius]

I have updated the lock files. As I had to force push all commits again, missing the per revision diff, I squashed the commits again.

[tcharding]

Not exactly sure what you mean by 'per revision diff'. So in case it helps, we use the stratergy that a PR is always a set of patches where each is a discreet change. Any review suggestions go in the original patch related to the suggestion. So we are constantly rebasing and force pushing. FTR that is why most of us have moved to jj because this workflow is easier/better with jj

https://docs.jj-vcs.dev/latest/

[tcharding]

@apoelstra is it useful to you if I review this. It would take me some time I think to get context enough to do so for anything other that just code review.

[nymius]

In the light of the last changes in the libsecp PRs and possible changes of the spec, I will move this again to draft.
Will keep it updated because is a great baseline, but I don't want to drag much attention if is changing constantly.