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

Adjust Optimism (Mainnet) TotalDifficulty calculation #7647

Merged
merged 6 commits into from
Oct 24, 2024

Conversation

emlautarom1
Copy link
Contributor

@emlautarom1 emlautarom1 commented Oct 23, 2024

Fixes #7626

Changes

  • Properly reset the TotalDifficulty at the Bedrock block in Optimism

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Tested by running a node using the op-mainnet configuration. All Old Headers were downloaded without any issues and proper TotalDifficulty is always calculated correctly.

Documentation

Requires documentation update

  • Yes
  • No

This change should be internal so no need to update docs.

Requires explanation in Release Notes

  • Yes
  • No

Remarks

Some changes required adjusting the DI modules specific for Optimism. It was not as straightforward as I would have expected it to be.

{
base.Load(builder);

if (provider.ChainId == OptimismMainnetChainId)
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is not needed. Just have one OptimismSynchronizerModule for just optimism configuration, the register it later after MergeSynchronizerModule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check is actually needed since we want to use a particular difficulty calculation strategy in Optimism Mainnet and not in, for example, Optimism Sepolia.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is unfortunate.

@emlautarom1 emlautarom1 merged commit cda75f4 into master Oct 24, 2024
75 checks passed
@emlautarom1 emlautarom1 deleted the feat/total-difficulty-strategy branch October 24, 2024 20:17
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.

Old Headers sync crashes on op-mainnet
5 participants