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 forks in AuRa #4679

Merged
merged 5 commits into from
Jan 6, 2023
Merged

Fix forks in AuRa #4679

merged 5 commits into from
Jan 6, 2023

Conversation

jmederosalvarado
Copy link
Member

@jmederosalvarado jmederosalvarado commented Sep 29, 2022

Summary of the changes

Most changes are refactors that I think improve the readability of the code. The main functional change is in the way we decide whether to call the validator contract finalizeChange function at the beginning of the block processing, this now no longer depends on some in-memory state being initialized having to be kept from the processing of block n to the processing of block n+1. Additionally, we are no longer using the AreFinalized field in PendingValidators, we keep it only for backward compatibility with existing databases. The code could probably be further improved to be more robust, especially when handling reorgs, however, since reorgs have not caused any issues so far, and it seems unlikely that they do so because of the way AuRa works, we can ignore this and keep the changes as simple as possible for now.

Changes:

  • Makes some adjustments to ContractBasedValidator class in AuRa code to prevent forks caused by invalid blocks

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Comments about testing

@LukaszRozmej
Copy link
Member

LukaszRozmej commented Sep 30, 2022

@LukaszRozmej
Copy link
Member

Are we progressing with this?

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

As log as it was tested on FullSyncs and Validator nodes it looks fine.

@jmederosalvarado jmederosalvarado marked this pull request as ready for review December 23, 2022 21:28
@jmederosalvarado jmederosalvarado merged commit e56ec5c into master Jan 6, 2023
@jmederosalvarado jmederosalvarado deleted the gnosis/aura-forks-fix branch January 6, 2023 21:18
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