Use checked arithmetic in types and state proc #1009

michaelsproul · 2020-04-16T02:25:57Z

Proposed Changes

Lint for integer arithmetic in the types and state_processing crates, so that we're forced to be explicit about what sort of arithmetic we want, and can catch unwanted overflow and consider it an invalid state transition (as per Clarity on uint overflow ethereum/consensus-specs#1701)
Replace most uses of arithmetic in these two crates with wrappers around the checked_ functions from the standard library. To make things more ergonomic, I introduced a safe_arith crate which returns Results from the checked operations instead of Options.
Run clippy on CI, but only for integer arithmetic for now (I fixed some clippy lints along the way, but decided to consider that out of scope for this PR).

michaelsproul · 2020-04-16T02:27:05Z

Still WIP, I've noticed a few things in the diff that need correcting

michaelsproul · 2020-04-17T02:04:28Z

Ok, ready for review. I'm just going to run some benchmarks of state processing to assess the impact on performance.

michaelsproul · 2020-04-17T07:01:31Z

Benchmarks have revealed no statistically significant deviation from master! 🎉

Implementation	Benchmark	Time (low avg high)
Checked	individual_signature_verification 32K	824.48 ms 828.67 ms 838.20 ms
Unchecked	individual_signature_verification 32K	824.62 ms 825.88 ms 827.44 ms
Checked	bulk_signature_verification 32K	1.8674 s 1.8794 s 1.8948 s
Unchecked	bulk_signature_verification 32K	1.8760 s 1.9050 s 1.9549 s
Checked	individual_signature_verification 300K	686.43 ms 689.30 ms 691.12 ms
Unchecked	individual_signature_verification 300K	697.59 ms 701.44 ms 704.60 ms
Checked	bulk_signature_verification 300K	1.3589 s 1.3666 s 1.3772 s
Unchecked	bulk_signature_verification 300K	1.4377 s 1.4448 s 1.4613 s

Worst-case blocks, i7-8550U, minimal spec for the 32K validators, mainnet spec for the 300K, master checked out to a8ee338

Weirdly the benchmark is faster for the larger blocks, and the bulk signature verification is a lot slower, I don't know what's going on there but we can investigate that separately.

pventuzelo · 2020-04-17T07:19:40Z

eth2/types/src/slot_epoch.rs


    pub fn epoch(self, slots_per_epoch: u64) -> Epoch {
-        Epoch::from(self.0 / slots_per_epoch)
+        Epoch::from(self.0) / Epoch::from(slots_per_epoch)


maybe use checked_div or safe_div

Hmm yeah, I figured this was quite innocuous here, because slots_per_epoch is unlikely to ever be 0, and it would at least panic rather than wrapping in a strange way. I'll see how much flow-on effect returning a Result would have though.

Perhaps we do an explicit panic if propagating the Result is too onerous (I suspect this will be the case). That way we get the panic in release too.

I used Epoch division here because it will explicitly panic:

lighthouse/eth2/types/src/slot_epoch_macros.rs

Lines 108 to 114 in 3e4dc7a

fn div(self, rhs: $other) -> $main {

let rhs: u64 = rhs.into();

$main::from(

self.0

.checked_div(rhs)

.expect("Cannot divide by zero-valued Slot/Epoch"),

)

I'm going to leave this as-is, if that's OK with everyone.. I count 60 uses of .epoch() all throughout different crates, almost always with T::slots_per_epoch() as the argument, so I don't think it's worth it.

Fine by me. Setting SLOTS_PER_EPOCH is not reasonable and there's plenty of other panics we could cause by setting "constants" to unreasonable values.

It's also not really "user supplied input". I'd say it's "programmer supplied input".

pventuzelo · 2020-04-17T07:38:45Z

eth2/utils/tree_hash/src/merkle_hasher.rs

            if let Some(root) = self.root {
                break Ok(root);
+            } else if let Some(node) = self.half_nodes.last() {
+                let right_child = node.id * 2 + 1;


mul and add can be checked here since we return Result

Oh yeah, we could. This refactor was just leftover from me trying to fix generic clippy lints project-wide (which I ended up disabling for now), and this crate isn't being linted currently. I agree we could expand the usage of checked arithmetic though, so I've made an issue to track that: #1013

paulhauner

Great effort! This would have been tedious but it's really great to see this overflow behavior locked down!

I just have a couple of questions, nothing major at all.

eth2/state_processing/src/per_block_processing/errors.rs

eth2/state_processing/src/per_block_processing/block_signature_verifier.rs

eth2/state_processing/src/per_epoch_processing/process_slashings.rs

paulhauner · 2020-04-19T00:11:31Z

eth2/types/src/slot_epoch.rs


    pub fn epoch(self, slots_per_epoch: u64) -> Epoch {
-        Epoch::from(self.0 / slots_per_epoch)
+        Epoch::from(self.0) / Epoch::from(slots_per_epoch)


Perhaps we do an explicit panic if propagating the Result is too onerous (I suspect this will be the case). That way we get the panic in release too.

paulhauner · 2020-04-20T01:26:28Z

Feel free to squerge when the tests pass :)

Use checked arithmetic in types and state proc

3e4dc7a

michaelsproul force-pushed the checked-arithmetic branch from 972de7f to 3e4dc7a Compare April 17, 2020 01:59

michaelsproul added the ready-for-review The code is ready for review label Apr 17, 2020

michaelsproul requested a review from paulhauner April 17, 2020 02:01

pventuzelo reviewed Apr 17, 2020

View reviewed changes

michaelsproul mentioned this pull request Apr 17, 2020

Expand checked arithmetic to more crates #1013

Open

paulhauner reviewed Apr 19, 2020

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Apr 19, 2020

Address review comments

7a85ca5

michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Apr 20, 2020

paulhauner approved these changes Apr 20, 2020

View reviewed changes

michaelsproul added ready-to-squerge and removed ready-for-review The code is ready for review labels Apr 20, 2020

michaelsproul merged commit 32074f0 into master Apr 20, 2020

michaelsproul deleted the checked-arithmetic branch April 20, 2020 02:35

	fn div(self, rhs: $other) -> $main {
	let rhs: u64 = rhs.into();
	$main::from(
	self.0
	.checked_div(rhs)
	.expect("Cannot divide by zero-valued Slot/Epoch"),
	)

Use checked arithmetic in types and state proc #1009

Use checked arithmetic in types and state proc #1009

Uh oh!

Conversation

michaelsproul commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

michaelsproul commented Apr 16, 2020

Uh oh!

michaelsproul commented Apr 17, 2020

Uh oh!

michaelsproul commented Apr 17, 2020

Uh oh!

pventuzelo Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelsproul Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhauner commented Apr 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelsproul commented Apr 16, 2020 •

edited

Loading

pventuzelo Apr 17, 2020 •

edited

Loading

michaelsproul Apr 17, 2020 •

edited

Loading