Skip to content

Feature - EIP-4444 EraE Support#10812

Open
svlachakis wants to merge 128 commits intomasterfrom
feature/erae
Open

Feature - EIP-4444 EraE Support#10812
svlachakis wants to merge 128 commits intomasterfrom
feature/erae

Conversation

@svlachakis
Copy link
Copy Markdown
Contributor

@svlachakis svlachakis commented Mar 12, 2026

Fixes Closes Resolves #9378

Changes

EraE Archive Format Support

Implements the EraE archive format (*.erae) - an evolution of Era1 designed for full Ethereum history including post-merge blocks. Adds import, export, remote download, proof generation, and verification.

Spec: https://hackmd.io/@matt/eraE


New Projects

  • Nethermind.EraE - core library
  • Nethermind.EraE.Test - test suite

Format

  • Section-ordered TLV layout:
    Version | Headers | Bodies | SlimReceipts | Proofs | TotalDifficulty | AccumulatorRoot | ComponentIndex
  • Pre-merge epochs: Merkle proof of the block hash against a trusted accumulator root
  • Post-merge epochs: beacon proofs via HistoricalRoots / HistoricalSummaries
  • Slim receipts (no bloom - reconstructed from logs on read)
  • ComponentIndex replaces Era1’s flat BlockIndex - supports variable component counts per epoch type

Key Components

  • EraWriter / EraReader - write and read EraE files; random-access by block number via ComponentIndex
  • EraImporter / EraExporter - concurrent import/export pipelines with configurable worker count
  • EraStore - local epoch store with accumulator verification
  • RemoteEraStoreDecorator - on-demand remote download with SHA-256 manifest verification
  • Validator - verifies per-block Merkle proofs against trusted anchors: pre-merge blocks against accumulator roots (local file), post-merge blocks against historical roots (local file) or historical summaries (fetched from beacon node via RPC)
  • BeaconApiRootsProvider - fetches beacon block roots and state roots from a beacon node for proof generation
  • AdminEraService - import/export triggered via JSON-RPC admin endpoints
  • EraCliRunner - import/export triggered on node startup, consistent with Era1 behavior

Integration

  • IEraEConfig - full configuration (import/export directories, remote URL, beacon node URL, concurrency, epoch size)
  • EraEStep - initialization step wired into NethermindModule
  • ReceiptMessageDecoder extended with skipBloom mode for slim receipt encoding

Configuration

Mirrors Era1 configuration under the EraE config section. Same keys: ImportDirectory, ExportDirectory, From, To, TrustedAccumulatorFile. Additional EraE-only options:

BeaconNodeUrl (required for post-merge export),
RemoteBaseUrl (enables on-demand download),
RemoteDownloadDirectory.


Dependencies Extended

  • Nethermind.Core - BlockHeaderProof record
  • Nethermind.Era1 - AccumulatorCalculator.GetProof for Merkle proof generation

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 manually on hoodi sepolia and it works.
Screenshot 2026-03-12 at 6 41 12 PM Screenshot 2026-03-12 at 6 11 06 PM Screenshot 2026-03-12 at 6 11 26 PM
  • Tested manually on sepolia, imports everything successfully from https://data.ethpandaops.io/erae/sepolia/ & exports successfully when block bodies are in DB
  • Tested manually Full Circle, Import from ethpandaops, Sync,Export, DB cleanup and import of our own export and it works
  • Tested manually Mainnet and it works
  • Tested manually importing Nethermind export to geth and it works

Documentation

Requires documentation update

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

@svlachakis svlachakis changed the title Feature/erae Feature - EraE Support Mar 12, 2026
@github-actions github-actions bot added the evm label Mar 12, 2026
@github-actions github-actions bot removed the evm label Mar 12, 2026
@github-actions
Copy link
Copy Markdown
Contributor

EVM Opcode Benchmark Diff

Aggregated runs: base=3, pr=3
Noisy rerun opcodes: SLOTNUM

Regressions (1)

Opcode Base Median (ns) PR Median (ns) Delta Abs Δ (ns) Base CV PR CV Threshold Uncertainty Effective
SLOTNUM 11.280 14.110 +25.09% 2.830 4.4% 5.4% ±5.0% ±6.2% ±10.8%

