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: blockhash mismatch #913

Draft
wants to merge 13 commits into
base: feat/sync-directly-from-da
Choose a base branch
from

Conversation

jonastheis
Copy link

@jonastheis jonastheis commented Jul 19, 2024

1. Purpose or design rationale of this PR

This PR fixes the problem of mismatching block hashes due to the missing header fields difficulty and extraData in DA. It should be reviewed in conjunction with #903, which provides a way to prepare this missing data and describes the format in more detail.

Specifically, this PR implements a missing header fields manager that:

  • lazily downloads the missing header data if not present
  • verifies SHA256 checksum of the missing header fields data with hardcoded checksum that is part of the chain config
  • provides functionality to read the missing header fields when syncing from DA

Tested on mainnet:

inserted block with hash 0xd229dc5edaebccff15987f42bb5c23060b3f0372251e19e843f1bb69742bfab1 blockchain height=64500

https://scrollscan.com/block/64500: 0xd229dc5edaebccff15987f42bb5c23060b3f0372251e19e843f1bb69742bfab1

2. PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

3. Deployment tag versioning

Has the version in params/version.go been updated?

  • This PR doesn't involve a new deployment, git tag, docker image tag, and it doesn't affect traces
  • Yes

4. Breaking change label

Does this PR have the breaking-change label?

  • This PR is not a breaking change
  • Yes

…er fields file, verify its hash and download it. Also implement a Reader to read the file's format and provide the requested information to Manager
…tead of full requested bytes (65 in this case)
downloadCtx, downloadCtxCancel := context.WithTimeout(m.ctx, timeoutDownload)
defer downloadCtxCancel()

req, err := http.NewRequestWithContext(downloadCtx, http.MethodGet, m.downloadURL, nil)
Copy link

Choose a reason for hiding this comment

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

Risk: Affected versions of golang.org/x/net, golang.org/x/net/http2, and net/http are vulnerable to Uncontrolled Resource Consumption. An attacker may cause an HTTP/2 endpoint to read arbitrary amounts of header data by sending an excessive number of CONTINUATION frames.

Fix: Upgrade this library to at least version 0.23.0 at go-ethereum/go.mod:103.

Reference(s): GHSA-4v7x-pqxf-cx7m, CVE-2023-45288

Ignore this finding from ssc-46663897-ab0c-04dc-126b-07fe2ce42fb2.

@jonastheis jonastheis changed the base branch from feat/sync-directly-from-da to jt/execute-blocks-only-once July 26, 2024 07:31
@jonastheis jonastheis force-pushed the jt/execute-blocks-only-once branch from 38f46ed to 26dbf42 Compare July 30, 2024 04:57
@jonastheis jonastheis mentioned this pull request Aug 2, 2024
13 tasks
Base automatically changed from jt/execute-blocks-only-once to feat/sync-directly-from-da August 6, 2024 03:02
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.

1 participant