-
Notifications
You must be signed in to change notification settings - Fork 159
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
drand v14 update: fix fetching around null tipsets #1339
Conversation
|
||
for _ in 0..20 { | ||
let cbe = rand_ts.blocks()[0].beacon_entries(); | ||
for v in cbe.iter() { |
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.
You can omit iter()
here
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.
updated
let cbe = rand_ts.blocks()[0].beacon_entries(); | ||
for v in cbe.iter() { | ||
if v.round() == round as u64 { | ||
return Ok(v.clone()); |
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.
How costly is it to return by value here? (will depend of the size of the Vec
inside BeaconEntry
struct). Lotus returns a pointer on his side.
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.
I think it's a fixed size of 32 bytes, so never mind.
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.
ok! let me know if you want me to change this, maybe there is a better way to do it
let search_height = if round < 0 { 0 } else { round }; | ||
|
||
let res = self.cs.tipset_by_height(search_height, ts, lookback).await; | ||
match res { |
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.
Can use map_err
here for more idiomatic code
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.
nice, updated!
entropy: &[u8], | ||
) -> Result<[u8; 32], Box<dyn std::error::Error>> { | ||
let chain_rand = ChainRand::new(blocks.to_owned(), self.cs.clone(), self.beacon.clone()); | ||
match self.get_network_version(round) { |
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.
Would be better to match explicitly all cases of the enum instead of relying on the wildcard pattern (_
).
Rational: if we introduce a new network version (e.g. NetworkVersion::V15
), this would fall silently into get_beacon_randomness_v1
which is probably not what we want. Would be better that the code do not compile and be forced to inspect and handle the new case.
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.
good point, updated!
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.
What about having some tests?
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #1291
Other information and links