@LukaszRozmej
Copy link
Copy Markdown
Member

@claude re-review

@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Claude finished @LukaszRozmej's task in 4m 24s —— View job


Code Review: Feature - EIP-4444 EraE Support (Round 4)

  • Read coding rules and guidelines
  • Check resolution of round 3 issues
  • Review current state of all key files
  • Compile and post findings

Round 3 Issues — Resolution Status

Issue Status
🟡 RemoteEraStoreDecorator._openedReaders unbounded (file descriptor exhaustion) ❌ Still open
🟠 EraReader.VerifyContent concurrent blockMeta writes lack safety comment ❌ Still open
🟠 BeaconApiHttpClient 4xx returns null silently ❌ Still open
🟠 EraExporter.TrySkipExistingEpoch uses LINQ .FirstOrDefault() ❌ Still open
🟠 Parallel.ForEachAsync unnecessary async state machine per epoch ❌ Still open
🟠 EraImporter.InsertBlockAndReceipts silently discards blockTree.Insert result ❌ Still open

Round 3 Carry-Over Detail

🟡 Medium — RemoteEraStoreDecorator._openedReaders is still unbounded

RemoteEraStoreDecorator.cs:38,73

private readonly ConcurrentDictionary<int, EraReader> _openedReaders = new();
EraReader reader = _openedReaders.GetOrAdd(epoch, static (_, path) => new EraReader(path), localPath);

One EraReader (and its SafeFileHandle) is added per epoch, never evicted. Mainnet has ~1600 epochs through 2025; Linux default per-process file limit is 1024. A full historical import will exhaust OS file descriptors mid-way. EraStore caps its pool at ProcessorCount * 2 via _maxOpenFile. The same bounded pool pattern needs to be applied here. Fix this →

🟠 Low — EraReader.VerifyContent concurrent blockMeta writes missing safety comment

EraReader.cs:109-113

int idx = (int)(block.Header.Number - startBlock);
blockMeta[idx] = (block.Header.Hash!, block.TotalDifficulty ?? UInt256.Zero, !block.Header.IsPostMerge);

Multiple Task.Run workers write to blockMeta by index concurrently without a comment explaining the safety. Each dequeued blockNumber is unique so each idx is written by exactly one worker — but without a comment a future reader might incorrectly flag this as a data race. The same comment was already added to EraExporter.DoExport in round 2. Fix this →

🟠 Low — BeaconApiHttpClient 4xx still returns null silently

BeaconApiHttpClient.cs:33-39

if (!response.IsSuccessStatusCode)
{
    if ((int)response.StatusCode >= 500)
        throw new HttpRequestException(...);
    return null;  // 4xx still silent
}

A 404 for a genuinely missed slot is acceptable, but a 401/403 from a misconfigured beacon node URL returns null with no log entry. BeaconApiRootsProvider will record default(ValueHash256) roots in the exported file without any warning. Add a Warn log for non-404 4xx codes before returning null. Fix this →

🟠 Low — TrySkipExistingEpoch still uses LINQ .FirstOrDefault()

EraExporter.cs:233-235

string? existingFile = fileSystem.Directory
    .EnumerateFiles(destinationPath, ...)
    .FirstOrDefault();

Coding style forbids LINQ where a simple loop works. Replace with a foreach that captures the first match and breaks. Fix this →

🟠 Low — Parallel.ForEachAsync lambda allocates unnecessary async state machine

EraExporter.cs:102

async (epochIdx, cancel) => await WriteEpoch(epochIdx, cancel)

The async/await wrapper is redundant — WriteEpoch already returns a Task. Change to (epochIdx, cancel) => WriteEpoch(epochIdx, cancel). Fix this →

🟠 Low — InsertBlockAndReceipts still silently discards blockTree.Insert result

EraImporter.cs:191,198

blockTree.Insert(block, BlockTreeInsertBlockOptions.SaveHeader | ..., headerOptions, ...);
// result discarded; CannotAccept/UnknownParent/InvalidBlock all treated as success

Silent failures from unexpected AddBlockResult values make the import appear successful when blocks were never stored. Capture the result and log a warning for anything other than Added or AlreadyKnown. Fix this →


New Findings

