op-acceptance-tests: add test to base gate for unsafe gossip / head progression by geoknee · Pull Request #18031 · ethereum-optimism/optimism

geoknee · 2025-10-27T12:46:25Z

We recently realised that acceptance tests do not give us good coverage for unsafe head progression via p2p gossip. See, for example, #17940 (a bug which wasn't caught by ATs) as well as a recent op-reth bug we only caught on devnet.

This test adds that coverage, and simply asserts that a) there are some non-sequencer verifier nodes and b) they catch up to the sequencer within a timeout period.

The test disables the sysgo batcher, because otherwise safe chain derivation would allow the test to pass even if gossip was broken. Therefore this test should not run against external networks.

I confirmed manually that introducing a bug, where op-node just rejects gossip payloads unconditionally, does indeed cause this test to fail.

geoknee · 2025-10-27T12:50:03Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+	// Stop batcher so there is no sync from deriving from L1
+	// This won't work on external networks.
+	sys.L2Batcher.Stop()


If this test doesn't make sense to run on external networks, how does the acceptance test framework know to skip it? Is there some helper we should call, or preset we should use for that? @scharissis

You'd do this with presets.WithCompatibleTypes(compat.SysGo) in your TestMain.
Good call-out.

Can this be done per test or only per package?

I believe only per package currently.

Ok in that case I should probably pull this out into it's own package, since I don't want to avoid running the neighbouring tests on external devnets.

What prevents it from being run on an external devnet?

It's the control of the batcher, see other thread #18031 (comment). I think we could potentially work towards it running on an external network, if we can be sure that it is safe (in case someone points it at a production network) and won't interfere with other tests.

geoknee · 2025-10-27T12:52:53Z

op-acceptance-tests/tests/base/chain/init_test.go

 func TestMain(m *testing.M) {
 	presets.DoMain(m,
-		presets.WithMinimal(),
+		presets.WithSingleChainMultiNode(),


Should we be using this for all base tests? Does it give us extra coverage in existing tests?

Quite possibly, yes!

Also, this is how you'd mark this test to be skipped on external devnets:

Suggested change

presets.WithSingleChainMultiNode(),

presets.WithSingleChainMultiNode(),

presets.WithCompatibleTypes(compat.SysGo),

geoknee · 2025-10-27T16:57:08Z

Similar tests exist in an unmerged PR here https://github.com/ethereum-optimism/optimism/pull/17751/files#diff-58dd5c61df58d9692977d4999df0077508b5575933bfe4e81c50a217bcdbe3ddR14 and here https://github.com/ethereum-optimism/optimism/pull/17751/files#diff-58dd5c61df58d9692977d4999df0077508b5575933bfe4e81c50a217bcdbe3ddR59

scharissis · 2025-10-27T22:19:18Z

op-acceptance-tests/tests/base/chain/init_test.go

 func TestMain(m *testing.M) {
 	presets.DoMain(m,
-		presets.WithMinimal(),
+		presets.WithSingleChainMultiNode(),


Quite possibly, yes!

Also, this is how you'd mark this test to be skipped on external devnets:

Suggested change

presets.WithSingleChainMultiNode(),

presets.WithSingleChainMultiNode(),

presets.WithCompatibleTypes(compat.SysGo),

scharissis · 2025-10-27T22:19:56Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+	// Stop batcher so there is no sync from deriving from L1
+	// This won't work on external networks.
+	sys.L2Batcher.Stop()


You'd do this with presets.WithCompatibleTypes(compat.SysGo) in your TestMain.
Good call-out.

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

theochap · 2025-10-27T22:38:04Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+			indexOfSequencer = i
+			break
+		}
+	}


nit: Instead of doing that check manually, can't we add a special case in the preset for the L2ELSequencer and the L2ELValidator?
No need to do it in this PR, I don't think it's worth blocking that change

match.WithSequencerActive+match.EngineFor is probably what we want here.

theochap · 2025-10-27T22:41:22Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+			verifierUnsafeHead := el.ChainSyncStatus(l2chainID, types.LocalUnsafe)
+			if verifierUnsafeHead.Number < initialUnsafeHeadNumber+NUM_UNSAFE_BLOCKS {
+				return false
+			}


Can we use el.AdvancedFn or el.ReachedFn here, and wait in parallel using the CheckAll method?

quite possibly! I can have a play with those utils.

joshklop · 2025-10-27T22:57:36Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+	sys := presets.NewSingleChainMultiNode(t)
+
+	// Stop batcher so there is no sync from deriving from L1
+	// This won't work on external networks.


From what I can tell, it calls admin_stopBatcher. If we enable the admin namespace, can we run this on external networks?

In principle, yes -- but I think that admin_stopBatcher is quite destructive and the batcher would need to be manually restarted (to restore the chain to a state where it can run additional ATs) with the current implementation.

joshklop · 2025-10-27T23:23:14Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+	const TIMEOUT = 5 * time.Second // time to wait for all verifier nodes to catch up to the sequencer
+
+	// In order for this test to be valid, we need at least 2 L2 EL nodes.
+	t.Require().Greater(len(sys.L2Chain.L2ELNodes()), 1, "expected at least 2 L2 EL nodes")


This should already be guaranteed by the preset when DEVNET_EXPECT_PRECONDITIONS_MET is true (the default).

OK. I'm tempted to leave this in as a sanity check / protect against regressions though.

joshklop · 2025-10-27T23:24:25Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+			indexOfSequencer = i
+			break
+		}
+	}


