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.

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

Global Network #1840

Open
Kixunil opened this issue May 7, 2023 · 25 comments
Open

Global Network #1840

Kixunil opened this issue May 7, 2023 · 25 comments
Labels
brainstorm minor API Change This PR should get a minor version bump

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented May 7, 2023

I just realized that many applications allow the user to configure them for a single network which is then used in the whole program. One would normally parse the network from configuration and then pass it around in arguments. This can be a bit annoying.

Maybe we could also provide a global (internally AtomicU8) that user could set in main and then use alternative functions that don't require Network. The functions would obviously need to panic if the network is not set. To avoid accidental use by libraries (and also target_has_atomic issues) this should be behind a feature flag. When the flag is activated we could also provide an implementation of FromStr/Deserialize for Address<NetworkChecked>. (NetworkUnchecked should keep its behavior regardless of the flag.)

Note that this is intended for applications only, libraries should continue to support arguments. Also our library should support both interfaces.

@Kixunil Kixunil added minor API Change This PR should get a minor version bump brainstorm labels May 7, 2023
@apoelstra
Copy link
Member

I like this idea.

I wonder if we should rename Network to Chain. Or just start deprecating Network in favor of a new Chain that we think through more carefully.

In parallel to this I'd like to zoom out a bit and think about what structures here are Network specific. It seems like

  • Address, Extended*Key, PrivateKey all have prefix bytes in them which (mostly) imply a network; Address also has clear money-losing failure modes associated with confusing which network is involved.
  • Block and Transaction (and their components) are inherently tied to a specific chain because they commit to data in a specific chain. It's impossible to use these on the wrong network but a network mismatch between Address and Transaction is the issue that we're trying to prevent by having Address by network-checked. There may also be ergonomic value in the user being able to check "what network is this tx for".
  • Script too. Address -> Script -> Transaction needs to make sure the network doesn't change.
  • Similar story for Txid; inherently tied to a network and can't be confused. But the user maybe wants to keep track of this nonetheless.

It seems like there ought to be an API where the user could construct some basic data structure with the correct Network set and then magically everything else would be inferred/verified to be consistent. Doing this in general would require dependent types.

@apoelstra
Copy link
Member

Doing this at compile-time would require dependent types (and require users produce proofs all over the place that e.g. two parsed-from-string addresses have the same network). But maybe there's a runtime solution:

  1. Basically every structure in the library (block, header, tx, txin, txout, script, scriptbuf) gets an Option<Network> field. The "normal" constructors set this to None except if @Kixunil's magic atomic is set, in which case they are set to Some with the user's global network choice.
  2. Constructors for things like Address which have their network encoded in the string always use that encoding[*].
  3. Whenever things are composed, network consistency is checked. So you can e.g. convert an address to a scriptpubkey, which will carry the network field, then when you try to put that into a TxOut it'll make sure the networks are consistent.
  4. PartialEq and Eq also check for network consistency before declaring equality.

Finally, consensus::Encode and serde::Serialize are gated on having the network field set.

[*] Ok, so the network encodings for things like Address are actaully ambiguous. So we would need to actually represent networks internally as a system of constraints and do unification, which will require a little bit of CS magic. But I think not much. So when I say Option<Network> I really mean "the empty set of network constraints".

@apoelstra
Copy link
Member