🟡 Medium — Post-merge epoch proofs are never verified during import

EraReader.cs:122-123

if (!e2.HasTotalDifficulty)
    return default;  // Post-merge epochs return without any beacon proof verification

For post-merge-only epochs (no TotalDifficulty), VerifyContent skips all proof verification and returns default. The BlocksRootContext writes HistoricalRoots / HistoricalSummaries proofs into exported files during export (via EraWriter.Finalize), but these proofs are never read or verified on import. The Validator.VerifyBlocksRootContext method exists and is fully implemented but is never called anywhere in the import pipeline. This means:

  • An attacker who can replace an exported .erae file can serve arbitrary post-merge blocks with no cryptographic verification
  • The EraE format's beacon proof mechanism provides no security guarantee in the current import path

This may be an intentional MVP deferral — the PR notes "PENDING: Full Sync". If so, please add a // TODO: comment at line 122 explicitly calling out missing beacon proof verification for post-merge epochs so the gap is not overlooked. Fix this →


🟠 Low — SPDX copyright year is 2025 in two new files

BeaconApiHttpClient.cs:1, HttpRemoteEraClient.cs:1

// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited

All other new EraE files use 2026. These two files were likely created in a prior year and not updated. Fix this →


🟠 Low — EraImporter progress interval is a magic literal

EraImporter.cs:175

if (processed % 10000 == 0)

EraExporter defines private const int ProgressLogInterval = 10000 for the same purpose. EraImporter uses the bare literal. Extract to a private const. Fix this →


🟠 Low — BeaconApiRootsProvider allows duplicate concurrent beacon API calls for the same slot

BeaconApiRootsProvider.cs:33-41

if (_cache.TryGetValue(slot, out ... cached))
    return cached;
// No lock — two concurrent callers for the same slot both miss cache,
// both call FetchWithRetryAsync, both write the result
...
_cache[slot] = result.Value;

Unlike HistoricalSummariesRpcProvider which uses a SemaphoreSlim to prevent duplicate loads, BeaconApiRootsProvider has no guard. During parallel export with many workers exporting post-merge blocks in the same epoch, the same slot could be fetched from the beacon node concurrency times simultaneously. The write is idempotent, so this is not a correctness bug — but it wastes beacon node quota. A per-slot SemaphoreSlim (or the existing ConcurrentDictionary<slot, Lazy<Task<...>>> pattern) would prevent the redundant calls. Fix this →


✅ Positive observations since last review

  • EraEModule correctly registers a shared singleton HttpClient and passes it (non-owned) to both BeaconApiRootsProvider and HistoricalSummariesRpcProvider — no double-dispose risk
  • EraStoreFactory correctly absorbs FileNotFoundException when remote fallback is available, making the local-store creation failure transparent and well-logged
  • BlocksRootContext.GetAccumulatorType correctly derives fork boundaries from ISpecProvider.MergeBlockNumber and IsEip4895Enabled — no mainnet-hardcoded values remain
  • EraStore.EnsureEpochVerified double-check semaphore pattern is correct and well-commented

Copy link
Copy Markdown
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.

@claude can you check if tests added in this PR could de-duplicated with helper methods/types or TestCase/TestCaseSource ?

@codex review

@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Claude finished @LukaszRozmej's task in 2m 49s —— View job


Test Deduplication Analysis

  • Gather context
  • Read all test files
  • Analyze for deduplication opportunities
  • Post findings

Several opportunities to reduce test duplication across the new test suite, grouped by impact:


High Impact

1. EraImporterTests — repeated source/target container setup (5+ tests)

Tests Import_WithValidEraFiles_*, Import_WithTrustedAccumulators_*, Import_WithModifiedChecksum_*, Import_WithWrongTrustedAccumulator_*, Import_WithPartialRange_*, Import_WhenCalledTwice_*, Import_WhenBlocksPrePopulatedWithoutTotalDifficulty_*, and Import_WhenBlockFailsValidation_* all repeat this 8-line block verbatim:

await using IContainer sourceCtx = await EraETestModule.CreateExportedEraEnv(32, from: 0, to: 0);
string exportPath = sourceCtx.ResolveTempDirPath();
IBlockTree sourceTree = sourceCtx.Resolve<IBlockTree>();
BlockTree targetTree = Build.A.BlockTree()
    .WithBlocks(sourceTree.FindBlock(0, BlockTreeLookupOptions.None)!)
    .TestObject;