match.WithSequencerActive+match.EngineFor is probably what we want here.

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

joshklop · 2025-10-27T23:26:25Z

op-acceptance-tests/tests/base/chain/unsafe_sync_test.go

+				continue
+			}
+			verifierUnsafeHead := el.ChainSyncStatus(l2chainID, types.LocalUnsafe)
+			if verifierUnsafeHead.Number < initialUnsafeHeadNumber+NUM_UNSAFE_BLOCKS {


Should we check if the hash matches? Not sure if it matters in this case.

We have Matched and MatchedFn that checks both hash and number. We can just use it.

opgitgovernance · 2025-11-14T16:45:49Z

This pr has been automatically marked as stale and will be closed in 5 days if no updates

opgitgovernance · 2025-12-03T16:46:27Z

This pr has been automatically marked as stale and will be closed in 5 days if no updates

opgitgovernance · 2025-12-22T16:46:17Z

This pr has been automatically marked as stale and will be closed in 5 days if no updates

geoknee added 4 commits October 27, 2025 11:05

WIP

1252674

fix

c7d7be1

simplify

f405ee5

turn of batcher

de8e1b2

geoknee commented Oct 27, 2025

View reviewed changes

geoknee added the H-jovian Hardfork: change is planned for jovian upgrade label Oct 27, 2025

geoknee changed the title ~~acceptance test for unsafe gossip~~ op-acceptance-tests: add test to base gate for unsafe gossip / head progression Oct 27, 2025

scharissis reviewed Oct 27, 2025

View reviewed changes

theochap reviewed Oct 27, 2025

View reviewed changes

fixes

13272ec

joshklop reviewed Oct 27, 2025

View reviewed changes

opgitgovernance added the S-stale Status: Will be closed unless there is activity label Nov 14, 2025

geoknee removed the S-stale Status: Will be closed unless there is activity label Nov 19, 2025

opgitgovernance added the S-stale Status: Will be closed unless there is activity label Dec 3, 2025

geoknee removed the S-stale Status: Will be closed unless there is activity label Dec 8, 2025

opgitgovernance added the S-stale Status: Will be closed unless there is activity label Dec 22, 2025

opgitgovernance closed this Jan 11, 2026

	presets.WithSingleChainMultiNode(),
	presets.WithSingleChainMultiNode(),
	presets.WithCompatibleTypes(compat.SysGo),

Conversation

geoknee commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshklop Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoknee commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opgitgovernance commented Nov 14, 2025

Uh oh!

opgitgovernance commented Dec 3, 2025

Uh oh!

opgitgovernance commented Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

geoknee commented Oct 27, 2025 •

edited

Loading

joshklop Oct 30, 2025 •

edited

Loading