Despite using the language of "constraints" and "unification" I actually think this could be implemented entirely in terms of bitmaps and bitwise AND. So initially the "network" field of each structure would be all-bits-1 "every network possible". Then:

  • If the global atomic is set (it will be a single bit, i.e. "only this specific network is possible") then we & the result with that.
  • If the user calls set_network explicitly on the type, then we & the result with that.
  • (In theory we could expose more nuanced modifiers, but we probably don't want to. Though it may be interesting to reserve some bits for e.g. bcash, and let users directly modify those if they want.)
  • If the type is parsed in a way that implies certain network(s) then we & with that
  • If a type is constructed where the network map is 0 "no network possible" this is an error.
  • If a type is compared where the network map &s to 0 this is an "unequal" comparsion regardless of how the other fields are set.
  • If a type is serialized but the network map has more than 1 bit set, this is an error.
  • Certain constants, e.g. genesis hashes, have their Network fixed from the beginning.
  • We would encourage crates like jsonrpc to somehow set the Network of incoming structures based on what daemon they're talking to. (Or maybe users need to call getblockchaininfo RPC and then set this themselves.)
  • We would encourage users to set the network flag explicitly when deserializing their own config files etc. So e.g. a wallet might have a testnet boolean and then the wallet-parsing logic should set all the wallet data to either mainnet or testnet, based on that.

Finally, we'd need some set of escape hatches -- a way to explicitly override the network, a way to say "ambiguous serialization is OK" (maybe we just want a bit in the map for this..), and maybe a way to directly set/clear network bits.

I think, with all this in place, "most" users would have their network usage automatically checked for consistency, without them needing to explicitly think about it except maybe at startup time, and without them being encouraged to "fix" a network at the type level so that it's impossible to use their stuff on testnet.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 7, 2023

I wonder if we should rename Network to Chain

IDK, bitcoind calls it chain but all other applications I know of call it network. I wouldn't change it. But maybe we should use Magic instead to allow custom signets.

Your ideas seem complicated so I will have to take more time to respond to them properly. My instict is to not over-complicate but there may be good reasons. Also I think that if every input is gated on valid network it should produce valid outputs. So we really only need this in Address, Json RPC (yes, I do agree it should use our API to support checking more directly).

Raw types like Transaction don't have the tag but it seems odd to add it. Having Block contain a bunch of identical bits isn't nice.

@apoelstra
Copy link
Member

My instict is to not over-complicate but there may be good reasons.

Right now from a user perspective, Address is extremely complicated, because our "simple" implementation completely pushes the responsibility for network-tracking onto them, and because it's not obvious what functionality is available under what conditions. (Downstream many crates have had to modify tons of unit tests, which don't care remotely about network stuff, to add .assume_unchecked() calls everywhere, which isn't obvious to do and isn't even obviously correct.)

Also I think that if every input is gated on valid network it should produce valid outputs.

I think that to achieve this we'll probably need something roughly as complicated as what I'm describing. Basically for the compiler to do it for us we'd need dependent types, and since we don't have them, we have to do our own run-time typing.

Raw types like Transaction don't have the tag but it seems odd to add it.

In your view, when converting a testnet address to a scriptpubkey, embedding this in a mainnet transaction, then broadcasting it to the mainchain, which step is the mistake and how should we determine that?

Having Block contain a bunch of identical bits isn't nice.

Hmmm, yeah, definitely agreed here. And it's even worse because every TxIn and TxOut, every Script etc., will also have the identical bits. We will need a better solution. (Maybe gating all these wasteful fields on debug_assertions?)

@tcharding
Copy link
Member

Hand wavey input: We should try really hard to make uses of Address be simple because its such a basic Bitcoin thing. When basic things are hard to use it puts devs off using the library, IMO. Obviously it should be correct first, simple second.

@dpc
Copy link
Contributor

dpc commented May 8, 2023

Global variables break composability and lead to action-at-distance.

