Stabalization - roadmap to v1.0
tcharding opened this issue · 14 comments
Tracking issue for what is needed for us to be able to stabalize the bitcoinconsensus crate.
If #67 merges is there anything else we want to do to stabalize this crate? If we stabalise this crate I think we can then start supporting LTS versions, one for each of the last 4 Bitcoin Core versions.
If #67 goes in then the rust-bitcoinconsensus API will be tightly coupled with the vendored Core version, this implies that to update the vendored Core version there will be changes to the Rust code too.
I rekon that once we are happy to stabalize we should release 4 LTS versions of this crate, one for each of the last 4 versions of Core. We can use the same version number as Core for major and minor, leaving patch version for us.
So rust-bitcoinconsenus v23.x.y vendors Bitcoin Core v23.x, includes patches specific to that Core version, and leaves room for security patches from us as needed. Same for 24, 25, 26.
Issues that might apply
- Consider issues described in rust-bitcoin/rust-bitcoin#2160
This all sounds good to me. I like the idea of using the Core major version numbers as major version numbers for us.
It does prevent us breaking our API without doing an update of Core but it's hard to imagine why we would do that.
We could state in the docs that the versioning in use is strange and that we reserve the right to do breaking changes in patch releases, justifiable by the fact that we flat-out should not be doing this unless its a security issue anyway (I'm thinking of perhaps some serious foot gun as an example of a fuzzy "security issue"). But I'm with you, I don't see why we would need to do this. Typically folk will be using this crate by way of rust-bitcoin so if someone is depending on this crate directly its kind of niche anyway, right?
As it is, the current versioning is not fitting in with "usual" cargo so people have to think about it. E.g cargo add adds version 0.17.1.
Ref: https://github.com/bitcoin/bitcoin/branches for why I thought we should do LTS for Bitcoin Core 23, 24, 26, 26
I think we should not mess with versions. For instance #73 wouldn't be a problem if we did. Also note that the converse is also a problem: if we use Core major version then we have to update the depending crates every time we upgrade. It could also mean that bitcoin can never be stable.
The versioning in Rust works certain way because of good reasons and it solves many problems. If we start messing with it we get even more problems. Let's not do that.
if we use Core major version then we have to update the depending crates every time we upgrade. It could also mean that bitcoin can never be stable.
No matter what, Core may break its API between updates so that updating Core will require a major version bump here. Maybe sometimes it would be avoidable if we were using Rust versioning but not always.
For this reason I think in bitcoin we need to encapsulate bitcoinconsensus so that users aren't exposed to such breakage.
Yes, but at least we can try to make the API compatible in some cases. If we use Core major version first we have guaranteed breakages every time they release a new one.
AFAIU Core is not using the patch version number, they are only using major.minor, I don't know how (or if) they do security updates? As long as they follow the semver rules we can use core-major.core-minor.rust-patch and be totally semver compliant. If I'm correct then we get rid of the 0.21.2-0.6.0 format which is the reason #73 was a problem. Or am I missing your point totally @Kixunil?
For this reason I think in bitcoin we need to encapsulate bitcoinconsensus so that users aren't exposed to such breakage.
I thought we had done this but using the check public api script I found these three, will fix:
impl core::convert::From<bitcoinconsensus::Error> for bitcoin::consensus::validation::BitcoinconsensusError
pub extern crate bitcoin::bitcoinconsensus
pub fn bitcoin::consensus::validation::BitcoinconsensusError::from(e: bitcoinconsensus::Error) -> Selfpub extern crate bitcoin::bitcoinconsensus
I'd like to keep this, I guess behind a feature gate that's clearly "unstable".
impl core::convert::Frombitcoinconsensus::Error for bitcoin::consensus::validation::BitcoinconsensusError
Nice :) this is super subtle and I doubt I'd have noticed it without the API script. We have this conversion for internal use only but it's technically exposed.
Yep, and its trivial to remove, working on it now!
As long as they follow the semver rules we can use core-major.core-minor.rust-patch
They don't. And even if they did, it's one version for multiple unrelated APIs - e.g. RPC. So we would get breaking number bump even if they changed RPC which is ridiculous.
Really, let's not drag other projects into our versioning and lets' use the tools provided to us as designed. I see no problem with our-semver+core-major.core.minor, do you?
this is super subtle
Heh, I'm pretty sure I already complained about hidden conversions but can't find it. Exactly because if issues like this. And unlike other hidden APIs, this one is super-easy to use by accident simply using the ? operator.
We are well on our way to getting to Core 26 with the new versioning number scheme, then once we do 27 we'll likely archive this repo. This issue is mute now.