Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @alpe's task in 1m 53s —— View job Code Review
SummaryThis PR tightens the staleness check for in-progress ( The Issues1. Missing test: inputs match but EL payload is stale ( The "stale_payload_when_inputs_match": {
execMetaTimestamp: 1700000012,
execMetaTxs: [][]byte{[]byte("tx-1")},
requestedTxs: [][]byte{[]byte("tx-1")},
requestedTime: time.Unix(1700000012, 0),
expectFound: false,
expectPayloadID: false,
expectGetPayloads: 1, // tried but failed
// requires payloads map to NOT contain the payloadID
},2. Log message missing tx hash context ( When the mismatch is due to differing tx hashes (not timestamps), the log field names are misleading — only c.logger.Debug().
Uint64("height", height).
Int64("execmeta_timestamp", execMeta.Timestamp).
Int64("requested_timestamp", timestamp.Unix()).
Bool("timestamp_match", execMeta.Timestamp == timestamp.Unix()).
Bool("txhash_match", bytes.Equal(execMeta.TxHash, requestedTxHash)).
Msg("ExecuteTxs: ignoring stale in-progress execution for different block inputs")Observations (non-blocking)
Test isolation: Overall: This is a focused, well-reasoned correctness fix. The refactored |
alpe
changed the title
alpe
changed the title
📝 WalkthroughWalkthroughThe changes add improved validation for cached execution metadata during reconciliation. When resuming a started execution, the system now verifies that the execution timestamp and transaction set match the stored metadata before attempting to retrieve the payload, reducing the risk of stale cached data being used. Supporting changes include a refactored transaction hash computation helper and comprehensive test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3221 +/- ##
=======================================
Coverage 61.43% 61.43%
=======================================
Files 120 120
Lines 12504 12504
=======================================
Hits 7682 7682
Misses 3960 3960
Partials 862 862
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
lgtm! this makes #3172 more readable as well so great
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
execution/evm/execution_reconcile_test.go (1)
19-57: Use a slice-backed table so the subtests run in a stable order.
rangeover a map randomizes the case order, which makes failures a bit noisier to reproduce. A slice keeps this table-driven without losing determinism.As per coding guidelines, "Use table-driven tests in Go unit tests" and "Ensure tests are deterministic in Go."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/execution_reconcile_test.go` around lines 19 - 57, The test currently builds specs as a map (variable specs) and ranges over it, which randomizes subtest order; change specs to a slice of structs that include a name field (e.g., type case struct { name string; execMetaTimestamp int64; execMetaTxs [][]byte; requestedTxs [][]byte; requestedTime time.Time; expectFound bool; expectPayloadID bool; expectGetPayloads int }) and populate it in the desired stable order, then iterate over that slice (for _, spec := range cases { t.Run(spec.name, func(t *testing.T) { ... }) }) instead of ranging the map so subtests run deterministically while keeping the same assertions and using the same symbols (specs -> cases, t.Run).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 333: Remove the orphan list marker '*' from the links footer in
CHANGELOG.md (the stray asterisk that renders as an empty bullet); open
CHANGELOG.md, locate the lone '*' in the footer area and delete that line so the
list renders correctly without the empty bullet.
- Around line 12-14: Move the entry "chore: Better check for stored meta not
stale" out of the "### Added" section into the "### Fixed" section and rewrite
it in user-facing language that explains the bug and the user impact (e.g.,
"Fixed stale execution-resume behavior by improving validation of stored
metadata to prevent resuming with outdated state"). Update the changelog entry
to remove commit-style phrasing and use a concise, user-first sentence
describing the fix and its effect.
In `@execution/evm/execution.go`:
- Around line 758-785: The resume path for ExecStageStarted currently only
checks timestamp and tx hash (execMeta.Timestamp, execMeta.TxHash via
hashTxs(txs)) before returning the stored PayloadID (pid); you must also
validate that the current parent hash matches execMeta.ParentHash to avoid
resuming on a changed parent. Retrieve the current parent hash for the block
(height-1) from the chain/headers API available in this package and compare it
to execMeta.ParentHash; if they differ, treat the payload as stale (log and fall
through to fresh execution) rather than returning pid even if
c.engineClient.GetPayload(ctx, pid) succeeds. Ensure you still keep the existing
GetPayload existence check (c.engineClient.GetPayload) but only return pid when
both the payload exists and the parent hash matches execMeta.ParentHash.
- Around line 1048-1059: The hashTxs function currently concatenates raw tx
bytes which can produce ambiguous collisions; modify hashTxs to make the tx-list
encoding unambiguous by prefixing each tx with its length (or hashing each tx
separately with a domain separator) before writing into the sha256 hasher: e.g.,
for each tx in hashTxs write the tx length as a fixed-size varint/uint64 or a
clear separator and then write the tx bytes (or write sha256(tx) with a
separator), then return h.Sum(nil) so the digest uniquely represents the ordered
list.
---
Nitpick comments:
In `@execution/evm/execution_reconcile_test.go`:
- Around line 19-57: The test currently builds specs as a map (variable specs)
and ranges over it, which randomizes subtest order; change specs to a slice of
structs that include a name field (e.g., type case struct { name string;
execMetaTimestamp int64; execMetaTxs [][]byte; requestedTxs [][]byte;
requestedTime time.Time; expectFound bool; expectPayloadID bool;
expectGetPayloads int }) and populate it in the desired stable order, then
iterate over that slice (for _, spec := range cases { t.Run(spec.name, func(t
*testing.T) { ... }) }) instead of ranging the map so subtests run
deterministically while keeping the same assertions and using the same symbols
(specs -> cases, t.Run).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e12b9912-7b63-430f-b6da-57e2d9c96b2c
📒 Files selected for processing (3)
CHANGELOG.mdexecution/evm/execution.goexecution/evm/execution_reconcile_test.go
| ### Added | ||
|
|
||
| * chore: Better check for stored meta not stale [#3221](https://github.com/evstack/ev-node/pull/3221) |
There was a problem hiding this comment.
Move this entry under Fixed and rewrite it user-first.
This change hardens stale execution-resume behavior, so it reads like a bug fix rather than a new feature, and chore: Better check for stored meta not stale is still commit-title/internal wording instead of changelog wording.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 12 - 14, Move the entry "chore: Better check for
stored meta not stale" out of the "### Added" section into the "### Fixed"
section and rewrite it in user-facing language that explains the bug and the
user impact (e.g., "Fixed stale execution-resume behavior by improving
validation of stored metadata to prevent resuming with outdated state"). Update
the changelog entry to remove commit-style phrasing and use a concise,
user-first sentence describing the fix and its effect.
| <!-- Links --> | ||
|
|
||
| - | ||
| * |
There was a problem hiding this comment.
Remove the orphan list marker in the links footer.
Line 333 renders as an empty bullet and looks like a leftover from the list-marker normalization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 333, Remove the orphan list marker '*' from the links
footer in CHANGELOG.md (the stray asterisk that renders as an empty bullet);
open CHANGELOG.md, locate the lone '*' in the footer area and delete that line
so the list renders correctly without the empty bullet.
| if execMeta.Stage == ExecStageStarted && len(execMeta.PayloadID) == 8 { | ||
| var pid engine.PayloadID | ||
| copy(pid[:], execMeta.PayloadID) | ||
| requestedTxHash := hashTxs(txs) | ||
| if execMeta.Timestamp != timestamp.Unix() || !bytes.Equal(execMeta.TxHash, requestedTxHash) { | ||
| c.logger.Debug(). | ||
| Uint64("height", height). | ||
| Int64("execmeta_timestamp", execMeta.Timestamp). | ||
| Int64("requested_timestamp", timestamp.Unix()). | ||
| Msg("ExecuteTxs: ignoring stale in-progress execution for different block inputs") | ||
| } else { | ||
| var pid engine.PayloadID | ||
| copy(pid[:], execMeta.PayloadID) | ||
|
|
||
| // Validate payload still exists by attempting to retrieve it | ||
| if _, err = c.engineClient.GetPayload(ctx, pid); err == nil { | ||
| c.logger.Info(). | ||
| // Validate payload still exists by attempting to retrieve it | ||
| if _, err = c.engineClient.GetPayload(ctx, pid); err == nil { | ||
| c.logger.Info(). | ||
| Uint64("height", height). | ||
| Str("stage", execMeta.Stage). | ||
| Msg("ExecuteTxs: found in-progress execution with payloadID, returning payloadID for resume") | ||
| return nil, &pid, true, nil | ||
| } | ||
| // Payload is stale (expired or node restarted) - proceed with fresh execution | ||
| c.logger.Debug(). | ||
| Uint64("height", height). | ||
| Str("stage", execMeta.Stage). | ||
| Msg("ExecuteTxs: found in-progress execution with payloadID, returning payloadID for resume") | ||
| return nil, &pid, true, nil | ||
| Str("payloadID", pid.String()). | ||
| Err(err). | ||
| Msg("ExecuteTxs: stale ExecMeta payloadID no longer valid in EL, will re-execute") | ||
| // Don't return - fall through to fresh execution | ||
| } |
There was a problem hiding this comment.
ExecStageStarted resumes still ignore parent-chain changes.
This gate only checks timestamp and tx hash before reusing pid. After a rollback/reorg, the parent at height-1 can change while both still match, so we can resume and later promote a payload that was built on the old parent. Please also validate the current parent hash before returning pid; ExecMeta already has a ParentHash field you can use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/evm/execution.go` around lines 758 - 785, The resume path for
ExecStageStarted currently only checks timestamp and tx hash
(execMeta.Timestamp, execMeta.TxHash via hashTxs(txs)) before returning the
stored PayloadID (pid); you must also validate that the current parent hash
matches execMeta.ParentHash to avoid resuming on a changed parent. Retrieve the
current parent hash for the block (height-1) from the chain/headers API
available in this package and compare it to execMeta.ParentHash; if they differ,
treat the payload as stale (log and fall through to fresh execution) rather than
returning pid even if c.engineClient.GetPayload(ctx, pid) succeeds. Ensure you
still keep the existing GetPayload existence check (c.engineClient.GetPayload)
but only return pid when both the payload exists and the parent hash matches
execMeta.ParentHash.
| func hashTxs(txs [][]byte) []byte { | ||
| if len(txs) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| h := sha256.New() | ||
| for _, tx := range txs { | ||
| h.Write(tx) | ||
| } | ||
|
|
||
| return h.Sum(nil) | ||
| } |
There was a problem hiding this comment.
Make the tx-list hash unambiguous.
hashTxs hashes the raw concatenation of every tx, so different [][]byte values can collide when they flatten to the same byte stream. That weakens the stale-meta guard this PR is adding. Hash each element independently or include a separator/length per element.
Possible fix
func hashTxs(txs [][]byte) []byte {
if len(txs) == 0 {
return nil
}
h := sha256.New()
for _, tx := range txs {
- h.Write(tx)
+ txHash := sha256.Sum256(tx)
+ h.Write(txHash[:])
}
return h.Sum(nil)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func hashTxs(txs [][]byte) []byte { | |
| if len(txs) == 0 { | |
| return nil | |
| } | |
| h := sha256.New() | |
| for _, tx := range txs { | |
| h.Write(tx) | |
| } | |
| return h.Sum(nil) | |
| } | |
| func hashTxs(txs [][]byte) []byte { | |
| if len(txs) == 0 { | |
| return nil | |
| } | |
| h := sha256.New() | |
| for _, tx := range txs { | |
| txHash := sha256.Sum256(tx) | |
| h.Write(txHash[:]) | |
| } | |
| return h.Sum(nil) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/evm/execution.go` around lines 1048 - 1059, The hashTxs function
currently concatenates raw tx bytes which can produce ambiguous collisions;
modify hashTxs to make the tx-list encoding unambiguous by prefixing each tx
with its length (or hashing each tx separately with a domain separator) before
writing into the sha256 hasher: e.g., for each tx in hashTxs write the tx length
as a fixed-size varint/uint64 or a clear separator and then write the tx bytes
(or write sha256(tx) with a separator), then return h.Sum(nil) so the digest
uniquely represents the ordered list.
2 participants
Overview
Enhanced stale block detection during failover to prevent resuming outdated execution states.
Summary by CodeRabbit