Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Move sr-arithmetic to a new crate and add in a fuzzer #3799

Merged
merged 70 commits into from
Oct 19, 2019
Merged

Move sr-arithmetic to a new crate and add in a fuzzer #3799

merged 70 commits into from
Oct 19, 2019

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Oct 11, 2019

This closes #3745. This commit splits up sr_arithmetic.rs into a new crate files, translates the previous 'fuzzing' tests into honggfuzz binaries (I was going off of parity-scale-codec which uses the same, happy to change fuzzing tool though) and adds a (currently failing) multiply_by_rational test that was uncovered via fuzzing.

@kianenigma

@parity-cla-bot
Copy link

It looks like @expenses signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@expenses expenses changed the title WIP: sr_arithmetic fuzzing Move sr-arithmetic to a new crate and add in a fuzzer Oct 11, 2019
@expenses
Copy link
Contributor Author

The fuzzing code found that this line:

if r > (c / 2) { q = q.add(&to_big_uint(1)); }

sometimes adds 1 to the value of the result when it shouldn't.

Copy link
Contributor

@kianenigma kianenigma 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 all in all! I'll take a last look before merging soon.

core/sr-arithmetic/fuzzer/src/biguint.rs Outdated Show resolved Hide resolved
core/sr-arithmetic/fuzzer/src/biguint.rs Outdated Show resolved Hide resolved
core/sr-arithmetic/fuzzer/src/rational128.rs Outdated Show resolved Hide resolved
core/sr-arithmetic/src/biguint.rs Outdated Show resolved Hide resolved
core/sr-arithmetic/src/biguint.rs Outdated Show resolved Hide resolved
core/sr-arithmetic/src/biguint.rs Outdated Show resolved Hide resolved
core/sr-arithmetic/src/fixed64.rs Outdated Show resolved Hide resolved
core/sr-arithmetic/src/fixed64.rs Outdated Show resolved Hide resolved

// to allow benchmarking
#![cfg_attr(feature = "bench", feature(test))]
#[cfg(feature = "bench")] extern crate test;
Copy link
Member

Choose a reason for hiding this comment

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

We use https://crates.io/crates/criterion for benchmarking.

core/sr-arithmetic/src/per_things.rs Outdated Show resolved Hide resolved
@expenses
Copy link
Contributor Author

expenses commented Oct 17, 2019

@bkchr I'm a little hesitant to make these changes to the arithmetic logic, because it might be better to save them for another PR. @kianenigma what do you think?

@kianenigma
Copy link
Contributor

@expenses The recommendations don't seem to be any logic change but rather syntactic improvements. Those can be applied safely.

The migration to criterion can be saved for another PR as it is a bit bulky and I don't want to stall this PR for it too much. @bkchr I have added benchmarks in the past both with and without criterion. If we really want to have as a "we benchmark only with criterion", we should at some point a) make it a strict rule b) migrate the already existing ones.

Personally, I don't think it is worth migrating them and making it strict rule. Given that the required benchmark is fairly simple (like the ones in this crate) the test crate gets the job done okay.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fuzzing and Organisation for Runtime Arithmetic Types
7 participants