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 Fuzzing #27

Merged
merged 4 commits into from
Jan 22, 2019
Merged

Add Fuzzing #27

merged 4 commits into from
Jan 22, 2019

Conversation

sgeisler
Copy link
Contributor

@sgeisler sgeisler commented Nov 26, 2018

Add fuzz tests to rust-bech32, fixes #21, fixes #28 . Not ready for review yet.

  • decode random data
  • encode/decode random data
  • decode random encoded data from bitcoin core implementation
  • encode random data and test if core can parse it
  • travis integration and cleanup
  • fix Decoding our own encoded bech32 strings can fail #28 to make fuzz tests not fail

@clarkmoody
Copy link
Member

👍 Thanks for working on this.

@sgeisler sgeisler force-pushed the fuzzing branch 11 times, most recently from 1989bc0 to 716471e Compare January 18, 2019 22:59
@sgeisler sgeisler changed the title WIP: Fuzzing Add Fuzzing Jan 18, 2019
@sgeisler
Copy link
Contributor Author

Since it's better to have some fuzzing than no fuzzing I'd like to get this merged without the core cross-fuzzing. At some point I will probably revisit this to implement cross-implementation fuzzing.

This PR also includes fixes for #28 which change the API. So if this is merged the version should be bumped. Depending on your preferences I could just add one commit doing that to this PR.

@clarkmoody
Copy link
Member

Please add a commit to bump version.

I will dig in and review.

/// * **InvalidLength**: if the HRP is outside 1..83 characters long
///
/// # Deviations from standard
/// * No length limits are enforced for the data part
Copy link
Contributor Author

@sgeisler sgeisler Jan 19, 2019

Choose a reason for hiding this comment

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

It was much simpler to not enforce this invariant because lightning uses much longer than standard bech32 invoices. But because we need to enforce the same invariants when parsing and constructing (which the fuzzer found to be not the case currently) that would require two structs Bech32 and LenientBech32. I think it's not worth the added complexity.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member

@clarkmoody clarkmoody left a comment

Choose a reason for hiding this comment

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

Tested locally, and it works. Thanks!

@clarkmoody clarkmoody merged commit 2836561 into rust-bitcoin:master Jan 22, 2019
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.

Decoding our own encoded bech32 strings can fail Implement fuzz testing
2 participants