Skip to content

test: cover execution payload envelope withdrawals mismatch#9252

Closed
lodekeeper wants to merge 1 commit intoChainSafe:unstablefrom
lodekeeper:test/process-execution-payload-envelope-coverage
Closed

test: cover execution payload envelope withdrawals mismatch#9252
lodekeeper wants to merge 1 commit intoChainSafe:unstablefrom
lodekeeper:test/process-execution-payload-envelope-coverage

Conversation

@lodekeeper
Copy link
Copy Markdown
Contributor

Summary

Adds the first direct unit coverage for processExecutionPayloadEnvelope() in packages/state-transition. Previously there were zero tests exercising this state-transition seam.

Two assertions:

  1. A payload whose withdrawals hash matches state.payloadExpectedWithdrawals processes cleanly and caches the payload hash on the state.
  2. A payload whose withdrawals_root differs from state.payloadExpectedWithdrawals throws immediately from processExecutionPayloadEnvelope().

Context

Surfaced from the v1.42.0 "Withdrawal mismatch at index=0" investigation. The cache-aliasing root cause is addressed in #9246; this PR is test-only coverage pinning the state-transition-side contract so a regression at this seam would fail an isolated unit test, independent of the loadState cache path.

The test uses a minimal mocked Gloas state surface rather than full cached-state construction, since the seam only needs a small subset of CachedBeaconStateGloas when signature/state-root verification are disabled.

Test plan

  • pnpm lint packages/state-transition/test/unit/block/processExecutionPayloadEnvelope.test.ts
  • pnpm test:unit packages/state-transition/test/unit/block/processExecutionPayloadEnvelope.test.ts → 2/2 passing
  • Adjacent processWithdrawals.test.ts unaffected → 7/7 passing

AI assistance

Drafted and validated with AI assistance.

@lodekeeper lodekeeper requested a review from a team as a code owner April 21, 2026 18:06
@nflaig
Copy link
Copy Markdown
Member

nflaig commented Apr 21, 2026

@lodekeeper these don't seem useful, closing

@nflaig nflaig closed this Apr 21, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces unit tests for the processExecutionPayloadEnvelope function, covering successful payload processing and error handling for withdrawal mismatches. The review feedback recommends replacing the magic number 8192 with the SLOTS_PER_HISTORICAL_ROOT constant from @lodestar/params to improve code robustness and maintainability across different network configurations.

@@ -0,0 +1,102 @@
import {describe, expect, it, vi} from "vitest";
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.

medium

Import SLOTS_PER_HISTORICAL_ROOT from @lodestar/params to avoid using magic numbers in the test assertions. This ensures the test remains correct across different network presets (e.g., mainnet vs minimal).

Suggested change
import {describe, expect, it, vi} from "vitest";
import {describe, expect, it, vi} from "vitest";
import {SLOTS_PER_HISTORICAL_ROOT} from "@lodestar/params";

});

expect(result.latestBlockHash).toEqual(blockHash);
expect(result.executionPayloadAvailability.set).toHaveBeenCalledWith(result.slot % 8192, true);
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.

medium

Use the SLOTS_PER_HISTORICAL_ROOT constant instead of the hardcoded magic number 8192. This aligns the test with the implementation in processExecutionPayloadEnvelope.ts and makes it more robust.

Suggested change
expect(result.executionPayloadAvailability.set).toHaveBeenCalledWith(result.slot % 8192, true);
expect(result.executionPayloadAvailability.set).toHaveBeenCalledWith(result.slot % SLOTS_PER_HISTORICAL_ROOT, true);

@lodekeeper
Copy link
Copy Markdown
Contributor Author

Ack, thanks — understood.

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