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 the Bech32 character mutation test. #332

Merged
merged 1 commit into from
May 28, 2019

Conversation

jonathanknowles
Copy link
Contributor

Issue Number

Issue #331

Overview

The character mutation test occasionally fails, because it attempts to be extremely specific when pattern matching the error produced by decoding a mutated string.

Unfortunately, changing just a single character in a valid Bech32 string can cause the decoder to produce many different types of error, depending on the location of the mutation, and the nature of the mutation.

Work Done

  • I have changed the character mutation test to be less specific: it now expects that the string is rejected with some error, but doesn't attempt to pattern match on the error.

This already gets us most of what we want, namely that mutating a character cause the resultant string to be rejected by the decoder.

@jonathanknowles jonathanknowles requested a review from KtorZ May 28, 2019 10:19
@jonathanknowles jonathanknowles self-assigned this May 28, 2019
@KtorZ KtorZ force-pushed the jonathanknowles/fix-bech32-mutate-test branch from 9db619a to 6dfa1dd Compare May 28, 2019 12:01
@KtorZ KtorZ force-pushed the jonathanknowles/fix-bech32-mutate-test branch from 6dfa1dd to 6ace1e4 Compare May 28, 2019 12:02
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.

@jonathanknowles I went for the approach of selecting indexes only within the data-part. This still gives us most of the property checks we care about and avoid replacing the separator by something that messes up the assertion

@KtorZ KtorZ merged commit 1820466 into master May 28, 2019
@KtorZ KtorZ deleted the jonathanknowles/fix-bech32-mutate-test branch May 28, 2019 13:44
@jonathanknowles
Copy link
Contributor Author

@KtorZ

@jonathanknowles I went for the approach of selecting indexes only within the data-part. This still gives us most of the property checks we care about and avoid replacing the separator by something that messes up the assertion

I think there are a few issues with this change:

  1. The function chooseWithinDataPart gives us an incorrect upper bound for the index in the insertion, mutation, and deletion tests. These tests should limit their indices in subtly different ways:

    Test Index Upper Bound Notes
    Insertion T.length originalString (we can insert after the end of a string)
    Mutation T.length originalString - 1
    Deletion T.length originalString - 1
    Swapping T.length originalString - 2 (we swap with the character to the right)
  2. Limiting our choice of index to just the data part means that our tests become weaker, as we're no longer testing across the full length of a Bech32 string. Since the checksum is a function of both the data part and the human readable part, it makes sense to mutate both parts.

  3. In particular, we do need to check for mixed case characters at all locations in the string, and not just in the data part:

    Decoders MUST NOT accept strings where some characters are uppercase and some are lowercase (such strings are referred to as mixed case strings).
    So for the mixed-case tests, it also makes sense to mutate across the string.

I've created a PR for this, which I'll post shortly. 😃

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