-
Notifications
You must be signed in to change notification settings - Fork 30
feat(code): Implement minimal gossip properties to ensure liveness #997
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
✅ Semver Check PassedGreat job! All semver violations have been resolved. This PR now complies with semantic versioning rules. If you made version updates, please ensure that:
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #997 +/- ##
=========================================
+ Coverage 0 77.64% +77.64%
=========================================
Files 0 159 +159
Lines 0 16814 +16814
Branches 0 16814 +16814
=========================================
+ Hits 0 13055 +13055
- Misses 0 2883 +2883
- Partials 0 876 +876
Flags with carried forward coverage won't be shown. Click here to find out more. |
@ancazamfir I just added a basic integration test, looking good so far :) |
@nenadmilosevic95 @romac one last thing, we need to decide on (from the round sync spec): "Open questions:
|
I enabled these rebroadcast mode tests in 0d7321e:
They all passed locally, however on CI we get I will disable them again and open an issue to solve the failures in a separate PR (issue #1029) |
Also |
What is the value that you currently use? |
I still need to do the change to reflect the round sync proposal. Currently we use the old timeout mechanism. We have two timers, We will have only one timer as proposed but what should be the duration? |
What was the motivation to have two timeouts? |
It was the initial rebroadcast design that starknet proposed. And (in main) we are sending only the last prevote on |
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.
LGTM, left some minor comments
* Implement solution 2 wrt rebroadcast timer * Change names according to spec * Cancel rebroadcast timer when round cert is received * Increase the rebroadcast timeout each round * Update the config.toml files used by tests
Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
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.
LGTM
Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com>
🎉 |
Closes: #998
Implement minimal gossip properties for liveness.
Summary of changes
Liveness messages:
/liveness
topic/ channel and message types for network communicationliveness.proto
for handling gossip messagesPolkaCertificate
andRoundCertificate
message typesNetwork Layer Changes:
Liveness
channel type alongside existing channels (Consensus, ProposalParts, Sync)Certificate Handling:
PolkaCertificate
and related types from sync proto to liveness protoRoundCertificate
type for handling round-specific certificatesCodec Updates:
Still to be done:
- [ ] Remove vote set mode- done in separate PR #1008- [ ] Move rebroadcast in its own module and outside core-consensus- will do in separate PRf+1
quorum or2f+1
precommit any) similarly withlast_prevote
andlast_precommit
or find the certificate when we enter a round > 0 (current approach)PR author checklist
For all contributors
RELEASE_NOTES.md
if the change warrants itBREAKING_CHANGES.md
if the change warrants itFor external contributors