-
Notifications
You must be signed in to change notification settings - Fork 220
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
Light Client: Follow-up #284
Conversation
Codecov Report
@@ Coverage Diff @@
## master #284 +/- ##
========================================
- Coverage 31.0% 30.5% -0.6%
========================================
Files 129 127 -2
Lines 4905 4580 -325
Branches 1549 1416 -133
========================================
- Hits 1525 1398 -127
+ Misses 2294 2215 -79
+ Partials 1086 967 -119
Continue to review full report at Codecov.
|
Amazing documentation with traceability to the english spec 😍 |
edfc773
to
d1a6357
Compare
304efcc
to
cd38cf3
Compare
0749e34
to
8b7a2aa
Compare
@@ -1,14 +1,25 @@ | |||
use crate::prelude::*; |
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.
Maybe in the preamble provide reference to the english spec such that the invariants can be looked up by the reader.
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.
@ebuchman suggested getting rid of the prelude entirely for better discoverability so that's perhaps not the right place to put those references. The crate-level documentation in lib.rs
might be a better fit.
#[post(valid_schedule(ret, target_height, next_height, light_store))] | ||
pub fn schedule( | ||
#[post(valid_schedule(ret, target_height, current_height, light_store))] | ||
pub fn basic_schedule( |
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.
Since this is actually performing bisection does it make sense to call it bisecting_schedule
?
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.
Sure, that's indeed a better name. I chose basic
here to express that this scheduler implementation does not do anything fancy for performance (like picking heights already present in the store, etc.)
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.
So perhaps basic_bisecting_schedule
or something like that?
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.
Great stuff. Tiny comments made but otherwise 👍
/// - i) If `latest_verified_height == current_height` and `latest_verified_height < target_height` | ||
/// then `current_height < scheduled_height <= target_height`. | ||
/// | ||
/// - ii) If `latest_verified_height < next_height` and `latest_verified_height < target_height` |
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.
s/next_height/current_height
/// | ||
/// ## Note | ||
/// | ||
/// - Case i. captures the case where the light block at height nextHeight has been verified, |
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.
s/nextHeight/current_height
/// and we can choose a height closer to the targetHeight. As we get the `light_store` as parameter, | ||
/// the choice of the next height can depend on the `light_store`, e.g., we can pick a height | ||
/// for which we have already downloaded a light block. | ||
/// - In Case ii. the header of `next_height` could not be verified, and we need to pick a smaller height. |
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.
same
/// - A commit validator | ||
/// - A header hasher | ||
/// | ||
/// For regular use, one can summon an implementation with `ProdVerifier::default()`. |
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.
s/summon/construct? 🧙
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.
Right, the default instance is indeed not fancy enough to warrant such witchcraft ;)
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.
👍
// assert_eq!(iter.next_back(), Some(1)); | ||
// assert_eq!(iter.next_back(), None); | ||
// } | ||
// } |
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.
Why are these commented out? Maybe better to add a comment or delete.
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.
Yeah this test is currently disabled because it fails on CI as we don't have access to this temp dir. I need to read up a bit on the GitHub Actions infra to figure out how to specify a proper temp dir, so I will just add a comment in the meantime.
Thanks for the comments, just addressed them. Feel free to merge once CI goes through (modulo the |
You should be able to merge the PR despite the failing integration tests (I think it's OK to ignore these for now). |
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.
👍
See #280