I understand the sentiment, as passing such a "context" seems like pure waste. I think generally contemporary programming languages have poor support for "context" passing. (Relevant: https://tmandry.gitlab.io/blog/posts/2021-12-21-context-capabilities/)

But all in all global variables ... 🤮 ... There's an already long thread of complex ideas involved, and that is only internally.

As a user now I'll have to pay attention to which APIs can be used with a flag, which can' etc. And poor developers that will have to debug issues due to someone somewhere abusing mutable global variables, etc.

IMO, if application writers want a global, they can write it themselves. Even write a global-singleton wrapper around rust-bitcoin, that simplifies their life.

@sanket1729
Copy link
Member

I was in between a long response. But @dpc got the most of it.

I strongly agree with dpc, I think this is bite us in the long run by break composability. This will increase more cognitive load for reviewers to think about "mutable global" things when looking at local changes.

IMO the benefits are not that impressive either.

  1. I don't think saving the developer who can accidentally pass a wrong network is a probable error that we should care about. For example, it is also possible the developer calls p2wsh instead of p2sh etc. Or calls a address on the wrong script.

  2. I am sure that there would be other things in global config file for application developer apart from just the network. So, adding this is not preventing them from having a global config file or network specific thing that might already do. They probably have a network dependent text like "Warning: testnet coins are not real" or "warning: Using mainnet on experimental software" etc.

  3. If there is a need to create simple interfaces for new developers with globals and fixed reasonable defaults, we can make another crate on top of rust-bitcoin to support it. There are not that many APIs with network parameter, a simple wrapper around rust-bitcoin can do.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 8, 2023

Right now from a user perspective, Address is extremely complicated

Yes, I think my proposal should mitigate much of the complication in vast majority of cases - you just get FromStr for Address and the only case where it gets weird is if you try to work with multiple networks.

unit tests, which don't care remotely about network stuff, to add .assume_unchecked() calls everywhere

I admit this sucks but I don't really know how to fix it. We could add this_is_test method equivalent to assume_checked just for semantics but not sure if it's a good idea.

In your view, when converting a testnet address to a scriptpubkey, embedding this in a mainnet transaction, then broadcasting it to the mainchain, which step is the mistake and how should we determine that?

Assuming the application is single-network (vast majority of applications) there are two places where the mistake could happen: when parsing the address or when connecting to the network. We have the first covered now, so only the second remains. I think JSON RPC should have some methods to either check the network by calling getblockchaininfo right after connecting or explicitly acknowledge that one doesn't want to check.

Multi-network applications don't get protection but I don't remember ever seeing one, so it's safe to assume we don't need to bother.

@dpc, @sanket1729 firstly I need to say that I hate global mutable variables with passion. But I think there is a case to be made for those which are loaded at the beginning of the program, set once and then never mutated again. Basically lazy_static. The negative effects of these are extremely limited because there's normally no "action" in "action-at-distance". I would personally support design that would outright forbid setting the value more than once (by panicking), including to the same value it already has. Yes, it still has problems around tests but that's a choice up to application developer.

As a user now I'll have to pay attention to which APIs can be used with a flag

Clearly marked in documentation and you can't use them by accident. Also you won't get bogus suggestions from RLS. Seems fine to me.

poor developers that will have to debug issues due to someone somewhere abusing mutable global variables, etc.

That can happen already. And banning setting the variable twice should make it less likely to happen. I would also be in favor of saving the Location of the initial set call so that it can be reported. ("The network was already set at {}") You'd literally get exact lines where things went wrong.

if application writers want a global, they can write it themselves

and

we can make another crate on top of rust-bitcoin to support it

unfortunately this doesn't work well due to orphan rules (can't impl FromStr for Address). The conversions required would suck even more than the solution.

saving the developer who can accidentally pass a wrong network

The current Address API already addresses that so it's not a design goal. The goal is to simplify the API for vast majority of applications, Possibly all of them.

So, adding this is not preventing them from having a global config file or network specific thing that might already do.

I don't understand what you mean, it looks like you meant "not adding"?

@dpc
Copy link
Contributor

dpc commented May 9, 2023

@Kixunil

If the goal is not have to pass Network around, users can do it themselves.

If the goal is to have FromStr ... then I just don't think it's worth it. fn parse(network: Network, s: &str) -> Address etc. is really not that much different from plain FromStr.

The conversions required would suck

If someone really needs to have the parsing of Address generic with all other types, one could have a bitcoin-network-ctx with:

pub trait NetworkCtxFromStr : Sized {
    type Err;

