Repository navigation
Conversation
| if err != nil { | ||
| // The notarization is already stored in memory but not on disk, so we must not keep running: | ||
| // we might otherwise sign both a finalize vote and an empty vote for this round. | ||
| e.haltedError = err |
There was a problem hiding this comment.
why dont we just set the halted error when Appending to the WAL fails for everything?
| } | ||
|
|
||
| func (e *Epoch) persistAndBroadcastNotarization(notarization common.Notarization) error { | ||
| err := e.persistNotarization(notarization) |
There was a problem hiding this comment.
can we set the error closer to where it happens? i.e the call to writeNotarizationToWal in persistNotarization?
| } | ||
|
|
||
| func (e *Epoch) maybeConstructFinalizeVoteMessage(md common.BlockHeader) (common.FinalizeVote, *common.Message, error) { | ||
| if e.haltedError != nil { |
There was a problem hiding this comment.
if we set it in peristNotarization we don't need to set it here
There was a problem hiding this comment.
also it seems weird to have these haltedError checks in parts of the code that are running on different go-routines. Ideally we just check if the epoch is halted once at the start of each go-routine and not have to worry about checking them here
There was a problem hiding this comment.
maybeConstructFinalizeVoteMessage and voteOnBlock can be called from multiple goroutines. There are quite a few goroutines such as those that are spawned from replication, regular consensus, etc.
I put the halted error check here and in voteOnBlock because that's the critical thing to avoid - to not broadcast a signature to other validators. I feel this way is much safer because we're putting the check as close as possible to what we're trying to avoid, instead of putting it far and hoping we get all entry points.
persistAndBroadcastNotarization returned the WAL append error without halting the epoch. The notarization was already stored in memory by storeNotarization, so the node kept running at the same round with a notarization that was never written to disk. A relayed finalization for an earlier round could then make it sign a finalize vote for that round via rebroadcastPastFinalizeVotes, and the round timer could make it sign an empty vote for the same round, which breaks Simplex safety. Set haltedError when persisting the notarization fails, so the epoch stops handling messages. Also refuse to vote, finalize vote, or empty vote once the epoch is halted just to be safe. Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
persistAndBroadcastNotarization returned the WAL append error without halting the epoch. The notarization was already stored in memory by storeNotarization, so the node kept running at the same round with a notarization that was never written to disk.
A relayed finalization for an earlier round could then make it sign a finalize vote for that round via rebroadcastPastFinalizeVotes, and the round timer could make it sign an empty vote for the same round, which breaks Simplex safety.
Set haltedError when persisting the notarization fails, so the epoch stops handling messages. Also refuse to vote, finalize vote, or empty vote once the epoch is halted just to be safe.