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

Add iter and checksum modules #105

Closed
wants to merge 1 commit into from

Conversation

tcharding
Copy link
Member

Add, as yet unused, modules that implement BCH codes and iterators (used for no-alloc access to the bech32 string parts).

Draft because I cannot work out a nice next step.

I've spent two good chunks of time today and yesterday and I'm stumped how to proceed. Sorry for the Wall Of Text.

  1. We cannot use the iterators to implement current traits/types in lib.rs because of the use of AsRef<[u8]>, which is not compatible with the iterators because we require ExactSizedIterator.
  2. I'm confused as to what abstraction we are aiming for for this crate
    1. is it a general purpose bech32 crate
    2. is it specifically there to support the rust-bitcoin crate i.e., layer one bitcoin

If its (i) then I don't even know what this means, my whole understanding of the crate is based solely on the two bips. If it is (ii) then I don't know how to do a simple-next-step without just re-writing the whole API.

More points on the abstraction

  1. What goes in this crate vs in the bitcoin::address module (e.g. who knows about "bc", "tb", "bcrt", valid witness versions, valid witness program length)?
  2. Why does rust-bech32-bitcoin exist and is the abstraction implied by its existence valid (I know you already said forget about that crate @apoelstra but it effects the abstraction discussion IMO)?

Moving Forward

  1. Can we just clobber the whole API (lib.rs) and write an API tailored to bitcoin layer one and let other users just go to primitives and use the more clunky but fully functional API?
  2. Do we need to just add a segwit module that implements the API in (1) and leave lib.rs as it is?

If (1) we need buyin from @clarkmoody because its his crate but also because he understands who else is using the crate.
If (2) I have no idea how we ever stabalize the crate because no one in rust-bitcoin will sign off on it.

Add, as yet unused, modules that implement BCH codes and iterators (used
for no-alloc access to the bech32 string parts).
@clarkmoody
Copy link
Member

The original purpose of the bech32-bitcoin crate was to have data structures for Witness Version and do address HRP validation, etc. This library was for the encoding of raw bytes and performing checksums. However, I'm not averse to this library growing to include support for Bitcoin-specific semantics around the structure of our Bech32-encoded strings.

@apoelstra
Copy link
Member

Re 1, if you mean the current Bech32Writer traits, IMHO we should drop those entirely. They are basically just less-flexible simulations of the Iterator trait. My proposed plan of action would be:

  1. A PR that introduces the checksum module (and maybe changes the existing API to use it, letting us drop much of the existing internal checksumming code, if that's not too much work).
  2. A PR that introduces the iter module, and introduces a new replacement API (which provides high-level encode/decode functions that are generic over all checksums, as well as a high-level encode/decode pair which understands the bitcoin-specific bech32 vs bech32m distinction).
  3. A PR that introduces the segwit module (this is the one where we'll need to haggle about API boundaries and what belongs in what crate)
  4. A PR that drops the existing BitWriter API and all its supporting code.

@tcharding
Copy link
Member Author

Awesome, cheers - I'll have a go with this strategy.

@tcharding
Copy link
Member Author

Closing as we don't need this now, it was only open as draft still for the comment #105 (comment) but I can remember where that is.

@tcharding tcharding closed this Jul 26, 2023
@tcharding tcharding deleted the 05-17-iterators branch September 18, 2023 21:49
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 this pull request may close these issues.

3 participants