await using IContainer targetCtx = EraETestModule.BuildContainerBuilder()
    .AddSingleton<IBlockTree>(targetTree)
    .Build();

Extract a CreateImportEnvironment(int chainLength = 32) helper returning (IContainer sourceCtx, IContainer targetCtx, string exportPath) to eliminate the repetition. Fix this →


2. EraWriterTests — repeated MemoryStream + EraWriter creation (10 tests)

Every test in this class independently constructs:

using MemoryStream stream = new();
using EraWriter sut = new(stream, Substitute.For<ISpecProvider>());

Provide a CreateSut() helper returning the writer over a memory stream. Additionally, five Finalize_* tests that write to a TempPath and then open an E2StoreReader duplicate 10+ lines each. The pre-merge/post-merge pairs (e.g., ComponentIndexHasFourComponents / ComponentIndexHasThreeComponents) are good candidates for [TestCase(bool isPostMerge, bool expectedHasTd)]:

// Before (two separate tests, ~25 lines each):
[Test]
public async Task Finalize_PreMergeEpoch_ComponentIndexHasFourComponents() { ... }
[Test]
public async Task Finalize_PostMergeEpoch_ComponentIndexHasThreeComponents() { ... }

// After:
[TestCase(false, true,  Description = "PreMerge: component-count=4")]
[TestCase(true,  false, Description = "PostMerge: component-count=3")]
public async Task Finalize_ComponentIndexReflectsEpochType(bool isPostMerge, bool expectedHasTd) { ... }

The same consolidation applies to AccumulatorRootOffset pair. Fix this →


3. EraStoreTests — four FindBlockAndReceipts tests with identical setup

FindBlockAndReceipts_WithKnownBlockNumber_ReturnsBlock, _WithOutOfRangeNumber_ReturnsNull, _WithValidBlockNumber_ReturnsCorrectBlock, and _WithNegativeBlockNumber_ThrowsArgumentOutOfRangeException all create the same CreateExportedEraEnv(32) context and resolve the same store, differing only in the assertion. A [SetUp]/[TearDown] pair with a shared IContainer _ctx and IEraStore _store field would eliminate the 4-line setup block repeated across all four. Fix this →


Medium Impact

4. EraSlimReceiptDecoderTests — repeated 3-line RLP construction (5 tests)

Decode_GethFormat_PostByzantiumSuccess, _PostByzantiumFailure, _TypedReceipt_*, _DecodesCumulativeGasUsed, _InvalidStatusLength_Throws, and _InvalidTxTypeLength_Throws all build the outer wrapper identically:

byte[] receiptContent = [...];
byte[] receipt = [(byte)(0xc0 + receiptContent.Length), .. receiptContent];
byte[] encoded = [(byte)(0xc0 + receipt.Length), .. receipt];

Extract static byte[] WrapAsGethReceipt(byte[] receiptContent) to remove the boilerplate. Fix this →

5. BlocksRootContextTests — repeated HistoricalRoots context construction (3 tests)

Constructor_WithParisBlockNumberAndPreShanghaiTimestamp_*, HistoricalRoot_WhenNotFinalized_*, and FinalizeContext_InHistoricalRootsContext_* each repeat:

MainnetSpecProvider specProvider = new();
specProvider.UpdateMergeTransitionInfo(MainnetSpecProvider.ParisBlockNumber + 1);
using BlocksRootContext sut = new(MainnetSpecProvider.ParisBlockNumber + 1, 1_600_000_000UL, specProvider);

Extract a CreateHistoricalRootsContext() helper. Fix this →

6. BeaconApiRootsProviderTests — no Build() helper (unlike HistoricalSummariesRpcProviderTests)

HistoricalSummariesRpcProviderTests already has a Build(string responseBody) factory method. BeaconApiRootsProviderTests repeats the SequentialHttpMessageHandler + new HttpClient(handler) + new BeaconApiRootsProvider(...) construction in every test. Add a matching Build(params (HttpStatusCode, string)[] responses) helper. Fix this →


Low Impact