    fn network_ctx_from_str(s: &str) -> Result<Self, Self::Err> {
}

impl NetworkCtxFromStr for T where T: FromStr { ... }

impl NetworkCtxFromStr for bitcoin::Address { /* use the `Network` lazy static */ }

then also a normal newtype-wrapper:

struct NetworkCtx<T>(pub T);

impl FromStr for NetworkCtx<bitcoin::Address> { ... }

maybe with impl Derive<Target=bitcoin::Address> and .into() for that times where need to work directly with stuff that requires plain T: FromStr.

For good or bad, this is how these sorts of things are handled in Rust all the time, and people are used to it.

@apoelstra
Copy link
Member

Assuming the application is single-network (vast majority of applications) there are two places where the mistake could happen: when parsing the address or when connecting to the network. We have the first covered now,

I really don't think we do. What happens now is that you parse an address and then find that basic functionality is missing and then you have to carefully scan the docs to figure out what generics you need, and what functions to call to get those generics.

IMO this generic solution is undiscoverable and confusing. If we want to make the user check the network at parse time we should get rid of from_str and only have from_str_with_network, although that will then make it impossible to write code that roundtrips addresses regardless of network, even though such code is perfectly safe. If we want to force a network check when converting to a scriptpubkey then we should stick a network check onto .script_pubkey even though this will probably result in redundant network checks for most users. But at least the user has some hope of finding these functions.

If we want to make it so that the user can just check the network and have that check propagated with the address from there on out, we basically cannot do it with the Rust type system. At best we can propagate a binary "I checked the network" which is what we've tried to do, but the result is noisy and hard to use.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 10, 2023

If the goal is to have FromStr ... then I just don't think it's worth it.

Yeah, that one would be probably not enough. serde is much more interesting.
And yes, the trait approach you suggest works OK.

have to carefully scan the docs to figure out what generics you need, and what functions to call to get those generics.

IMO this generic solution is undiscoverable and confusing.

I don't think so. Reading the docs is a fundamental part of development. I read them all the time. We have a whole section about it in the docs. If people come up with specific ways the section can be improved I'd be happy to.

get rid of from_str and only have from_str_with_network

The issue is people use parse() rather than from_str. I don't remember ever seeing from_str outside of cases where it was written by a newbie (and possibly remained unchanged for a long time). But I would be open to adding a dedicated method that does .parse()?.require_method() in one step if it helps.

stick a network check onto .script_pubkey even though this will probably result in redundant network checks

It will lead to something worse: moving validation further down the line which makes error handling and debugging harder.

the user has some hope of finding these functions.

They are literally in the docs. But it just occurred to me that we could add a note to each of them that they need checked address.

@apoelstra
Copy link
Member

I don't read documentation end-to-end and I don't think most anybody else does either. If I need a function then I scroll (or more likely search) the list of available functions. If search yields no results then I look for other functions that seem to need the missing functionality and read their source code. If searching finds something, but it's gated on some set of generics I'll get annoyed, then find source code to read. If this happens too many times I just drop the library, or revert to some old version before they started doing crazy architecture experiments. (I use time 0.1 for example rather than 0.2 for this reason. AFAICT 0.2 is actually missing critical functionality because they tried to Rustify it, while 0.1 more-or-less replicated the C API which works. In 0.3 they seem to have fixed it but have a high MSRV.)

Only for well-maintained and mature software like serde (and rust-bitcoin is definitiely not mature!) would I consider actually reading the documentation. Otherwise that sounds like a good way to waste a bunch of time without actually learning how to use the library.

Having said this, I checked the Address docs on rust-bitcoin, which I've never read (though probably I checked it for typos and rough correctness when it was being PR'd), and it does indeed describe how to deal with the marker trait. It suggests this is "for type safety" which sounds good on first reading, but after trying to use it and finding how inconvenient it is (and that "type safety" does not give me any hint what methods require validation and which ones don't) I think as a user I'd like there to be a better reason. It's especially confusing because the type doesn't actually carry any information about the network, just a boolean "did you call this API function?".

Yeah, that one would be probably not enough. serde is much more interesting.

I take your point about serde and parse(), neither of which have a sensible way for us to tack a Network parameter onto. But the current situation leads to the user being unable to serde-decode something they just serde-encoded, without doing a network check on data that was encoded in the address, and then having the code just throw away the network that was checked.

It will lead to something worse: moving validation further down the line which makes error handling and debugging harder.

It will lead to moving validation to somewhere where it's logically needed, instead of making users deal with it when they don't have to and won't understand why they have to.

@stevenroose
Copy link
Collaborator

nACK on the idea of adding a global to the library. Like the OP says, applications do that, I don't think libraries should introduce sidechannels in tons of functions. Our functions should be deterministic.

It's really not hard for an application to have a const NET: Network and pass that in any library function that needs a network.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 10, 2023

@apoelstra could you hint what in time you found confusing? From quick look it seems sensible.

It suggests this is "for type safety"

I don't see any mention of type safety there. It's for user safety. When I upgraded rust-bitcoin after we forced the check I discovered several other instances missing it! Also I noticed a similar bug in LND - if they used a library forcing the check they wouldn't have it.

Note that I personally had to help numerous people recover their coins sending to a wrong address (was BTC vs LTC but still same category of issue). Not only that, I once was the one who hastily forgot to manually check the address even when being aware of the problem (was an ATM and the manufacturer was slow fixing this). So forgetting a check like this is a serious problem that is unsolvable for 99% of users.

In my eyes, this is totally worth a bit more annoying API - because it's the correct API. Programming is complex and rather than hiding this complexity under the carpet where it'll kick you in the ass later we should make tools to deal with it. Especially in security-critical software like Bitcoin.

just a boolean "did you call this API function?".

That's enough. It's similar to Rust not auto-inserting unwrap (unlike Java) whenever you pass Option<T> where T is expected and it often reminds people that it needs to be checked. Saved my butt million times.

It will lead to moving validation to somewhere where it's logically needed, instead of making users deal with it when they don't have to and won't understand why they have to.

If this is actually logical in user code (I highly doubt so) they can just pass around Address<NetworkUnchecked>.

@stevenroose we would still have the same deterministic functions, just in addition we would have simpler options. As I say single-network applications are extremely common. Actually, the only one I know that isn't is Ride The Lightning but that one doesn't directly work on Bitcoin stuff, just makes API calls into LN node. Every single wallet, Bitcoin node, LN node, ... everything else is like this. So why should everyone reinvent this when it can be in the library? Especially considering orphan rules? If not for orphan rules and annoyance of extension traits I'd be onboard with putting it into a separate library.

to have a const NET: Network

That's actually a terrible design since you have to rebuild the whole application if you want to test something. (Although it's billion times better than having it both hard-coded and scattered around the code. Yes, sadly, there's one such popular wallet with mainnet hard-coded. No, I have no clue how they test it.) Having these helper functions could actually incentivize people to design the application properly. Even if they hard code bitcoin::set_network(Network::Bitcoin) in main() it's super easy for someone to make a PR.

@dpc
Copy link
Contributor

dpc commented May 10, 2023

Note: I only read the documentation when I'm investigating something specific. Modern life is just too complex and busy. And as a maintainer of a handful open source project noticed that just because something is documented, does not prevent large chunk of users routinely not being aware of it. Even if you put it in bold capital letters on the front and center. That's why I like type system forcing user's hand or at least long informative names bringing attention to certain aspects.

In my eyes, this is totally worth a bit more annoying API - because it's the correct API.

Agreed. As a user I'm happy to deal with an extra type steps if it ensures correctness. I'm only allergic to global values breaking composition, and introducing conditionally available set of APIs.

I've already mentioned 2 separate "If the goal is". This seems to be the 3rd one.

If the goal is to force users (developers) to verify network address, session types seem like the way to go:

trait VerificationStatus;

struct Verified;
impl VerificationStatus for Verified{};

struct Unverified;
impl VerificationStatus for Unverified {};

struct Address<Status = Unverified>{ .. };

// Since `Address` == `Address<Unverified>, maybe an alias like this:
type VerifiedAddress = Address<Verified>;


impl Address<T> {
  fn verify(&self, network: Network) -> Address<Verified> { ... }
  fn assume_verified_yes_i_am_sure(&self) -> Address<Verified> { ... }
}

etc.

Then if the crucial places at which point the address should have been already verified, require VerifiedAddress.

A runtime flag is also an option, if the API noise of the above seem undesirable.

@apoelstra
Copy link
Member

In my eyes, this is totally worth a bit more annoying API

For sure.

  • because it's the correct API

But it's not correct, since it won't actually prevent you confusing two (checked) addresses that correspond to different networks, and because it won't let you do perfectly safe things like deserializing then re-serializing an address without adding extra .assume_unchecked.

Sure, in practice this is good enough for safety, but because it's ad-hoc and "good enough" there's no mental model the user can have that will let them predict what they're able to do and not do with Address. At best this adds friction, but at worst it means they'll just learn to stick .assume_checked() everywhere to make the API work. Or switch to a different library.

@dpc
Copy link
Contributor

dpc commented May 10, 2023

it won't let you do perfectly safe things like deserializing then re-serializing an address without adding extra .assume_unchecked

Isn't this very reasonable? In a general case if you serialize you loose a lot of context, metdata, and sometimes even more core properties (ordering, types, etc.). If you deserialize, you can't really be sure if you got correct addresses. You can assume it if you trust the source or have some other pre-existing knowledge.

As for mixing checked addresses from different networks: it's not unlike core tenants of Rust like e.g. unsafe. When using unsafe { ... } it is not required that this particular unsafe block guarantees locally that the usage is correct. It's a global promise that safety requirements of unsafe functions used in it will be upheld here and everywhere else. There is no practical way to express complex safety contracts in certain situations, so the language / APIs do the best they can.

So being a little bit "ad-hoc" does not seem like a big deal to me. It's just a pragmatic tradeoff. Forgetting to check the address is a common source of important bugs, so you guide the developer to do the right thing as much as practical.

but at worst it means they'll just learn to stick .assume_checked() everywhere to make the API work.

You can lead a horse to water.

Nothing prevents Rust developers from throwing random unsafe { ... } blocks without too much thinking. A yet people care and projects that do get shunned (see actix-web drama).

By having explicit guide rails and checks, it makes it easier for other people to verify it when reviewing the code.

@apoelstra
Copy link
Member

@dpc I'm talking about deserializing then serializing. There is no data loss here.

@Kixunil I'd be fine with an API where you could do anything with an unchecked address except get a scriptpubkey out of it. And this should be documented.

Right now the Address API is too complicated for me to use and I really don't want to update my software to bitcoin 0.30 because of it.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 10, 2023

@dpc the goal is to make it easier to use the Address API in contexts that are affected by orphan rules while continuing to prevent network-related bugs. And as I said, this is super-common - like literally every application except for RTL. (If you can find 10 other applications, I will close this right away.)

We already use session types, so I'm not sure what you mean by that example.

since it won't actually prevent you confusing two (checked) addresses that correspond to different networks

I can't even imagine hypothetical scenario where an application needs to check equality of networks of two addresses while at the same time not check it against some global network setting. As I said, there's at most one application I know of which can't be excluded straight away.

, in practice this is good enough for safety, but because it's ad-hoc and "good enough" there's no mental model the user can have that will let them predict what they're able to do and not do with Address

Rust can't prevent all bugs and yet people use Rust just fine. I think this is just guessing. To conclusively determine whether the API works we need to ask a bunch of people who actually use it.

Notably, we should probably also document that assume_checked is completely fine in tests.

could do anything with an unchecked address except get a scriptpubkey out of it

Yeah, this was my original idea as well. The cool thing is we can do this now without breaking anything. I'm pretty open to this, will think about it more.

Right now the Address API is too complicated for me to use and I really don't want to update my software to bitcoin 0.30 because of it.

😢 This is a pretty strong evidence for the proposal above but note that bunch of upgrade problems were caused by lack of PartialEq between the address types and lack of reference-returning assume_checked. These were resolved in master.

@apoelstra
Copy link
Member

Ok! Cool, I think we're (sufficiently) in agreement. Let's try to work toward a "Address<Unchecked> can do everything except script_pubkey (and maybe others; but it should be a short clearly-deliminated list)" model.

but note that bunch of upgrade problems were caused by lack of PartialEq between the address types and lack of reference-returning assume_checked. These were resolved in master.

Oh, phew :).

