-
Notifications
You must be signed in to change notification settings - Fork 137
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
feat: perform randomness hashing in the kernel #1842
Conversation
@@ -117,10 +117,7 @@ pub enum RandomnessKind { | |||
#[derive(Debug, Deserialize_tuple, PartialEq, Eq, Clone)] | |||
pub struct RandomnessRule { | |||
pub kind: RandomnessKind, | |||
pub dst: i64, |
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.
This is a somewhat unfortunate decrease in test vector granularity that I'm guessing will also break the existing vectors we have? Any clever ideas about how we can prevent this?
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.
Unfortunately, we probably just need to recreate them.
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 could, in theory, preserve the current behavior by mocking in the kernel. But then we wouldn't be testing the randomness function.
Also seeking feedback on whether we should make the same change on the FVM2 line, or just require clients continue to perform that work. |
ab6c017
to
9c22c04
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1842 +/- ##
==========================================
- Coverage 75.27% 75.19% -0.09%
==========================================
Files 149 149
Lines 14577 14596 +19
==========================================
+ Hits 10973 10975 +2
- Misses 3604 3621 +17
|
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.
LGTM!
fvm/src/kernel/default.rs
Outdated
state.write_i64::<byteorder::BigEndian>(round)?; | ||
state.write_all(entropy)?; | ||
let mut ret = [0u8; 32]; | ||
ret.clone_from_slice(state.finalize().as_bytes()); |
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.
ret.clone_from_slice(state.finalize().as_bytes()); | |
state.finalize().as_bytes().try_into() |
Should work?
@@ -117,10 +117,7 @@ pub enum RandomnessKind { | |||
#[derive(Debug, Deserialize_tuple, PartialEq, Eq, Clone)] | |||
pub struct RandomnessRule { | |||
pub kind: RandomnessKind, | |||
pub dst: i64, |
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.
Unfortunately, we probably just need to recreate them.
@@ -117,10 +117,7 @@ pub enum RandomnessKind { | |||
#[derive(Debug, Deserialize_tuple, PartialEq, Eq, Clone)] | |||
pub struct RandomnessRule { | |||
pub kind: RandomnessKind, | |||
pub dst: i64, |
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 could, in theory, preserve the current behavior by mocking in the kernel. But then we wouldn't be testing the randomness function.
I'd actually prefer to backport it if possible. The nice thing about this patch is that it's entirely backwards compatible. Then let's immediately make the breaking changes against the |
Ah... but the conformance tests.... Here are the ones that pull randomness:
|
We can probably drop them for now, but we'd definitely want to re-create them. As I noted above, technically we could keep them by moving the "replay" logic into the |
e23eb70
to
c505cb1
Compare
c505cb1
to
d710e06
Compare
Fixes #1277
We currently rely on clients like Lotus to do the hashing required for drawing randomness. This PR pulls that into the kernel.
We should eventually push the work of hashing into the actor directly, which can request it over syscalls.