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

Fix Bech32 decoder + encoder #312

Merged
merged 8 commits into from
May 24, 2019
Merged

Fix Bech32 decoder + encoder #312

merged 8 commits into from
May 24, 2019

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented May 23, 2019

Issue Number

This PR fixes the following issues:

Overview

For #311:

  • Adds a new type DataPart, which wraps [Word5].
  • Changes the types of decode and encode so that they operate on DataPart instead of ByteString.
  • Ensures that the round-trip relationship between encode and decode is preserved.
  • Provides auxilliary functions dataPartFromBytes and dataPartToBytes to convert to (and from) ByteString and DataPart:
    • dataPartFromBytes pads with trailing zeros where appropriate.
    • dataPartToBytes trims trailing zeros where appropriate.
  • Provides unit tests to ensure that all reference Bech32 strings mentioned in the Bech32 standard can be decoded successfully.

For #314:

  • Reworks smart constructors for HumanReadablePart and DataPart so that inputs are always converted to lower case.

Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

The change itself looks good, a few remarks about style & infinite shrinkers 😅

lib/bech32/test/Codec/Binary/Bech32Spec.hs Outdated Show resolved Hide resolved
lib/bech32/src/Codec/Binary/Bech32/Internal.hs Outdated Show resolved Hide resolved
lib/bech32/src/Codec/Binary/Bech32/Internal.hs Outdated Show resolved Hide resolved
…h32 alphabet.

Also add an accompanying `Read` instance for `DataPart` to accompany the
existing `Show` instance.

This change also adds a QuickCheck property for the following relationship:

>>> read (show (dp :: DataPart)) == dp
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/bech32-fix branch 2 times, most recently from 1f6b14c to 3e0ab16 Compare May 24, 2019 05:18
Use `Text` as the internal representation for `HumanReadablePart` and
`DataPart`.

As a side-effect, we can discard the `Read` and `Show` instances for
`DataPart`.
The `encode` function was mistakenly expected (by the specification) to produce
an invalid Bech32 string, when given an upper-case human readable part. If this
invalid string is passed to the `decode` function, it fails (unsurprisingly) to
decode.

This change updates the specification so that the `encode` function is
now expected to produce a valid Bech32 string.
@jonathanknowles jonathanknowles changed the title Fix Bech32 decoder Fix Bech32 decoder + encoder May 24, 2019
In the `Arbitrary` instance for `HumanReadablePart`, we don't need to
(and shouldn't) convert the generated string to lower-case before
calling `humanReadablePartFromText`.

The `humanReadablePartFromText` function itself already converts to lower case.
Copy link
Member

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

Looks good. Are you planning on making a PR upstream to fix the reference implementation ?

@jonathanknowles
Copy link
Member Author

Looks good. Are you planning on making a PR upstream to fix the reference implementation ?

I'm happy to do that, as it's relatively simple to fix. But perhaps we should wait for upstream to confirm first? I've raised an issue here: sipa/bech32#49

@KtorZ
Copy link
Member

KtorZ commented May 24, 2019

Well, looking at the JavaScript implementation on bitcoinjs/bech32

We have the following behavior:

> bech32.encode('test', [])
'test12hrzfj'
> bech32.encode('Test', [])
'test12hrzfj'
> bech32.encode('TEST', [])
'test12hrzfj'
> bech32.encode('TEsT', [])
'test12hrzfj'
> bech32.encode('TesT', [])
'test12hrzfj'

> bech32.decode("test12hrzfj")
{ prefix: 'test', words: [] }
> bech32.decode("test13jgcyw")
Error: Invalid checksum for test13jgcyw

which is quite aligned to what we now expect. This seems like a fair indication that there's indeed a bug in the Haskell reference implementation.

@KtorZ KtorZ merged commit 055c68e into master May 24, 2019
@KtorZ KtorZ deleted the jonathanknowles/bech32-fix branch May 24, 2019 09:39
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.

2 participants