@dpc
Copy link
Contributor

dpc commented May 10, 2023

We already use session types, so I'm not sure what you mean by that example.

My bad, sorry. I'm out of context, got here because of "global variables", haven't even checked, and didn't use Address in the new version. blush

Looking ...

Wouldn't you want some type UncheckedAddress = Address<NetworkUnchecked>? It eliminates the need to type <...>, and make them look more like distinct types with same set of methods.

Also - some Address::from_str_checked(&str, Network) and Address:to_string_checked(Network) could be useful?

Seems like the documentation and API insist on using the generic methods, but that's not necessary. Generic methods are required when dealing with abstractions that need them. For concrete use:

let address = Address:from_str_checked(s, self.network)?;

seems handier than:

let address = Address::from_str(s)?.require_network(Network::Bitcoin)?;

and autocompletion might suggest it due to common prefix.

Right now the Address API is too complicated for me to use and I really don't want to update my software to bitcoin 0.30 because of it.

Is there somewhere I can read about the biggest pain points? I don't understand "easier to use the Address API in contexts that are affected by orphan rules".

@dpc I'm talking about deserializing then serializing. There is no data loss here.

But there's a context-loss.

@Kixunil
Copy link
Collaborator Author

Kixunil commented May 10, 2023

Wouldn't you want some type UncheckedAddress = Address<NetworkUnchecked>?

