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

Network is not fully descriptive enough #2169

Closed
tcharding opened this issue Nov 3, 2023 · 21 comments · Fixed by #2541
Closed

Network is not fully descriptive enough #2169

tcharding opened this issue Nov 3, 2023 · 21 comments · Fixed by #2541
Milestone

Comments

@tcharding
Copy link
Member

Our Network enum has a single variant for signet but these days folks are using multiple signets e.g., mutinynet. It would be sweet if we could support this some how.

@tcharding
Copy link
Member Author

cc @benthecarman @justinmoon

@benthecarman
Copy link
Contributor

I am honored that Mutinynet has caused this issue 😂

I think a simple soultion would just be to make the signet enum variant contain the challenge script

@apoelstra
Copy link
Member

This might be the right way to go, though it makes the enum non-Copy and significantly more expensive to copy around. It also makes it impossible to exhaustively match on it (though maybe we've already made it #[non_exhaustive] to discourage this..)

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 5, 2023

Maybe put it into Params and then accept Params instead of Network everywhere where it matters?

Also we would have to break FromStr if we added it to Network which could be annoying for many projects (electrs, definitely a few of my own...).

@apoelstra
Copy link
Member

We could move the FromStr impl to Params. And/or change Network to be a newtype struct around &Params.

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 5, 2023

I've been thinking about it a bit more and I believe it'd be a wrong move to unify them. Network represents one of the well-known, well-defined networks. Params represents any network that can be defined using those parameters. Thus Params is a superset of Network.

There are already multiple applications using Network that don't know how to deal with the abstract Params. Specifically situations like Display and to_core_arg are awkward as they suddenly become fallible or have to fall back to something silly (unknown or so). We could also force Params to contain the names but that is messy and ultimately there might not be one in some cases - this is basically #2173.

However there is a legit issue having them separated: multiple methods accept Network where they could just use Params and accepting Params would be too annoying for people using Network. The Network::params method helps but we can do better: have a trait AsParams { fn as_params(&self) -> &Params; } and accept T: AsParams. This could be AsRef<Params> but I guess implementing AsRef<Params> for Network would be awkward if Network doesn't contain the reference.

Back to the original issue, neither Network nor Params is great for signet. Network has to_core_arg which returns a single argument which doesn't describe custom signet sufficiently for Core to know about the key, Params should not force Core arguments. It seems we should have struct CustomSignet { params: Params, signet_params: SignetParams } with its own handling.

Possible counterpoint: this is too many structs and applications should just support custom signets and are already broken if they don't. If we were to accept this then custom parameters should live in Network. I personally think it's too much of a dick move and regtest is much more useful than custom signets anyway.

Rant

It really sucks that "signet" is the name used for both "the signet" - global, well-defined signet network, and custom networks. Confusing and annoying to deal with (in code).

Also it really sucks you can't just load arbitrary Params into Core. Not only would it resolve the to_core_arg issue but I have some ideas around combining regtest with signet - easy mining but only authorized person can mine. I believe it'd be very useful.

@apoelstra
Copy link
Member

Also it really sucks you can't just load arbitrary Params into Core. Not only would it resolve the to_core_arg issue but I have some ideas around combining regtest with signet - easy mining but only authorized person can mine. I believe it'd be very useful.

Agreed. We have this in Elements (you can use -chain=X for every chain, define custom ones in elements.conf, and if you want you can still set every individual parameter on the command line). It's naturally less useful for Core because Core's centered around the Bitcoin mainnet ... but maybe signet is enough of a usecase for them to consider this.

@apoelstra
Copy link
Member

Also, I agree with your comments about the relationship between Params and Network.

I wonder whether we want a dedicated AsParams trait or to just use AsRef<Params> or what.

@tcharding
Copy link
Member Author

tcharding commented Nov 5, 2023

How does this fit in with #1832? In that we add KnownHrp, which is network specific. What HRP do folk use on custom signets? Should address serialization stuff be considered as part of Params?

@benthecarman
Copy link
Contributor

Custom signet is the same hrp, only real difference is the network magic and challenge script. For mutinynet we have a different target block time but that's not officially supported by core.

@dpc
Copy link
Contributor

dpc commented Nov 5, 2023

Unhelpful note:

Reminder that this is all caused in the first place by a completely unnecessary complication.

There is just one distinction that Bitcoin ecosystem at large really needs to make: is it a real Bitcoin, or is it not. With the exception software connecting with p2p network itself, which are a minority and can handle it separately, like they already do (example: Mutinynet requiring patching / custom args).

One bit. isMainnet : bool.

We only care about protecting users from loosing real Bitcoin. Any other distinctions are a waste of time. Someone sent testnet BTC to signet wallet? Boohoo. Go to a faucet and try again.

Bitcoin community could still migrate to a state where all software is using either mainnet or testnet addresses and constants for everything but p2p, and all wallet/blockchain code in particular could be oblivious to everything but isMainnet bit. All existing networks could be obsoleted and abandoned with time. The software that do need a "p2p network" check could use whatever hash (genesis block, or hash of network params).

@apoelstra
Copy link
Member

apoelstra commented Nov 6, 2023

Morally I agree with you @dpc. But there are a few things we can help with (and which test networks are hard to use, without):

  • Supporting arbitrary/custom HRPs in addresses.
  • Supporting alternate prefixes for xpubs, pre-segwit addresses and WIF-encoded keys.
  • Having some sort of structure that contains the network magic etc. And perhaps min/max constants to guide difficulty calculations.

But I agree we don't need to foreground these and we shouldn't waste API complexity trying to make it easy for people to distinguish between different test networks, or for us to try to exhaustively enumerate all test networks. For test networks we should assume all the constants match testnet (though have some sort of escape hatch) because there's no harm if we're wrong.

Maybe we could collapse our network enum into Main Test Custom(Box<Params>), have an is_mainnet accessor on it, and call it a day.

@apoelstra
Copy link
Member

If we had such an enum we could also make it exhaustive. We got a lot of pushback on putting #[non_exhaustive] on it (and I forget whether we actually did it). Since we could add more "blessed" networks by just adding new Params constants.

@Kixunil
Copy link
Collaborator

Kixunil commented Nov 6, 2023

I really don't like making it non-Copy. That'd break a lot of code.

@tcharding
Copy link
Member Author

I would really like to have this issue closed for the next release but have no idea how to make forward progress.

@apoelstra
Copy link
Member

I think replacing Network with AsRef<Params> in every function that accepts it, would close this issue.

@tcharding
Copy link
Member Author

Ok, this morning (I'm working mornings without internet access) I replaced everything with impl Into<Network>, I'll push that up in case it is ok - no offence if you nack and want to use Paramas still though.

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 7, 2024

From the description of the issue I don't see how Into<Network> makes sense. Only Params can hold the signet magic and key.

@tcharding
Copy link
Member Author

@benthecarman if you have the time and the inclination could you please confirm that #2541 solves the issue for Mutiny. (Just a quick "I think so" is fine, thanks.)

@benthecarman
Copy link
Contributor

Seems fine to me, using the Network enum for most things seems easier, but since you can do Params::from(network) it is not the end of the world

@tcharding
Copy link
Member Author

We have added impl AsRef<Params> for Network now in #2541 so hopefully many call sites will continue to work unmodified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants