Skip to content

Discard equivocated round when processing notarizations - #457

Draft
yacovm wants to merge 5 commits into
mainfrom
equivocationNotarization
Draft

Discard equivocated round when processing notarizations#457
yacovm wants to merge 5 commits into
mainfrom
equivocationNotarization

Conversation

@yacovm

@yacovm yacovm commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

When receiving a notarization, check if the block received beforehand corresponds to the received notarization, and if not, delete the round.

@yacovm
yacovm force-pushed the equivocationNotarization branch from ab103ae to 6922dc1 Compare July 23, 2026 21:53
Comment thread simplex/epoch.go
Comment thread simplex/epoch.go

if round.finalization != nil {
e.Logger.Debug("Received finalization for an already finalized round", zap.Uint64("round", message.Finalization.Round))
if err := e.storeFinalization(message); err != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if we receive a valid finalization, but the leader equivocated shouldn't we trigger replication?

@yacovm yacovm Jul 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Comment thread simplex/epoch.go
zap.Uint64("round", roundNum),
zap.Stringer("block header", &expectedBlockHeader),
zap.Stringer("finalized block header", &finalization.Finalization.BlockHeader))
delete(e.rounds, roundNum)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hmm, I'm wondering if anything bad can happen if we delete the round.

Maybe a couple things? For instance we could have already progressed to the next round and incremented e.round. This means we will have no entry in the rounds map, and we may have no entry in the empty rounds map so I wonder if any lookup breaks.

Also if we restart the node it will re-set the rounds map when we read from the wal. But I don't think this is an issue

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

we can't progress to the next round at the point where we delete the round because we discard the notarization.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also if we restart the node it will re-set the rounds map when we read from the wal. But I don't think this is an issue

It will simply delete the round once more, but in order to have a higher round than that, it will need to replicate the round, no? so it will eventually overwrite the rounds map when it reloads the WAL.

But good point, I should make a UT that checks this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@yacovm
yacovm force-pushed the equivocationNotarization branch 2 times, most recently from c8dc152 to 5b282a6 Compare July 27, 2026 23:27
@yacovm
yacovm marked this pull request as draft July 27, 2026 23:32
yacovm added 4 commits July 28, 2026 23:00
When receiving a finalization, check if the block received beforehand corresponds to
the received finalization, and if not, delete the round.

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
When receiving a notarization, check if the block received beforehand corresponds to the received notarization, and if not, delete the round.

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
@yacovm
yacovm force-pushed the equivocationNotarization branch from 5f18372 to cab5e29 Compare July 28, 2026 22:12
@yacovm
yacovm marked this pull request as ready for review July 28, 2026 22:12
samliok
samliok previously approved these changes Jul 28, 2026
Comment thread simplex/epoch_test.go Outdated
Co-authored-by: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
Signed-off-by: yacovm <yacovm@users.noreply.github.com>
@yacovm
yacovm force-pushed the equivocationNotarization branch from 986dfd9 to 7dc9a2f Compare July 28, 2026 23:03
@yacovm
yacovm dismissed samliok’s stale review July 28, 2026 23:22

actually we might have a problem

@yacovm
yacovm marked this pull request as draft July 28, 2026 23:22
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