add a test that creates two arbitrary objects

Ah! Good idea! 💡

Sadly, this doesn't solve the issue because when this test fails it's already too late - incompatible release of secp256k1 is already out.

We can test rust-bitcoin with pre-release versions of rust-secp. But without this patch even that won't help.

Using this check involves a bunch of manual work.
Alternative check suggestion: in secp256k1 CI detect if it contians breaking version bump, if yes, check if there's no bitcoin_hashes crate with higher major (or 0.minor) version in crates.io. This still has issue that if bitcoin_hashes is released after secp256k1 bump PR merges it doesn't get caught. We could check against bitcoin_hashes git repo instead including PRs but that's pprobably too much.

Sadly, this doesn't solve the issue because when this test fails it's already too late - incompatible release of secp256k1 is already out.

🤔 Hmm... But surely this does help to a certain extent. For instance, if rust-bitcoin has bitcoin_hashes version X.Y.Z and rust-secp256k1 is released with bitcoin_hashes version X'.Y'.Z' (where X'.Y'.Z' > X.Y.Z) and there is an intent to upgrade rust-bitcoin to this new version of rust-secp256k1, this check will "complain" and the solution would be to also bump the bitcoin_hashes version used in rust-bitcoin.

Alternatively, if the current rust-secp256k1 dependency of rust-bitcoin uses X.Y.Z version of bitcoin_hashes, and there is a bump in the bitcoin_hashes version to X'.Y'.Z' (whereX'.Y'.Z' > X.Y.Z ), then, whenever anyone tries to upgrade the bitcoin_hashes dependency to this new version, this check will also complain. The solution would be to upgrade rust-secp256k1 to a version using this new X'.Y'.Z' version of bitcoin_hashes. Of course, this can only be done once there is an appropriate rust-secp256k1 release for that, but, in the meantime, the discrepancy will be avoided in rust-bitcoin.

It may be that I'm biting way more than I can chew here 😅 , because I've not got much experience with release management, but that is how I understood this check. Am I missing something? I'd love to know if there's a flaw there!

Also, @tcharding, may I suggest that we add an error message here explaining briefly why it's necessary for these two to match? Otherwise, I think the failure of this check might be a bit cryptic to a new-comer like me! 😁

Ah! I think I just understood what you meant, though!

The problem arises whenever there is a bump in rust-secp256k1, but the bitcoin_hashes dependency there stays at an "outdated" version...

I think this check would still be helpful for this case in the sense that it will not be possible to upgrade both rust-secp256k1 AND bitcoin_hashes at the same time, which is what happened in 36f29d4 and 1e46eea, right?

So this check would've failed at 1e46eea.

Using this check involves a bunch of manual work. Alternative check suggestion: in secp256k1 CI detect if it contians breaking version bump, if yes, check if there's no bitcoin_hashes crate with higher major (or 0.minor) version in crates.io. This still has issue that if bitcoin_hashes is released after secp256k1 bump PR merges it doesn't get caught. We could check against bitcoin_hashes git repo instead including PRs but that's pprobably too much.

This is creative. I like it.

Sadly, this doesn't solve the issue because when this test fails it's already too late - incompatible release of secp256k1 is already out.

The issue this test solves is updating bitcoin_hashes here before in secp. If we update it in secp first and release its no problem because we have to update the secp dependency here to pick it up, at which time this test fails if we forgot to do bitcoin_hashes. Or am I missing some other way we can fail?

may I suggest that we add an error message here explaining briefly why it's necessary

The test never runs, it fails at build time if the types are from different versions. The error message looks like this:

error[E0308]: mismatched types
--> tests/dependencies.rs:8:5
|
8 |     assert_eq!(our_dep, secp_dep)
|     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `bitcoin_hashes::hex::Error`, found enum `secp256k1::bitcoin_hashes::hex::Error`
|
= note: perhaps two different versions of crate `bitcoin_hashes` are being used?

And also the file has

+//! This file does arbitrary comparisons of various types to ensure all the dependency versions
+//! are equal. It exists to catch our mistakes when upgrading crates in the stack.

Is that sufficient?

 = note: perhaps two different versions of crate bitcoin_hashes are being used?

Oh! I see. This is definitely explicit enough, I think. Thanks for clarifying that @tcharding! 👍

OK, so this is summary of what I understand now, LMK if I'm wrong:

• @arturomf94 argues (in the long comment above) that while this check doesn't prevent forgetting to bump secp256k1 when releasing breaking version it's still valuable for other cases: it may happen we bump one but forget about the other.
• @tcharding seems to agree and it was the entire point of this PR.

I agree that such test is valuable, I just have two objections to the way it's impemented:

• It seems the check will be redundant once we start using the ThirtyTwoBytesHash integration since different version will cause the same error in our lib code. Tests may be a bit more explicit but library will fail first anyway.
• If we ever use semver trick (we should whenever reasonable!) this test will pass even if the library versions do not match. The best solution seems to be just testing whether cargo tree -d --all-features output is empty. This will also catch any other library mismatch, not just bitcoin_hashes/secp256k1. We can put the test before anything else if we want to fail it fast (probably a good thing; or maybe put it in style workflows so others can still run). Happens to solve the above issue as well.

oo, I like the idea of running tree -d in CI

testing whether cargo tree -d --all-features output is empty [...] will also catch any other library mismatch

Ah! Didn't know this existed! Yeah, I agree: using that would be better because it's a generalization of what this is trying to achieve. Is it always the case that library mismatches are a no-go? I guess the collision/conflict wrt bitcoin_hashes comes from the fact that rust-bitcoin uses it directly and via secp256k1, right?

Could something along the lines of cargo tree -d --all-features | grep -E "library1|library2|library3"" improve upon the suggestion to test for the specific libraries amongst which a mismatch wants to be avoided?

Is it always the case that library mismatches are a no-go?

Probably there are cases where it's fine, but we can explicitly whitelist those.

Closing in favour of #1114