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#3799kianenigma merged 70 commits intoparitytech:masterfrom expenses:sr-arithmetic-fuzzing
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. |
kianenigma
left a comment
There was a problem hiding this comment.
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.
We use https://crates.io/crates/criterion for benchmarking.
Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>
|
@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.rsinto a new crate files, translates the previous 'fuzzing' tests intohonggfuzzbinaries (I was going off ofparity-scale-codecwhich uses the same, happy to change fuzzing tool though) and adds a (currently failing)multiply_by_rationaltest that was uncovered via fuzzing.@kianenigma