7. EraFileFormatComplianceTests — shared TestEraFile instance

Multiple tests use TestEraFile.Create(preMergeCount: 2, postMergeCount: 0) or (preMergeCount: 3, postMergeCount: 0). A [SetUp]/[TearDown] per-variant, or splitting into two nested classes (PreMergeTests / PostMergeTests) each with a single shared file, would avoid recreating the same file for each test case.

8. AccumulatorCalculatorTests — two out-of-range tests for GetProof

// Could be:
[TestCase(-1, Description = "negative")]
[TestCase(1,  Description = "at count")]
public void GetProof_WithOutOfBoundsIndex_ThrowsArgumentOutOfRangeException(int index) { ... }

9. EraPathUtilsTests — two ExtractHashFromChecksumEntry tests with identical assertion

Both _WithHashOnly and _WithHashAndFilename assert result.ToByteArray()[0] == 0xaa. These can be a [TestCase]. Fix this →


Already Good

The following already use [TestCase] / [TestCaseSource] well and don't need changes:

  • EraExporterTests.Export_WithVaryingChainLength_* (5 TestCase entries)
  • EraStoreTests.FirstAndLastBlock_AfterExport_MatchSpecifiedRange (3 TestCase entries)
  • AccumulatorCalculatorTests.GetProof_WhenCalled_ProofZeroIsTotalDifficultyLE (3 TestCase entries)
  • EraSlimReceiptDecoderTests.Decode_GethFormat_TypedReceipt_SetsTxType (3 TestCase entries)
  • EraPathUtilsTests.Filename_WithValidParameters_ReturnsExpected (3 TestCase entries)

@svlachakis
Copy link
Copy Markdown
Contributor Author

@claude review again the feature Feature - EIP-4444 EraE Support, and also review again Test Deduplication

@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Claude finished @svlachakis's task in 3m 59s —— View job


Code Review: Feature - EIP-4444 EraE Support (Round 5)

  • Read coding rules and guidelines
  • Check resolution of Round 4 issues
  • Review current state of key files for new issues
  • Review test deduplication status
  • Compile and post findings

Round 4 Issues — Resolution Status

Issue Status
🟡 RemoteEraStoreDecorator._openedReaders unbounded (file descriptor exhaustion) ❌ Still open (4th round)
🟠 EraReader.VerifyContent concurrent blockMeta writes missing safety comment ✅ Fixed — comment added at EraReader.cs:109
🟠 BeaconApiHttpClient 4xx returns null silently ✅ Fixed — Warn log for non-404 4xx at BeaconApiHttpClient.cs:41-43
🟠 TrySkipExistingEpoch uses LINQ .FirstOrDefault() ✅ Fixed — foreach + break at EraExporter.cs:233-238
🟠 Parallel.ForEachAsync unnecessary async state machine ✅ Fixed — new ValueTask(WriteEpoch(...)) at EraExporter.cs:102
🟠 InsertBlockAndReceipts silently discards blockTree.Insert result ✅ Fixed — Warn log for unexpected AddBlockResult at EraImporter.cs:194-195, 203-204
🟡 Post-merge epoch proofs never verified during import ✅ Fixed — // TODO: comment at EraReader.cs:121-123
🟠 SPDX copyright year 2025 in HttpRemoteEraClient.cs ❌ Still open
🟠 EraImporter progress interval magic literal ✅ Fixed — private const int ProgressLogInterval = 10000 at EraImporter.cs:39
🟠 BeaconApiRootsProvider concurrent duplicate beacon API calls ❌ Still open

Carry-Over (Still Open)

🟡 Medium — RemoteEraStoreDecorator._openedReaders is still unbounded (4th round)

RemoteEraStoreDecorator.cs:38, 73

private readonly ConcurrentDictionary<int, EraReader> _openedReaders = new();
// ...
EraReader reader = _openedReaders.GetOrAdd(epoch, static (_, path) => new EraReader(path), localPath);

One EraReader (holding a SafeFileHandle) is added per epoch and never evicted until Dispose(). Mainnet has ~1600 epochs through 2025; Linux default per-process fd limit is 1024 → a full historical import will exhaust OS file descriptors mid-way. EraStore correctly caps its pool at ProcessorCount * 2 via _maxOpenFile. Fix this →

