feat: add toolkit for exporting and transforming missing block header fields #903
Conversation
|
why sometimes use "-" sometimes use "_" in the path? |
|
|
rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go
Outdated
Show resolved
Hide resolved
… deduplicated headers
…o add vanity count as part of the bitmask
jonastheis
force-pushed
the
jt/export-headers-toolkit
branch
from
5a60fd0 to
dcdd6f0
Compare
WalkthroughA new Go-based toolkit named Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant L2Node
participant FileSystem
User->>CLI: Run 'fetch' command with parameters
CLI->>L2Node: Fetch headers via RPC (batched, parallel, with retries)
L2Node-->>CLI: Return header data
CLI->>FileSystem: Write headers to binary and optional CSV files in order
User->>CLI: Run 'dedup' command with input file
CLI->>FileSystem: Read input headers file
CLI->>CLI: Analyze unique vanities, difficulties, seal lengths
CLI->>CLI: Deduplicate and encode headers compactly
CLI->>FileSystem: Write deduplicated output file
CLI->>User: Print SHA256 checksum of output file
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[config_reader] The configuration option 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (11)
rollup/missing_header_fields/export-headers-toolkit/.gitignore (1)
1-1: Optional: Extend ignore patterns for local artifacts
You might also consider ignoring other ephemeral files produced during development, such as Go build outputs (*.exe,*.o, or/bin/), log files (*.log), or temporary database files (*.db), if they’re generated in this folder and not covered by the root.gitignore.rollup/missing_header_fields/export-headers-toolkit/Dockerfile (1)
1-13: Consider using multi-stage builds to reduce image size.The Dockerfile follows good practices with proper separation of dependency installation and build steps. For a production image, you might want to consider using multi-stage builds to create a smaller final image.
-FROM golang:1.22 +FROM golang:1.22 AS builder WORKDIR /app COPY go.mod go.sum ./ RUN go mod download COPY . . RUN go build -o main . +FROM gcr.io/distroless/base-debian12 + +WORKDIR /app + +COPY --from=builder /app/main . + ENTRYPOINT ["./main"]rollup/missing_header_fields/export-headers-toolkit/README.md (2)
6-6: Consider using "Among" instead of "Amongst".The preposition "Amongst" is correct but considered somewhat old-fashioned or literary. A more modern alternative is "Among".
-We are using the [Clique consensus](https://eips.ethereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following header fields: +We are using the [Clique consensus](https://eips.ethereum.org/EIPS/eip-225) in Scroll L2. Among others, it requires the following header fields:🧰 Tools
🪛 LanguageTool
[style] ~6-~6: The preposition ‘Amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...thereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following heade...(AMONGST)
34-36: Use spaces instead of tabs in Markdown.The markdown file contains hard tabs in these lines, which can cause inconsistent rendering across different platforms. It's recommended to use spaces for indentation in markdown files.
- - bit 0-5: index of the vanity in the sorted vanities list - - bit 6: 0 if difficulty is 2, 1 if difficulty is 1 - - bit 7: 0 if seal length is 65, 1 if seal length is 85 + - bit 0-5: index of the vanity in the sorted vanities list + - bit 6: 0 if difficulty is 2, 1 if difficulty is 1 + - bit 7: 0 if seal length is 65, 1 if seal length is 85🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
34-34: Hard tabs
Column: 1(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1(MD010, no-hard-tabs)
rollup/missing_header_fields/export-headers-toolkit/types/header.go (2)
74-78:FromBytestrusts caller blindly
FromBytespanics on short buffers. Return an error instead:-func (h *Header) FromBytes(buf []byte) *Header { - h.Number = binary.BigEndian.Uint64(buf[:8]) +func (h *Header) FromBytes(buf []byte) (*Header, error) { + if len(buf) < 16 { + return nil, fmt.Errorf("header buffer too small: %d", len(buf)) + } + h.Number = binary.BigEndian.Uint64(buf[:8])Propagate the error to callers.
31-47: Usebytes.Equalfor readabilityThe byte-wise loop in
Equalis correct but verbose and slightly slower.-if len(h.ExtraData) != len(other.ExtraData) { - return false -} -for i, b := range h.ExtraData { - if b != other.ExtraData[i] { - return false - } -} -return true +return bytes.Equal(h.ExtraData, other.ExtraData)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go (1)
60-78: Unreferenced fieldsseenDifficulty/seenSealLen– dead code?
missingHeaderWriterstoresseenDifficultyandseenSealLenbut never updates or reads them.
Either remove or integrate them (e.g., emit statistics at the end) to keep the struct minimal.rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer_test.go (1)
97-100: Ignore-error pattern inrandomSealhides entropy failures
rand.Readcan theoretically fail (e.g., exhausted entropy on container start-up). Capture the error andt.Fatalfinstead of discarding it.-func randomSeal(length int) []byte { - buf := make([]byte, length) - _, _ = rand.Read(buf) - return buf +func randomSeal(length int) []byte { + buf := make([]byte, length) + if _, err := rand.Read(buf); err != nil { + panic(err) // or t.Fatalf in caller + } + return buf }rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go (1)
11-12: TODO signals code duplication – consider factoring out shared readerThe comment notes this file duplicates logic that already exists in
missing_header_fields.Reader.
Centralising the implementation avoids divergence and halves the maintenance
burden. If this duplication is temporary, please open an issue with a clear
follow-up plan.rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go (2)
145-160: Simplify producer loop & avoid unnecessaryokbranchThe canonical pattern is
for t := range tasks { … }; this is shorter and
prevents subtle mistakes (e.g. forgetting to handle!ok).-go func() { - for { - t, ok := <-tasks - if !ok { - break - } - fetchHeaders(client, t.start, t.end, headersChan) - } - wgProducers.Done() -}() +go func() { + for t := range tasks { + fetchHeaders(client, t.start, t.end, headersChan) + } + wgProducers.Done() +}()
31-35: Remember to close the RPC client
ethclient.Clientmaintains an underlying connection that should be closed.
Adddefer client.Close()immediately after a successful dial.client, err := ethclient.Dial(rpc) if err != nil { log.Fatalf("Error connecting to RPC: %v", err) } +defer client.Close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rollup/missing_header_fields/export-headers-toolkit/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
rollup/missing_header_fields/export-headers-toolkit/.gitignore(1 hunks)rollup/missing_header_fields/export-headers-toolkit/Dockerfile(1 hunks)rollup/missing_header_fields/export-headers-toolkit/README.md(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer_test.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/root.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/go.mod(1 hunks)rollup/missing_header_fields/export-headers-toolkit/main.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/types/header.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
rollup/missing_header_fields/export-headers-toolkit/main.go (1)
rollup/missing_header_fields/export-headers-toolkit/cmd/root.go (1)
Execute(23-28)
rollup/missing_header_fields/export-headers-toolkit/types/header.go (1)
common/bytes.go (1)
Bytes2Hex(74-76)
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go (1)
rollup/missing_header_fields/export-headers-toolkit/types/header.go (2)
Header(13-17)HeaderHeap(82-82)
🪛 LanguageTool
rollup/missing_header_fields/export-headers-toolkit/README.md
[style] ~6-~6: The preposition ‘Amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...thereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following heade...
(AMONGST)
🪛 markdownlint-cli2 (0.17.2)
rollup/missing_header_fields/export-headers-toolkit/README.md
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
rollup/missing_header_fields/export-headers-toolkit/.gitignore (1)
1-1: Appropriate .gitignore entry for data/
Ignoring thedata/directory ensures that large or intermediate datasets generated by the toolkit aren’t accidentally committed.rollup/missing_header_fields/export-headers-toolkit/main.go (1)
1-9: LGTM - Clean and minimal entry point.The main function correctly delegates to the cmd.Execute() function, following Go's best practices for Cobra-based CLI applications.
rollup/missing_header_fields/export-headers-toolkit/README.md (2)
21-37: Great documentation of the binary layout.The explanation of the binary layout is clear and detailed, making it easy to understand how the data is structured and encoded in the file. The bit allocation in the flags byte is particularly well documented.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
34-34: Hard tabs
Column: 1(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1(MD010, no-hard-tabs)
51-60: Good Docker setup instructions with networking considerations.The Docker instructions are comprehensive and include important notes about networking considerations when connecting to an RPC endpoint, which is often a source of confusion.
rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go (1)
122-140:newBitMaskconvertsdifficultyintbut caller passesuint64A malicious (or just incorrect) header with
Difficulty = math.MaxUint64would convert to a negativeinton 32-bit platforms and bypass the validation logic.Add an explicit upper-bound check before the cast in
write()or change the API to acceptuint64.rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go (1)
232-247: Write CSV rows with a terminating newline
header.String()may not include a trailing\n; if not, all rows will be
concatenated. Safer:- if _, err = f.csvWriter.WriteString(header.String()); err != nil { + if _, err = f.csvWriter.WriteString(header.String() + "\n"); err != nil { log.Fatalf("Error writing to human readable file: %v", err) }rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go (1)
160-192: Potential OOM – unbounded allocation based on on-disk size field
headerSizeis read from disk and used directly to allocate a slice. A
corrupted or malicious file could advertise a huge size and exhaust memory.
Validate the value against a sane upper limit (e.g. 10 KB).const maxHeaderSize = 10 * 1024 … if headerSize == 0 || headerSize > maxHeaderSize { log.Fatalf("Suspicious header size: %d", headerSize) }
rollup/missing_header_fields/export-headers-toolkit/cmd/root.go
Outdated
Show resolved
Hide resolved
rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go
Outdated
Show resolved
Hide resolved
rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go
Show resolved
Hide resolved
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go
Outdated
Show resolved
Hide resolved
rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go
Outdated
Show resolved
Hide resolved
rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go (1)
96-104:⚠️ Potential issueCritical: Add validation for unseen vanity to prevent silent mis-encoding.
The code currently retrieves the vanity index without checking if it exists in the map. If
header.Vanity()is not insortedVanitiesMap, Go returns 0, causing the header to incorrectly reference the first vanity in the sorted list.Apply this fix to validate the vanity exists:
func (m *missingHeaderWriter) write(header *types.Header) { // 1. prepare the bitmask bits := newBitMask(int(header.Difficulty), header.SealLen()) - vanityIndex := m.sortedVanitiesMap[header.Vanity()] + vanityIndex, ok := m.sortedVanitiesMap[header.Vanity()] + if !ok { + log.Fatalf("vanity %#x not registered in writer", header.Vanity()) + } if vanityIndex >= maxVanityCount { log.Fatalf("Vanity index %d exceeds maximum allowed %d", vanityIndex, maxVanityCount-1) }rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go (1)
217-238:⚠️ Potential issueAdd validation for CSV format to prevent panic on malformed data.
The code assumes each CSV line contains at least 3 comma-separated fields. A malformed or empty line would cause an index out of range panic.
Add validation after splitting:
func (h *csvHeaderReader) readNext() *types.Header { line, err := h.reader.ReadString('\n') if err != nil { if err == io.EOF { return nil } log.Fatalf("Error reading line: %v", err) } - s := strings.Split(line, ",") + s := strings.Split(strings.TrimSpace(line), ",") + if len(s) < 3 { + log.Fatalf("Malformed CSV line (expected at least 3 fields): %q", line) + } extraString := strings.Split(s[2], "\n")
🧹 Nitpick comments (5)
rollup/missing_header_fields/export-headers-toolkit/README.md (2)
3-3: Apply minor grammar improvements for better readability.Consider these grammatical refinements:
- Line 3: "before EuclidV2 upgrade" → "before the EuclidV2 upgrade"
- Line 6: "Amongst others" → "Among others" (more commonly used)
- Line 10: "before EuclidV2" → "before the EuclidV2 upgrade"
-A toolkit for exporting and transforming missing block header fields of Scroll before EuclidV2 upgrade. +A toolkit for exporting and transforming missing block header fields of Scroll before the EuclidV2 upgrade.-We are using the [Clique consensus](https://eips.ethereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following header fields: +We are using the [Clique consensus](https://eips.ethereum.org/EIPS/eip-225) in Scroll L2. Among others, it requires the following header fields:-However, before EuclidV2, these fields were not stored on L1/DA. +However, before the EuclidV2 upgrade, these fields were not stored on L1/DA.Also applies to: 6-6, 10-10
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...ng block header fields of Scroll before EuclidV2 upgrade. ## Context We are using the [...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
39-39: Add missing comma for better readability.-Each of the commands has its own set of flags and options. To display the help message run with `--help` flag. +Each of the commands has its own set of flags and options. To display the help message, run with `--help` flag.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Possible missing comma found.
Context: ... flags and options. To display the help message run with--helpflag. 1. Fetch the m...(AI_HYDRA_LEO_MISSING_COMMA)
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go (3)
132-133: Consider seeding the random number generator.Using
rand.Int()without seeding will produce the same sequence on each run. While this doesn't affect correctness for load distribution, consider seeding for better randomness.Add this at the beginning of the function:
rand.Seed(time.Now().UnixNano())Or use
rand.New(rand.NewSource(time.Now().UnixNano()))for a local random generator.
155-155: Fix typo in log message.-log.Printf("Fetched %d header in %s (avg=%s, wrote to channel in avg %s", end-start+1, totalDuration, fetchTimeAvg, writeTimeAvg) +log.Printf("Fetched %d headers in %s (avg=%s, wrote to channel in avg %s)", end-start+1, totalDuration, fetchTimeAvg, writeTimeAvg)
297-312: Consider propagating write errors instead of terminating.Using
log.Fatalffor write errors terminates the program immediately. For long-running fetch operations, this might be too aggressive, especially if the error is temporary (e.g., disk space temporarily full).Consider returning errors and letting the caller decide how to handle them:
-func (f *filesWriter) write(header *types.Header) { +func (f *filesWriter) write(header *types.Header) error { bytes, err := header.Bytes() if err != nil { - log.Fatalf("Error converting header to bytes: %v", err) + return fmt.Errorf("error converting header to bytes: %w", err) } if _, err = f.binaryWriter.Write(bytes); err != nil { - log.Fatalf("Error writing to binary file: %v", err) + return fmt.Errorf("error writing to binary file: %w", err) } if f.humanReadable { if _, err = f.csvWriter.WriteString(header.String()); err != nil { - log.Fatalf("Error writing to human readable file: %v", err) + return fmt.Errorf("error writing to human readable file: %w", err) } } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
rollup/missing_header_fields/export-headers-toolkit/README.md(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/root.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rollup/missing_header_fields/export-headers-toolkit/cmd/root.go
- rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go
🧰 Additional context used
🪛 LanguageTool
rollup/missing_header_fields/export-headers-toolkit/README.md
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...ng block header fields of Scroll before EuclidV2 upgrade. ## Context We are using the [...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~6-~6: The preposition ‘Amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...thereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following heade...
(AMONGST)
[uncategorized] ~39-~39: Possible missing comma found.
Context: ... flags and options. To display the help message run with --help flag. 1. Fetch the m...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
rollup/missing_header_fields/export-headers-toolkit/README.md
34-34: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go (1)
31-36: 🛠️ Refactor suggestionFix binary format documentation to match actual implementation.
The documentation incorrectly states that bits 0-5 are used for the vanity index within the flags byte. The actual implementation writes the vanity index as a separate byte after the flags.
- for each header: - - 1 byte (bitmask, lsb first): - - bit 0-5: index of the vanity in the sorted vanities list - - bit 6: 0 if difficulty is 2, 1 if difficulty is 1 - - bit 7: 0 if seal length is 65, 1 if seal length is 85 - - 65 or 85 bytes for the seal`, + - 1 byte flags (bitmask): + - bit 6: 0 if difficulty is 2, 1 if difficulty is 1 + - bit 7: 0 if seal length is 65, 1 if seal length is 85 + - 1 byte vanity_index: index of the vanity in the sorted vanities list (0-255) + - 65 or 85 bytes for the seal`,Likely an incorrect or invalid review comment.
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go
Outdated
Show resolved
Hide resolved
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rollup/missing_header_fields/export-headers-toolkit/README.md (4)
3-3: Add missing article “the” in upgrade description
The sentence “before EuclidV2 upgrade” reads more naturally as “before the EuclidV2 upgrade”.🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...ng block header fields of Scroll before EuclidV2 upgrade. ## Context We are using the [...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
6-6: Use “Among” instead of “Amongst”
Modern technical writing generally prefers “Among” over the more literary “Amongst.”🧰 Tools
🪛 LanguageTool
[style] ~6-~6: The preposition ‘Amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...thereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following heade...(AMONGST)
31-38: Clarify flags bitmask and bit indexing
Consider expanding “lsb” to “least significant bit” and noting that bits are numbered from 0. For example:- <flags:uint8><vanity_index:uint8><seal:[65|85]byte> - - flags: bitmask, lsb first - - bit 6: 0 if difficulty is 2, 1 if difficulty is 1 - - bit 7: 0 if seal length is 65, 1 if seal length is 85 + <flags:uint8><vanity_index:uint8><seal:[65|85]byte> + - flags: 8-bit mask using only bits 6 and 7 (bit 0 = LSB) + - bit 6: 0 ⇒ difficulty = 2, 1 ⇒ difficulty = 1 + - bit 7: 0 ⇒ seal length = 65 bytes, 1 ⇒ seal length = 85 bytes + - vanity_index: index into the sorted list of unique vanities (0–255) + - seal: the 65‐ or 85‐byte seal data
1-2: Suggest adding a Prerequisites section
It may help new users to know required Go version, module setup, or other dependencies before running the toolkit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rollup/missing_header_fields/export-headers-toolkit/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
rollup/missing_header_fields/export-headers-toolkit/README.md(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer_test.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- rollup/missing_header_fields/export-headers-toolkit/go.mod
- rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer_test.go
- rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go
- rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go
🧰 Additional context used
🪛 LanguageTool
rollup/missing_header_fields/export-headers-toolkit/README.md
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...ng block header fields of Scroll before EuclidV2 upgrade. ## Context We are using the [...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~6-~6: The preposition ‘Amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...thereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following heade...
(AMONGST)
🔇 Additional comments (2)
rollup/missing_header_fields/export-headers-toolkit/README.md (2)
47-49: Verify CLI invocation instructions
The example usesgo run main.go fetch …. Please confirm that there is amain.goat the project root and that this invocation works as expected (versusgo run . fetch ...or invoking the built binary).
59-60: Confirm--humanOutputflag semantics
Earlier you use--humanOutput=trueto enable CSV output, but in the Docker example you pass a filepath (--humanOutput=/app/result/headers.csv). Please verify whether this flag accepts a boolean or a path, and update the examples to match the actual CLI behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
rollup/missing_header_fields/export-headers-toolkit/README.md (2)
3-3: Replace the{{upgrade_name}}placeholder with the actual upgrade name.The placeholder still needs to be replaced with "EuclidV2" to maintain consistency with line 10.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...ng block header fields of Scroll before EuclidV2 upgrade. ## Context We are using the [...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
34-41: 🛠️ Refactor suggestionFix formatting and clarify the binary format documentation.
The documentation has formatting issues with hard tabs and needs clarification about the header structure.
- header: <flags:uint8><vanity_index:uint8><state_root:[32]byte><seal:[65|85]byte> - - flags: bitmask, lsb first - - bit 6: 0 if difficulty is 2, 1 if difficulty is 1 - - bit 7: 0 if seal length is 65, 1 if seal length is 85 + - flags: bitmask using only bits 6-7 + - bit 6: 0 if difficulty is 2, 1 if difficulty is 1 + - bit 7: 0 if seal length is 65, 1 if seal length is 85 - vanity_index: index of the vanity in the sorted vanities list (0-255) - state_root: 32 bytes of state root data - seal: 65 or 85 bytes of seal datarollup/missing_header_fields/export-headers-toolkit/types/header.go (2)
54-65:⚠️ Potential issueAdd overflow validation for header size.
The header size is cast to uint16 without validation, which will silently wrap if the size exceeds 65535 bytes.
func (h *Header) Bytes() ([]byte, error) { size := 8 + 8 + common.HashLength + len(h.ExtraData) + if size > math.MaxUint16 { + return nil, fmt.Errorf("header size %d exceeds maximum uint16 value", size) + } buf := make([]byte, HeaderSizeSerialized+size) binary.BigEndian.PutUint16(buf[:2], uint16(size))
67-77:⚠️ Potential issueFix compilation error and add bounds checking.
The code has compilation errors and potential runtime panics due to invalid type conversion and missing bounds checks.
func (h *Header) Vanity() [VanitySize]byte { - return [VanitySize]byte(h.ExtraData[:VanitySize]) + var v [VanitySize]byte + if len(h.ExtraData) >= VanitySize { + copy(v[:], h.ExtraData[:VanitySize]) + } else if len(h.ExtraData) > 0 { + copy(v[:], h.ExtraData) + } + return v } func (h *Header) Seal() []byte { + if len(h.ExtraData) <= VanitySize { + return nil + } return h.ExtraData[VanitySize:] }rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go (1)
96-118:⚠️ Potential issueAdd validation for unseen vanity values.
Accessing
sortedVanitiesMapwith an unknown vanity returns 0, causing silent mis-encoding.func (m *missingHeaderWriter) write(header *types.Header) { // 1. prepare the bitmask bits := newBitMask(int(header.Difficulty), header.SealLen()) - vanityIndex := m.sortedVanitiesMap[header.Vanity()] + vanityIndex, ok := m.sortedVanitiesMap[header.Vanity()] + if !ok { + log.Fatalf("vanity %#x not found in writer", header.Vanity()) + } if vanityIndex >= maxVanityCount { log.Fatalf("Vanity index %d exceeds maximum allowed %d", vanityIndex, maxVanityCount-1) }rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go (3)
29-56:⚠️ Potential issueClose the file on early-exit to avoid file descriptor leaks.
The file opened with
os.Openis not closed on error paths after opening, causing file descriptor leaks.Add a defer statement immediately after opening the file to ensure it's closed on all error paths:
f, err := os.Open(filePath) if err != nil { return nil, fmt.Errorf("failed to open file: %v", err) } + +// Ensure file is closed on error during initialization +defer func() { + if err != nil { + _ = f.Close() + } +}()
68-72:⚠️ Potential issueFix header-skipping loop to prevent over-reading or duplication.
The loop uses a fixed counter
i, butr.lastReadHeader.headerNumis mutated byReadNext(). This causes incorrect behavior where the loop may execute more or fewer iterations than intended.Change the loop to check against the current header number dynamically:
-for i := r.lastReadHeader.headerNum; i < headerNum; i++ { - if _, _, err = r.ReadNext(); err != nil { - return 0, common.Hash{}, nil, err - } -} +for r.lastReadHeader.headerNum < headerNum { + if _, _, err = r.ReadNext(); err != nil { + return 0, common.Hash{}, nil, err + } +}
131-131:⚠️ Potential issueFix ReadNext to return the correct difficulty value.
The named return variable
difficultyis never assigned, causing it to always return 0. The difficulty is correctly stored in the struct but not returned to the caller.Assign the difficulty value to the named return parameter:
-return difficulty, b.Bytes(), nil +difficulty = uint64(bits.difficulty()) +extraData = b.Bytes() +return
🧹 Nitpick comments (3)
rollup/missing_header_fields/export-headers-toolkit/README.md (1)
6-6: Consider using "among" instead of "amongst" for modern style.While "amongst" is grammatically correct, "among" is more commonly used in modern technical documentation.
-We are using the [Clique consensus](https://eips.ethereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following header fields: +We are using the [Clique consensus](https://eips.ethereum.org/EIPS/eip-225) in Scroll L2. Among others, it requires the following header fields:🧰 Tools
🪛 LanguageTool
[style] ~6-~6: The preposition ‘Amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...thereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following heade...(AMONGST)
rollup/missing_header_fields/export-headers-toolkit/types/header.go (1)
33-52: Simplify byte slice comparison.Use
bytes.Equalfor more efficient and readable comparison.func (h *Header) Equal(other *Header) bool { if h.Number != other.Number { return false } if h.Difficulty != other.Difficulty { return false } if h.StateRoot != other.StateRoot { return false } - if len(h.ExtraData) != len(other.ExtraData) { - return false - } - for i, b := range h.ExtraData { - if b != other.ExtraData[i] { - return false - } - } - return true + return bytes.Equal(h.ExtraData, other.ExtraData) }rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go (1)
13-13: Address the TODO comment to eliminate code duplication.The TODO comment indicates that this file duplicates functionality from
missing_header_fields.Readerand should be refactored to use the existing implementation instead.Would you like me to help identify the specific duplicated functionality and suggest a refactoring approach to eliminate this duplication?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rollup/missing_header_fields/export-headers-toolkit/README.md(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_reader.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer_test.go(1 hunks)rollup/missing_header_fields/export-headers-toolkit/types/header.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rollup/missing_header_fields/export-headers-toolkit/cmd/dedup.go
- rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer_test.go (2)
common/types.go (1)
HexToHash(66-66)rollup/missing_header_fields/export-headers-toolkit/types/header.go (1)
NewHeader(20-27)
rollup/missing_header_fields/export-headers-toolkit/types/header.go (2)
common/bytes.go (1)
Bytes2Hex(74-76)common/types.go (2)
HashLength(39-39)BytesToHash(54-58)
🪛 LanguageTool
rollup/missing_header_fields/export-headers-toolkit/README.md
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...ng block header fields of Scroll before EuclidV2 upgrade. ## Context We are using the [...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~6-~6: The preposition ‘Amongst’ is correct, but some people think that it is old-fashioned or literary. A more frequently used alternative is the preposition “among”.
Context: ...thereum.org/EIPS/eip-225) in Scroll L2. Amongst others, it requires the following heade...
(AMONGST)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: build-mock-ccc-geth
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer_test.go (2)
15-111: Well-structured test coverage!The test thoroughly validates the binary serialization format with all combinations of difficulty values and seal lengths, and properly verifies vanity sorting and indexing.
113-117: LGTM!The helper function is appropriate for test usage.
rollup/missing_header_fields/export-headers-toolkit/cmd/missing_header_writer.go (1)
132-148: Good validation of bit mask constraints!The function properly validates that difficulty is restricted to 1 or 2 and seal length to 65 or 85 bytes, matching the documented binary format.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go (3)
146-149: Consider more graceful error handling for network failures.Using
log.Fatalfterminates the entire program on any header fetch failure, even after retries. This might be too aggressive for a long-running fetch operation where some blocks might be temporarily unavailable.
168-179: Use defer for reader.close() to ensure cleanup.The reader should be closed with defer to ensure it's closed even if an error or panic occurs during the read operation.
297-300: Check errors when closing files.File close operations can fail (e.g., when the filesystem is full and buffered data cannot be written). These errors should be checked and handled.
🧹 Nitpick comments (3)
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go (3)
128-128: Consider exponential backoff instead of linear delay.The retry delay increases linearly (
i*200ms) rather than exponentially, which may not be optimal for handling temporary network issues or rate limiting.- time.Sleep(time.Duration(i*200) * time.Millisecond) + time.Sleep(time.Duration(1<<uint(i)) * 100 * time.Millisecond) // exponential backoff
137-137: Improve randomization for client selection.Using
rand.Int()without seeding or proper initialization may not provide good randomization distribution across clients.+import "math/rand" + func fetchHeaders(clients []*ethclient.Client, start, end uint64, headersChan chan<- *types.Header) { - // randomize client selection to distribute load - r := uint64(rand.Int()) + // randomize client selection to distribute load + r := rand.New(rand.NewSource(time.Now().UnixNano())) + offset := uint64(r.Intn(len(clients)))Then update the client selection:
- client := clients[(r+i)%uint64(len(clients))] // round-robin client selection + client := clients[(offset+i)%uint64(len(clients))] // round-robin client selection
203-203: Consider making channel buffer size configurable.The buffer size is hardcoded as
maxGoroutines*int(batchSize), which could lead to excessive memory usage with large batch sizes or many goroutines.Consider adding a maximum buffer size limit or making it configurable:
- headersChan := make(chan *types.Header, maxGoroutines*int(batchSize)) + bufferSize := maxGoroutines * int(batchSize) + if bufferSize > 10000 { // reasonable upper limit + bufferSize = 10000 + } + headersChan := make(chan *types.Header, bufferSize)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
rollup/missing_header_fields/export-headers-toolkit/cmd/fetch.go (3)
74-93: Well-implemented continuation logic with proper validation.The continue file handling correctly reads the last seen header and validates that the adjusted start block doesn't exceed the end block. The validation at lines 90-92 prevents invalid ranges.
208-212: Proper WaitGroup usage with Add before goroutine.The
wgConsumer.Add(1)is correctly called before starting the goroutine, preventing the race condition whereWait()might be called beforeAdd().
182-197: Efficient in-order header processing using min-heap.The implementation uses a min-heap to maintain headers in order by block number, ensuring sequential writing even when headers arrive out of order from concurrent fetchers. This is a well-designed approach for handling the producer-consumer pattern with ordering requirements.
rollup/missing_header_fields/export-headers-toolkit/types/header.go
Outdated
Show resolved
Hide resolved
rollup/missing_header_fields/export-headers-toolkit/types/header.go
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| func init() { | ||
| rootCmd.CompletionOptions.DisableDefaultCmd = true |
There was a problem hiding this comment.
from the cobra source code:
// DisableDefaultCmd prevents Cobra from creating a default 'completion' command
There was a problem hiding this comment.
Got it. Thanks! So this flag is used to simplify the output of --help? This looks good, and (to open discussion) an alternative would be using rootCmd.CompletionOptions.HiddenDefaultCmd = true.
1. Purpose or design rationale of this PR
We are using the Clique consensus in Scroll L2. Amongst others, it requires the following header fields:
extraDatadifficultyHowever, these fields are currently not stored on L1/DA and we're planning to add them in a future upgrade.
In order for nodes to be able to reconstruct the correct block hashes when only reading data from L1,
we need to provide the historical values of these fields to these nodes through a separate file.
This toolkit provides commands to export the missing fields, deduplicate the data and create a file
with the missing fields that can be used to reconstruct the correct block hashes when only reading data from L1.
Analysis of data
Mainnet until block 7455960
Sepolia until block 5422047
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:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Documentation
Tests