Skip to content

Attempt to fix progression halting via tainted SyncStatus #435

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.

Sign up for GitHub

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

Open
Ayiga wants to merge 1 commit into
base: celo-integration-rebase-17
Choose a base branch
from
Open

Attempt to fix progression halting via tainted SyncStatus #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions espresso/environment/8_reorg_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,8 @@ func TestE2eDevnetWithL1Reorg(t *testing.T) {
if have, want := err, error(nil); have != want {
t.Fatalf("failed to start dev environment with espresso dev node:\nhave:\n\t\"%v\"\nwant:\n\t\"%v\"\n", have, want)
}
defer env.Stop(t, system)
defer env.Stop(t, devNode)

caffNode, err := env.LaunchCaffNode(t, system, devNode)
if have, want := err, error(nil); have != want {
Expand All @@ -254,3 +256,35 @@ func TestE2eDevnetWithL1Reorg(t *testing.T) {

runL1Reorg(ctx, t, system)
}

// TestE2eEspressoStreamerSyncStatusTainting is a test that is meant to ensure
// that a tainted `SyncStatus` coming in to set the `safeBatchNumber` of the
// `Refresh` call to something like `0`, will ultimately not result in halting
// the progression of the chain through `Espresso`.
func TestE2eEspressoStreamerSyncStatusTainting(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

launcher := new(env.EspressoDevNodeLauncherDocker)

system, devNode, err := launcher.StartE2eDevnet(ctx, t)
require.NoError(t, err)

defer env.Stop(t, system)
defer env.Stop(t, devNode)

l2Seq := system.NodeClient(e2esys.RoleSeq)
// l1Client := system.NodeClient(e2esys.RoleL1)
// caffClient := system.NodeClient(env.RoleCaffNode)
Comment on lines +276 to +278

@claude claude Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: these commented-out client declarations suggest the test may be incomplete or these were used during development. If they're planned for future use, that's fine; otherwise consider removing them to keep the test clean.

geth.WaitForBlockToBeFinalized(big.NewInt(5), l2Seq, time.Minute*2)

l2Node := system.RollupClient(e2esys.RoleSeq)

status, err := l2Node.SyncStatus(ctx)
require.NoError(t, err)

err = system.BatchSubmitter.EspressoStreamer().Refresh(ctx, status.FinalizedL1, 0, status.FinalizedL2.L1Origin)

@claude claude Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good test — this directly exercises the tainted SyncStatus path by calling Refresh with safeBatchNumber = 0. The subsequent WaitForBlockToBeFinalized with status.UnsafeL2.Number + 2 validates that the chain continues to progress despite the tainted refresh.

One consideration: with a 5-minute timeout and only needing 2 more blocks finalized, this should be robust. But if the devnet is slow to start, the initial WaitForBlockToBeFinalized(5) with a 2-minute timeout on line 279 might be tight. Worth monitoring in CI.

require.NoError(t, err)

geth.WaitForBlockToBeFinalized(big.NewInt(int64(status.UnsafeL2.Number)+2), l2Seq, time.Minute*5)
}
64 changes: 48 additions & 16 deletions op-batcher/batcher/espresso.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,28 @@ func (l *BatchSubmitter) espressoSyncAndRefresh(ctx context.Context, newSyncStat
}
}

// skipStreamerToTargetBlockHeight will consume batches from the
// `EspressoStreamer` until we reach the target block height, or run out
// of batches to consume.
//
// NOTE: this is **NOT** guaranteed to ensure that the next `Batch` will
// be the expected height, it's just a best effort.
func (l *BatchSubmitter) skipStreamerToTargetBlockHeight(ctx context.Context, targetHeight uint64) {
streamer := l.EspressoStreamer()
batch := streamer.Peek(ctx)
for batch != nil && batch.Number() < targetHeight {
// Consume the Batch
streamer.Next(ctx)
batch = streamer.Peek(ctx)
}
}

// isHashNoptSet is a helper function to check if a hash is the zero
Comment on lines +877 to +878