🟠 Low — BeaconApiRootsProvider still allows concurrent duplicate calls for the same slot

BeaconApiRootsProvider.cs:35-44

if (_cache.TryGetValue(slot, out ... cached))
    return cached;
// No guard — concurrent callers for the same slot both miss cache,
// both fetch from beacon node, both write result
...
_cache[slot] = result.Value;

Write is idempotent so this is not a correctness bug, but wastes beacon node quota during parallel export of the same epoch. HistoricalSummariesRpcProvider already uses a SemaphoreSlim for this. A ConcurrentDictionary<slot, Lazy<Task<...>>> would prevent redundant calls. Fix this →


New Findings

🟡 Medium — RemoteEraStoreDecorator._openedReaders.GetOrAdd can leak EraReader instances

RemoteEraStoreDecorator.cs:73

EraReader reader = _openedReaders.GetOrAdd(epoch, static (_, path) => new EraReader(path), localPath);

ConcurrentDictionary.GetOrAdd with a factory may call the factory more than once when two threads access the same key simultaneously (docs). If two threads first-access the same epoch concurrently (both pass EnsureEpochAvailableAsync after the semaphore is released), both may create a new EraReader(path) — only one gets stored; the other is discarded without being Dispose()d, leaking its SafeFileHandle. Fix by using GetOrAdd(epoch, new Lazy<EraReader>(() => new EraReader(path))).Value so that at most one EraReader is ever constructed per epoch slot. Fix this →


🟠 Low — SPDX copyright year still 2025 in HttpRemoteEraClient.cs

HttpRemoteEraClient.cs:1

// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited

All other new EraE files use 2026. Fix this →


🟠 Low — LINQ still present in two test methods

EraStoreTests.cs:119-122

ISet<ValueHash256> trusted = (await File.ReadAllLinesAsync(accPath))
    .Select(EraPathUtils.ExtractHashFromChecksumEntry)
    .ToHashSet();

EraImporterTests.cs:88-90

string[] fakeLines = accLines.Select(l =>
    "0x000...  " + l.Split(' ')[^1]).ToArray();

Coding style prohibits LINQ where a simple loop works. Both should be foreach + Add/array assignment. Fix this →


Test Deduplication — Resolution Status

All deduplication improvements from Round 4 have been fully addressed:

Deduplication Item Status
EraImporterTests repeated source/target container setup ImportEnvironment record + CreateImportEnvironment helper added
EraWriterTests repeated MemoryStream + EraWriter construction CreateSut() extracted; pre/post-merge pairs merged with [TestCase(bool isPostMerge)]
EraStoreTests repeated FindBlockAndReceipts setup CreateDefaultEraStoreEnv() + EraStoreEnv record added
EraSlimReceiptDecoderTests repeated RLP wrapper construction WrapAsGethReceipt() extracted
BlocksRootContextTests repeated HistoricalRoots context setup CreateHistoricalRootsContext() extracted
BeaconApiRootsProviderTests missing Build() factory Build(params ...) factory added
AccumulatorCalculatorTests two out-of-range tests ✅ Merged with [TestCase(-1)] / [TestCase(1)]
EraPathUtilsTests two ExtractHashFromChecksumEntry tests ✅ Merged with [TestCase]

The remaining deduplication items are the two LINQ violations noted above.


✅ Positive observations since last review

  • InsertBlockAndReceipts now surfaces unexpected AddBlockResult values via warn logging — import failures are no longer silent
  • TrySkipExistingEpoch correctly uses foreach + break instead of LINQ
  • EraExporter.Parallel.ForEachAsync lambda correctly avoids the unnecessary async state machine allocation
  • EraWriter XML summary doc is thorough and well-structured
  • Test infrastructure refactoring (ImportEnvironment, EraStoreEnv) produces significantly more readable tests

@svlachakis
Copy link
Copy Markdown
Contributor Author

@claude review again

@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Claude finished @svlachakis's task in 3m 59s —— View job


Code Review: Feature - EIP-4444 EraE Support (Round 6)

  • Read coding rules and guidelines
  • Check resolution of Round 5 carry-over issues
  • Review current state of key files
  • Compile and post findings

