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 OP Stack header validation / Remove snapshot from snap-synced chains #7588

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jmederosalvarado
Copy link
Member

@jmederosalvarado jmederosalvarado commented Oct 11, 2024

Changes

  • Removes snapshot from non-archive configs, for op networks that support snap sync
  • Fix header validation logic for pre-bedrock block to only verify parent header matches parent hash. This can be done because bedrock block is always hardcoded in op chains, so it's enough to verify that the reverse chain hashes match from BedrockBlock to 0.
  • Use OptimismPoSSwitcher that decides whether block is pre or post merge based on bedrock block number

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

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

@emlautarom1
Copy link
Contributor

The issue seems to be specific to OP-mainnet (I could not reproduce the issue on OP-sepolia)

Copy link
Contributor

@emlautarom1 emlautarom1 left a comment

Choose a reason for hiding this comment

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

In the PR description you mention:

Removes snapshot from non-archive configs, for op networks that support snap sync

Is this still WIP?

@@ -73,7 +73,7 @@
"rip7212TransitionTimestamp": "0x668eb001",
"opGraniteTransitionTimestamp": "0x66e1be81",

"terminalTotalDifficulty": "0"
"terminalTotalDifficulty": "210470125"
Copy link
Contributor

Choose a reason for hiding this comment

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

From where are we getting this value?

Comment on lines +96 to +100
"enode://87a32fd13bd596b2ffca97020e31aef4ddcc1bbd4b95bb633d16c1329f654f34049ed240a36b449fda5e5225d70fe40bc667f53c304b71f8e68fc9d448690b51@3.231.138.188:30301",
"enode://ca21ea8f176adb2e229ce2d700830c844af0ea941a1d8152a9513b966fe525e809c3a6c73a2c18a12b74ed6ec4380edf91662778fe0b79f6a591236e49e176f9@184.72.129.189:30301",
"enode://acf4507a211ba7c1e52cdf4eef62cdc3c32e7c9c47998954f7ba024026f9a6b2150cd3f0b734d9c78e507ab70d59ba61dfe5c45e1078c7ad0775fb251d7735a2@3.220.145.177:30301",
"enode://8a5a5006159bf079d06a04e5eceab2a1ce6e0f721875b2a9c96905336219dbe14203d38f70f3754686a6324f786c2f9852d8c0dd3adac2d080f4db35efc678c5@3.231.11.52:30301",
"enode://cdadbe835308ad3557f9a1de8db411da1a260a98f8421d62da90e71da66e55e98aaa8e90aa7ce01b408a54e4bd2253d701218081ded3dbe5efbbc7b41d7cef79@54.198.153.150:30301",
Copy link
Contributor

Choose a reason for hiding this comment

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

From where are we getting these nodes?

public override bool Validate(BlockHeader header, BlockHeader? parent, bool isUncle, [NotNullWhen(false)] out string? error)
{
error = null;
return ValidateParent(header, parent, ref error);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the PR description you mention:

Fix header validation logic for pre-bedrock block to only verify parent header matches parent hash

But we're not actually doing that since ValidateParent checks that:

  • For non genesis block, we have a non-null parent
  • For genesis block, we perform a specific check on certain fields.

Is this something in progress? Otherwise, I would expect to see something like

if (header.ParentHash != parent.Hash)
{
    if (_logger.IsDebug) _logger.Debug($"Parent hash mismatch in block header ({header.Hash})");
    error = BlockErrorMessages.MismatchedParentHash;
    return false;
}

@emlautarom1
Copy link
Contributor

Also, does this PR supersede #7565 ?

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