WIP: Use new bech32 crate rewrite #1809
Conversation
a6c4087 to
932d9b7
Compare
bitcoin/src/address.rs
Outdated
| @@ -32,7 +32,8 @@ use core::fmt; | |||
| use core::marker::PhantomData; | |||
| use core::str::FromStr; | |||
|
|
|||
| use bech32; | |||
| use bech32::primitives::iter::{ByteIterExt, Fe32IterExt}; | |||
| use bech32::{witness_version, Bech32, Bech32m, WitnessVersion}; | |||
There was a problem hiding this comment.
I find it odd that WitnessVersion lives in bech32 now. Can you explain the decision more?
There was a problem hiding this comment.
This wasn't my decision, but I would guess that @tcharding figured that since we have to support witness versions at least in the form of arbitrary bech32 characters (since the bech32 spec itself treats witness versions specially, decoding the witness version character as an 8-bit byte rather than a 5-bit thing), we might as well give it a dedicated type so we could enforce that only the bech32-valid values are used.
I am of two minds on this ... on the one hand it decreases the generality of the library a bit by undermining other checksums that use a wider notion of "witness version" ... but on the other hand, AFAICT only bech32 and bech32m have a notion of "witness version" at all, and the other checksums used in Bitcoin just decode bech32 strings as strings.
There was a problem hiding this comment.
Note that any checksum that did want to have their own non-bip173-like version of witness versions would be able to do this by creating another iterator adaptor, and then adding some new high-level funcitons to wrap it.
There was a problem hiding this comment.
Yes, it seems to me that the bips are pretty tightly coupled with the concept of witness version and also the same arguments apply to Network so I thought we should have types for them.
Note that any checksum that did want to have their own non-bip173-like version of witness versions would be able to do this by creating another iterator adaptor, and then adding some new high-level funcitons to wrap it.
Unless I'm missing something I think users wanting to opt out of WitnessVersion can use Parsed::new and the fist byte can have any meaning they want, right?
I am of two minds on this ... on the one hand it decreases the generality of the library a bit by undermining other checksums that use a wider notion of "witness version" ... but on the other hand, AFAICT only bech32 and bech32m have a notion of "witness version" at all, and the other checksums used in Bitcoin just decode bech32 strings as strings.
So this is my main concern as well, that is why I added a couple of new methods in primitives that allow bypassing the Network type (using a string HRP). I was steering this towards making the crate root API functions super bip-173/350 specific and really easy to use for the typical Bitcoin usecase. All other niche usecases have to go to primitives.
We hadn't really talked about the Network type @apoelstra, to discuss that was one reason for this draft.
There was a problem hiding this comment.
Of course we don't have to remove the WitnessVersion from rust-bitcoin (I believe the abstraction is exactly the same so that's why I removed it. The Network abstraction however is not the same in rus-bitcoin as in bech32, if I'm not confused, as far as bip173/350 are concerned there is only mainnet, testnet, and regtest i.e., there is no separation between signet and testnet for segwit addresses.
There was a problem hiding this comment.
Oh, I think I know what's the correct rule: types that don't depend on anything else from
bitcoin*exceptinternals. They should operate only on Rust types. So ifbitcoinis analogous tostdthenbitcoin-primitivesis analogous tocore(calling itcorewould be obviously confusing).
I like this. Let's try to use this rule of thumb and see if we can make it work.
primitives is fine :). core is not :P.
There was a problem hiding this comment.
bitcoin-primitives is taken on crates.io already.
There was a problem hiding this comment.
rust-bitcoin-primitives - primitives types used by the 'rust-bitcoin' ecosystem?
We could take rust-bitcoin-internals while we are at it.
There was a problem hiding this comment.
I'd rather not use a rust-bitcoin prefix. It feels a bit too redundant to have rust in the name of a Rust crate.
Other suggestions are bitcoin-basics, bitocin-data, bitcoin-essential.
There was a problem hiding this comment.
I'd almost like bitcoin-entities if Clean Architecture was more known - because that's what they are named there.
-primitive-types is free but kinda long. -common is free, short and understandable I think.
I tried ask ChatGPT and it didn't come up with anything else I like.
I also realized the rule of thumb has a flaw: it'd exclude anything that can be parsed and has FromStr because of the associated Err type (and making it too lax would just import whole bitcoin crate). So I think it should make exception for when the type implements any std traits that have associated types then the associated type would be in the crate as well and doesn't count as dependency. Further, the rule of thumb could also include some silly types that are used in a specific submodule of bitcoin so I think the rule also needs to require that the type is shared among multiple crates.
Further, I think that maybe we're doing this a bit wrong. bech32 should be probably project-agnostic - so no dependency on bitcoin and whatever functionality we need should be added inside the address module (crate). Since presumably if people are parsing addresses they are parsing Address and if they are parsing something else they don't need that stuff. Note that this still implies wanting to have a shared crate that will be depended on by address crate, network crate and others.
I totally turned your thread into a discussion about |
What I mean is that as far as segwit addresses are concerned there is a one-to-one mapping from HRP -> network (if we define network to be one of mainnet, testnet, regtest).
From BIP-173: "We first describe the general checksummed base32[1] format called Bech32 and then define Segregated Witness addresses using it." So, my current thought is that the crate root module should implement the segwit address format and the That is what I've tried to do with the "main" API functions (all taking This idea has made me unsure about the name Note that I moved some bip test vector tests into unit tests while playing with the error (based on that blogpost) but now we are going with having
I don't think that |
|
I don't think there's any value in taking a
If it parses the witness version separately it should have some indication that it's doing so. If it's also doing length checks then yes, I think it should have |
I strongly disagree. Only rust-bitcoin knows what HRPs are bitcoin-specific, and if this library is supposed to maintain a list of all the HRPs in use everywhere that's a pretty big scope increase. (And if it's supposed to maintain only the three in BIP-173 that have named networks associated with them, excluding the others used in unit tests and the large set tof HRPs which it describes as "valid", that would be confusing to users and would require them to read the source every time to figure out what's going on.) |
Interesting, we must be thinking of
If I am not mistaken in the view I posted above I had hoped that clear rustdocs in the main module would explain the view and then anyone wanting more from the lib would go straight to FTR the benefit of using An alternate idea would be to make the lib general purpose and make a |
The word "network" is not used in either BIP. The only text supporting the
We can have an
This would be reasonable, though I think we should have a better name than |
From BIP 141: > A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that > consists of a 1-byte push opcode (for 0 to 16) followed by a data push > between 2 and 40 bytes gets a new special meaning. The value of the > first push is called the "version byte". The following byte vector > pushed is called the "witness program". `WitnessVersion` and `WitnessProgram` are scriptPubkey concerns and scriptPubkey is basically synonymous with address so in one way it makes sense that these types are in `address` however we are in the process of overhauling the `Address` (and `AddressInner`) types so lets move the witness stuff to `script` and put it in individual sub-modules. This move helps simplify the address error type also. Note please, there are a bunch of formatting changes in here in the error type that I cannot explain and could not remove.
The version byte appears _before_ the witness program in a segwit scriptPubkey however our `WitnessProgram` struct includes it. Either the struct is incorrectly named of the version byte should not be part of it. Observe also that we typically use the version byte at the same time as the witness program and also that it is required to do the program length checks. Pull the version byte out of `WitnessProgram` and inline it into the `Payload` segwit variant after first renaming the variant from `Payload::WitnessProgram` to `Payload::Segwit`. This change makes the code more semantically correct. Note that it does introduce a non-explicit assumption that the same version be passed to `WitnessProgram::new` as is used in the `Payload` - is this acceptable engineering?
932d9b7 to
128473d
Compare
|
Superseded by #1951. |
Demonstrate usage of the proposed new
rust-bech32API.