Round 5 Issues — Resolution Status

All Round 5 issues have been addressed:

Issue Status
🟡 RemoteEraStoreDecorator._openedReaders unbounded fd exhaustion (4 rounds!) ✅ Fixed — _maxOpenReaders = Math.Max(ProcessorCount * 2, 8) with LRU eviction in RentReader
🟡 RemoteEraStoreDecorator.GetOrAdd leaks EraReader on factory race ✅ Fixed — ConcurrentDictionary<int, Lazy<EraReader>> ensures at most one EraReader constructed per slot
🟠 SPDX copyright year 2025 in HttpRemoteEraClient.cs ✅ Fixed — now 2026
🟠 LINQ in EraStoreTests.cs ✅ Fixed — foreach + HashSet.Add
🟠 LINQ in EraImporterTests.cs ✅ Fixed — for loop
🟠 BeaconApiRootsProvider concurrent duplicate beacon calls ✅ Fixed — _inFlight: ConcurrentDictionary<long, Lazy<Task<...>>> deduplicates in-flight fetches

New Findings

🟠 Low — 38 files across Nethermind.EraE and Nethermind.EraE.Test have SPDX copyright year 2025

Previous reviews only flagged HttpRemoteEraClient.cs. There are 18 source files and 20 test files (38 total) still showing // SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited. The full list includes: BlocksRootContext.cs, BlockProofs.cs, BeaconApiRetry.cs, EraPathUtils.cs, IEraStore.cs, IEraStoreFactory.cs, IRemoteEraClient.cs, IEraExporter.cs, IEraImporter.cs, EraAdminRpcModule.cs, IEraAdminRpcModule.cs, IBeaconRootsProvider.cs, NullBeaconRootsProvider.cs, IAdminEraService.cs, EraEConfig.cs, EntryTypes.cs, EraJobRunner.cs, EraFormatException.cs, and all 20 test files. Fix this →


🟠 Low — HistoricalSummariesRpcProvider 4xx warnings are silently dropped

EraEModule.cs:41-43

new HistoricalSummariesRpcProvider(new Uri(url), ctx.Resolve<HttpClient>())

BeaconApiHttpClient was updated in round 3 to log a Warn for non-404 4xx responses — but only when a logger is passed. BeaconApiRootsProvider correctly passes a logManager:

new BeaconApiRootsProvider(new Uri(url), ctx.Resolve<HttpClient>(), logManager: ctx.Resolve<ILogManager>())

HistoricalSummariesRpcProvider has no logManager parameter at all, so BeaconApiHttpClient is constructed with _logger = default (null logger). A 401/403 from a misconfigured beacon node URL will log nothing when fetching historical summaries, while the same error from BeaconApiRootsProvider correctly surfaces a warning. Add a logManager parameter to HistoricalSummariesRpcProvider and pass it to BeaconApiHttpClient, then thread it through the DI registration in EraEModule. Fix this →


✅ Positive observations since last review

  • RemoteEraStoreDecorator.RentReader correctly uses LRU eviction (lowest-epoch key first) matching the ascending-order access pattern of historical imports — this is an intentional optimization, not just a coincidence
  • RemoteEraStoreDecorator.Dispose correctly guards .IsValueCreated before calling .Value.Dispose() on each Lazy<EraReader> — no risk of triggering lazy initialization at dispose time
  • BeaconApiRootsProvider._inFlight correctly uses CancellationToken.None for the shared fetch factory, preventing a first-caller cancellation from aborting a fetch that other concurrent callers are waiting on; the per-request timeout in BeaconApiHttpClient bounds the worst case
  • EraImporter.InsertBlockAndReceipts now correctly warns on unexpected AddBlockResult for both insert paths (new block and TD re-insert)
  • The HistoricalSummariesRpcProvider double-check SemaphoreSlim pattern (round 2 fix) is still correct and well-structured
  • EraWriter.Finalize correctly gates ArrayPool<long>.Shared.Return(tdOffsets) with if (needsTd) — the empty array literal [] assigned when !needsTd is never returned to the pool

This is the last round with blocking issues. The two remaining findings are both cosmetic. The implementation is well-structured and the core correctness concerns from earlier rounds have all been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EraE Import / Export

4 participants