-
Notifications
You must be signed in to change notification settings - Fork 228
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] Supervisor #302
Conversation
romac
commented
Jun 5, 2020
- Referenced an issue explaining the need for the change
- Updated all relevant documentation in docs
- Updated all code comments where relevant
- Wrote tests
- Updated CHANGES.md
Codecov Report
@@ Coverage Diff @@
## master #302 +/- ##
========================================
- Coverage 30.5% 28.2% -2.3%
========================================
Files 127 130 +3
Lines 4581 4880 +299
Branches 1416 1511 +95
========================================
- Hits 1398 1378 -20
- Misses 2216 2533 +317
- Partials 967 969 +2
Continue to review full report at Codecov.
|
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.
Made some comments, can be done in follow up if need be, otherwise 👍
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.
👍 👍 👍 Thanks for updating the example, too.
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 work @romac 💯 left a few comments. don't know if they will be useful, but still
|
||
state | ||
.light_store | ||
.update(trusted_state.clone(), VerifiedStatus::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.
why do we update the current state to verified here? not sure I understand
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.
The light client needs to have at least one trusted (Verified
) block to perform verification.
We should perhaps provide a nicer API on State
for interacting with the store, and make such things more explicit and higher-level.
light-client/src/supervisor.rs
Outdated
trusted_state: &LightBlock, | ||
) -> Result<Option<Vec<Fork>>, Error> { | ||
if self.peers.secondaries().is_empty() { | ||
return Ok(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.
this seems very insecure to me.. even though we rely on 1 correct node assumption, my personal opinion is that we need at least 1 witness. we can't assume primary will always be correct.
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 looks good to go with any potential gaps covered by follow up issues 👍
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 also love to see this merged in the context of the rpc light-node and the cli stuff 👍