rust-bitcoin / rust-bitcoinconsensus

Bitcoin's libbitcoinconsenus.a with Rust binding. Built from Bitcoin sources with cargo.
Apache License 2.0
46 stars 34 forks source link

Improve crate #63

Closed tcharding closed 1 year ago

tcharding commented 1 year ago

Go through https://github.com/rust-bitcoin/rust-bitcoin/issues/1649 and attempt to verify that we have every t crossed and i dotted.

With this PR I believe we could consider stabilising the bitcoinconsensus crate, am I wrong? Are there any considerations re the semver number we are using or the fact that we are wrapping 0.20.0 Bitcoin Core?

apoelstra commented 1 year ago

It's a good question what we should do about semver and the underlying Bitcoin Core versions. Probably we do want to release major versions alongside Core versions. It's probably fine to commit to not making any breaking API changes except when Core releases and gives us an opportunity to. This has a pretty narrow API anyway.

Hopefully we can also semver-trick our way into exposing the same types every time we do a major release, so that rust-bitcoin can re-expose its functionality without needing to constantly do major releases itself.

Anyway this has some clippy/fmt failures but overall looks good.

tcharding commented 1 year ago

Pushed three patches to the front of the PR to fix fmt and clippy issues. No changes to the other patches.

Kixunil commented 1 year ago

I think we could just make the interface around libbitcoinconsensus wrapped in bitcoin so if the API changes here we can still avoid breakage downstream.

tcharding commented 1 year ago

Can you clarify please, do you mean

  1. Hide libbitcoinconsensus within rust-bitcoinconsesus (eg make bitcoinconsensus_verify_script_with_amount private)
  2. Don't expose rust-bitcoinconsensus in the public API of rust-bitcoin (eg, remove the bitcoinconsensus error type from our public errors)
stevenroose commented 1 year ago

For crates like this, to avoid change-the-world changes, we could use range dependency indicators like bitcoin = ">=0.28.0,<0.31.0". I'm thinking about those a lot recently, unfortunately one would have to test all the versions in between in CI somehow. But for a crate like this that only uses some basic types, it makes sense to not make dependency version bumps breaking changes.

Kixunil commented 1 year ago

@tcharding I mean 2.

@stevenroose I was thinking the same recently. I might start doing it in some of my projects.

apoelstra commented 1 year ago

This is a really cool idea, to have ranges that cross major versions. We should make an effort in rust-bitcoin to make this possible, since it will simplify upgrades across the ecosystem.

stevenroose commented 1 year ago

rust-bitcoin doesn't have to do much as a crate. Other than try be mindful of breaking changes which we already are. The org/ecosystem could develop a way to extend the github CI spec to allow building with various versions of a certain crate pinned.

Crates like bitcoincore-rpc should be able to do that fairly easily since they only use the really code types like Address and Transaction etc and they shouldn't really change much. Same goes for the crates that only use _hashes.