Skip to content

Fix AuRaMergeEngineModuleTests#10872

Merged
Marchhill merged 6 commits intomasterfrom
fix/aura-merge-engine
Mar 23, 2026
Merged

Fix AuRaMergeEngineModuleTests#10872
Marchhill merged 6 commits intomasterfrom
fix/aura-merge-engine

Conversation

@Marchhill
Copy link
Contributor

Changes

  • Fix test to actually use AuRaBlockProcessor code path, previously used regular BlockProcesssor

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: Test fix

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

@LukaszRozmej
Copy link
Member

2 tests are failing

@Marchhill
Copy link
Contributor Author

@claude review

@claude
Copy link

claude bot commented Mar 20, 2026

Claude finished @Marchhill's task in 2m 48s —— View job


Code Review

  • Gather context and read changed files
  • Analyze changes and provide review feedback

Summary

The core fix is correct: the original MergeAuRaTestBlockchain was silently overriding the DI-registered IBlockProcessor with a plain BlockProcessor, bypassing AuRaBlockProcessor entirely. This PR removes that workaround and properly configures the AuRa dependencies. However, there are a few issues worth addressing.


Issues

1. Base class modification concerns (aligns with reviewer's request) — Blocking

The changes to EngineModuleTests.V1.cs (making tests virtual + adding bool isAura parameter) address an AuRa-specific need but modify the base class interface. The reviewer's question is valid: are all derived test classes now affected?

The [TestCase(false)] in the base class means that any other class inheriting from EngineModuleTests will also run these tests with an isAura=false parameter where they previously ran with [Test]. While functionally equivalent, it changes the test signatures. NUnit attribute inheritance for [TestCase] on virtual methods can also be ambiguous — derived classes that do NOT override these methods will still run with isAura=false, while AuRaMergeEngineModuleTests overrides with [TestCase(true)] to replace it.

A cleaner alternative that avoids touching the base class would be:

  1. Keep the base tests as [Test] with no isAura parameter
  2. In AuRaMergeEngineModuleTests, override both tests and apply [Ignore] to suppress the non-AuRa variants, then add separate [Test]-annotated methods that build blocks with .WithAura(0, []) explicitly

However, if code duplication is the concern, the current parameterized approach is defensible, provided the NUnit inheritance behavior is confirmed and there are no regressions in other derived test classes.

2. Duplicate AddDecorator<AuRaNethermindApi> in ConfigureContainer — Minor

AuRaMergeEngineModuleTests.cs lines 156–161 add a new decorator for AuRaNethermindApi to set EngineSigner and NonceManager, while lines 175–183 hold the existing decorator for TxPool, TransactionComparerProvider, and FinalizationManager. These two decorators could be merged into one for readability.

// Current — two separate decorators
.AddDecorator<AuRaNethermindApi>((_, api) =>
{
    api.EngineSigner = NullSigner.Instance;
    api.NonceManager = Substitute.For<INonceManager>();
    return api;
})
// ... other registrations ...
.AddDecorator<AuRaNethermindApi>((_, api) =>
{
    api.TxPool = TxPool;
    // ...
    return api;
});

Not a blocking issue since Autofac decorator chains work correctly, but it's an unnecessary split.

Fix this →

3. Failing tests — needs investigation

LukaszRozmej mentioned 2 tests are failing. With the CI showing no recent run results, it's hard to identify which tests these are without running them. Likely candidates are tests in AuRaMergeEngineModuleTests that inherit from the base but use blocks that don't have AuRa headers — specifically ones that go through the AuRaBlockProcessor path. The addition of ValidatorsJson and EngineSigner/NonceManager in this PR addresses some of those gaps, but any other test that builds plain Block objects (without .WithAura(0, [])) and processes them through the AuRa chain could still fail.


What's correct

  • Removing .AddScoped<IBlockProcessor, BlockProcessor>() is the right fix — the old comment literally said "AuRa was never configured correctly in test."
  • Adding ValidatorsJson with List = [Address.Zero] is necessary for AuRaContractValidator initialization.
  • Setting api.EngineSigner = NullSigner.Instance and api.NonceManager = Substitute.For<INonceManager>() correctly satisfies the AuRa signing path.
  • The WithGenesisPostProcessor AuRa header seeding and the block producer wiring look correct and unchanged.

@Marchhill Marchhill requested a review from LukaszRozmej March 20, 2026 15:56
@Marchhill Marchhill requested a review from svlachakis March 23, 2026 15:44
@Marchhill Marchhill merged commit 871e9c7 into master Mar 23, 2026
420 checks passed
@Marchhill Marchhill deleted the fix/aura-merge-engine branch March 23, 2026 16:20
benaadams added a commit that referenced this pull request Mar 24, 2026
commit 5a6c779
Author: Amirul Ashraf <asdacap@gmail.com>
Date:   Tue Mar 24 09:46:36 2026 +0800

    fix(flat): periodically clear ReadOnlySnapshotBundle cache (#10922)

    * fix(flat): periodically clear ReadOnlySnapshotBundle cache to prevent stale readers

    The snapshot bundle cache was only cleared on compaction/persistence events.
    If persistence stalled, old entries held RefCountingPersistenceReader leases
    indefinitely, preventing database compaction. Add a 15-second periodic timer
    to force-clear stale cache entries.

    Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

    * test: add unit test for periodic bundle cache clearing

    Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

    * refactor: iterate ConcurrentDictionary directly instead of copying keys

    Address PR review feedback: use TryRemove while iterating the
    ConcurrentDictionary directly, avoiding the temporary key list copy.

    Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

    * refactor: remove IsEmpty check as it acquires all bucket locks

    Address review feedback: ConcurrentDictionary.IsEmpty acquires all
    bucket locks, making it more expensive than just iterating directly.

    Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

    ---------

    Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
    Co-authored-by: Ben {chmark} Adams <thundercat@illyriad.co.uk>

commit d9e819e
Author: Ben {chmark} Adams <thundercat@illyriad.co.uk>
Date:   Mon Mar 23 22:42:46 2026 +0000

    test: update pyspec fixtures to v5.4.0/v5.5.1 and adapt to forked release layout (#10931)

    * Adapt pyspec fixtures to forked release layout

    * Fix Amsterdam SSTORE gas ordering

    * fix: add missing usings for TestItem and InsertCode extension in Eip8037 test

    * Restore prestate for invalid access-list state tests

    * fix: add missing usings and fix Ether extension in pre-Berlin access list test

    * hmm

    * fix: detect AccessList tx type by field presence, not list emptiness

    JsonToEthereumTest.Convert set TxType.AccessList only when the built
    access list was non-empty. Pyspec fixtures with empty accessLists: [[]]
    were misclassified as legacy txs, so pre-Berlin rejection didn't fire
    and the post-state root diverged.

    Check whether the accessLists/accessList JSON field was present rather
    than whether the parsed list has entries. Rebuild regression test
    programmatically using the expected hash from pyspec fixture
    test_eip2930_tx_validity[fork_Istanbul-invalid-state_test].

    * test: add Convert regression test for empty accessLists field detection

commit 057441c
Author: Gaurav Dhiman <newmanifold000@gmail.com>
Date:   Tue Mar 24 04:02:29 2026 +0530

    Fix buffer leak tests to use PooledBufferLeakDetector (#10887)

commit ae8e0ee
Author: Tomass <155266802+zeroprooff@users.noreply.github.com>
Date:   Mon Mar 23 22:32:03 2026 +0000

    Remove duplicate assertion in SnapshotCompactorTests (#10923)

    Co-authored-by: Ben {chmark} Adams <thundercat@illyriad.co.uk>

commit d4214dd
Author: Marc <Marchhill@users.noreply.github.com>
Date:   Mon Mar 23 17:59:29 2026 +0000

    Gnosis Osaka (#10906)

    osaka gnosis config

    Co-authored-by: Marc Harvey-Hill <10379486+Marchhill@users.noreply.github.com>

commit 4228cb3
Author: Alexey Osipov <me@flcl.me>
Date:   Mon Mar 23 20:50:00 2026 +0300

    Dispose on exception (#10921)

    * Dispose more

    * Dispose in rare case

    * Catch more; cleanup

    ---------

    Co-authored-by: Ben {chmark} Adams <thundercat@illyriad.co.uk>

commit 871e9c7
Author: Marc <Marchhill@users.noreply.github.com>
Date:   Mon Mar 23 16:20:47 2026 +0000

    Fix AuRaMergeEngineModuleTests (#10872)
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.

3 participants