Handle empty notarizations #97
Conversation
yacovm
force-pushed
the
handleEmptyNot
branch
3 times, most recently
from
65b2e16 to
3354ca8
Compare
| @@ -918,7 +902,8 @@ func (e *Epoch) maybeAssembleEmptyNotarization() error { | |||
|
|
|||
| emptyNotarization := &EmptyNotarization{QC: qc, Vote: popularEmptyVote} | |||
|
|
|||
| return e.persistEmptyNotarization(emptyNotarization) | |||
| // Persist the empty notarization and also broadcast it to everyone | |||
| return e.persistEmptyNotarization(emptyNotarization, true) | |||
There was a problem hiding this comment.
i think we should avoid adding the shouldBroadcast flag to the function and just broadcast it after calling persist
There was a problem hiding this comment.
That's not possible. Inside persistEmptyNotarization we increment the round and call errors.Join(e.startRound(), e.maybeLoadFutureMessages()), so if we do it outside we'll advance to the next round without broadcasting it. While it's possible that we stop at some layer of the recursion and fold it back and then broadcast, I don't like this because we would be sending messages for higher rounds before this round, and I think we should finish doing everything regarding this round before we proceed to the next one.
| } | ||
|
|
||
| // Ignore votes for rounds too far ahead | ||
| if vote.Round-e.round > e.maxRoundWindow { |
There was a problem hiding this comment.
not for this pr, but we have a lot of checks for messages that are too far ahead in other handle functions. Maybe its worth creating a helper e.isMessageTooFarAhead(round) ? i don't mind making this pr
There was a problem hiding this comment.
makes sense, feel free to go for it.
epoch.go
Outdated
| @@ -962,11 +947,12 @@ func (e *Epoch) persistEmptyNotarization(emptyNotarization *EmptyNotarization) e | |||
|
|
|||
| e.emptyVotes[emptyNotarization.Vote.Round].emptyNotarization = emptyNotarization | |||
There was a problem hiding this comment.
why do we set the empty notarization here? Since we are increasing the round, shouldn't we delete the round from emptyVotes? If not here, i think we should be deleting somewhere
This commit adds handling for empty notarizations received from other nodes. It prevents re-broadcasting when persisting such empty notarizations. We can only handle an empty notarization if we have not notarized the round. Signed-off-by: Yacov Manevich <[email protected]>
yacovm
force-pushed
the
handleEmptyNot
branch
from
3354ca8 to
b8a9d7d
Compare
No description provided.