-
Notifications
You must be signed in to change notification settings - Fork 14
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(code/consensus): Add support for full nodes #750
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #750 +/- ##
==========================================
+ Coverage 75.51% 75.54% +0.03%
==========================================
Files 165 165
Lines 14301 14329 +28
==========================================
+ Hits 10799 10824 +25
- Misses 3502 3505 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Co-authored-by: Romain Ruetschi <[email protected]> Signed-off-by: DevOrbitlabs <[email protected]>
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! 🚀
) | ||
); | ||
// Only sign and publish if we're in the validator set | ||
if state.is_validator() { |
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.
Not sure about the proposal filtering, seems we are trying to defend against a bug where get_proposer()
returns a non-validator/ full node address...but in this case the is_validator()
should also fail.
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.
Yes it's just defensive coding. Maybe we can change the check to do a debug_assert
or even an assert
in order to ensure this code path is never hit.
Can we start a full node from command line? |
Wondering if we should send |
So for the receive side, |
Good question, perhaps there is a higher chance that non-validators might be byzantine and in that case we should restrict those queries to validators only? |
Not at the moment, you have to remove the full node address from the validator set in the genesis of all validator nodes. |
Not sure myself, let's discuss next time we meet. On a side note, we should allow vote sync when blocksync is disabled (iirc this is not the case today) |
* feat(code/consensus): Add support for full nodes * fix lint * Update code/crates/core-consensus/src/state.rs Co-authored-by: Romain Ruetschi <[email protected]> Signed-off-by: DevOrbitlabs <[email protected]> * update * update --------- Signed-off-by: DevOrbitlabs <[email protected]> Co-authored-by: Romain Ruetschi <[email protected]>
Closes: #742
PR author checklist
For all contributors
For external contributors