Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExecution layer now controls proposer rotation: ExecuteTxs returns an ExecuteResult (UpdatedStateRoot + NextProposerAddress). Node initialization and block production/validation use execution-provided next-proposer (with genesis fallback). Proposer checks moved from header-level/genesis checks into state-driven validations and persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Node (Produce/Apply)
participant Executor as ev-node Executor wrapper
participant Engine as Execution Engine / EngineClient
participant State as Persisted State
Client->>Executor: ProduceBlock(header, txs)
Executor->>State: Read NextProposerAddress (fallback genesis)
State-->>Executor: nextProposer
Executor->>Engine: ExecuteTxs(txs, prevStateRoot)
Engine-->>Executor: ExecuteResult{UpdatedStateRoot, NextProposerAddress}
Executor->>Executor: Use nextProposer for Header.Signer/ValidatorHasher
Executor->>State: Store UpdatedStateRoot and NextProposerAddress
Executor-->>Client: BlockProduced (signed/ready)
Client->>Executor: ApplyBlock(header, txs)
Executor->>Engine: ExecuteTxs(txs, prevStateRoot)
Engine-->>Executor: ExecuteResult{UpdatedStateRoot, NextProposerAddress}
Executor->>State: Verify header signer == previous State.NextProposerAddress
alt header.NextProposerAddress set
Executor->>Executor: Compare header.NextProposerAddress == ExecuteResult.NextProposerAddress
end
Executor->>State: Persist UpdatedStateRoot and NextProposerAddress
Executor-->>Client: BlockApplied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3282 +/- ##
==========================================
+ Coverage 61.84% 62.43% +0.59%
==========================================
Files 122 122
Lines 16241 13072 -3169
==========================================
- Hits 10044 8162 -1882
+ Misses 5312 4009 -1303
- Partials 885 901 +16
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
block/internal/executing/executor.go (1)
757-762:⚠️ Potential issue | 🟠 MajorPopulate the scheduled pubkey when producing unsigned based-sequencer blocks.
When
e.signer == nil, a schedule entry withPubKeyset still produces a header withSigner.PubKey == nil. That conflicts withValidateProposer, which rejects pinned schedule entries when the provided pubkey is missing.Proposed fix
} else { - validatorHash, err = e.options.ValidatorHasherProvider(proposer.Address, nil) + if len(proposer.PubKey) > 0 { + pubKey, err = proposer.PublicKey() + if err != nil { + return nil, nil, fmt.Errorf("failed to get scheduled proposer public key: %w", err) + } + } + + validatorHash, err = e.options.ValidatorHasherProvider(proposer.Address, pubKey) if err != nil { return nil, nil, fmt.Errorf("failed to get validator hash: %w", err) } }Also applies to: 782-785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/executing/executor.go` around lines 757 - 762, When producing unsigned based-sequencer blocks (the branch where e.signer == nil and you call e.options.ValidatorHasherProvider to compute validatorHash), ensure you also populate the header's Signer.PubKey from the schedule entry so pinned schedule entries are not missing a pubkey; specifically, after computing validatorHash in that branch set the header's Signer.PubKey (and any equivalent scheduled pubkey field) to the schedule entry's PubKey used to compute the hash. Apply the same fix in the second similar branch around the code at the other occurrence (the block referenced by the reviewer at 782-785).
🧹 Nitpick comments (3)
docs/adr/adr-023-proposer-key-rotation.md (1)
140-140: Minor wording nit."requires preplanned scheduling" reads awkwardly; consider rephrasing for clarity.
✏️ Proposed tweak
-- emergency rotation still requires preplanned scheduling or a later authority-based mechanism +- emergency rotation still requires scheduling to be planned in advance, or a later authority-based mechanism🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-023-proposer-key-rotation.md` at line 140, The sentence "emergency rotation still requires preplanned scheduling or a later authority-based mechanism" is awkwardly phrased; update that clause in adr-023 to clearer wording such as "emergency rotation still requires a preplanned schedule or, alternatively, a subsequent authority-based mechanism" or "emergency rotation still requires prior scheduling or a later authority-based mechanism"—locate the exact phrase and replace it with one of these clearer alternatives to improve readability.block/internal/submitting/da_submitter.go (1)
486-495: Schedule-aware signer check looks correct; consider fail-fast behavior.Validating the signer against the schedule per-height is right. One observation: if the signer becomes non-authoritative for a future height (e.g., a rotation boundary was crossed and this aggregator still holds pending data at the new height), the loop errors out on the first offending item and discards the whole batch, including already-built earlier entries. That's acceptable correctness-wise (we must not sign data we're not authorized for), but you may want to log the height and skip/stop more explicitly so operators can tell "rotation reached, stopping signer" apart from a config mismatch. Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/submitting/da_submitter.go` around lines 486 - 495, The loop currently returns an error from genesis.ValidateProposer when it encounters the first height the signer is no longer authorized for, discarding any prior entries; change this to fail-fast with an explicit log and stop-processing behavior so earlier built entries are preserved and operators can see a clear "rotation reached" message. Specifically, in the block that calls genesis.ValidateProposer(unsignedData.Height(), addr, pubKey) inside the unsignedDataList loop, replace the immediate return with a log statement that includes unsignedData.Height(), addr and pubKey (or a short identifier) and then break out of the loop (or otherwise stop further processing) so the function can return the already-accumulated results instead of an error; ensure the log message clearly states the signer lost authority due to schedule rotation.pkg/genesis/proposer_schedule.go (1)
80-95: Deep-copy schedule entry byte slices.
EffectiveProposerSchedulecopies the slice header only; callers can mutateAddressorPubKeyon returned entries and accidentally alterGenesis.ProposerSchedule.Proposed fix
if len(g.ProposerSchedule) > 0 { out := make([]ProposerScheduleEntry, len(g.ProposerSchedule)) - copy(out, g.ProposerSchedule) + for i, entry := range g.ProposerSchedule { + out[i] = ProposerScheduleEntry{ + StartHeight: entry.StartHeight, + Address: bytes.Clone(entry.Address), + PubKey: bytes.Clone(entry.PubKey), + } + } return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/genesis/proposer_schedule.go` around lines 80 - 95, EffectiveProposerSchedule currently only copies the slice header for Genesis.ProposerSchedule so returned ProposerScheduleEntry.Address and .PubKey still reference underlying byte slices; modify EffectiveProposerSchedule to deep-copy the byte slices for each entry when building out (iterate over g.ProposerSchedule, copy each ProposerScheduleEntry, and set entry.Address = bytes.Clone(entry.Address) and if entry.PubKey != nil set entry.PubKey = bytes.Clone(entry.PubKey)) and ensure the single-entry return path also clones any PubKey in addition to the existing bytes.Clone(g.ProposerAddress).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/syncing/p2p_handler.go`:
- Around line 84-86: Check that the height embedded in the incoming P2P header
matches the requested height before validating the proposer: call
p2pHeader.SignedHeader.Height() (or use header.Height() where applicable) and if
it differs from the requested variable height, log a debug/error via h.logger
(include both heights) and return an error immediately; only call
h.assertExpectedProposer(p2pHeader.SignedHeader) after the heights match. Apply
the same explicit height check in the other occurrence around the second
h.assertExpectedProposer call (the block at lines ~128-130).
In `@pkg/genesis/proposer_schedule_test.go`:
- Around line 31-37: Replace nondeterministic StartTime values in the Genesis
test fixtures by using a fixed, deterministic time constant (e.g., a
time.Date(...) value assigned to a variable like fixedGenesisTime) instead of
time.Now().UTC(); update the Genesis struct literal that sets StartTime (and any
other Genesis literals in this file such as the other fixtures) to use that
fixedGenesisTime so tests become deterministic while keeping fields like
ChainID, InitialHeight and ProposerSchedule unchanged.
In `@pkg/genesis/proposer_schedule.go`:
- Around line 152-181: The ValidateProposer function currently accepts any
provided pubKey when the schedule entry has no PubKey; change behavior so that
when entry.PubKey is empty but the caller passed a non-nil pubKey you must
derive the address from that pubKey (e.g., call pubKey.Address() or the
appropriate address-derivation on crypto.PubKey) and compare it to
entry.Address, returning an error if they differ; retain the existing path where
entry.PubKey is present (marshal via crypto.MarshalPublicKey and compare) and
keep allowing nil pubKey only when entry.PubKey is empty.
---
Outside diff comments:
In `@block/internal/executing/executor.go`:
- Around line 757-762: When producing unsigned based-sequencer blocks (the
branch where e.signer == nil and you call e.options.ValidatorHasherProvider to
compute validatorHash), ensure you also populate the header's Signer.PubKey from
the schedule entry so pinned schedule entries are not missing a pubkey;
specifically, after computing validatorHash in that branch set the header's
Signer.PubKey (and any equivalent scheduled pubkey field) to the schedule
entry's PubKey used to compute the hash. Apply the same fix in the second
similar branch around the code at the other occurrence (the block referenced by
the reviewer at 782-785).
---
Nitpick comments:
In `@block/internal/submitting/da_submitter.go`:
- Around line 486-495: The loop currently returns an error from
genesis.ValidateProposer when it encounters the first height the signer is no
longer authorized for, discarding any prior entries; change this to fail-fast
with an explicit log and stop-processing behavior so earlier built entries are
preserved and operators can see a clear "rotation reached" message.
Specifically, in the block that calls
genesis.ValidateProposer(unsignedData.Height(), addr, pubKey) inside the
unsignedDataList loop, replace the immediate return with a log statement that
includes unsignedData.Height(), addr and pubKey (or a short identifier) and then
break out of the loop (or otherwise stop further processing) so the function can
return the already-accumulated results instead of an error; ensure the log
message clearly states the signer lost authority due to schedule rotation.
In `@docs/adr/adr-023-proposer-key-rotation.md`:
- Line 140: The sentence "emergency rotation still requires preplanned
scheduling or a later authority-based mechanism" is awkwardly phrased; update
that clause in adr-023 to clearer wording such as "emergency rotation still
requires a preplanned schedule or, alternatively, a subsequent authority-based
mechanism" or "emergency rotation still requires prior scheduling or a later
authority-based mechanism"—locate the exact phrase and replace it with one of
these clearer alternatives to improve readability.
In `@pkg/genesis/proposer_schedule.go`:
- Around line 80-95: EffectiveProposerSchedule currently only copies the slice
header for Genesis.ProposerSchedule so returned ProposerScheduleEntry.Address
and .PubKey still reference underlying byte slices; modify
EffectiveProposerSchedule to deep-copy the byte slices for each entry when
building out (iterate over g.ProposerSchedule, copy each ProposerScheduleEntry,
and set entry.Address = bytes.Clone(entry.Address) and if entry.PubKey != nil
set entry.PubKey = bytes.Clone(entry.PubKey)) and ensure the single-entry return
path also clones any PubKey in addition to the existing
bytes.Clone(g.ProposerAddress).
🪄 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: 614bc46d-68db-47cd-bd87-9f6f34553c74
📒 Files selected for processing (20)
block/internal/executing/executor.goblock/internal/executing/executor_test.goblock/internal/submitting/da_submitter.goblock/internal/submitting/da_submitter_test.goblock/internal/syncing/assert.goblock/internal/syncing/da_retriever.goblock/internal/syncing/p2p_handler.goblock/internal/syncing/p2p_handler_test.goblock/internal/syncing/raft_retriever.godocs/.vitepress/config.tsdocs/adr/adr-023-proposer-key-rotation.mddocs/guides/create-genesis.mddocs/guides/operations/proposer-key-rotation.mddocs/guides/operations/upgrades.mdnode/failover.gonode/full.gopkg/genesis/genesis.gopkg/genesis/io.gopkg/genesis/proposer_schedule.gopkg/genesis/proposer_schedule_test.go
| if err := h.assertExpectedProposer(p2pHeader.SignedHeader); err != nil { | ||
| h.logger.Debug().Uint64("height", height).Err(err).Msg("invalid header from P2P") | ||
| return err |
There was a problem hiding this comment.
Reject P2P headers whose embedded height differs from the requested height.
Line 84 now validates the proposer schedule using header.Height(). If the store returns a header for a different height, it can validate against that height’s scheduled proposer and later mark the requested height as processed. Add an explicit height check before proposer validation.
🐛 Proposed fix
p2pHeader, err := h.headerStore.GetByHeight(ctx, height)
if err != nil {
if ctx.Err() == nil {
h.logger.Debug().Uint64("height", height).Err(err).Msg("header unavailable in store")
}
return err
}
+ if headerHeight := p2pHeader.SignedHeader.Height(); headerHeight != height {
+ err := fmt.Errorf("header height mismatch: requested %d, got %d", height, headerHeight)
+ h.logger.Warn().
+ Uint64("requested_height", height).
+ Uint64("header_height", headerHeight).
+ Err(err).
+ Msg("discarding mismatched header from P2P")
+ return err
+ }
if err := h.assertExpectedProposer(p2pHeader.SignedHeader); err != nil {
h.logger.Debug().Uint64("height", height).Err(err).Msg("invalid header from P2P")
return err
}As per coding guidelines, validate all inputs from external sources in Go code.
Also applies to: 128-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/syncing/p2p_handler.go` around lines 84 - 86, Check that the
height embedded in the incoming P2P header matches the requested height before
validating the proposer: call p2pHeader.SignedHeader.Height() (or use
header.Height() where applicable) and if it differs from the requested variable
height, log a debug/error via h.logger (include both heights) and return an
error immediately; only call h.assertExpectedProposer(p2pHeader.SignedHeader)
after the heights match. Apply the same explicit height check in the other
occurrence around the second h.assertExpectedProposer call (the block at lines
~128-130).
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/executing/executor_test.go`:
- Around line 143-218: Tests use wall-clock times (time.Now()) making fixtures
non-deterministic; replace those with a fixed timestamp constant (e.g., a single
time variable declared in the test like fixedTime := time.Unix(0, 0).UTC()) and
use that for genesis.StartTime, prevHeader/BaseHeader.Time, and BatchData.Time
when constructing genesis.Genesis, prevHeader, and the CreateBlock call; apply
the same fixed-time pattern to the other new genesis and BatchData fixtures in
this file so NewExecutor, setLastState, CreateBlock, BatchData and prevHeader
use deterministic times.
In `@docs/adr/adr-023-proposer-key-rotation.md`:
- Around line 56-73: The ADR text incorrectly states `pub_key` is required for
every schedule entry; update the wording to reflect that `pub_key` is optional
at runtime (entries may be address-only with `entry.PubKey` empty) and that
validation accepts entries lacking `pub_key`; adjust the sentences around the
bullet list and the rules that currently say “each entry's `address` must match
the configured `pub_key`” and “the first entry must start at `initial_height`”
to instead state that when `pub_key` is present it must match the configured
value and, when absent, the entry is interpreted by `address` only (also clarify
that `proposer_address`, when present, must match the first schedule entry’s
address regardless of `pub_key` presence).
In `@pkg/genesis/genesis_test.go`:
- Line 143: Replace the nondeterministic wall-clock timestamp used for the test
fixture: locate the declaration of validTime in genesis_test.go (the line
setting validTime := time.Now().UTC()) and change it to a fixed, non-zero UTC
timestamp (e.g., construct with time.Date and time.UTC) so the test becomes
deterministic while preserving the same type and layout expected by the rest of
the test.
- Around line 227-238: Change the test case so the Genesis.InitialHeight matches
the first ProposerScheduleEntry's start_height (i.e., use the same value as
entry20) so validation doesn't fail on the initial-height equality check and
instead proceeds to the strictly-increasing start_height check for
ProposerSchedule (where entry20 is followed by entry10). Update the mutate
function to set InitialHeight to the start_height value used by entry20 while
keeping ProposerSchedule: []ProposerScheduleEntry{entry20, entry10} and the same
wantErr.
In `@pkg/genesis/proposer_schedule_test.go`:
- Around line 271-286: The test TestEffectiveProposerSchedule_LegacyFallback
mutates schedule[0].Address which shares backing memory with
legacy.ProposerAddress; before mutating, make a defensive copy of the expected
address (e.g. copy addr or legacy.ProposerAddress into a new byte slice) or copy
schedule[0].Address into a separate slice and mutate that copy, then assert the
original legacy.ProposerAddress equals the copied original; update the test to
perform the mutation on a cloned slice so the assertion against
legacy.ProposerAddress is valid.
🪄 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: 311c4ae3-f34f-40e8-b7a7-d1eee5506200
📒 Files selected for processing (6)
block/internal/executing/executor_test.goblock/internal/syncing/p2p_handler_test.godocs/adr/adr-023-proposer-key-rotation.mdpkg/genesis/genesis_test.gopkg/genesis/proposer_schedule.gopkg/genesis/proposer_schedule_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/genesis/proposer_schedule.go
| name: "invalid - non-increasing (decreasing start_heights)", | ||
| mutate: func() Genesis { | ||
| return Genesis{ | ||
| ChainID: "c", | ||
| StartTime: validTime, | ||
| InitialHeight: 1, | ||
| ProposerSchedule: []ProposerScheduleEntry{entry20, entry10}, | ||
| DAEpochForcedInclusion: 1, | ||
| } | ||
| }, | ||
| wantErr: "start_height must equal initial_height", | ||
| }, |
There was a problem hiding this comment.
Make the decreasing-height case reach the strictly-increasing check.
With entry20 first and InitialHeight: 1, validation fails on proposer_schedule[0].start_height before it can test the decreasing schedule path.
🧪 Proposed fix
{
name: "invalid - non-increasing (decreasing start_heights)",
mutate: func() Genesis {
return Genesis{
ChainID: "c",
StartTime: validTime,
InitialHeight: 1,
- ProposerSchedule: []ProposerScheduleEntry{entry20, entry10},
+ ProposerSchedule: []ProposerScheduleEntry{entry1, entry20, entry10},
DAEpochForcedInclusion: 1,
}
},
- wantErr: "start_height must equal initial_height",
+ wantErr: "strictly increasing",
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/genesis/genesis_test.go` around lines 227 - 238, Change the test case so
the Genesis.InitialHeight matches the first ProposerScheduleEntry's start_height
(i.e., use the same value as entry20) so validation doesn't fail on the
initial-height equality check and instead proceeds to the strictly-increasing
start_height check for ProposerSchedule (where entry20 is followed by entry10).
Update the mutate function to set InitialHeight to the start_height value used
by entry20 while keeping ProposerSchedule: []ProposerScheduleEntry{entry20,
entry10} and the same wantErr.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/genesis/proposer_schedule_test.go (3)
34-59: Consider consolidatingProposerAtHeightcases into a table-driven test.The three sub-cases here (height 3, 9, 10) share setup and only differ in input/expected. A table-driven form would scale better as rotation scenarios grow (e.g., boundary heights, gap heights) and matches the repository convention.
♻️ Sketch of table-driven form
- proposer, err := genesis.ProposerAtHeight(3) - require.NoError(t, err) - require.Equal(t, entry1.Address, proposer.Address) - - proposer, err = genesis.ProposerAtHeight(9) - require.NoError(t, err) - require.Equal(t, entry1.Address, proposer.Address) - - proposer, err = genesis.ProposerAtHeight(10) - require.NoError(t, err) - require.Equal(t, entry2.Address, proposer.Address) + cases := []struct { + name string + height uint64 + expected []byte + }{ + {"first entry start", 3, entry1.Address}, + {"within first entry range", 9, entry1.Address}, + {"second entry start", 10, entry2.Address}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := genesis.ProposerAtHeight(tc.height) + require.NoError(t, err) + require.Equal(t, tc.expected, got.Address) + }) + }As per coding guidelines: "Use table-driven tests where appropriate."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/genesis/proposer_schedule_test.go` around lines 34 - 59, Refactor TestGenesisProposerAtHeight into a table-driven test: keep the shared setup (makeProposerScheduleEntry, Genesis instantiation and Validate) and replace the three repeated calls to genesis.ProposerAtHeight with a slice of test cases (each having name, height, expectedAddress, expectErr) and loop using t.Run; inside each case call genesis.ProposerAtHeight(height), assert error presence/absence and compare proposer.Address to expectedAddress (using require.NoError/Equal or require.Error as appropriate). Ensure you reference the existing helpers and types (makeProposerScheduleEntry, Genesis, ProposerAtHeight, ProposerScheduleEntry) so the test logic remains identical but is driven by the table.
79-96: Strengthen the address-only happy-path assertions.The test confirms that
ValidateProposeraccepts the correct pubkey for an address-only entry, but becauseentry1.PubKey/entry2.PubKeyare cleared in place (Lines 82-83), a regression inEffectiveProposerSchedule/ProposerAtHeightthat accidentally retained a pinned pubkey would not be caught here. Consider assertinggenesis.ProposerSchedule[i].PubKeyis nil (or thatProposerAtHeight(...).PubKeyis empty) to pin the address-only intent explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/genesis/proposer_schedule_test.go` around lines 79 - 96, The test TestGenesisValidateAddressOnlyProposerSchedule currently clears entry1.PubKey/entry2.PubKey in-place but doesn't assert the schedule actually retained a nil pubkey; add explicit assertions that genesis.ProposerSchedule[0].PubKey and genesis.ProposerSchedule[1].PubKey are nil (or call EffectiveProposerSchedule/ProposerAtHeight and assert the returned ProposerScheduleEntry.PubKey is empty/nil) before calling ValidateProposer so the address-only intent is pinned and regressions that reintroduce a pinned pubkey are caught.
320-341: Also assert legacy normalization fromProposerAddress-only genesis.This test covers schedule-present normalization, but the complementary case — a genesis with only
ProposerAddressset, loaded and normalized into anEffectiveProposerSchedule— is where the legacy compatibility path actually matters at runtime. A sibling test that writes a legacy-only genesis JSON and assertsLoadGenesisproduces a single-entryEffectiveProposerScheduleatInitialHeightwould close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/genesis/proposer_schedule_test.go` around lines 320 - 341, Add a complementary test that writes a legacy-only genesis JSON with ProposerAddress set (and no ProposerSchedule), calls LoadGenesis, and asserts the loader normalized it into a single-entry proposer schedule at InitialHeight: create a test (e.g. TestLoadGenesisNormalizesLegacyProposerAddressOnly) that sets Genesis{ChainID, StartTime, InitialHeight, ProposerAddress: <addr>, ProposerSchedule: nil}, marshal/write it, call LoadGenesis, then require that loaded.ProposerSchedule has length 1, the sole entry.Address equals the original ProposerAddress, and the entry.Height equals the genesis InitialHeight (also optionally assert loaded.ProposerAddress still matches); reference LoadGenesis, Genesis.ProposerAddress, Genesis.ProposerSchedule and InitialHeight to locate the code to exercise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/genesis/proposer_schedule_test.go`:
- Around line 34-59: Refactor TestGenesisProposerAtHeight into a table-driven
test: keep the shared setup (makeProposerScheduleEntry, Genesis instantiation
and Validate) and replace the three repeated calls to genesis.ProposerAtHeight
with a slice of test cases (each having name, height, expectedAddress,
expectErr) and loop using t.Run; inside each case call
genesis.ProposerAtHeight(height), assert error presence/absence and compare
proposer.Address to expectedAddress (using require.NoError/Equal or
require.Error as appropriate). Ensure you reference the existing helpers and
types (makeProposerScheduleEntry, Genesis, ProposerAtHeight,
ProposerScheduleEntry) so the test logic remains identical but is driven by the
table.
- Around line 79-96: The test TestGenesisValidateAddressOnlyProposerSchedule
currently clears entry1.PubKey/entry2.PubKey in-place but doesn't assert the
schedule actually retained a nil pubkey; add explicit assertions that
genesis.ProposerSchedule[0].PubKey and genesis.ProposerSchedule[1].PubKey are
nil (or call EffectiveProposerSchedule/ProposerAtHeight and assert the returned
ProposerScheduleEntry.PubKey is empty/nil) before calling ValidateProposer so
the address-only intent is pinned and regressions that reintroduce a pinned
pubkey are caught.
- Around line 320-341: Add a complementary test that writes a legacy-only
genesis JSON with ProposerAddress set (and no ProposerSchedule), calls
LoadGenesis, and asserts the loader normalized it into a single-entry proposer
schedule at InitialHeight: create a test (e.g.
TestLoadGenesisNormalizesLegacyProposerAddressOnly) that sets Genesis{ChainID,
StartTime, InitialHeight, ProposerAddress: <addr>, ProposerSchedule: nil},
marshal/write it, call LoadGenesis, then require that loaded.ProposerSchedule
has length 1, the sole entry.Address equals the original ProposerAddress, and
the entry.Height equals the genesis InitialHeight (also optionally assert
loaded.ProposerAddress still matches); reference LoadGenesis,
Genesis.ProposerAddress, Genesis.ProposerSchedule and InitialHeight to locate
the code to exercise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c6efd07-4dc8-4e18-89a0-70df0315c574
📒 Files selected for processing (7)
block/internal/executing/executor.goblock/internal/executing/executor_test.goblock/internal/syncing/p2p_handler.goblock/internal/syncing/p2p_handler_test.godocs/adr/adr-023-proposer-key-rotation.mdpkg/genesis/genesis_test.gopkg/genesis/proposer_schedule_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- block/internal/executing/executor_test.go
- block/internal/syncing/p2p_handler_test.go
- pkg/genesis/genesis_test.go
| 2. Add a new `proposer_schedule` entry with `start_height = H` | ||
| 3. Distribute the updated genesis/config to node operators | ||
| 4. Upgrade follower/full nodes before activation | ||
| 5. Stop the old sequencer before `H` |
There was a problem hiding this comment.
In theory, you should stop it as soon as you have the genesis, to avoid timing H-1. The schedule trigger then anyway.
| @@ -0,0 +1,186 @@ | |||
| # Rotate proposer key | |||
|
|
|||
| Use this guide to rotate a sequencer proposer key without restarting the chain. The active proposer is selected from `proposer_schedule` in `genesis.json` based on block height. | |||
There was a problem hiding this comment.
I would reword this. You effectively need to restart everything.
|
|
||
| ### Security considerations | ||
|
|
||
| This design improves safety by allowing validation against the scheduled public key when one is pinned. |
There was a problem hiding this comment.
This makes me thing as a possible vulnerability.
If bridges are trusting a full node (and do not have access to the sequencer), a full node being hacked, could itself replace its genesis with a new key, become the defacto sequencer.
I wonder how the execution client would behave, but in theory you could steal some funds if you managed to convinced the right node to update their genesis.
We should add a field in the genesis with a signature, the key swap need to be signed by the previous key.
| - This is a coordinated network upgrade. Every node must run a binary that supports `proposer_schedule`. | ||
| - Every node must use the same updated `genesis.json` before the activation height. | ||
| - `ev-node` loads `genesis.json` when the node starts. Updating the file on disk is not enough; you must restart nodes after replacing it. | ||
| - The old proposer key remains valid until the block before the activation height. If the old key cannot safely produce until then, stop the sequencer and coordinate operator recovery first. |
There was a problem hiding this comment.
This makes me this of a possible social engineering attack
|
|
||
| Example with a file-based signer: | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
UX wise we should make the sequencer load all the keys and swap the the right time without a restart.
| ``` | ||
|
|
||
| :::tip | ||
| If you want to plan a future proposer key migration without restarting the chain, define a `proposer_schedule` in your genesis and roll it out as a coordinated upgrade. See [Rotate proposer key](./operations/proposer-key-rotation.md). |
|
|
||
| if g.ProposerAddress == nil { | ||
| return fmt.Errorf("proposer_address cannot be nil") | ||
| if len(g.ProposerSchedule) == 0 { |
There was a problem hiding this comment.
See my proposal above. I think we should add a signature field and just verify it.
This does not solve entirely the attack if the sequencer key was lost (could even worsen it - if the attacker know how it works), but that would limit the easy social engineering vector that can be done by any node without the original key
|
doing a different design after discussing with julien |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
node/execution_test.go (1)
41-42:⚠️ Potential issue | 🔴 CriticalMock
Returnvalue type no longer matchesExecuteTxssignature — test will panic.
ExecuteTxsnow returns(coreexecutor.ExecuteResult, error), but the mock is set up to returnexpectedNewStateRoot(a[]byte). When the mocked method is invoked, the generated mockery wrapper will try to type-assert the return toExecuteResultand panic withinterface conversion: []uint8 is not coreexecutor.ExecuteResult.🐛 Proposed fix
- mockExec.On("ExecuteTxs", mock.Anything, txs, blockHeight, mock.AnythingOfType("time.Time"), expectedInitialStateRoot). - Return(expectedNewStateRoot, nil).Once() + mockExec.On("ExecuteTxs", mock.Anything, txs, blockHeight, mock.AnythingOfType("time.Time"), expectedInitialStateRoot). + Return(coreexecutor.ExecuteResult{UpdatedStateRoot: expectedNewStateRoot}, nil).Once()As per coding guidelines ("Ensure tests are deterministic"), mock setups must match the real method signature or the test becomes non-deterministic / always-failing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/execution_test.go` around lines 41 - 42, The mock Return value for mockExec.On("ExecuteTxs", ...) is using expectedNewStateRoot (a []byte) but ExecuteTxs now returns (coreexecutor.ExecuteResult, error); update the mock to return a properly constructed coreexecutor.ExecuteResult instance (e.g., expectedExecuteResult) and nil instead of a []byte so the type assertion in the mock wrapper succeeds; locate the mock setup around mockExec.On and replace the second return value with the ExecuteResult object (ensure it contains the new state root previously held in expectedNewStateRoot and any other required fields) and keep the error as nil.docs/getting-started/custom/implement-executor.md (1)
22-58:⚠️ Potential issue | 🟡 MinorExample
InitChainsignature contradicts the documented interface.Line 9 declares
InitChain(ctx, genesisTime, initialHeight, chainID) ([]byte, error), but the example on lines 22 and 44 (and the test call on line 208) still use the oldInitChain(ctx, genesis Genesis)form. Since this PR realigns the docs with the new contract, it's worth updating the examples too to avoid confusing implementers.📝 Proposed alignment
-func (e *MyExecutor) InitChain(ctx context.Context, genesis Genesis) ([]byte, error) +func (e *MyExecutor) InitChain(ctx context.Context, genesisTime time.Time, initialHeight uint64, chainID string) ([]byte, error)And update the example body and the test call site (line 208) accordingly so they match the interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/custom/implement-executor.md` around lines 22 - 58, The documented example and test call use the old InitChain(ctx context.Context, genesis Genesis) signature while the interface was changed to InitChain(ctx, genesisTime, initialHeight, chainID) ([]byte, error); update the example and test to match the new signature by changing the function declaration for MyExecutor.InitChain to accept (ctx context.Context, genesisTime time.Time, initialHeight int64, chainID string) and adapt the example body to parse/derive state from those parameters (replace references to Genesis and GenesisState with values derived from genesisTime/initialHeight/chainID), and update calls to e.db.Set and e.db.Hash accordingly; also modify the test invocation (the call at line 208) to pass the new arguments genesisTime, initialHeight and chainID to InitChain.block/internal/executing/executor_logic_test.go (1)
56-57:⚠️ Potential issue | 🔴 CriticalMock return types are incorrect and will cause runtime failures.
After
Executor.ExecuteTxschanged its return type to(execution.ExecuteResult, error), the mock setup calls still return raw[]byteinstead of the struct. When test code accessesresult.UpdatedStateRoot(line 260), it will fail because the mocks returned[]byterather thanExecuteResult{UpdatedStateRoot: ...}.Affected lines:
- Lines 56-57, 147-148, 362-363:
fx.MockExec.EXPECT().ExecuteTxs(...).Return([]byte(...), nil)- Lines 176-177, 185-188, 196-199, 206-207, 216-217:
exec.On("ExecuteTxs", ...).Return([]byte(...), ...)All must return
execution.ExecuteResult{UpdatedStateRoot: ...}instead.Example fix
- fx.MockExec.EXPECT().ExecuteTxs(mock.Anything, mock.Anything, uint64(1), mock.AnythingOfType("time.Time"), fx.InitStateRoot). - Return([]byte("new_root"), nil).Once() + fx.MockExec.EXPECT().ExecuteTxs(mock.Anything, mock.Anything, uint64(1), mock.AnythingOfType("time.Time"), fx.InitStateRoot). + Return(execution.ExecuteResult{UpdatedStateRoot: []byte("new_root")}, nil).Once()For error cases:
- exec.On("ExecuteTxs", mock.Anything, mock.Anything, uint64(100), mock.Anything, mock.Anything). - Return([]byte(nil), errors.New("temporary failure")).Once() + exec.On("ExecuteTxs", mock.Anything, mock.Anything, uint64(100), mock.Anything, mock.Anything). + Return(execution.ExecuteResult{}, errors.New("temporary failure")).Once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/executing/executor_logic_test.go` around lines 56 - 57, Mocks for ExecuteTxs are returning []byte but the real signature now returns (execution.ExecuteResult, error); update all mock Return(...) calls (fx.MockExec.EXPECT().ExecuteTxs(...) and exec.On("ExecuteTxs", ...)) to return execution.ExecuteResult{UpdatedStateRoot: []byte("new_root")} (or the appropriate byte slice for each case) and the corresponding error (nil or an error) so tests receive an ExecuteResult struct and result.UpdatedStateRoot is valid.types/header.go (1)
85-98:⚠️ Potential issue | 🟠 MajorAdd
String()method to Header that includesNextProposerAddress.Verification confirms
Header.Hash()correctly includesNextProposerAddressthrough the serialization layer (MarshalBinary()), so the hash is cryptographically sound. However, Header lacks aString()method entirely. Per coding guidelines, all core types must implementString()for debugging and inspection. Once added, it must includeNextProposerAddresssince it's a consensus-critical field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/header.go` around lines 85 - 98, Add a String() method to the Header type that returns a human-readable representation and explicitly includes the NextProposerAddress field; implement Header.String() so it formats core fields (including NextProposerAddress) for debugging, and ensure it does not mutate state (so it must not change cachedHash or call InvalidateHash()); refer to existing serialization/hash helpers (Header.Hash, MarshalBinary) for canonical field ordering if needed, and place the method on the Header receiver to satisfy the coding guideline requiring String() on core types.block/internal/syncing/syncer_test.go (1)
294-295:⚠️ Potential issue | 🔴 CriticalMockExecutor.ExecuteTxs return type mismatch: tests must return ExecuteResult, not []byte.
The mock's signature (confirmed in test/mocks/execution.go) is
ExecuteTxs(...) (execution.ExecuteResult, error), but the following test files still use.Return([]byte(...), nil), which will fail to compile:
- syncer_test.go: 295, 354, 363, 431, 931, 940, 942, 951, 953, 962, 1508, 1655
- executor_lazy_test.go: 94, 118, 210
- executor_restart_test.go: 96, 210, 379
- replay_test.go: 73, 282, 352, 423, 496
- executor_logic_test.go: 57, 148, 177, 186, 188, 197, 199, 208, 217
Each must be updated to
Return(execution.ExecuteResult{UpdatedStateRoot: []byte(...)}, nil)or equivalent. The syncer_test.go example in the original comment (lines 231–235) shows the correct pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer_test.go` around lines 294 - 295, The mock return types for mockExec.ExecuteTxs are incorrect: update all .Return(...) calls that currently pass []byte(...) to instead return an execution.ExecuteResult struct with the UpdatedStateRoot set to that byte slice and nil error; locate calls to mockExec.EXPECT().ExecuteTxs(...) in syncer_test.go (and the other listed test files) and replace .Return([]byte("app1"), nil) style returns with .Return(execution.ExecuteResult{UpdatedStateRoot: []byte("app1")}, nil) (or equivalent byte values) so the mock signature ExecuteTxs(...)(execution.ExecuteResult, error) is satisfied.
🧹 Nitpick comments (14)
core/README.md (1)
30-37: Consider documentingExecutionInfoalongsideExecuteResultfor completeness.Since
GetExecutionInfois now part of this excerpt, adding theExecutionInfostruct here would make the contract self-contained for readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/README.md` around lines 30 - 37, Add a self-contained definition and brief doc comment for the ExecutionInfo struct next to ExecuteResult so readers can see the full contract; create a clear comment above the type and include the fields used by GetExecutionInfo (e.g., any execution parameters like StateRoot, BlockNumber, Timestamp, ProposerAddress or other project-specific fields) under the name ExecutionInfo to mirror how ExecuteResult is declared and referenced by GetExecutionInfo.block/internal/syncing/assert.go (1)
11-17: Remove unusedgenesisparameter fromassertValidSignedData.The
genesisparameter is not referenced in the function body. With proposer validation moved toState.AssertValidForNextState, this parameter can be safely removed along with the unused import. Update the caller inda_retriever.go:355to remove ther.genesisargument.♻️ Proposed change
import ( "errors" "fmt" - "github.com/evstack/ev-node/pkg/genesis" "github.com/evstack/ev-node/types" ) -func assertValidSignedData(signedData *types.SignedData, genesis genesis.Genesis) error { +func assertValidSignedData(signedData *types.SignedData) error {func (r *daRetriever) assertValidSignedData(signedData *types.SignedData) error { - return assertValidSignedData(signedData, r.genesis) + return assertValidSignedData(signedData) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/assert.go` around lines 11 - 17, Remove the unused genesis parameter and its type from the function signature of assertValidSignedData and delete the unused genesis import; then update any call sites (e.g., the call that currently passes r.genesis) to stop supplying that extra argument so they match the new signature. Ensure the function remains: func assertValidSignedData(signedData *types.SignedData) error and that all callers (such as the one in da retriever code that passed r.genesis) are adjusted accordingly.test/mocks/height_aware_executor.go (2)
74-91: Silent short-circuit can mask missing expectations.Returning a zero-value
ExecutionInfowhen noGetExecutionInfoexpectation is set makes existing tests pass without opting in, but it also means any new code path that begins callingGetExecutionInfowill go untested / unasserted in mocks that used to flag the unexpected call. Consider scoping this fallback behind an explicit opt-in (e.g., a helper likeWithDefaultExecutionInfo()), or at minimum leaving a comment explaining the compatibility intent so future maintainers don't remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mocks/height_aware_executor.go` around lines 74 - 91, The mock currently silently returns a zero-value from MockHeightAwareExecutor.GetExecutionInfo when no expectation is present; change this to be explicit by adding an opt-in flag (e.g., add a bool field defaultExecutionInfo and a helper method WithDefaultExecutionInfo() on MockHeightAwareExecutor) and modify GetExecutionInfo to return the zero-value only when that flag is set, otherwise call m.Called(ctx) and surface the missing-expectation (e.g., return the call result or an error/assertion) so unexpected calls are not silently ignored; include a short comment on the compatibility intent adjacent to the new helper/flag.
47-59: Default switch branch is effectively a panic; consider making the failure explicit.The
defaultcase re-type-asserts withargs.Get(0).(execution.ExecuteResult)which will panic with a generic type-assertion message for any unexpected type. A clearer mock-level error helps tests fail faster when a wrong type is stubbed. Non-blocking.♻️ Proposed clearer failure message
switch result := args.Get(0).(type) { case nil: return execution.ExecuteResult{}, args.Error(1) case execution.ExecuteResult: return result, args.Error(1) case []byte: return execution.ExecuteResult{UpdatedStateRoot: result}, args.Error(1) default: - return args.Get(0).(execution.ExecuteResult), args.Error(1) + panic(fmt.Sprintf("MockHeightAwareExecutor.ExecuteTxs: unsupported return type %T (expected execution.ExecuteResult, []byte, or nil)", result)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mocks/height_aware_executor.go` around lines 47 - 59, The default branch in MockHeightAwareExecutor.ExecuteTxs currently re-type-asserts args.Get(0).(execution.ExecuteResult) which will panic for unexpected types; change the default to return a clear error instead of panicking by constructing and returning an error (e.g., via fmt.Errorf) that describes the unexpected type from args.Get(0) and the call context (function ExecuteTxs and parameters such as txs/blockHeight), so tests get an explicit, informative failure when a wrong type is stubbed.pkg/telemetry/executor_tracing_test.go (1)
180-190: Make the mock return value explicit.
Return(expectedStateRoot, nil)still passes a raw[]byteand depends on the legacy[]byte→execution.ExecuteResult{UpdatedStateRoot: ...}shim intest/mocks/execution.go. Passingcoreexec.ExecuteResultdirectly makes the test independent of that shim and future-proof against a clean mock regeneration.mockExec.EXPECT(). ExecuteTxs(mock.Anything, txs, blockHeight, timestamp, prevStateRoot). - Return(expectedStateRoot, nil) + Return(coreexec.ExecuteResult{UpdatedStateRoot: expectedStateRoot}, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/telemetry/executor_tracing_test.go` around lines 180 - 190, The mock currently returns a raw []byte (Return(expectedStateRoot, nil)) which relies on a legacy shim; update the mock expectation for mockExec.EXPECT().ExecuteTxs(...) to return a coreexec.ExecuteResult value instead (e.g. Return(coreexec.ExecuteResult{UpdatedStateRoot: expectedStateRoot}, nil)), and add or adjust the import for the coreexec package if needed so the test no longer depends on the legacy []byte→ExecuteResult shim.test/mocks/execution.go (2)
58-66: Type-erasedReturn(interface{}, error)and legacy[]bytecase are also hand-edits.The switch at Lines 58–66 accepts a bare
[]byteand auto-wraps it intoexecution.ExecuteResult{UpdatedStateRoot: result}, andReturnat Line 124 was loosened from the typedexecution.ExecuteResulttointerface{}. Together these let existing tests keep calling.Return(stateRootBytes, nil)unchanged, but they lose compile-time type safety (RunAndReturnat Line 129 still uses the typed signature, so the two helpers are now inconsistent) and will be wiped by the nextmockeryrun.Prefer migrating the remaining call sites to pass
execution.ExecuteResult{UpdatedStateRoot: ...}explicitly and restoring the strongly-typedReturn(result execution.ExecuteResult, err error)so the file can be regenerated cleanly.Also applies to: 124-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mocks/execution.go` around lines 58 - 66, The mock loosened the Return signature to Return(result interface{}, err error) and added a special-case for []byte in the switch, which defeats compile-time safety and will be lost on regen; revert Return to the strongly-typed signature Return(result execution.ExecuteResult, err error), remove the legacy []byte handling in the switch inside the mocked method (restore the original branches to handle nil and execution.ExecuteResult only), and update any test call sites that currently call .Return(stateRootBytes, nil) to pass execution.ExecuteResult{UpdatedStateRoot: stateRootBytes} instead so RunAndReturn and Return share the same typed execution.ExecuteResult signature.
220-258: Hand-edit in mockery-generated file silently swallows unexpected calls.This guard inside
GetExecutionInfodiverges from stock mockery output (which the file header says not to edit) and masks missing expectations: callers that forget to registerEXPECT().GetExecutionInfo(...)will get a silent zero-valueexecution.ExecutionInfo{}back instead of the normal mockery "unexpected call" panic. Any futurego generateof mocks will overwrite these lines, regressing this behavior.Additionally, the
len(_mock.ExpectedCalls) == 0check at Line 222 is redundant with thehasExpectationloop below and can be removed.If the intent is to make
GetExecutionInfooptional for existing tests, prefer one of:
- Add an explicit helper (e.g.,
mockExec.EXPECT().GetExecutionInfo(mock.Anything).Return(execution.ExecutionInfo{}, nil).Maybe()) in a shared test setup, or- Move this accommodation into a separate non-generated wrapper mock so the mockery output stays clean and regeneration-safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/mocks/execution.go` around lines 220 - 258, The GetExecutionInfo method on MockExecutor has a hand-edited guard (the len(_mock.ExpectedCalls)==0 check and the hasExpectation loop) that silently returns a zero-value instead of letting the mockery-generated Called(...) path raise the usual "unexpected call" panic; remove the early-return logic (both the len(...) check and the hasExpectation loop and related conditional return) so GetExecutionInfo always calls _mock.Called(ctx) and lets Called handle unexpected calls, and instead implement optional behavior outside this generated file (either add a shared test helper that sets EXPECT().GetExecutionInfo(...).Maybe() or create a non-generated wrapper around MockExecutor for accommodating optional calls).proto/evnode/v1/state.proto (1)
19-19: Add a doc comment to matchHeader.next_proposer_address.The analogous field in
evnode.proto(Header.next_proposer_address = 13) carries a descriptive comment, but the newState.next_proposer_addressis undocumented. Add a short comment describing the semantics (e.g., "Proposer expected to sign block atLastBlockHeight+1; initialized from genesis, then updated from execution results.") so the protobuf schema stays self-describing.bytes last_header_hash = 9; + // Proposer expected to sign the block at last_block_height + 1. + // Initialized from genesis and then updated from execution results. bytes next_proposer_address = 10;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/evnode/v1/state.proto` at line 19, Add a short protobuf doc comment for the State.next_proposer_address field so the schema is self-describing: in the message State, above the field next_proposer_address, add a comment like "Proposer expected to sign block at LastBlockHeight+1; initialized from genesis and updated from execution results." to mirror the documentation on Header.next_proposer_address and clarify semantics.execution/grpc/client_test.go (1)
145-188: Consider assertingNextProposerAddressround-trips through the gRPC client.
TestClient_ExecuteTxsonly validatesUpdatedStateRoot; the newNextProposerAddressfield onexecution.ExecuteResultis not exercised end-to-end through the gRPC client/server. A single additional assertion in the existing mock would close the gap.Illustrative diff
executeTxsFunc: func(ctx context.Context, txsIn [][]byte, bh uint64, ts time.Time, psr []byte) (execution.ExecuteResult, error) { ... - return execution.ExecuteResult{UpdatedStateRoot: expectedStateRoot}, nil + return execution.ExecuteResult{ + UpdatedStateRoot: expectedStateRoot, + NextProposerAddress: []byte("next_proposer"), + }, nil }, } ... if string(result.UpdatedStateRoot) != string(expectedStateRoot) { t.Errorf(...) } + if string(result.NextProposerAddress) != "next_proposer" { + t.Errorf("expected next proposer to be propagated, got %q", result.NextProposerAddress) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/grpc/client_test.go` around lines 145 - 188, The test TestClient_ExecuteTxs only checks UpdatedStateRoot; modify the mockExecutor.executeTxsFunc to return a non-empty NextProposerAddress in the execution.ExecuteResult and add an assertion after calling client.ExecuteTxs to verify result.NextProposerAddress equals the expected value, ensuring the field round-trips through NewExecutorServiceHandler and NewClient via client.ExecuteTxs.execution/grpc/server_test.go (1)
183-291: Add coverage forNextProposerAddresspropagation.
server.ExecuteTxsnow wiresresult.NextProposerAddressinto the response (Line 118 ofexecution/grpc/server.go), andGetExecutionInfoexposesinfo.NextProposerAddress(Line 155). Neither is asserted here, so the new proposer-rotation propagation is not exercised by tests. Consider adding a case that returns a non-emptyNextProposerAddressfrom the mock and assertsresp.Msg.NextProposerAddressequals it (and similarly forGetExecutionInfo).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/grpc/server_test.go` around lines 183 - 291, Add a test case that exercises NextProposerAddress propagation: in TestServer_ExecuteTxs add a "next proposer" case where mockExecutor.executeTxsFunc returns execution.ExecuteResult{UpdatedStateRoot: expectedStateRoot, NextProposerAddress: []byte("proposer_addr")}; call server.ExecuteTxs and assert resp.Msg.NextProposerAddress equals that byte slice (compare as bytes or string like other assertions). Also add/extend the GetExecutionInfo test to have the mock for GetExecutionInfo/ExecuteInfo return info.NextProposerAddress set and assert resp.Msg.NextProposerAddress equals it so both server.ExecuteTxs and GetExecutionInfo (info.NextProposerAddress) are covered.types/state.go (1)
46-54: Prefer an explicit parameter over a variadic override.
nextProposerAddress ...[]byteis used as a single optional override, which obscures call sites (readers can't tell whether zero, one, or multiple values are meaningful without reading the body). Consider an explicit signature such asNextState(header Header, stateRoot, nextProposerOverride []byte)and letting callers passnilfor the common case. This also removes thenextProposerAddress[0]indexing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/state.go` around lines 46 - 54, Change NextState to accept an explicit optional nextProposerOverride []byte instead of the variadic nextProposerAddress ...[]byte: update the signature of State.NextState(header Header, stateRoot []byte, nextProposerOverride []byte), replace the variadic logic in the body (the len(nextProposerAddress)/indexing) with a nil-check on nextProposerOverride, set nextProposer := header.NextProposerAddress then if nextProposerOverride != nil && len(nextProposerOverride) > 0 use it, and fall back to header.ProposerAddress if still empty; update all callers of NextState to pass nil when no override is required.block/internal/common/replay.go (1)
188-201: Consider extracting the proposer-rotation validation into a shared helper.The block at lines 188–201 is the same check implemented in
block/internal/syncing/syncer.go(lines 840–849) andblock/internal/executing/executor.go(lines 864–873 per the cross-file snippet). The "expected next proposer" resolution at 226–233 also re-implements the precedence logic from(*State).NextState. Pulling this into a single helper (e.g.types.ValidateNextProposer(header, executionNext) error+types.ResolveNextProposer(header, executionNext) []byte) would prevent the three sites from drifting apart.Also applies to: 226-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/common/replay.go` around lines 188 - 201, The proposer-rotation validation and resolution logic duplicated in block/internal/common/replay.go (the NextProposerAddress checks), block/internal/syncing/syncer.go, and block/internal/executing/executor.go should be extracted into shared helpers (suggested names: types.ValidateNextProposer(header, executionNext) error and types.ResolveNextProposer(header, executionNext) []byte) that implement the same precedence logic used by (*State).NextState; replace the duplicated checks in replay.go (the NextProposerAddress branch and the "execution unchanged" branch) and the other callers with calls to these helpers, ensuring return error behavior and resolved proposer byte slice are preserved and update any unit tests/usages accordingly.block/internal/syncing/syncer.go (1)
405-415: Consider extractinginitialProposerAddressto a shared helper.This helper is a byte-for-byte duplicate of
(*Executor).initialProposerAddressinblock/internal/executing/executor.go(see lines 381-391). Extracting it into a shared location (e.g.,block/internal/common) would keep the fallback semantics consistent if either side evolves (different log message, additional validation, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer.go` around lines 405 - 415, The method initialProposerAddress in Syncer is byte-for-byte identical to (*Executor).initialProposerAddress; extract the logic into a shared helper (e.g., in block/internal/common) and have both Syncer.initialProposerAddress and Executor.initialProposerAddress call it. Create a helper function (e.g., InitialProposerAddress(ctx, exec, genesis, logger)) that accepts the executor interface used by both types, the genesis struct, and a logger, implements the current fallback semantics and copying behavior, and return []byte; then replace the bodies of Syncer.initialProposerAddress and Executor.initialProposerAddress to delegate to that helper.execution/evm/execution.go (1)
347-464: LGTM — with a behavioral note.The signature change is propagated cleanly through all success, resume, and error paths. Since this executor never sets
NextProposerAddress, the consensus layer will treat EVM-backed chains as non-rotating (any header signaling rotation will be rejected by the syncer/executor mismatch check). If that is the intended boundary for this PR, consider a short comment here documenting that EVM execution does not participate in proposer rotation, so future contributors don't try to populateNextProposerAddressfromGetExecutionInfowithout a matching execution-layer contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/evm/execution.go` around lines 347 - 464, Add a short clarifying comment in ExecuteTxs (near the payload attributes/forkchoice update logic where suggestedFeeRecipient is set or where ExecMeta is saved) stating that this EVM executor never sets NextProposerAddress and therefore EVM-backed chains are treated as non-rotating by the consensus layer; also note that contributors should not populate NextProposerAddress from GetExecutionInfo without a corresponding execution-layer contract change to avoid syncer/executor mismatch. Reference ExecuteTxs, evPayloadAttrs, and saveExecMeta in the comment so future readers know where the behavior is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/submitting/da_submitter.go`:
- Line 472: Remove the unused genesis parameter from the unexported method
signData by updating its signature to drop the genesis.Genesis parameter and
removing any corresponding argument passed from SubmitData; update the call site
in SubmitData (where signData is invoked) to stop passing genesis, and then
remove or evaluate the DASubmitter.genesis field and SubmitData's genesis
parameter (DASubmitter.genesis and SubmitData) — if they are not used elsewhere,
delete the field and parameter to clean up dead code.
In `@block/internal/syncing/syncer.go`:
- Around line 332-334: The code unconditionally repairs an empty
state.NextProposerAddress to the genesis proposer; change this so the repair
only runs for true genesis states (check state.LastBlockHeight ==
state.InitialHeight - 1 or equivalent) and otherwise do not overwrite
NextProposerAddress; additionally, where you skip the repair, emit a warning
(e.g., s.logger.Warn/Warnf) indicating that NextProposerAddress was empty on
upgrade so operators can observe it; update the block around the call to
s.initialProposerAddress(s.ctx) to perform the LastBlockHeight guard and
warning.
In `@core/execution/execution.go`:
- Around line 135-150: Decide and document a single authoritative source for the
proposer rotation: prefer ExecuteResult.NextProposerAddress as authoritative for
the proposer of the immediately next block (i.e., applies to executedHeight+1),
and update ExecutionInfo.NextProposerAddress to be advisory/deprecated (used
only when no ExecuteResult is available from ExecuteTxs); change the comments
for ExecuteResult.NextProposerAddress to state it deterministically applies to
executedHeight+1 and update the comment on ExecutionInfo.NextProposerAddress to
explicitly say it is a best-effort view only and not authoritative (or remove
the field from ExecutionInfo entirely if you prefer a single-source API).
- Around line 142-150: Clarify that ExecuteResult.NextProposerAddress, returned
by ExecuteTxs for a call with blockHeight=H, indicates the proposer to be used
for block H+1 (not H), state that the field is optional (empty means “retain
current proposer”), and explicitly permit executors to only set it at agreed
rotation points; update the ExecuteResult doc comment to state this
unambiguously and ensure ExecuteTxs and State.NextProposerAddress
handling/validation (and the dummy executor) conform to this contract.
---
Outside diff comments:
In `@block/internal/executing/executor_logic_test.go`:
- Around line 56-57: Mocks for ExecuteTxs are returning []byte but the real
signature now returns (execution.ExecuteResult, error); update all mock
Return(...) calls (fx.MockExec.EXPECT().ExecuteTxs(...) and
exec.On("ExecuteTxs", ...)) to return execution.ExecuteResult{UpdatedStateRoot:
[]byte("new_root")} (or the appropriate byte slice for each case) and the
corresponding error (nil or an error) so tests receive an ExecuteResult struct
and result.UpdatedStateRoot is valid.
In `@block/internal/syncing/syncer_test.go`:
- Around line 294-295: The mock return types for mockExec.ExecuteTxs are
incorrect: update all .Return(...) calls that currently pass []byte(...) to
instead return an execution.ExecuteResult struct with the UpdatedStateRoot set
to that byte slice and nil error; locate calls to
mockExec.EXPECT().ExecuteTxs(...) in syncer_test.go (and the other listed test
files) and replace .Return([]byte("app1"), nil) style returns with
.Return(execution.ExecuteResult{UpdatedStateRoot: []byte("app1")}, nil) (or
equivalent byte values) so the mock signature
ExecuteTxs(...)(execution.ExecuteResult, error) is satisfied.
In `@docs/getting-started/custom/implement-executor.md`:
- Around line 22-58: The documented example and test call use the old
InitChain(ctx context.Context, genesis Genesis) signature while the interface
was changed to InitChain(ctx, genesisTime, initialHeight, chainID) ([]byte,
error); update the example and test to match the new signature by changing the
function declaration for MyExecutor.InitChain to accept (ctx context.Context,
genesisTime time.Time, initialHeight int64, chainID string) and adapt the
example body to parse/derive state from those parameters (replace references to
Genesis and GenesisState with values derived from
genesisTime/initialHeight/chainID), and update calls to e.db.Set and e.db.Hash
accordingly; also modify the test invocation (the call at line 208) to pass the
new arguments genesisTime, initialHeight and chainID to InitChain.
In `@node/execution_test.go`:
- Around line 41-42: The mock Return value for mockExec.On("ExecuteTxs", ...) is
using expectedNewStateRoot (a []byte) but ExecuteTxs now returns
(coreexecutor.ExecuteResult, error); update the mock to return a properly
constructed coreexecutor.ExecuteResult instance (e.g., expectedExecuteResult)
and nil instead of a []byte so the type assertion in the mock wrapper succeeds;
locate the mock setup around mockExec.On and replace the second return value
with the ExecuteResult object (ensure it contains the new state root previously
held in expectedNewStateRoot and any other required fields) and keep the error
as nil.
In `@types/header.go`:
- Around line 85-98: Add a String() method to the Header type that returns a
human-readable representation and explicitly includes the NextProposerAddress
field; implement Header.String() so it formats core fields (including
NextProposerAddress) for debugging, and ensure it does not mutate state (so it
must not change cachedHash or call InvalidateHash()); refer to existing
serialization/hash helpers (Header.Hash, MarshalBinary) for canonical field
ordering if needed, and place the method on the Header receiver to satisfy the
coding guideline requiring String() on core types.
---
Nitpick comments:
In `@block/internal/common/replay.go`:
- Around line 188-201: The proposer-rotation validation and resolution logic
duplicated in block/internal/common/replay.go (the NextProposerAddress checks),
block/internal/syncing/syncer.go, and block/internal/executing/executor.go
should be extracted into shared helpers (suggested names:
types.ValidateNextProposer(header, executionNext) error and
types.ResolveNextProposer(header, executionNext) []byte) that implement the same
precedence logic used by (*State).NextState; replace the duplicated checks in
replay.go (the NextProposerAddress branch and the "execution unchanged" branch)
and the other callers with calls to these helpers, ensuring return error
behavior and resolved proposer byte slice are preserved and update any unit
tests/usages accordingly.
In `@block/internal/syncing/assert.go`:
- Around line 11-17: Remove the unused genesis parameter and its type from the
function signature of assertValidSignedData and delete the unused genesis
import; then update any call sites (e.g., the call that currently passes
r.genesis) to stop supplying that extra argument so they match the new
signature. Ensure the function remains: func assertValidSignedData(signedData
*types.SignedData) error and that all callers (such as the one in da retriever
code that passed r.genesis) are adjusted accordingly.
In `@block/internal/syncing/syncer.go`:
- Around line 405-415: The method initialProposerAddress in Syncer is
byte-for-byte identical to (*Executor).initialProposerAddress; extract the logic
into a shared helper (e.g., in block/internal/common) and have both
Syncer.initialProposerAddress and Executor.initialProposerAddress call it.
Create a helper function (e.g., InitialProposerAddress(ctx, exec, genesis,
logger)) that accepts the executor interface used by both types, the genesis
struct, and a logger, implements the current fallback semantics and copying
behavior, and return []byte; then replace the bodies of
Syncer.initialProposerAddress and Executor.initialProposerAddress to delegate to
that helper.
In `@core/README.md`:
- Around line 30-37: Add a self-contained definition and brief doc comment for
the ExecutionInfo struct next to ExecuteResult so readers can see the full
contract; create a clear comment above the type and include the fields used by
GetExecutionInfo (e.g., any execution parameters like StateRoot, BlockNumber,
Timestamp, ProposerAddress or other project-specific fields) under the name
ExecutionInfo to mirror how ExecuteResult is declared and referenced by
GetExecutionInfo.
In `@execution/evm/execution.go`:
- Around line 347-464: Add a short clarifying comment in ExecuteTxs (near the
payload attributes/forkchoice update logic where suggestedFeeRecipient is set or
where ExecMeta is saved) stating that this EVM executor never sets
NextProposerAddress and therefore EVM-backed chains are treated as non-rotating
by the consensus layer; also note that contributors should not populate
NextProposerAddress from GetExecutionInfo without a corresponding
execution-layer contract change to avoid syncer/executor mismatch. Reference
ExecuteTxs, evPayloadAttrs, and saveExecMeta in the comment so future readers
know where the behavior is enforced.
In `@execution/grpc/client_test.go`:
- Around line 145-188: The test TestClient_ExecuteTxs only checks
UpdatedStateRoot; modify the mockExecutor.executeTxsFunc to return a non-empty
NextProposerAddress in the execution.ExecuteResult and add an assertion after
calling client.ExecuteTxs to verify result.NextProposerAddress equals the
expected value, ensuring the field round-trips through NewExecutorServiceHandler
and NewClient via client.ExecuteTxs.
In `@execution/grpc/server_test.go`:
- Around line 183-291: Add a test case that exercises NextProposerAddress
propagation: in TestServer_ExecuteTxs add a "next proposer" case where
mockExecutor.executeTxsFunc returns execution.ExecuteResult{UpdatedStateRoot:
expectedStateRoot, NextProposerAddress: []byte("proposer_addr")}; call
server.ExecuteTxs and assert resp.Msg.NextProposerAddress equals that byte slice
(compare as bytes or string like other assertions). Also add/extend the
GetExecutionInfo test to have the mock for GetExecutionInfo/ExecuteInfo return
info.NextProposerAddress set and assert resp.Msg.NextProposerAddress equals it
so both server.ExecuteTxs and GetExecutionInfo (info.NextProposerAddress) are
covered.
In `@pkg/telemetry/executor_tracing_test.go`:
- Around line 180-190: The mock currently returns a raw []byte
(Return(expectedStateRoot, nil)) which relies on a legacy shim; update the mock
expectation for mockExec.EXPECT().ExecuteTxs(...) to return a
coreexec.ExecuteResult value instead (e.g.
Return(coreexec.ExecuteResult{UpdatedStateRoot: expectedStateRoot}, nil)), and
add or adjust the import for the coreexec package if needed so the test no
longer depends on the legacy []byte→ExecuteResult shim.
In `@proto/evnode/v1/state.proto`:
- Line 19: Add a short protobuf doc comment for the State.next_proposer_address
field so the schema is self-describing: in the message State, above the field
next_proposer_address, add a comment like "Proposer expected to sign block at
LastBlockHeight+1; initialized from genesis and updated from execution results."
to mirror the documentation on Header.next_proposer_address and clarify
semantics.
In `@test/mocks/execution.go`:
- Around line 58-66: The mock loosened the Return signature to Return(result
interface{}, err error) and added a special-case for []byte in the switch, which
defeats compile-time safety and will be lost on regen; revert Return to the
strongly-typed signature Return(result execution.ExecuteResult, err error),
remove the legacy []byte handling in the switch inside the mocked method
(restore the original branches to handle nil and execution.ExecuteResult only),
and update any test call sites that currently call .Return(stateRootBytes, nil)
to pass execution.ExecuteResult{UpdatedStateRoot: stateRootBytes} instead so
RunAndReturn and Return share the same typed execution.ExecuteResult signature.
- Around line 220-258: The GetExecutionInfo method on MockExecutor has a
hand-edited guard (the len(_mock.ExpectedCalls)==0 check and the hasExpectation
loop) that silently returns a zero-value instead of letting the
mockery-generated Called(...) path raise the usual "unexpected call" panic;
remove the early-return logic (both the len(...) check and the hasExpectation
loop and related conditional return) so GetExecutionInfo always calls
_mock.Called(ctx) and lets Called handle unexpected calls, and instead implement
optional behavior outside this generated file (either add a shared test helper
that sets EXPECT().GetExecutionInfo(...).Maybe() or create a non-generated
wrapper around MockExecutor for accommodating optional calls).
In `@test/mocks/height_aware_executor.go`:
- Around line 74-91: The mock currently silently returns a zero-value from
MockHeightAwareExecutor.GetExecutionInfo when no expectation is present; change
this to be explicit by adding an opt-in flag (e.g., add a bool field
defaultExecutionInfo and a helper method WithDefaultExecutionInfo() on
MockHeightAwareExecutor) and modify GetExecutionInfo to return the zero-value
only when that flag is set, otherwise call m.Called(ctx) and surface the
missing-expectation (e.g., return the call result or an error/assertion) so
unexpected calls are not silently ignored; include a short comment on the
compatibility intent adjacent to the new helper/flag.
- Around line 47-59: The default branch in MockHeightAwareExecutor.ExecuteTxs
currently re-type-asserts args.Get(0).(execution.ExecuteResult) which will panic
for unexpected types; change the default to return a clear error instead of
panicking by constructing and returning an error (e.g., via fmt.Errorf) that
describes the unexpected type from args.Get(0) and the call context (function
ExecuteTxs and parameters such as txs/blockHeight), so tests get an explicit,
informative failure when a wrong type is stubbed.
In `@types/state.go`:
- Around line 46-54: Change NextState to accept an explicit optional
nextProposerOverride []byte instead of the variadic nextProposerAddress
...[]byte: update the signature of State.NextState(header Header, stateRoot
[]byte, nextProposerOverride []byte), replace the variadic logic in the body
(the len(nextProposerAddress)/indexing) with a nil-check on
nextProposerOverride, set nextProposer := header.NextProposerAddress then if
nextProposerOverride != nil && len(nextProposerOverride) > 0 use it, and fall
back to header.ProposerAddress if still empty; update all callers of NextState
to pass nil when no override is required.
🪄 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: eab9081d-7d38-4df3-8192-2a0e92e9e2d4
⛔ Files ignored due to path filters (3)
types/pb/evnode/v1/evnode.pb.gois excluded by!**/*.pb.gotypes/pb/evnode/v1/execution.pb.gois excluded by!**/*.pb.gotypes/pb/evnode/v1/state.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (50)
apps/evm/go.modapps/grpc/go.modapps/testapp/go.modapps/testapp/kv/kvexecutor.goapps/testapp/kv/kvexecutor_test.goblock/internal/common/replay.goblock/internal/executing/executor.goblock/internal/executing/executor_benchmark_test.goblock/internal/executing/executor_logic_test.goblock/internal/reaping/bench_test.goblock/internal/submitting/da_submitter.goblock/internal/syncing/assert.goblock/internal/syncing/da_retriever.goblock/internal/syncing/da_retriever_test.goblock/internal/syncing/p2p_handler.goblock/internal/syncing/p2p_handler_test.goblock/internal/syncing/raft_retriever.goblock/internal/syncing/syncer.goblock/internal/syncing/syncer_test.goclient/crates/types/src/proto/evnode.v1.messages.rsclient/crates/types/src/proto/evnode.v1.services.rscore/README.mdcore/execution/dummy.gocore/execution/dummy_test.gocore/execution/execution.godocs/adr/adr-023-execution-owned-proposer-rotation.mddocs/getting-started/custom/implement-executor.mddocs/reference/interfaces/executor.mdexecution/evm/execution.goexecution/evm/go.modexecution/evm/test/go.modexecution/grpc/client.goexecution/grpc/client_test.goexecution/grpc/go.modexecution/grpc/server.goexecution/grpc/server_test.gogo.modnode/execution_test.gopkg/rpc/server/server.gopkg/telemetry/executor_tracing.gopkg/telemetry/executor_tracing_test.goproto/evnode/v1/evnode.protoproto/evnode/v1/execution.protoproto/evnode/v1/state.prototest/e2e/go.modtest/mocks/execution.gotest/mocks/height_aware_executor.gotypes/header.gotypes/serialization.gotypes/state.go
💤 Files with no reviewable changes (3)
- block/internal/syncing/p2p_handler.go
- block/internal/syncing/raft_retriever.go
- block/internal/syncing/da_retriever.go
✅ Files skipped from review due to trivial changes (3)
- apps/evm/go.mod
- proto/evnode/v1/evnode.proto
- docs/adr/adr-023-execution-owned-proposer-rotation.md
🚧 Files skipped from review as they are similar to previous changes (1)
- block/internal/executing/executor.go
|
|
||
| // signData signs unsigned SignedData structs returned from cache | ||
| func (s *DASubmitter) signData(ctx context.Context, unsignedDataList []*types.SignedData, unsignedDataListBz [][]byte, signer signer.Signer, genesis genesis.Genesis) ([]*types.SignedData, [][]byte, error) { | ||
| func (s *DASubmitter) signData(ctx context.Context, unsignedDataList []*types.SignedData, unsignedDataListBz [][]byte, signer signer.Signer, _ genesis.Genesis) ([]*types.SignedData, [][]byte, error) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining usages of genesis inside SubmitData/DASubmitter to confirm the param is dead weight.
rg -nP -C3 '\bSubmitData\s*\(' --type=go
rg -nP -C2 '\.genesis\b' --type=go block/internal/submitting/Repository: evstack/ev-node
Length of output: 10867
🏁 Script executed:
# Check the signData implementation and its call site
cat -n block/internal/submitting/da_submitter.go | sed -n '430,480p'Repository: evstack/ev-node
Length of output: 2303
🏁 Script executed:
# Look at the call site around line 438
cat -n block/internal/submitting/da_submitter.go | sed -n '435,445p'Repository: evstack/ev-node
Length of output: 462
🏁 Script executed:
# Search for all usages of s.genesis in da_submitter.go (not in tests)
rg -n 's\.genesis' block/internal/submitting/da_submitter.goRepository: evstack/ev-node
Length of output: 41
🏁 Script executed:
# Also check if SubmitData method body uses the genesis parameter
sed -n '428,440p' block/internal/submitting/da_submitter.goRepository: evstack/ev-node
Length of output: 735
🏁 Script executed:
# Check the full SubmitData implementation to see if genesis is used anywhere
sed -n '428,469p' block/internal/submitting/da_submitter.goRepository: evstack/ev-node
Length of output: 1711
🏁 Script executed:
# Verify s.genesis struct field usage in the entire da_submitter.go file
rg -c 's\.genesis' block/internal/submitting/da_submitter.go || echo "No matches found"Repository: evstack/ev-node
Length of output: 76
🏁 Script executed:
# Check the DASubmitter struct definition and constructor to see where genesis is set
sed -n '100,200p' block/internal/submitting/da_submitter.go | head -100Repository: evstack/ev-node
Length of output: 3382
Remove the unused genesis parameter from signData.
The genesis parameter is marked as explicitly unused (with _) and is neither read nor referenced in the signData implementation. Within SubmitData, it is only passed through to signData on line 438. Since signData is unexported, this is a safe local cleanup.
Changes required:
-func (s *DASubmitter) signData(ctx context.Context, unsignedDataList []*types.SignedData, unsignedDataListBz [][]byte, signer signer.Signer, _ genesis.Genesis) ([]*types.SignedData, [][]byte, error) {
+func (s *DASubmitter) signData(ctx context.Context, unsignedDataList []*types.SignedData, unsignedDataListBz [][]byte, signer signer.Signer) ([]*types.SignedData, [][]byte, error) {- signedDataList, signedDataListBz, err := s.signData(ctx, unsignedDataList, marshalledData, signer, genesis)
+ signedDataList, signedDataListBz, err := s.signData(ctx, unsignedDataList, marshalledData, signer)Additionally, consider whether SubmitData's genesis parameter and the DASubmitter.genesis field have any actual use; they appear to exist only for threading through to signData and are never otherwise referenced in the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/submitting/da_submitter.go` at line 472, Remove the unused
genesis parameter from the unexported method signData by updating its signature
to drop the genesis.Genesis parameter and removing any corresponding argument
passed from SubmitData; update the call site in SubmitData (where signData is
invoked) to stop passing genesis, and then remove or evaluate the
DASubmitter.genesis field and SubmitData's genesis parameter
(DASubmitter.genesis and SubmitData) — if they are not used elsewhere, delete
the field and parameter to clean up dead code.
| if len(state.NextProposerAddress) == 0 { | ||
| state.NextProposerAddress = s.initialProposerAddress(s.ctx) | ||
| } |
There was a problem hiding this comment.
Upgrade path silently resets NextProposerAddress to genesis proposer.
If a pre-existing on-disk state (with LastBlockHeight > 0) has an empty NextProposerAddress — e.g., a node upgrading across this change — it will be repaired to the genesis proposer. That is correct only when execution has never rotated the proposer. Consider either guarding this path so the repair only applies at genesis (LastBlockHeight == InitialHeight - 1), or logging a warning to make the reset observable during upgrades.
🛠️ Suggested guard
- if len(state.NextProposerAddress) == 0 {
- state.NextProposerAddress = s.initialProposerAddress(s.ctx)
+ if len(state.NextProposerAddress) == 0 {
+ state.NextProposerAddress = s.initialProposerAddress(s.ctx)
+ if state.LastBlockHeight > s.genesis.InitialHeight-1 {
+ s.logger.Warn().
+ Uint64("height", state.LastBlockHeight).
+ Msg("state loaded without NextProposerAddress; repaired from execution/genesis — verify chain has not rotated proposer before upgrade")
+ }
}📝 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.
| if len(state.NextProposerAddress) == 0 { | |
| state.NextProposerAddress = s.initialProposerAddress(s.ctx) | |
| } | |
| if len(state.NextProposerAddress) == 0 { | |
| state.NextProposerAddress = s.initialProposerAddress(s.ctx) | |
| if state.LastBlockHeight > s.genesis.InitialHeight-1 { | |
| s.logger.Warn(). | |
| Uint64("height", state.LastBlockHeight). | |
| Msg("state loaded without NextProposerAddress; repaired from execution/genesis — verify chain has not rotated proposer before upgrade") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/syncing/syncer.go` around lines 332 - 334, The code
unconditionally repairs an empty state.NextProposerAddress to the genesis
proposer; change this so the repair only runs for true genesis states (check
state.LastBlockHeight == state.InitialHeight - 1 or equivalent) and otherwise do
not overwrite NextProposerAddress; additionally, where you skip the repair, emit
a warning (e.g., s.logger.Warn/Warnf) indicating that NextProposerAddress was
empty on upgrade so operators can observe it; update the block around the call
to s.initialProposerAddress(s.ctx) to perform the LastBlockHeight guard and
warning.
|
|
||
| // NextProposerAddress is the proposer address that should sign the next | ||
| // block from the execution layer's current view. Empty means unchanged or | ||
| // unavailable, and callers should fall back to their current proposer. | ||
| NextProposerAddress []byte | ||
| } | ||
|
|
||
| // ExecuteResult contains execution output that consensus must persist. | ||
| type ExecuteResult struct { | ||
| // UpdatedStateRoot is the new state root after executing transactions. | ||
| UpdatedStateRoot []byte | ||
|
|
||
| // NextProposerAddress is the proposer address selected by execution for the | ||
| // next block. Empty means the current proposer remains active. | ||
| NextProposerAddress []byte | ||
| } |
There was a problem hiding this comment.
Clarify the relationship between ExecutionInfo.NextProposerAddress and ExecuteResult.NextProposerAddress.
The same field is now exposed in two places with subtly different semantics:
ExecutionInfo.NextProposerAddress— "the proposer address that should sign the next block from the execution layer's current view" (polled viaGetExecutionInfo).ExecuteResult.NextProposerAddress— "the proposer address selected by execution for the next block" (returned fromExecuteTxs).
Without a documented precedence rule, consumers (block executor, syncer, replay) may consult either source and reach inconsistent conclusions, especially around rotation boundaries. Please either:
- Keep only one authoritative source (prefer
ExecuteResultsince it is tied to a specific executed height and deterministic), or - Document which takes precedence and for which height each value applies (is
ExecutionInfo.NextProposerAddressthe proposer forlatestHeight+1orlatestHeight+2?).
Given the PR comment that the design is being revised, this is worth pinning down before the interface is finalized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/execution/execution.go` around lines 135 - 150, Decide and document a
single authoritative source for the proposer rotation: prefer
ExecuteResult.NextProposerAddress as authoritative for the proposer of the
immediately next block (i.e., applies to executedHeight+1), and update
ExecutionInfo.NextProposerAddress to be advisory/deprecated (used only when no
ExecuteResult is available from ExecuteTxs); change the comments for
ExecuteResult.NextProposerAddress to state it deterministically applies to
executedHeight+1 and update the comment on ExecutionInfo.NextProposerAddress to
explicitly say it is a best-effort view only and not authoritative (or remove
the field from ExecutionInfo entirely if you prefer a single-source API).
| // ExecuteResult contains execution output that consensus must persist. | ||
| type ExecuteResult struct { | ||
| // UpdatedStateRoot is the new state root after executing transactions. | ||
| UpdatedStateRoot []byte | ||
|
|
||
| // NextProposerAddress is the proposer address selected by execution for the | ||
| // next block. Empty means the current proposer remains active. | ||
| NextProposerAddress []byte | ||
| } |
There was a problem hiding this comment.
Specify the reference block height for NextProposerAddress and whether the field is mandatory.
ExecuteResult.NextProposerAddress documents that an empty value means "the current proposer remains active", which is a safe default. Two things that will bite downstream implementations if not nailed down in this interface doc:
- For a call
ExecuteTxs(..., blockHeight=H, ...), is the returnedNextProposerAddressthe proposer for blockH+1or for blockHitself? Consensus persists this intoState.NextProposerAddressand validates subsequent headers against it (seetypes/state.go:79-80), so ambiguity here will cause consensus mismatches between executor implementations. - Is an execution layer allowed to change the proposer at arbitrary heights, or only at specific rotation points? Implementations such as
core/execution/dummy.gocurrently leave this field unset — make sure the documented contract explicitly permits that.
📝 Proposed doc clarification
// ExecuteResult contains execution output that consensus must persist.
type ExecuteResult struct {
// UpdatedStateRoot is the new state root after executing transactions.
UpdatedStateRoot []byte
- // NextProposerAddress is the proposer address selected by execution for the
- // next block. Empty means the current proposer remains active.
+ // NextProposerAddress is the proposer address selected by the execution
+ // layer to sign block blockHeight+1 (the block immediately after the one
+ // just executed). An empty value signals that the current proposer
+ // remains active; implementations that do not support proposer rotation
+ // MUST leave this field empty.
NextProposerAddress []byte
}📝 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.
| // ExecuteResult contains execution output that consensus must persist. | |
| type ExecuteResult struct { | |
| // UpdatedStateRoot is the new state root after executing transactions. | |
| UpdatedStateRoot []byte | |
| // NextProposerAddress is the proposer address selected by execution for the | |
| // next block. Empty means the current proposer remains active. | |
| NextProposerAddress []byte | |
| } | |
| // ExecuteResult contains execution output that consensus must persist. | |
| type ExecuteResult struct { | |
| // UpdatedStateRoot is the new state root after executing transactions. | |
| UpdatedStateRoot []byte | |
| // NextProposerAddress is the proposer address selected by the execution | |
| // layer to sign block blockHeight+1 (the block immediately after the one | |
| // just executed). An empty value signals that the current proposer | |
| // remains active; implementations that do not support proposer rotation | |
| // MUST leave this field empty. | |
| NextProposerAddress []byte | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/execution/execution.go` around lines 142 - 150, Clarify that
ExecuteResult.NextProposerAddress, returned by ExecuteTxs for a call with
blockHeight=H, indicates the proposer to be used for block H+1 (not H), state
that the field is optional (empty means “retain current proposer”), and
explicitly permit executors to only set it at agreed rotation points; update the
ExecuteResult doc comment to state this unambiguously and ensure ExecuteTxs and
State.NextProposerAddress handling/validation (and the dummy executor) conform
to this contract.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
block/internal/syncing/syncer_test.go (1)
928-960:⚠️ Potential issue | 🔴 CriticalUpdate all
ExecuteTxsmock setups insyncer_test.goto returnExecuteResultinstead of[]byte.The
ExecuteTxsinterface was changed to return(ExecuteResult, error)(seecore/execution/execution.go:68), but 23 mock setups in this file still return[]byteas the first value. When the test runs, mockery will panic attempting to type-assert[]bytetoExecuteResultat runtime.Affected test locations: lines 292–293, 351–352, 360–361, 428–429, 928–960 (four table entries), 1505–1506, and 1652–1653. Wrap all
[]byte(...)returns inExecuteResult{UpdatedStateRoot: ...}and useExecuteResult{}for error cases.Example fix for lines 928–960
{ name: "success on first attempt", setupMock: func(exec *testmocks.MockExecutor) { exec.On("ExecuteTxs", mock.Anything, mock.Anything, uint64(100), mock.Anything, mock.Anything). - Return([]byte("new-hash"), nil).Once() + Return(coreexecutor.ExecuteResult{UpdatedStateRoot: []byte("new-hash")}, nil).Once() }, expectSuccess: true, expectHash: []byte("new-hash"), }, { name: "success on second attempt", setupMock: func(exec *testmocks.MockExecutor) { exec.On("ExecuteTxs", mock.Anything, mock.Anything, uint64(100), mock.Anything, mock.Anything). - Return([]byte(nil), errors.New("temporary failure")).Once() + Return(coreexecutor.ExecuteResult{}, errors.New("temporary failure")).Once() exec.On("ExecuteTxs", mock.Anything, mock.Anything, uint64(100), mock.Anything, mock.Anything). - Return([]byte("new-hash"), nil).Once() + Return(coreexecutor.ExecuteResult{UpdatedStateRoot: []byte("new-hash")}, nil).Once() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer_test.go` around lines 928 - 960, The ExecuteTxs mock returns are still providing []byte where the real signature now returns (ExecuteResult, error); update every mock On("ExecuteTxs", ...) in syncer_test.go (e.g., the table entries using setupMock and the calls around ExecuteTxs) to return ExecuteResult{UpdatedStateRoot: []byte("new-hash")} for successful cases and ExecuteResult{} (empty struct) for error cases (e.g., temporary/persistent failures), keeping the same Errors for the second return value; ensure you update all occurrences referenced in the comment (including the entries around the "success on second/third attempt" and "failure after max retries" tests and other listed lines) so mockery type-asserts receive ExecuteResult instead of []byte.
♻️ Duplicate comments (1)
block/internal/syncing/syncer.go (1)
332-334:⚠️ Potential issue | 🟠 MajorAvoid repairing historical state before the execution layer is replayed.
This runs before
execReplayer.SyncToHeight, so an upgraded/restarted node can persist aNextProposerAddressfrom stale execution state or the genesis fallback. That can make the next valid block fail state validation. Only synthesize this field for true genesis, or repair it after execution has been synced tostate.LastBlockHeight.🛠️ Suggested guard
- if len(state.NextProposerAddress) == 0 { - state.NextProposerAddress = s.initialProposerAddress(s.ctx) - } + if len(state.NextProposerAddress) == 0 { + if state.LastBlockHeight == state.InitialHeight-1 { + state.NextProposerAddress = s.initialProposerAddress(s.ctx) + } else { + s.logger.Warn(). + Uint64("height", state.LastBlockHeight). + Msg("state loaded without NextProposerAddress; defer proposer repair until execution is synced") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer.go` around lines 332 - 334, The code currently backfills state.NextProposerAddress unconditionally using s.initialProposerAddress(s.ctx), which can repair historical state before the execution layer is replayed and lead to invalid proposer info; instead only synthesize NextProposerAddress for true genesis (when state.LastBlockHeight == 0) or perform this repair only after the execution layer has been synced (after execReplayer.SyncToHeight finishes syncing to state.LastBlockHeight). Locate the block that sets state.NextProposerAddress and change it to guarded logic: if state.LastBlockHeight == 0 then set using s.initialProposerAddress(s.ctx), otherwise defer setting until after execReplayer.SyncToHeight has completed (or add a post-sync repair step that checks and sets state.NextProposerAddress).
🧹 Nitpick comments (5)
block/internal/syncing/syncer_test.go (1)
194-215: Add a happy‑path case and consider a table-driven form.The new test only exercises the error path ("unexpected proposer"). Without a matching‑proposer case, a regression that causes
ValidateBlockto always error would still pass this test. Consider restructuring as a small table covering bothNextProposerAddress == header.Signer.Address(expect nil) and the mismatch case.♻️ Proposed table‑driven form
func TestSyncer_ValidateBlock_UsesStateNextProposer(t *testing.T) { - addr, _, _ := buildSyncTestSigner(t) - badAddr, badPub, badSigner := buildSyncTestSigner(t) - - gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second)} - data := makeData(gen.ChainID, 1, 1) - _, header := makeSignedHeaderBytes(t, gen.ChainID, 1, badAddr, badPub, badSigner, []byte("app0"), data, nil) - - s := &Syncer{logger: zerolog.Nop()} - state := types.State{ - ChainID: gen.ChainID, - InitialHeight: gen.InitialHeight, - LastBlockHeight: gen.InitialHeight - 1, - LastBlockTime: gen.StartTime, - AppHash: []byte("app0"), - NextProposerAddress: addr, - } - - err := s.ValidateBlock(t.Context(), state, data, header) - require.Error(t, err) - require.Contains(t, err.Error(), "unexpected proposer") + addr, pub, signer := buildSyncTestSigner(t) + badAddr, badPub, badSigner := buildSyncTestSigner(t) + gen := genesis.Genesis{ChainID: "tchain", InitialHeight: 1, StartTime: time.Now().Add(-time.Second)} + + tests := []struct { + name string + proposer []byte + pub crypto.PubKey + signer signerpkg.Signer + expectErr string + }{ + {name: "matches state next proposer", proposer: addr, pub: pub, signer: signer}, + {name: "mismatches state next proposer", proposer: badAddr, pub: badPub, signer: badSigner, expectErr: "unexpected proposer"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + data := makeData(gen.ChainID, 1, 1) + _, header := makeSignedHeaderBytes(t, gen.ChainID, 1, tc.proposer, tc.pub, tc.signer, []byte("app0"), data, nil) + s := &Syncer{logger: zerolog.Nop()} + state := types.State{ + ChainID: gen.ChainID, + InitialHeight: gen.InitialHeight, + LastBlockHeight: gen.InitialHeight - 1, + LastBlockTime: gen.StartTime, + AppHash: []byte("app0"), + NextProposerAddress: addr, + } + err := s.ValidateBlock(t.Context(), state, data, header) + if tc.expectErr == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tc.expectErr) + } + }) + } }As per coding guidelines: "Use table-driven tests where appropriate".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/syncing/syncer_test.go` around lines 194 - 215, Add a happy-path case (and optionally convert to a small table-driven test) for TestSyncer_ValidateBlock_UsesStateNextProposer so it asserts that Syncer.ValidateBlock returns nil when state.NextProposerAddress matches header.Signer.Address; keep the existing mismatch case that expects an "unexpected proposer" error. Use the existing helpers (buildSyncTestSigner, makeSignedHeaderBytes) to produce a header signed by addr and a header signed by badAddr, and reference TestSyncer_ValidateBlock_UsesStateNextProposer, Syncer.ValidateBlock, NextProposerAddress and header.Signer.Address when locating where to add the passing case or table entries.docs/adr/adr-023-execution-owned-proposer-rotation.md (1)
1-84: ADR is well-scoped and consistent with the implementation. A few small additions would help.Content, structure, and scope look good — the ADR accurately documents the
ExecuteResult/GetExecutionInfocontract, the "empty = unchanged" rule, and the signer validation against the previous state'sNextProposerAddress. Minor suggestions:
- Migration / upgrade section: The ADR doesn't mention behavior for existing deployments whose persisted
types.StatepredatesNextProposerAddress. The code falls back toinitialProposerAddress(ctx)on the fly (executor.go Lines 247–249). A short "Migration" note would close that gap.- Operator rotation UX:
Executor.initializeStatenow fails withErrNotProposerwhen the local signer is not the current next proposer. That's correct, but it implies operators must stop the executor and start the syncer when they're rotated out. Worth calling out under "Consequences" or "Security Considerations".- Light-client gap: the "Negative" bullet already flags this, but it would be useful to explicitly reference a future ADR or issue number for the header-proof approach so the follow-up is tracked.
The LanguageTool hint about hyphenating "future proof/certificate mechanism" is a false positive in context (the slash separates two alternatives), feel free to ignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/adr-023-execution-owned-proposer-rotation.md` around lines 1 - 84, Add a short "Migration" note and two clarifying sentences: mention that persisted types.State lacking NextProposerAddress will fallback at runtime via initialProposerAddress (see Executor.InitializeState / initialProposerAddress behavior), call out operator UX that Executor.initializeState can return ErrNotProposer requiring the operator to stop the executor and run the syncer when rotated out, and add a follow-up reference (ADR or issue) for the header-proof/light-client gap referenced under "Consequences"/"Negative" so the follow-up work is tracked (referencing ExecuteResult/GetExecutionInfo and NextProposerAddress).block/internal/executing/executor.go (3)
393-405:assertConfiguredSignersafely dereferencese.signerbecause of the NewExecutor guard — worth a short doc comment.
e.signer.GetAddress()is called unconditionally after theBasedSequencershort‑circuit. That's fine today becauseNewExecutorrejectssigner == nilwhen!BasedSequencer, but this invariant is easy to miss if the constructor is refactored. Consider adding a one-line comment on this helper (and on the exported constructor's signer contract) to make the coupling explicit.As per coding guidelines: "Document exported types and functions in Go code" — extending this to internal helpers with non-obvious preconditions avoids future NPEs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/executing/executor.go` around lines 393 - 405, Add a short doc comment to assertConfiguredSigner explaining its precondition that e.signer must be non-nil when e.config.Node.BasedSequencer is false (it calls e.signer.GetAddress()), and also annotate the NewExecutor constructor comment to explicitly state that signer must be non-nil for non-BasedSequencer nodes; reference the functions/assertConfiguredSigner, NewExecutor, and the e.signer / BasedSequencer coupling so future readers won't miss the invariant.
722-774: CreateBlock: proposer derivation and new signer check look correct.Pulling
proposerAddressfromcurrentState.NextProposerAddress(with a genesis fallback) and then asserting the configured signer actually matches it is a solid defense-in-depth against "wrong node is producing" scenarios and aligns with the ADR. Raisingcommon.ErrNotProposerhere gives the caller a typed sentinel to handle cleanly.Minor suggestion: the same
proposerAddress := currentState.NextProposerAddress; if len(...)==0 { ... genesis ... }pattern now appears ininitialProposerAddress(different source) and here. Consider a tiny helperresolveProposer(state, genesis)to keep the fallback rule in one place if this pattern proliferates (e.g., the syncer may have an equivalent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/executing/executor.go` around lines 722 - 774, Duplicate proposer-address fallback logic appears in CreateBlock (proposerAddress derived from currentState.NextProposerAddress with fallback to e.genesis.ProposerAddress) and in initialProposerAddress; extract this into a single helper function (e.g., resolveProposer(state, genesis) or resolveProposerAddress(nextProposer []byte, genesisProposer []byte) and call it from both locations to centralize the fallback rule, update callers in CreateBlock (proposerAddress variable) and wherever initialProposerAddress logic exists to use the new helper, and ensure behavior and error semantics remain unchanged.
888-916: Finalreturn ExecuteResult{}, nilat Line 915 is unreachable.The loop runs
attempt = 1..MaxRetriesBeforeHaltwith a return on both the success and final-failure branches, so the tail return after the loop is only reachable whenMaxRetriesBeforeHalt == 0. Since halting silently with a zero-valuedExecuteResultwould be worse than panicking, either make the intent explicit or drop this fall‑through.♻️ Proposed fix
return result, nil } - - return coreexecutor.ExecuteResult{}, nil + // Unreachable while MaxRetriesBeforeHalt > 0; guard against misconfiguration. + return coreexecutor.ExecuteResult{}, fmt.Errorf("executeTxsWithRetry: no attempts executed (MaxRetriesBeforeHalt=%d)", common.MaxRetriesBeforeHalt) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@block/internal/executing/executor.go` around lines 888 - 916, The final return in executeTxsWithRetry is unreachable unless common.MaxRetriesBeforeHalt == 0; remove the silent zero-valued return and make the intent explicit by returning an error when the loop exits without result (e.g., return coreexecutor.ExecuteResult{}, fmt.Errorf("executeTxsWithRetry: no attempts made; MaxRetriesBeforeHalt == 0") ), or alternatively panic/log.Fatal if that better matches project policy; update the tail return accordingly and keep references to executeTxsWithRetry and common.MaxRetriesBeforeHalt to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@block/internal/common/replay.go`:
- Around line 152-160: When constructing prevState for the first block in replay
(the height == s.genesis.InitialHeight branch), don't hardcode
s.genesis.ProposerAddress; instead mirror Syncer.initializeState() by querying
GetExecutionInfo().NextProposerAddress and using it if non-empty, otherwise fall
back to s.genesis.ProposerAddress. Ensure you copy the byte slice (e.g., via
append([]byte(nil), ...)) when assigning NextProposerAddress to prevState so
replay uses the same initial proposer source as normal startup.
In `@block/internal/executing/executor.go`:
- Around line 237-252: Update ADR-023 and migration notes to document two
operational requirements introduced by
initializeState()/assertConfiguredSigner(): (1) Explicitly state that a node
whose signer/key has been rotated out will fail to start the Executor because
assertConfiguredSigner will detect a mismatch—include guidance for operators on
key rotation and restart workflows (e.g., ensure keys are present or
remove/reconfigure the node before restart). (2) Clarify startup state
derivation behavior when State.NextProposerAddress is empty: the executor uses
live execution state via GetExecutionInfo() as the source of truth per ADR-023,
and document expected recovery semantics for replay scenarios (whether to replay
from persisted block height or to trust live execution info) plus guidance to
avoid having execution state advance beyond persisted block height before
startup.
---
Outside diff comments:
In `@block/internal/syncing/syncer_test.go`:
- Around line 928-960: The ExecuteTxs mock returns are still providing []byte
where the real signature now returns (ExecuteResult, error); update every mock
On("ExecuteTxs", ...) in syncer_test.go (e.g., the table entries using setupMock
and the calls around ExecuteTxs) to return ExecuteResult{UpdatedStateRoot:
[]byte("new-hash")} for successful cases and ExecuteResult{} (empty struct) for
error cases (e.g., temporary/persistent failures), keeping the same Errors for
the second return value; ensure you update all occurrences referenced in the
comment (including the entries around the "success on second/third attempt" and
"failure after max retries" tests and other listed lines) so mockery
type-asserts receive ExecuteResult instead of []byte.
---
Duplicate comments:
In `@block/internal/syncing/syncer.go`:
- Around line 332-334: The code currently backfills state.NextProposerAddress
unconditionally using s.initialProposerAddress(s.ctx), which can repair
historical state before the execution layer is replayed and lead to invalid
proposer info; instead only synthesize NextProposerAddress for true genesis
(when state.LastBlockHeight == 0) or perform this repair only after the
execution layer has been synced (after execReplayer.SyncToHeight finishes
syncing to state.LastBlockHeight). Locate the block that sets
state.NextProposerAddress and change it to guarded logic: if
state.LastBlockHeight == 0 then set using s.initialProposerAddress(s.ctx),
otherwise defer setting until after execReplayer.SyncToHeight has completed (or
add a post-sync repair step that checks and sets state.NextProposerAddress).
---
Nitpick comments:
In `@block/internal/executing/executor.go`:
- Around line 393-405: Add a short doc comment to assertConfiguredSigner
explaining its precondition that e.signer must be non-nil when
e.config.Node.BasedSequencer is false (it calls e.signer.GetAddress()), and also
annotate the NewExecutor constructor comment to explicitly state that signer
must be non-nil for non-BasedSequencer nodes; reference the
functions/assertConfiguredSigner, NewExecutor, and the e.signer / BasedSequencer
coupling so future readers won't miss the invariant.
- Around line 722-774: Duplicate proposer-address fallback logic appears in
CreateBlock (proposerAddress derived from currentState.NextProposerAddress with
fallback to e.genesis.ProposerAddress) and in initialProposerAddress; extract
this into a single helper function (e.g., resolveProposer(state, genesis) or
resolveProposerAddress(nextProposer []byte, genesisProposer []byte) and call it
from both locations to centralize the fallback rule, update callers in
CreateBlock (proposerAddress variable) and wherever initialProposerAddress logic
exists to use the new helper, and ensure behavior and error semantics remain
unchanged.
- Around line 888-916: The final return in executeTxsWithRetry is unreachable
unless common.MaxRetriesBeforeHalt == 0; remove the silent zero-valued return
and make the intent explicit by returning an error when the loop exits without
result (e.g., return coreexecutor.ExecuteResult{},
fmt.Errorf("executeTxsWithRetry: no attempts made; MaxRetriesBeforeHalt == 0")
), or alternatively panic/log.Fatal if that better matches project policy;
update the tail return accordingly and keep references to executeTxsWithRetry
and common.MaxRetriesBeforeHalt to locate the change.
In `@block/internal/syncing/syncer_test.go`:
- Around line 194-215: Add a happy-path case (and optionally convert to a small
table-driven test) for TestSyncer_ValidateBlock_UsesStateNextProposer so it
asserts that Syncer.ValidateBlock returns nil when state.NextProposerAddress
matches header.Signer.Address; keep the existing mismatch case that expects an
"unexpected proposer" error. Use the existing helpers (buildSyncTestSigner,
makeSignedHeaderBytes) to produce a header signed by addr and a header signed by
badAddr, and reference TestSyncer_ValidateBlock_UsesStateNextProposer,
Syncer.ValidateBlock, NextProposerAddress and header.Signer.Address when
locating where to add the passing case or table entries.
In `@docs/adr/adr-023-execution-owned-proposer-rotation.md`:
- Around line 1-84: Add a short "Migration" note and two clarifying sentences:
mention that persisted types.State lacking NextProposerAddress will fallback at
runtime via initialProposerAddress (see Executor.InitializeState /
initialProposerAddress behavior), call out operator UX that
Executor.initializeState can return ErrNotProposer requiring the operator to
stop the executor and run the syncer when rotated out, and add a follow-up
reference (ADR or issue) for the header-proof/light-client gap referenced under
"Consequences"/"Negative" so the follow-up work is tracked (referencing
ExecuteResult/GetExecutionInfo and NextProposerAddress).
🪄 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: 09b895be-e291-4a71-858c-246696ddf70a
⛔ Files ignored due to path filters (1)
types/pb/evnode/v1/evnode.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
block/internal/common/replay.goblock/internal/executing/executor.goblock/internal/executing/executor_logic_test.goblock/internal/syncing/syncer.goblock/internal/syncing/syncer_test.goclient/crates/types/src/proto/evnode.v1.messages.rsclient/crates/types/src/proto/evnode.v1.services.rsdocs/adr/adr-023-execution-owned-proposer-rotation.mdproto/evnode/v1/evnode.prototypes/header.gotypes/serialization.gotypes/signed_header_test.gotypes/state.go
✅ Files skipped from review due to trivial changes (2)
- proto/evnode/v1/evnode.proto
- client/crates/types/src/proto/evnode.v1.messages.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- block/internal/executing/executor_logic_test.go
| if height == s.genesis.InitialHeight { | ||
| // For the first block, use genesis state. | ||
| prevState = types.State{ | ||
| ChainID: s.genesis.ChainID, | ||
| InitialHeight: s.genesis.InitialHeight, | ||
| LastBlockHeight: s.genesis.InitialHeight - 1, | ||
| LastBlockTime: s.genesis.StartTime, | ||
| AppHash: header.AppHash, // Genesis app hash (input to first block execution) | ||
| ChainID: s.genesis.ChainID, | ||
| InitialHeight: s.genesis.InitialHeight, | ||
| LastBlockHeight: s.genesis.InitialHeight - 1, | ||
| LastBlockTime: s.genesis.StartTime, | ||
| AppHash: header.AppHash, // Genesis app hash (input to first block execution) | ||
| NextProposerAddress: append([]byte(nil), s.genesis.ProposerAddress...), |
There was a problem hiding this comment.
Seed replay with the same initial proposer source as normal startup.
Syncer.initializeState() now prefers GetExecutionInfo().NextProposerAddress and only falls back to genesis when execution returns empty, but replay still hardcodes s.genesis.ProposerAddress here. If execution selects a different proposer at genesis, replaying the first block will compute a different proposer chain and can trip the new NextProposerAddress consistency check on restart.
🛠️ Suggested fix
- prevState = types.State{
+ nextProposer := append([]byte(nil), s.genesis.ProposerAddress...)
+ if info, infoErr := s.exec.GetExecutionInfo(ctx); infoErr == nil && len(info.NextProposerAddress) > 0 {
+ nextProposer = append([]byte(nil), info.NextProposerAddress...)
+ }
+ prevState = types.State{
ChainID: s.genesis.ChainID,
InitialHeight: s.genesis.InitialHeight,
LastBlockHeight: s.genesis.InitialHeight - 1,
LastBlockTime: s.genesis.StartTime,
AppHash: header.AppHash, // Genesis app hash (input to first block execution)
- NextProposerAddress: append([]byte(nil), s.genesis.ProposerAddress...),
+ NextProposerAddress: nextProposer,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/common/replay.go` around lines 152 - 160, When constructing
prevState for the first block in replay (the height == s.genesis.InitialHeight
branch), don't hardcode s.genesis.ProposerAddress; instead mirror
Syncer.initializeState() by querying GetExecutionInfo().NextProposerAddress and
using it if non-empty, otherwise fall back to s.genesis.ProposerAddress. Ensure
you copy the byte slice (e.g., via append([]byte(nil), ...)) when assigning
NextProposerAddress to prevState so replay uses the same initial proposer source
as normal startup.
| ChainID: e.genesis.ChainID, | ||
| InitialHeight: e.genesis.InitialHeight, | ||
| LastBlockHeight: e.genesis.InitialHeight - 1, | ||
| LastBlockTime: e.genesis.StartTime, | ||
| AppHash: stateRoot, | ||
| NextProposerAddress: e.initialProposerAddress(e.ctx), | ||
| // DA start height is usually 0 at InitChain unless it is a re-genesis or a based sequencer. | ||
| DAHeight: e.genesis.DAStartHeight, | ||
| } | ||
| } | ||
| if len(state.NextProposerAddress) == 0 { | ||
| state.NextProposerAddress = e.initialProposerAddress(e.ctx) | ||
| } | ||
| if err := e.assertConfiguredSigner(state.NextProposerAddress); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm NextState() semantics when NextProposerAddress arg is empty (unchanged vs overwrite)
ast-grep --lang go --pattern 'func (s State) NextState($$$) ($$$) {
$$$
}'
# Where else is State.NextProposerAddress written?
rg -nP --type=go -C2 '\bNextProposerAddress\s*[:=]' types blockRepository: evstack/ev-node
Length of output: 4654
🏁 Script executed:
# 1. Get full NextState() method signature and implementation
sed -n '55,70p' types/state.go
# 2. Find all calls to NextState() to see what proposer values are passed
rg -nP --type=go '\.NextState\(' block typesRepository: evstack/ev-node
Length of output: 1034
🏁 Script executed:
# Find initialProposerAddress() implementation
ast-grep --lang go --pattern 'func ($_ $_) initialProposerAddress($$$) ($$$) {
$$$
}'
# Also check if there's block version being set in initializeState
sed -n '230,260p' block/internal/executing/executor.goRepository: evstack/ev-node
Length of output: 1291
🏁 Script executed:
# Find initialProposerAddress() method definition
rg -nP --type=go -A10 'func \(.*\) initialProposerAddress' block/internal/executing/executor.go
# Also check syncer for the same method
rg -nP --type=go -A10 'func \(.*\) initialProposerAddress' block/internal/syncing/syncer.goRepository: evstack/ev-node
Length of output: 1023
🏁 Script executed:
# Check State version field and any version 11 references
rg -nP --type=go 'Version.*=' types/state.go | head -20
rg -nP '11' types/state.go
# Check if there's any ADR or migration docs
fd -e md -e txt | xargs grep -l -i 'proposer\|rotation\|version.*11' 2>/dev/null | head -10Repository: evstack/ev-node
Length of output: 542
🏁 Script executed:
# Check ADR-012 for proposer rotation details
sed -n '1,200p' docs/adr/adr-012-based-sequencing.md | head -150
# Also search for "NextProposerAddress" in ADRs to understand design decisions
rg -l 'NextProposerAddress' docs/adr/Repository: evstack/ev-node
Length of output: 5959
🏁 Script executed:
# Check ADR-023 for execution-owned proposer rotation details
cat docs/adr/adr-023-execution-owned-proposer-rotation.mdRepository: evstack/ev-node
Length of output: 4995
Document operational requirements for proposer rotation in migration/ADR consequences.
The code correctly implements execution-owned proposer rotation per ADR-023. Two documentation gaps should be addressed:
-
Breaking operational change: A node that has been rotated out as proposer cannot start the Executor. It will fail at
initializeState()whenassertConfiguredSigner()detects signer mismatch. This should be documented in ADR-023's Consequences section or in migration notes, as it affects how operators manage key rotation and node restart workflows. -
Startup state derivation: When a persisted
Statehas emptyNextProposerAddress,initializeState()(line 248) derives the active proposer fromGetExecutionInfo()(live execution state). This is correct—execution is the source of truth per ADR-023—but ADR-023 should clarify that nodes must not have execution state ahead of or divergent from the persisted block height during startup. For recovery scenarios (e.g., block replay after a crash), document whether to replay from persisted height or trust live execution info.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/executing/executor.go` around lines 237 - 252, Update ADR-023
and migration notes to document two operational requirements introduced by
initializeState()/assertConfiguredSigner(): (1) Explicitly state that a node
whose signer/key has been rotated out will fail to start the Executor because
assertConfiguredSigner will detect a mismatch—include guidance for operators on
key rotation and restart workflows (e.g., ensure keys are present or
remove/reconfigure the node before restart). (2) Clarify startup state
derivation behavior when State.NextProposerAddress is empty: the executor uses
live execution state via GetExecutionInfo() as the source of truth per ADR-023,
and document expected recovery semantics for replay scenarios (whether to replay
from persisted block height or to trust live execution info) plus guidance to
avoid having execution state advance beyond persisted block height before
startup.
2 participants
Overview
Add key rotation for proposers
Summary by CodeRabbit
New Features
Documentation