Skip to content

feat(code): Implement Solution 2 for the RoundSync #1032

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

Merged
merged 7 commits into from
May 16, 2025

Conversation

ancazamfir
Copy link
Collaborator

@ancazamfir ancazamfir commented May 16, 2025

Closes: #XXX

From the Round sync spec:

  • upon entering the round r processes broadcast the certificate EnterRoundCertificate, reset lastPrevote and lastPrecommit, and start rebroadcastTimeout
  • if rebroadcastTimeout expires and the process still did not receive a round certificate for a new round a process calls REBROADCAST() and starts the rebroadcastTimeout again
  • timeoutRebroadcast is approximation on how long the whole round should take so it can be something like timeoutPropose(r)+timeoutPrevote(r)+timeoutPrecommit(r)

REBROADCAST() - broadcasts the current non-nil values of EnterRoundCertificate, lastPrevote and lastPrecommit

Summary of changes as compared to parent branch anca/gossip:

  • set the timeout value for rebroadcast timer to timeout_propose + timeout_prevote + timeout_precommit
  • start the rebroadcast timer when entering a new round

The rest of the requirements above are already in the parent branch


PR author checklist

For all contributors

For external contributors

Copy link

codecov bot commented May 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (35b4604) to head (94eb764).
Report is 1 commits behind head on anca/gossip.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@         Coverage Diff         @@
##   anca/gossip   #1032   +/-   ##
===================================
===================================

Copy link
Contributor

@nenadmilosevic95 nenadmilosevic95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I did the review of this together with #997 because my understanding is that it complements it and that we want to merge it there. I left some comments but two main things are:

  • do we cancel the timeout when receiving the round cert?
  • do we recalculate timeoutRebroadcast to account for changes/increasing in the values of other timeouts as the round increase?

@ancazamfir ancazamfir marked this pull request as ready for review May 16, 2025 14:27
@ancazamfir ancazamfir requested a review from romac as a code owner May 16, 2025 14:27
@ancazamfir ancazamfir merged commit 5ca73e4 into anca/gossip May 16, 2025
23 checks passed
@ancazamfir ancazamfir deleted the anca/rbcast_option2 branch May 16, 2025 15:00
github-merge-queue bot pushed a commit that referenced this pull request May 16, 2025
)

* Add gossip messages and handling

* Add the new gossip and msg handling files

* Add serialization test for round certificate

* Send gossip messages over /gossip channel

* Reuse exsiting encode and decode vote type functions

* Update version and breaking changes

* Store the round certificate when new round condition occurs

* On hidden lock suspicion (round > x), restream also the proposal

* Add vote by vote verification for round certificate

* Fix standalone build, less cloning

* Less cloning

* Rebroadcast the round certificate

* Fix clippy

* Add `RebroadcastRoundCertificate` event

* Move middlewares in their own module

* Add round and vote type to `expect_vote_rebroadcast`

* Show log when `on_event` step fails

* Add basic test for vote and round certificate rebroadcast

* Update comments in test

* Rename gossip to liveness

* Rename gossip to liveness

* For hidden lock rebroadcast the proposal message also.

* Apply suggestions from code review

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com>
Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

* Correct comments

* Rebroadcast proposal and polka for HIDDEN_LOCK_ROUND also

* Apply suggestions from code review

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com>
Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

* Fix formatting

* Add comments to liveness message processing

* Fix spelling

* Panic when certificates are not found suggesting internal errors

* Add test for hidden lock

* Fix reuse of RestreamProposal with nil valid round for hidden lock

* chore(code): Send rebroadcast votes to the liveness topic (#1012)

* Send rebroadcasted votes over the gossip topic

* Send rebroadcast vote event

* Rename `GossipMessage` proto to `LivenessMessage`

---------

Co-authored-by: Romain Ruetschi <romain@informal.systems>

* Re-use `HIDDEN_LOCK_ROUND` constant from liveness test

* Update BREAKING_CHANGES.md

* Cleanup debugs and consensus traces

* Enable the rebroadcast wal tests

* Disable again the rebroadcast mode tests

* Use a single rebroadcast timer

* Try fix for round_certificate_rebroadcast test

* Store the enter_round with the certificate, check it matches current
round on (re)broadcast

* Check in test broadcast of round certificate only, no rebroadcast

* feat(code): Implement Solution 2 for the RoundSync (#1032)

* 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

* Apply suggestions from code review

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com>
Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

* Apply suggestions from code review

* Apply suggestions from code review

* Update comments

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com>

---------

Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants