Skip to content
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

Fix: discard report processing on lost consensus #708

Conversation

skozin
Copy link
Member

@skozin skozin commented Mar 27, 2023

On consensus loss, discard the report if its processing hasn't started yet. This increases the chance that the DAO can react in time to an invalid report that still passes the onchain sanity checks.

HashConsensus tests:

  • upon changing processor, no report is submitted to a new processor if consensus was lost for the slot
  • submitting a different report by a member can lead to consensus loss (partially tested)
  • increasing quorum can lead to consensus loss
  • consensus cannot be lost if the report is already processing
  • submitReprot reverts on zero slot

BaseOracle tests:

  • discardConsensusReport access control
  • discardConsensusReport does nothing if there's no report for the current frame
  • discardConsensusReport does nothing if the passed slot is in the future
  • discardConsensusReport reverts if the slot is less than the last submitted one
  • discardConsensusReport discards the report that's not yet being processed and emits the event
  • discardConsensusReport reverts if the report is already being processed
  • _startProcessing fails when there's no report to process

AccountingOracle tests:

  • cannot submit main data if the report was discarded
  • getProcessingState returns the same state for a discarded report as if it was not submitted

ValidatotExitBusOracle tests:

  • cannot submit data if the report was discarded
  • getProcessingState returns the same state for a discarded report as if it was not submitted

@skozin skozin force-pushed the fix/clear-report-processing-on-lost-consensus branch from f2a9fb8 to 39db313 Compare March 27, 2023 15:57
@TheDZhon TheDZhon changed the base branch from feature/shapella-upgrade to fix/shapella-upgrade-from-rc0-to-rc1 March 28, 2023 06:16
@skozin skozin marked this pull request as ready for review March 28, 2023 08:49
@skozin skozin marked this pull request as draft March 28, 2023 08:51
skozin added 3 commits April 3, 2023 13:21
processing deadline is already checked in _startProcessing so it makes no sense to also check it in _checkConsensusData
@skozin skozin force-pushed the fix/clear-report-processing-on-lost-consensus branch from 39db313 to a27650c Compare April 3, 2023 10:22
Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you, @skozin and @Jeday 👍

@TheDZhon TheDZhon marked this pull request as ready for review April 3, 2023 16:22
@TheDZhon TheDZhon merged commit 588f19f into fix/shapella-upgrade-from-rc0-to-rc1 Apr 3, 2023
@TheDZhon TheDZhon deleted the fix/clear-report-processing-on-lost-consensus branch April 3, 2023 16: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.

3 participants