Skip to content

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

Merged
ancazamfir merged 7 commits intoanca/gossipfrom
anca/rbcast_option2
May 16, 2025
Merged

feat(code): Implement Solution 2 for the RoundSync#1032
ancazamfir merged 7 commits intoanca/gossipfrom
anca/rbcast_option2

Conversation

@ancazamfir
Copy link
Copy Markdown
Contributor

@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

@codecov
Copy link
Copy Markdown

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
Copy Markdown
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?

Comment thread code/crates/engine/src/consensus.rs
Comment thread code/crates/core-consensus/src/handle/liveness.rs
Comment thread code/crates/test/app/config.toml
@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