Yeah, that could help. Will be happy to accept a PR after I decide if I like UncheckedAddress more than AddressUnchecked. :)

Address::from_str_checked(&str, Network)

Yeah, it's a bit shorter and if people already like to say from_str instead of .parse() then this is better.

I don't understand "easier to use the Address API in contexts that are affected by orphan rules".

We don't implement FromStr or Deserialize for Address<NetworkChecked> and users who want a global can't implement it themselves due to orphan rules. And while I hate globals, this one is among the most reasonable globals one could want - all applications are single-network.

I also realized there's an additional utility in having this behind a feature flag: if you decide to refactor the code to remove the global, just turn it off and fix all compile errors.

@dpc
Copy link
Contributor

dpc commented May 10, 2023

We don't implement FromStr or Deserialize for Address<NetworkChecked> and users who want a global can't implement it themselves due to orphan rules.

But that's the whole point? When you parse/deserialize, you don't know if what you're parsing is actually valid. So you have to pause, scratch your head, maybe read some the docs, and come up with a solution.

You can use AddressUnchecked instead, and check later in your code.

You can use some hypothetical AssumeCheckedAddress newtype that impls these traits implicitly, and pinky promise to check later (if required).

You can use some hypothetical #[serde(deserialize_with = "bitcoin::address::deserialize_assume_checked")], and pinky promise to check later (if required). Probably preferable if you want retain plain Address (checked) in the type.

At least 3 decent options, that are all local, and fairly standard.

The FromStr has similar approaches.

@stevenroose
Copy link
Collaborator

Not really following entirely, but from some snipper above: it should definitely be struct Address<S = Verified> so that Address itself is verified.. The unverified variant should be the special/temporary one. So that the normal API is on impl Address {, which is on the verified type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
brainstorm minor API Change This PR should get a minor version bump
Projects
None yet
Development

No branches or pull requests

6 participants