Should update crates metadata for both crates, rename secp256k1-sys directory to secp256k1-zkp-sys

[apoelstra]

No description provided.

[thomaseizinger]

We thought renaming the crate but that has some maybe undesired consequences.

Here is how we (at least for now) intended to use our fork: https://github.com/comit-network/droplet/blob/replace-lib-wally-with-secp256k1-zkp/Cargo.toml#L5

That has the interesting consequence that the rust-secp256k1 dependency of rust-bitcoin (which is used by rust-elements) is also replaced and we have compatibility between all the re-exported types, i.e. SecretKey etc

If the crate is renamed, then we can't [patch] it anymore.

Now, the [patch] mechanism wouldn't work anyway if the lib is on crates.io ([patch] needs to use a different source) which raises the question whether we can have / want this kind of compatibility.

I am not sure what the best approach is here:

1. Drop the rust-bitcoin dependency of rust-elements so we can point to rust-secp256k1-zkp instead?
2. Provide a zkp feature-flag on rust-secp256k1 to switch to a different vendor directory?
3. Accept that we will have to link against both C libraries anyway.

(1) sounds like the least hacky solution but I am unsure how much work it would actually be. Do you intend rust-bitcoin to be a permanent dependency of rust-elements?

[apoelstra]

I think rust-bitcoin will always be a dep of rust-elements. We could maybe get away with disabling rust-secp on our bitcoin dependency (see rust-bitcoin/rust-bitcoin#527) but other things in a realistic dep tree would just turn it back on. For example elements-miniscript will depend on rust-miniscript which depends on a secp-enabled rust-bitcoin. And elements-miniscript will be the sanctioned way to keep track of pegin claims etc, whenever it's ready.

Given the way that Cargo works I'm not sure you can avoid (3).

[thomaseizinger]

I think rust-bitcoin will always be a dep of rust-elements.

Unless we explicitly want this, I think we could get rid of that reasonably easy (at least within the current feature-set of rust-elements). From having a quick look, its primary use seem to be:

• use bitcoin::hashes
• use bitcoin::bech32
• use bitcoin::secp256k1
• using some of the encoding / decoding stuff

We should be able to directly depend on the first two and the encode / decode traits have already been moved to into rust-elements anyway. Hence, the remaining implementations that we depend on could also be moved over.

Re (3): Having to link against both C libraries is not such an issue (at least for me). However, it would be a bit tedious if we have to convert between secp256k1_zkp::SecretKey and bitcoin::secp256k1::SecretKey all the time when working within rust-elements.

[apoelstra]

rust-elements currently doesn't, but definitely should, parse pegin witnesses. These consist of bitcoin transactions, block headers, and a Merkle proof.

It may be that we want to split rust-bitcoin's primitives out into a new rust-bitcoin-primitives library (and maybe also put constants in there, several people have been pining to have a separate constants repo for a while). But rust-bitcoin is fairly slow moving because we have a 2-reviewer requirement and everyone with permission is busy with other projects.

I think this is the only "hard" dependency on rust-bitcoin. You're right that everything else, including everything currently implemented, could be dropped so we could avoid the dependency.

[apoelstra]

Regarding the conversions of SecretKey et al ... we could make rust-secp-zkp depend on rust-secp, and re-export the secret key types.

[thomaseizinger]

Regarding the conversions of SecretKey et al ... we could make rust-secp-zkp depend on rust-secp, and re-export the secret key types.

I think out of all possibilities, I like this one the most.

rust-secp256k1-zkp would not be a literal fork of rust-secp256k1 then which makes it easier to manage anyway. Currently, I have to rebase our patches on top of it every time I want to update to latest HEAD.

[thomaseizinger]

I think this is the only "hard" dependency on rust-bitcoin. You're right that everything else, including everything currently implemented, could be dropped so we could avoid the dependency.

It might still be worth to give this a shot and only add the rust-bitcoin dependency again once the parse pegin witness feature lands.

[apoelstra]

I think it's still reasonable to start it as a literal fork, so that we can get the vendoring script and stuff from it, but we probably shouldn't try to maintain it as a fork (we could perhaps backport individual PRs from time to time).

@stevenroose has proposed to make rust-elements a fork of rust-bitcoin (which does not depend on rust-bitcoin). I'd like to get some pretty serious API changes into rust-bitcoin before doing that, but I think there's a good chance that we wind up going that direction.

[thomaseizinger]

I think it's still reasonable to start it as a literal fork, so that we can get the vendoring script and stuff from it, but we probably shouldn't try to maintain it as a fork (we could perhaps backport individual PRs from time to time).

Agreed, the infrastructure around the vendoring script is useful.

I was hoping that we can re-export basically everything from rust-secp256k1 but just realized that we probably can't reuse the secp context because it links to a different C library.

People will have to create a dedicated zkp context but maybe that is actually a good thing.

I will try this approach and see what it looks like!

[apoelstra]

I think we can make the contexts reusable. The context object hasn't changed structure on either side of the fork in several years.

[thomaseizinger]

So I've had a go at this. Here is the result: rust-bitcoin/rust-secp256k1@master...comit-network:alternative-history-re-exporting

I tried splitting it into sensible commits, hope that helps!

Literally reusing the context doesn't work though. The tests fail with a segfault of invalid memory reference. That might just be a minor bug though? If the layout of the context object is the same, they should be interoperable no?

[apoelstra]

No, we changed the memory layout in rust-secp since you forked. We need to re-sync with upstream rust-secp probably because there were good reasons for that.

[apoelstra]

I went over your changes, and it looks like the right approach. Need to think more about what we can safely do about the context objects.

[thomaseizinger]

No, we changed the memory layout in rust-secp since you forked. We need to re-sync with upstream rust-secp probably because there were good reasons for that.

Okay, that explains the segfault then. Note that I am depending latest HEAD of rust-secp256k1 at the moment.

I compared the context structs of the C code and discovered that -zkp doesn't have the endomorphism feature flag. See here vs here.

I guess this patch is what you are talking about?

In any case, I unconditionally enabled the endomorphism feature flag and our tests are passing now! No more segfault!

[apoelstra]

I was referring to more recent changes in rust-secp, which I realize I haven't actually merged yet.

I didn't realize that rust-secp was so far behind libsecp that we still have the endo flag. This should be removed in the next version.

[thomaseizinger]

I see! It sounds like it might get tricky at times to get everything to align nicely but nevertheless I am liking this approach.

I am going to re-write this repo's history then.

[apoelstra]

Sounds like a plan.

[thomaseizinger]

Is b2a7d2a roughly what you had in mind in regards to the original issue title?

[jonasnick]

What's the plan if there's a rust-secp256k1 release with a context that's incompatible with a deployed version of -zkp? It's not impossible as we've seen ITT. Do we assume that the UB is caught in the user's tests and they update to a newer version of -zkp?

[apoelstra]

@jonasnick that shouldn't ever happen without a major version bump to rust-secp-sys.

[jonasnick]

So the plan is to keep track of changes to the context in libsecp and make sure the first rust-secp release including that code triggers a major version bump? It seems that normally this wouldn't require a >minor version bump. I don't know how rust-secp dealt with that in the past. That's perhaps a fair price to pay given that reusing the context is quite a bit more ergonomic and efficient.

[thomaseizinger]

I have already opened an issue about this here although it would be nice if we can also have a test in rust-secp-sys that asserts the layout of the C struct.

That would make it quite easy to catch changes during vendoring.

[apoelstra]

In the past we didn't really think about it .... and probably it would've been impossible to deal with, since the endomorpism cargo feature flag on rust-secp would actually change the representation of the compiled context object. I guess rust-secp-zkp would've had to force it to always be on.

[thomaseizinger]

I guess rust-secp-zkp would've had to force it to always be on.

Yep, that is what I am currently doing to make things work:

rust-secp256k1-zkp/secp256k1-zkp-sys/Cargo.toml

Line 26 in b2a7d2a

secp256k1-sys = { git = "https://github.com/rust-bitcoin/rust-secp256k1", features = ["endomorphism"] }