Further enhancements to the Bech32 library. #277

jonathanknowles · 2019-05-17T08:40:37Z

Issue Number

Issue #238

Overview

This PR:

Implements error location detection for the Bech32 encoding, based on the JavaScript reference implementation.
Provides a set of property tests to verify that corrupted Bech32 strings are correctly rejected by the decode function. Corruption is simulated in various ways:
- deleting characters
- inserting characters
- swapping characters
- mutating characters

Caveats

The error location detection is best effort:
- In cases where it is possible to unambiguously identify the locations of erroneous characters, the decoder will return the StringToDecodeContainsInvalidChars error with a suitable list of locations.
- In cases where it's not possible to unambiguously identify the locations of erroneous characters, the decoder will return the StringToDecodeContainsInvalidChars with the empty list.
Currently, there is no automated test to confirm that the locateErrors and syndrome functions behave identically to the equivalent functions in the reference JavaScript implementation. ⚠️ It is possible that these implementations differ (and this is something that we should pay attention to).

Nevertheless, in the test suite, we verify that if the position of an invalid character is detected, then it is detected at the correct location.

rvl

Is nice and readable (except for locateErrors).

rvl · 2019-05-20T02:57:17Z

lib/bech32/src/Codec/Binary/Bech32/Internal.hs

+                           Error Location Detection
+-------------------------------------------------------------------------------}
+
+gf_1024_exp :: Array Int Int


What are these tables? Are they something like p^i where i is the i'th entry in the table and p is some special number?

yes - it would be nice to have a comment/reference/description of these tables somewhere if there is a description available. I see the source is from https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js but adding few words would help maintain it

These tables are a direct translation of the tables found in the reference implementation here:

https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js#L4
https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js#L77

As for what they do: they seem to be a pair of lookup tables that store the values of a pair of pre-computed functions (an exponent and a logarithm). As to the exact nature of these functions, I currently do not know the answer. We would almost certainly have to ask the creator of the reference implementation. (https://github.com/sipa)

We would almost certainly have to ask the creator of the reference implementation.

I would go for this - if we can't find how tables are generated ourself. It is possible maintainer won't be responsive in which case we would try to figure it long term - but its worth a try to ping him

rvl · 2019-05-20T03:00:36Z

lib/bech32/src/Codec/Binary/Bech32/Internal.hs

+    575, 992, 463, 983, 243, 360, 970, 350, 267, 615, 766, 494, 31, 1009,
+    452, 710, 552, 128, 612, 600, 275, 322, 193 ]
+
+syndrome :: (Bits a, Num a) => a -> a


What is this function?

It could also be shortened a little by using foldr xor 0 and factoring the if testBit residue n then s else 0.

I see its a direct translation of https://github.com/sipa/bech32/blob/bdc264f84014c234e908d72026b7b780122be11f/ecc/javascript/bech32_ecc.js#L154 .

As this is translation from js -> haskell I would expect first iteration to have more js-ish style and I am ok with that. With more direct translation as here it is much easier to convince the reviewers that implementation is correct (by correct I mostly mean that it is a dirrect translation of reference implementation). On second or third implementation we can modify this to look more functional or more haskell-ish.

Alternativelly we as reviewers might all want to fully understand underlying mechanisms of beach32 at this point and check it - but I think it is much more feasible to go with first approach at this stage.

@rvl wrote:

What is this function?

This is a direct translation of the corresponding function in the reference JavaScript implementation:
https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js#L154

It could also be shortened a little by using foldr xor 0 and factoring the if testBit residue n then s else 0.

That's a good idea. I originally wrote this in a style that was as close as possible to the original implementation (for easier verification). If I have time, I'll try to rewrite this in a shorter way.

akegalj · 2019-05-20T07:21:11Z

What is a difference between https://github.com/sipa/bech32/tree/master/ref/haskell and https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js ? Is js implementation more complete ?

akegalj

I have just skimmed over it and it looks good.

To approve it I would have to go for another run to compare in more details with referance implementation (ping me when it is ready)

akegalj · 2019-05-20T07:17:45Z

lib/bech32/src/Codec/Binary/Bech32/Internal.hs

    | otherwise =
        Right $ HumanReadablePart hrp
  where
-    (validPortion, invalidPortion) = BS.break (not . valid) hrp
+    invalidCharPositions = CharPosition . fst <$>


