-
Notifications
You must be signed in to change notification settings - Fork 28
Account for flashblocks time drift #123
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
Conversation
9b5a4b6
to
80d0eaf
Compare
@danyalprout this is my take on the FB timing issue |
57428b1
to
e161d65
Compare
Account for dynamic lag in the beginning of building process
e161d65
to
ee3ae84
Compare
d2ebe5b
to
896046f
Compare
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 am not familar enough with the logic of flashblocks to be authorative, but the code looks good apart from some code clarity issues.
// FCU(a) could arrive with `fb_time < delay < block_time - fb_time` - in this case we will issue less flashblocks | ||
let time = std::time::SystemTime::UNIX_EPOCH + Duration::from_secs(timestamp) | ||
- self.config.specific.leeway_time; | ||
let time_drift = time.duration_since(std::time::SystemTime::now()).ok(); |
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 it not be more clear to match on the error directly, instead of assigning and unpacking with .ok()?
something like
match time.duration_since(std::time::SystemTime::now()) { Ok(drift) , Err(SystemTimeError(amount) }
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'm using it later and it makes more sense for it to being the option imo
π Summary
π‘ Motivation and Context
β I have completed the following steps:
make lint
make test