Skip to content

eth/executionclient: order packed logs by logIndex within a transaction - #2993

Open
iurii-ssv wants to merge 1 commit into
stagefrom
fix/packlogs-within-tx-log-order
Open

eth/executionclient: order packed logs by logIndex within a transaction#2993
iurii-ssv wants to merge 1 commit into
stagefrom
fix/packlogs-within-tx-log-order

Conversation

@iurii-ssv

Copy link
Copy Markdown
Contributor

Summary

PackLogs sorted logs by (block, txIndex) using a non-stable sort.Slice, so logs emitted by the same transaction (same txIndex) were left in unspecified relative order.

A single transaction can emit multiple order-dependent registry events: bulkRegisterValidator emits one ValidatorAdded per validator, each bumping the owner's per-owner nonce. If those get packed out of logIndex order, the event handler's GetNextNonce/BumpNonce sequence no longer matches the events' signed nonces → verifySignature fails → MalformedEventError (and the nonce is still bumped), silently rejecting otherwise-valid registrations.

It's usually masked in practice — Go's sort.Slice is stable for small slices — but it's unsound and can surface with a larger batch or a runtime change.

Fix

Add a logIndex (Index) tiebreaker so PackLogs sorts by (block, txIndex, logIndex)logIndex is unique and monotonic within a block, so packing now preserves canonical on-chain order both across and within a transaction. Existing (block, txIndex) behavior is unchanged for logs in distinct transactions.

Regression test added with shuffled same-transaction logs.

Noticed during review of #2991.

PackLogs sorted by (block, txIndex) with a non-stable sort.Slice, leaving
logs emitted by the same transaction (same txIndex) in unspecified relative
order. A single transaction can emit multiple order-dependent registry
events — bulkRegisterValidator emits one ValidatorAdded per validator, each
bumping the owner's per-owner nonce — so reordering them makes the event
handler read nonces out of order and reject otherwise-valid registrations
(MalformedEventError, with the nonce still bumped).

This is usually masked (Go's sort is stable in practice for small inputs)
but is unsound and can surface with larger batches or a runtime change.

Add a logIndex tiebreaker so packing preserves canonical on-chain order
both across and within a transaction, plus a regression test with shuffled
same-transaction logs.
@iurii-ssv
iurii-ssv requested review from a team as code owners August 14, 2026 09:51
@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes execution-log packing preserve canonical on-chain order within each transaction and adds regression coverage for shuffled same-transaction logs.

  • Sorts logs by block number, transaction index, and log index.
  • Verifies that same-transaction logs are packed in ascending log-index order.

Confidence Score: 5/5

The PR appears safe to merge, with the new comparator consistently preserving canonical event order across historical and streaming fetch paths.

The production fetch paths populate block, transaction, and log indices before calling PackLogs, and the grouping logic preserves the resulting order; no blocking or independently actionable issue remains.

Important Files Changed

Filename Overview
eth/executionclient/logs.go Adds the log-index tiebreaker needed to preserve canonical ordering before block-level event processing.
eth/executionclient/logs_test.go Adds focused regression coverage for shuffled logs from the same transaction.

Reviews (1): Last reviewed commit: "eth/executionclient: order packed logs b..." | Re-trigger Greptile

@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.2%. Comparing base (48d4f3a) to head (de7575b).

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ovidiu-ssv-labs ovidiu-ssv-labs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correct, minimal, well-targeted fix: adding the logIndex tiebreaker makes PackLogs produce true canonical on-chain order, which the nonce-sequenced ValidatorAdded handler depends on. Verified the new regression test actually fails against the pre-PR comparator, and that the bug was deterministic (not just theoretical) for same-tx batches above ~12 logs. One minor follow-up: a second copy of the old comparator still lives in bloom.go. [verdict: yes]

Finding 1 · [MINOR] Deduplicate the log comparator — bloom.go still sorts by (block, txIndex) onlyeth/executionclient/bloom.go:86

verifyLogsWithBloom re-sorts after appending bloom-recovered logs using a comparator that is the exact pre-PR version this PR just fixed — (block, txIndex) only, no logIndex tiebreak. No impact today: verifyLogsWithBloom has exactly one caller, and its output flows straight into PackLogs, which now re-sorts with the full corrected comparator — so the weaker sort here is currently harmless and redundant.

It still matters because the codebase now has two comparators over the same data that disagree, and the weaker one sits directly in the path that appends recovered logs — precisely the operation that destroys incoming order. If verifyLogsWithBloom is ever reused outside the PackLogs path, or these two steps are reordered, the exact nonce-mismatch failure this PR fixes silently returns — and would only trigger on the rare bloom-recovery path, making it extremely hard to reproduce. For scale: the pre-PR comparator, run over 40 same-tx logs in reverse order, returned them still fully reversed — above ~12 elements Go's sort.Slice is not merely unspecified, it's deterministically wrong.

@ovidiu-ssv-labs ovidiu-ssv-labs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 minor comment only, see above.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants