Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add non_exhaustive compiler directive to AddressType #1011

Merged
merged 1 commit into from Jun 2, 2022
Merged

Add non_exhaustive compiler directive to AddressType #1011

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/util/address.rs
View file
Open in desktop
Expand Up @@ -139,6 +139,7 @@ impl From<bech32::Error> for Error {

/// The different types of addresses.
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[non_exhaustive]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm a little dubious of this - I can imagine matching on this and doing...something, eg maybe I want to know if an address is a witness and map every type to a bool or something, but with non_exhaustive now you have to _ => ...panic, I guess?. There are definitely reasons to want forward compat without breaking downstream, but for "we added a new address type" kinda things is so rare that I'm not sure we need to worry about that breakage, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I'm not totally sure of the merit of this PR myself, whether we should use non_exhaustive, or whether we should use it but on some different set of types. There has been multiple folks speak up in the past positively about non_exhaustive, the intention of this PR is to bring out all the opinions and then I'll just try to shepherd them in. As such, I'll leave this sitting open for a while and see what comes of it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are much better ways to detect whether an address is witness (or something else) than matching each type. I think bumping the major version just because there's a new soft fork is annoying. The issue with this library is that pretty much any Bitcoin application written in Rust uses it, so it's a shitton of breakage.

IMO any public enum and struct that could be possibly extended in the future should be #[non_exhaustive]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean philosophically I agree with you, it'd be cool to have the library not have ballot changes for soft forks. In practice, though, that just makes the downstream code that would have broken have panics that they may hit instead, which isn't an improvement. More generally, Bitcoin does soft forks really really rarely, I'm not sure we need to worry too much about changing a few enums when a new soft fork happens - we probably have to change a ton of fields to add support for it anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with @Kixunil here ... my feeling is that matching on these enums is a bit of a weird thing to do, and if you are doing this, you ought to have an error path for the case that some unsupported softfork has happened. (Or in some cases perhaps you could do some sane default.)

On the other hand, soft forks in Bitcoin are really rare, so this is something of an academic discussion. Maybe the time to discuss this is whenever the next fork actually seems to be happening? (My guess is that the next fork will be addition of some covenant opcodes, which won't even bump any version numbers, though it could also be APO..)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is it's not "just updating a few enums". Due to breaking change there needs to be a coordinated major version bump across all ecosystem. This is a huge library and it is the only (rich) Bitcoin library available in Rust today, so pretty much every Bitcoin project written in Rust depends on it. Perhaps if Address was a separate crate it'd be less painful but I guess it'd still be significant.

This is my main motivation behind trying to make the library stable as you might have noticed from my efforts.

Maybe the time to discuss this is whenever the next fork actually seems to be happening?

I don't think so, if at that time we have 1.0 it's already too late. At the latest we need to decide before making it 1.0.

pub enum AddressType {
/// Pay to pubkey hash.
P2pkh,
Expand Down