This looks like a good candidate for NonEmpty instead of current [] . Additionally this would simplify L104 (and maybe others) as you would be guaranteed to have failure there

This looks like a good candidate for NonEmpty instead of current [] . Additionally this would simplify L104 (and maybe others) as you would be guaranteed to have failure there

Actually, it's possible for the locateErrors function to return the empty list, if it is not able to reliably determine the locations of errors (even if the string is provably erroneous).

I'm going to add a comment to the locateErrors function to make this clear.

akegalj · 2019-05-20T07:26:55Z

lib/bech32/src/Codec/Binary/Bech32/Internal.hs

+                           Error Location Detection
+-------------------------------------------------------------------------------}
+
+gf_1024_exp :: Array Int Int


yes - it would be nice to have a comment/reference/description of these tables somewhere if there is a description available. I see the source is from https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js but adding few words would help maintain it

akegalj · 2019-05-20T07:33:06Z

lib/bech32/src/Codec/Binary/Bech32/Internal.hs

+    575, 992, 463, 983, 243, 360, 970, 350, 267, 615, 766, 494, 31, 1009,
+    452, 710, 552, 128, 612, 600, 275, 322, 193 ]
+
+syndrome :: (Bits a, Num a) => a -> a


I see its a direct translation of https://github.com/sipa/bech32/blob/bdc264f84014c234e908d72026b7b780122be11f/ecc/javascript/bech32_ecc.js#L154 .

As this is translation from js -> haskell I would expect first iteration to have more js-ish style and I am ok with that. With more direct translation as here it is much easier to convince the reviewers that implementation is correct (by correct I mostly mean that it is a dirrect translation of reference implementation). On second or third implementation we can modify this to look more functional or more haskell-ish.

Alternativelly we as reviewers might all want to fully understand underlying mechanisms of beach32 at this point and check it - but I think it is much more feasible to go with first approach at this stage.

jonathanknowles · 2019-05-20T07:50:20Z

@akegalj wrote:

What is a difference between https://github.com/sipa/bech32/tree/master/ref/haskell and https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js ? Is js implementation more complete ?

The JavaScript implementation is apparently more complete. While the Haskell reference implementation is capable of stating whether (or not) a given string is valid Bech32 (returning a simple Boolean value to reflect validity), the JavaScript reference implementation is also capable of detecting the locations of errors, in certain cases. This information can be used to give the user feedback.

See the following demonstration: http://bitcoin.sipa.be/bech32/demo/demo.html

jonathanknowles · 2019-05-20T09:32:15Z

lib/bech32/src/Codec/Binary/Bech32/Internal.hs

@@ -14,18 +14,19 @@
 --
 -- From an original implementation by Marko Bencun:
 --
