search

rust-bitcoin / rust-bitcoinconsensus

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

Manually create a newtype for bit flags with overloaded operators. #61

Closed Kixunil closed 7 months ago

Kixunil commented 2 years ago

Replacement of #58

Since an external macro is not desired we could just copy it and make adjustments to support MSRV or manually expand it. It'd be best to have it as close as possible to the output of said macro because it's somewhat a standard. Consumers don't even need to know it's not used under the hood if the API is the same.

apoelstra commented 2 years ago

Maybe the function in question should just take a bunch of booleans?

Kixunil commented 2 years ago

That's not forward-compatible. But a builder would be.

apoelstra commented 2 years ago

Good observation.

I do think a builder (with a try_from_u32 shortcut for people who just want to use bitflags) would be the most Rustic thing to do... and that if we're going to go into the effort of properly-typing this then we should go all the way to a builder.

Kixunil commented 2 years ago

So people would need to know which exact bit flags are used by libsecp256k1 to call try_from_u32? Sounds like not providing it wouldn't hurt much and we could even be compatible if the underlying library breaks the API to use different values (not that I expect it to happen).

apoelstra commented 2 years ago

It's libbitcoinconsensus, not libsecp, which is taking these flags .. and yes, I would expect that somebody using the rust bindings to libbitcoinconsensus would know its quirks.

But yes, I agree it is a good API goal to make this avoidable.

tcharding commented 7 months ago

Do we care about this still enough to implement it and backport it to all the versions we decide to do LTS for?

apoelstra commented 7 months ago

I certainly don't. I think we should just finish this crate and move on.

With libbitcoinkernel we can try to make a good API.

Kixunil commented 7 months ago

@tcharding can't be backported since it's a newtype.

If we accept that "just use rust-bitcoin is the proper way to get highl-level good API then indeed, this is not needed. But if we do want to expose the flags we really should use bitflags.

tcharding commented 7 months ago

can't be backported since it's a newtype.

We have made the version numbers have gaps in them so that we can backport API breaking changes to any version.

Current is v0.105.0+25.1.

Kixunil commented 7 months ago

That's not how backporting works. The entire point of backporting is to add fixes to old versions without breaking the API. If you're breaking the API you're not backporting. Using newtypes instead of raw integers is API-breaking.

tcharding commented 7 months ago

Oh I must have been on crack this morning when I made that last comment, we only have the point release number - you are 100% correct.

But if we do want to expose the flags we really should use bitflags.

I'm going to go ahead and assume this is never going to get done then because it would mean re-releasing all the core versions again - I definitely do not want to go through all that again.

Kixunil commented 7 months ago

re-releasing all the core versions again

Why? Just support the change in the latest one.

tcharding commented 7 months ago

This crate is about to be EOL'd, I don't see a reason to have support for this in one of them only when the idea is that the last 3 or 4 are meant to be used by folk that want a particular version of Core.

apoelstra commented 7 months ago

Agreed with "about to be EOL'd, let's not try to improve the API in half-hearted ways".

But FWIW I think it's pretty niche for people to want to use a specific non-latest version of Core. In my mind the reason that we did independent releases for all of them was for symmetry and possibly to help people upgrading so that they wouldn't need to jump too many versions at once.

Kixunil commented 7 months ago

This crate is about to be EOL'd

Whaat? Where was this decided? I didn't notice...

apoelstra commented 7 months ago

@Kixunil in https://github.com/bitcoin/bitcoin/pull/29189

Kixunil commented 7 months ago

Oh, wow! I had a potential real use case for it but it wasn't super-important so it wouldn't hurt too bad but I think the comment period for this kind of thing was very short. Anyway, if it's getting EOLed just remove it from rust-bitcoin (possibly with deprecation but the deprecation will have to say "you're screwed").

apoelstra commented 7 months ago

I think we should just encapsulate it in rust-bitcoin so that it won't be visible when we swap it out for libbitcoinkernel or whatever.

The upstream deprecation really doesn't matter until there is some actual consensus change in Bitcoin.

Kixunil commented 7 months ago

libbitcoinkernel

That one is stateful which is a huge can of worms.

The upstream deprecation really doesn't matter until there is some actual consensus change in Bitcoin.

But then it will be too late for people who use it.