-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Move sr-arithmetic to a new crate and add in a fuzzer #3799
Move sr-arithmetic to a new crate and add in a fuzzer #3799
Conversation
It looks like @expenses signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
The fuzzing code found that this line: substrate/core/sr-primitives/src/sr_arithmetic.rs Line 1667 in 0cd9bbf
sometimes adds 1 to the value of the result when it shouldn't. |
There was a problem hiding this 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.
|
||
// to allow benchmarking | ||
#![cfg_attr(feature = "bench", feature(test))] | ||
#[cfg(feature = "bench")] extern crate test; |
There was a problem hiding this comment.
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.
Co-Authored-By: Bastian Köcher <[email protected]>
@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? |
@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. |
This closes #3745. This commit splits up
sr_arithmetic.rs
into a new crate files, translates the previous 'fuzzing' tests intohonggfuzz
binaries (I was going off ofparity-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