@claude claude Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: the doc comment says isHashNoptSet but the function is named isHashEmpty. The comment should be updated to match.

Suggested change
// isHashNoptSet is a helper function to check if a hash is the zero
// isHashEmpty is a helper function to check if a hash is the zero
// value (not set).

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a typo in the function documentation: isHashNoptSet should be isHashEmpty to match the function name.

Suggested change
// isHashNoptSet is a helper function to check if a hash is the zero
// isHashEmpty is a helper function to check if a hash is the zero

// value (not set).
func isHashEmpty(hash common.Hash) bool {
return hash == (common.Hash{})
}

// peekNextBatch returns the next batch from the streamer, performing a fork check
// against an expected parent hash.
//
Expand All @@ -867,26 +889,36 @@ func (l *BatchSubmitter) espressoSyncAndRefresh(ctx context.Context, newSyncStat
// the one position where we can set tip to the known safe head. Otherwise we accept the batch as-is.
func (l *BatchSubmitter) peekNextBatch(ctx context.Context, syncStatus *eth.SyncStatus) *derive.EspressoBatch {
l.channelMgrMutex.Lock()
tip := l.channelMgr.tip
var tipBlock SizedBlock
if len(l.channelMgr.blocks) > 0 {
tipBlock = l.channelMgr.blocks[len(l.channelMgr.blocks)-1]
Comment on lines +893 to +894

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is an inconsistency in how the length of l.channelMgr.blocks is accessed. In op-batcher/batcher/channel_manager.go (e.g., line 98 and 632), the code uses the .Len() method, whereas here it uses the built-in len() function. For consistency and to avoid potential compilation issues if queue.Queue is a struct type, it is recommended to use the .Len() method throughout the package.

Suggested change
if len(l.channelMgr.blocks) > 0 {
tipBlock = l.channelMgr.blocks[len(l.channelMgr.blocks)-1]
if l.channelMgr.blocks.Len() > 0 {
tipBlock = l.channelMgr.blocks[l.channelMgr.blocks.Len()-1]

}
l.channelMgrMutex.Unlock()

targetBlock := syncStatus.SafeL2.Number + 1
tipHash := syncStatus.SafeL2.Hash
if tipBlock != (SizedBlock{}) && tipBlock.Block != nil {
Comment on lines +898 to +900

@claude claude Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is the core logic change and the heart of the fix. In the old code, tip was read from channelMgr.tip — a hash that's only set after AddL2Block succeeds. After a Clear(), tip would be common.Hash{} and the only way to recover was if the peeked batch happened to be exactly SafeL2.Number + 1.

The new approach improves on this significantly:

  1. Uses the last block in channelMgr.blocks as the authoritative tip (since blocks is what actually tracks pending state)
  2. Falls back to SafeL2 when blocks is empty, immediately setting tipHash instead of requiring a second Peek check
  3. skipStreamerToTargetBlockHeight advances the streamer past already-known blocks

This should handle the tainted SyncStatus case well — if SafeL2.Number regresses to 0, targetBlock becomes 1, and skipStreamerToTargetBlockHeight won't skip anything meaningful if the streamer is already ahead, so the streamer just continues from where it is.

One question: when channelMgr.blocks is empty and the peeked batch number does not equal targetBlock (neither branch of the if/else if matches), we fall through with tipHash = syncStatus.SafeL2.Hash and targetBlock = syncStatus.SafeL2.Number + 1. The skip call will then consume batches up to targetBlock. But if the streamer is already well past SafeL2.Number + 1 (e.g., after a tainted SyncStatus resets SafeL2.Number to 0 then recovers), won't this skip consume batches we actually need?

I think this is fine because Refresh with safeBatchNumber=0 would trigger a streamer reset (per the interface: "This will only automatically reset the Streamer if the safeBatchNumber moves backwards"), so the streamer would be repositioned. But it'd be worth verifying that the skip + peek sequence is correct in the scenario where SyncStatus recovers from a tainted value between ticks.

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The check tipBlock != (SizedBlock{}) is redundant if tipBlock.Block != nil is also checked. Simplifying this check improves readability and ensures that we only proceed if we have a valid block pointer to access.

Suggested change
if tipBlock != (SizedBlock{}) && tipBlock.Block != nil {
if tipBlock.Block != nil {

targetBlock = tipBlock.NumberU64() + 1
tipHash = tipBlock.Hash()
} else if batch := l.EspressoStreamer().Peek(ctx); batch != nil && batch.Number() == targetBlock {

@claude claude Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This Peek call is done outside the mutex, then skipStreamerToTargetBlockHeight (line 915) and another Peek (line 916) follow. But since espressoBatchLoadingLoop calls peekNextBatch in a tight for loop (line 969-971) and then immediately AddL2Block (which updates blocks), there's a TOCTOU window: between reading blocks (line 893) and using the derived targetBlock for skipping (line 915), the loading loop could concurrently add blocks that change what targetBlock should be.

In practice this may be safe because peekNextBatch is only called from espressoBatchLoadingLoop (single goroutine), so the blocks can't change between reading them and using them here. But it's worth confirming — is peekNextBatch guaranteed to only be called from the single loading loop goroutine? If so, a brief comment noting the single-caller invariant would help future readers.

// Log indicating that we utilized the SafeL2 Hash as the next
// Streamer entry matched our expected Safe L2, and we didn't
// have any tip information from the Channel Manager.
l.Log.Debug(
"setting tip to safe l2 hash",
"batchNr", batch.Number(),
"batchParent", batch.Header().ParentHash.Hex(),
"tip", tipHash,
Comment on lines +909 to +911

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For consistency in logging, it is recommended to use .Hex() for all common.Hash types. This ensures that the output is always formatted as a hexadecimal string, matching the style used for batchParent on line 910.

Suggested change
"batchNr", batch.Number(),
"batchParent", batch.Header().ParentHash.Hex(),
"tip", tipHash,
"batchNr", batch.Number(),
"batchParent", batch.Header().ParentHash.Hex(),
"tip", tipHash.Hex(),

)
}

l.skipStreamerToTargetBlockHeight(ctx, targetBlock)
batch := l.EspressoStreamer().Peek(ctx)
if batch == nil {
return nil
}

// Check if we can set the tip if not set
if tip == (common.Hash{}) && (*batch).Number() == syncStatus.SafeL2.Number+1 {
l.Log.Info(
"setting tip to safe l2 hash",
"batchNr", (*batch).Number(),
"batchParent", (*batch).Header().ParentHash.Hex(),
"tip", tip,
)
tip = syncStatus.SafeL2.Hash
}

if tip == (common.Hash{}) {
if isHashEmpty(tipHash) {
l.Log.Warn(
"tip is not set, taking available batch",
"blockParentHash", (*batch).Header().ParentHash.Hex(),
Expand All @@ -895,14 +927,14 @@ func (l *BatchSubmitter) peekNextBatch(ctx context.Context, syncStatus *eth.Sync
return batch
}

if (*batch).Header().ParentHash != tip {
if batch.Header().ParentHash != tipHash {
l.Log.Warn(
"head batch fork mismatch, seeking to proper head",
"batchNr", (*batch).Number(),
"batchParent", (*batch).Header().ParentHash,
"tip", tip,
"tip", tipHash,
Comment on lines 933 to +935

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The logging here is inconsistent with previous log entries in the same function (e.g., line 910). It is recommended to use .Hex() for common.Hash types. Additionally, the dereference (*batch) is unnecessary in Go when calling methods on a pointer to a struct.

Suggested change
"batchNr", (*batch).Number(),
"batchParent", (*batch).Header().ParentHash,
"tip", tip,
"tip", tipHash,
"batchNr", batch.Number(),
"batchParent", batch.Header().ParentHash.Hex(),
"tip", tipHash.Hex(),

)
l.EspressoStreamer().SetProperHead(tip)
l.EspressoStreamer().SetProperHead(tipHash)
return nil
}

Expand Down
Loading