-- [sipa/bech32](https://github.com/sipa/bech32/tree/bdc264f84014c234e908d72026b7b780122be11f/ref/haskell)
-
+-- [sipa/bech32](https://git.io/fj8FV)


These shortened URLs are an official part of GitHub (and are easier to paste into source code).

akegalj · 2019-05-20T11:10:56Z

lib/bech32/test/Codec/Binary/Bech32Spec.hs

+                Bech32.decode (getValidBech32String v) `shouldBe`
+                    Right (humanReadablePart v, unencodedDataPart v)
+
+    describe "Decoding a corrupted string should fail" $ do


did you have a look at these check bits https://github.com/sipa/bech32/blob/master/ecc/javascript/segwit_addr_ecc.js#L27 ? (maybe they are not relevant)

@akegalj wrote:

did you have a look at these check bits https://github.com/sipa/bech32/blob/master/ecc/javascript/segwit_addr_ecc.js#L27 ? (maybe they are not relevant)

The Haskell translation of that particular function is at the following location:
lib/bech32/src/Codec/Binary/Bech32/Internal.hs:392

The test for that function is here:
lib/bech32/test/Codec/Binary/Bech32Spec.hs:235

akegalj · 2019-05-20T11:32:47Z

lib/bech32/test/Codec/Binary/Bech32Spec.hs

@@ -87,6 +96,116 @@ spec = do
            Bech32.encode hrp mempty
                `shouldBe` Right (B8.pack "hrp1g9xj8m")

+    describe "Arbitrary ValidBech32String" $


would it make sense to ffi C implementation from https://github.com/sipa/bech32/blob/master/ref/c/segwit_addr.h and run property test comparing address encode/decode and encode/decode against our implementation ?

This way we would have complete test against implementation from another language. We could even use these implementation if there are more performant - but also we could only use these test in order to have complete run when we will be refactoring the implementation

Actually, the most complete reference implementation seems to be the JavaScript one, which also provides error location detection. (The other implementations only provide a simple Boolean check to determine whether a given encoded string is valid.) I agree that a potentially useful side project would be to compare these two implementations. Perhaps it would be possible to use https://github.com/tweag/inline-js to make this easier.

akegalj

lgtm

I have partially looked and compared js and haskell implementation. As these bits are only relevant for error report and not actual encoding and decoding mechanism - it doesn't seem super important even if we did make an error in the translation js -> hs (which doesn't seems so from what I have seen)

Tests seem valuable

(there is a posibility to compare our bech32 encoder with other reference implementation out there, like C, if there would be suspicion that something is off. On another hand most of the implementation from https://github.com/sipa/bech32/ use same test vectors and I believe that should be enough)

KtorZ · 2019-05-20T21:39:59Z

bors r+

277: Further enhancements to the Bech32 library. r=KtorZ a=jonathanknowles ## Issue Number Issue #238 ## Overview **This PR:** 1. Implements **error location detection** for the Bech32 encoding, based on the JavaScript [reference implementation](https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js). 2. Provides a set of **property tests** to verify that corrupted Bech32 strings are correctly **rejected** by the `decode` function. Corruption is simulated in various ways: * deleting characters * inserting characters * swapping characters * mutating characters ## Caveats 1. The error location detection is **best effort**: * In cases where it **is** possible to unambiguously identify the locations of erroneous characters, the decoder will return the `StringToDecodeContainsInvalidChars` error with a suitable list of locations. * In cases where it's **not** possible to unambiguously identify the locations of erroneous characters, the decoder will return the `StringToDecodeContainsInvalidChars` with the empty list. 2. Currently, there is no automated test to confirm that the `locateErrors` and `syndrome` functions behave identically to the equivalent functions in the reference JavaScript implementation. ⚠️ **It is possible that these implementations differ** (and this is something that we should pay attention to). Nevertheless, in the test suite, we verify that **if** the position of an invalid character **is** detected, then it is **detected at the correct location**. 283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl Relates to issue #154. This PR branch is based on #282. # Overview - Implemented saving and loading of transaction history to SQLite - Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery. # Comments - I haven't made any decision about what to do with internal functions such as the `Wallet` constructor. - The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage. - Cascading deletes are not working with persistent-sqlite, which is annoying. - DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later) Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>

iohk-bors · 2019-05-20T22:53:10Z

Timed out (retrying...)

277: Further enhancements to the Bech32 library. r=KtorZ a=jonathanknowles ## Issue Number Issue #238 ## Overview **This PR:** 1. Implements **error location detection** for the Bech32 encoding, based on the JavaScript [reference implementation](https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js). 2. Provides a set of **property tests** to verify that corrupted Bech32 strings are correctly **rejected** by the `decode` function. Corruption is simulated in various ways: * deleting characters * inserting characters * swapping characters * mutating characters ## Caveats 1. The error location detection is **best effort**: * In cases where it **is** possible to unambiguously identify the locations of erroneous characters, the decoder will return the `StringToDecodeContainsInvalidChars` error with a suitable list of locations. * In cases where it's **not** possible to unambiguously identify the locations of erroneous characters, the decoder will return the `StringToDecodeContainsInvalidChars` with the empty list. 2. Currently, there is no automated test to confirm that the `locateErrors` and `syndrome` functions behave identically to the equivalent functions in the reference JavaScript implementation. ⚠️ **It is possible that these implementations differ** (and this is something that we should pay attention to). Nevertheless, in the test suite, we verify that **if** the position of an invalid character **is** detected, then it is **detected at the correct location**. Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2019-05-21T02:23:10Z

Timed out

The new name is more accurate. The previous name could be taken to imply that the checksum is separate, which it is not.

This way, we can fail fast before performing any further parsing.

…ing. This implementation is based upon the implementation found here: https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js

…r counterparts in the JavaScript reference implementation.

…test code to consume.

…ejects corrupted strings.

… is detected at the correct location.

jonathanknowles requested a review from KtorZ May 17, 2019 08:41

jonathanknowles self-assigned this May 17, 2019

rvl reviewed May 20, 2019

View reviewed changes

akegalj reviewed May 20, 2019

View reviewed changes

piotr-iohk mentioned this pull request May 20, 2019

Bech32 Library #238

Closed

jonathanknowles force-pushed the jonathanknowles/bech32-error-location-detection branch from 3b268b4 to 02547ec Compare May 20, 2019 08:30

jonathanknowles changed the title ~~[WIP] Bech32 Error Location Detection~~ Further enhancements to Bech32 library. May 20, 2019

jonathanknowles force-pushed the jonathanknowles/bech32-error-location-detection branch from bfc71b8 to adcbcc8 Compare May 20, 2019 08:53

jonathanknowles marked this pull request as ready for review May 20, 2019 08:55

jonathanknowles requested review from akegalj and rvl May 20, 2019 08:56

jonathanknowles changed the title ~~Further enhancements to Bech32 library.~~ Further enhancements to the Bech32 library. May 20, 2019

jonathanknowles commented May 20, 2019

View reviewed changes

akegalj reviewed May 20, 2019

View reviewed changes

akegalj approved these changes May 20, 2019

View reviewed changes

KtorZ force-pushed the jonathanknowles/bech32-error-location-detection branch 3 times, most recently from 1e22147 to a668f72 Compare May 20, 2019 21:39

jonathanknowles added 4 commits May 21, 2019 05:09

Export the DecodingError type and data constructors.

3b37cfc

Rename function parseDataPart to parseDataWithChecksumPart.

865a838

The new name is more accurate. The previous name could be taken to imply that the checksum is separate, which it is not.

Check elementary preconditions earlier within function Bech32.decode.

e56be40

This way, we can fail fast before performing any further parsing.

Implement logic to detect the positions of errors within a Bech32 str…

f76c91c

…ing. This implementation is based upon the implementation found here: https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js

jonathanknowles added 5 commits May 21, 2019 05:09

Add comments clarifying the relationship of Haskell functions to thei…

3d6a674

…r counterparts in the JavaScript reference implementation.

Expose the Bech32 character set in a way that is more convenient for …

55ffeb2

…test code to consume.

Add property tests to demonstrate that the Bech32 decoder correctly r…

fa3c34a

…ejects corrupted strings.

Add further documentation to functions within module Bech32.

e1326f8

If the position of an erroneous character is detected, verify that it…

501f515

… is detected at the correct location.

jonathanknowles force-pushed the jonathanknowles/bech32-error-location-detection branch from a668f72 to 501f515 Compare May 21, 2019 05:11

Unexport functions from Bech32Spec that are not called externally.

4f16daa

jonathanknowles merged commit b116caa into master May 21, 2019

iohk-bors bot deleted the jonathanknowles/bech32-error-location-detection branch May 21, 2019 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further enhancements to the Bech32 library. #277

Further enhancements to the Bech32 library. #277

jonathanknowles commented May 17, 2019 •

edited

Loading

rvl left a comment

rvl May 20, 2019

akegalj May 20, 2019

jonathanknowles May 20, 2019

akegalj May 20, 2019

rvl May 20, 2019

akegalj May 20, 2019

jonathanknowles May 20, 2019

jonathanknowles May 20, 2019

akegalj commented May 20, 2019

akegalj left a comment

akegalj May 20, 2019

jonathanknowles May 20, 2019

akegalj May 20, 2019

akegalj May 20, 2019

jonathanknowles commented May 20, 2019

jonathanknowles May 20, 2019

akegalj May 20, 2019 •

edited

Loading

jonathanknowles May 21, 2019

akegalj May 20, 2019

jonathanknowles May 21, 2019

akegalj left a comment

KtorZ commented May 20, 2019

iohk-bors bot commented May 20, 2019

iohk-bors bot commented May 21, 2019

Further enhancements to the Bech32 library. #277

Further enhancements to the Bech32 library. #277

Conversation

jonathanknowles commented May 17, 2019 • edited Loading

Issue Number

Overview

Caveats

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akegalj commented May 20, 2019

akegalj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles commented May 20, 2019

Choose a reason for hiding this comment

akegalj May 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akegalj left a comment

Choose a reason for hiding this comment

KtorZ commented May 20, 2019

iohk-bors bot commented May 20, 2019

Timed out (retrying...)

iohk-bors bot commented May 21, 2019

Timed out

jonathanknowles commented May 17, 2019 •

edited

Loading

akegalj May 20, 2019 •

edited

Loading