Open
Conversation
Contributor
📝 WalkthroughWalkthroughProposer key rotation added via a height-indexed Changes
Sequence DiagramsequenceDiagram
participant G as Genesis (ProposerSchedule)
participant Exec as Executor
participant Signer as Signer
participant DA as DA Submitter
participant Sync as Sync Validator
Exec->>G: LoadGenesis() -> EffectiveProposerSchedule
Exec->>G: HasScheduledProposer(signerAddr) at startup
G-->>Exec: membership OK/Err
Exec->>G: ProposerAtHeight(H)
G-->>Exec: ProposerEntry (address, pubkey?)
Exec->>Signer: GetAddress()/PubKey()
Signer-->>Exec: addr, pubkey
Exec->>G: ValidateProposer(H, addr, pubkey)
G-->>Exec: Validated / Error
Exec->>DA: SubmitData(height H')
DA->>G: ProposerAtHeight(H')
DA->>Signer: SignData()
DA->>G: ValidateProposer(H', signerAddr, pubKey)
G-->>DA: Validated / Error
Sync->>G: ProposerAtHeight(H)
Sync->>Sync: Validate header signer via ValidateProposer(H, addr, pubKey)
G-->>Sync: Validated / Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
Contributor
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Contributor
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3282 +/- ##
==========================================
+ Coverage 61.84% 62.82% +0.98%
==========================================
Files 122 123 +1
Lines 16241 13151 -3090
==========================================
- Hits 10044 8262 -1782
+ Misses 5312 3993 -1319
- Partials 885 896 +11
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}
Contributor
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
Comment on lines
+84
to
86
| if err := h.assertExpectedProposer(p2pHeader.SignedHeader); err != nil { | ||
| h.logger.Debug().Uint64("height", height).Err(err).Msg("invalid header from P2P") | ||
| return err |
Contributor
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).
julienrbrt
reviewed
Apr 23, 2026
| This is insufficient because the chain itself does not encode when the proposer changed, so historical sync | ||
| and validation become ambiguous. | ||
|
|
||
| ### 2. Re-issue a new genesis on each rotation |
Member
There was a problem hiding this comment.
Unless I missed something, this is the currently implemented way. Genesis will need to be updated in each node and restarted. Same for sequencer. I suppose this is OK, but the ADR make it seem it isn't
Contributor
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
Comment on lines
+143
to
+218
| gen := genesis.Genesis{ | ||
| ChainID: "test-chain", | ||
| InitialHeight: 1, | ||
| StartTime: time.Now().Add(-time.Second), | ||
| ProposerAddress: entry1.Address, | ||
| ProposerSchedule: []genesis.ProposerScheduleEntry{entry1, entry2}, | ||
| DAEpochForcedInclusion: 1, | ||
| } | ||
|
|
||
| executor, err := NewExecutor( | ||
| memStore, | ||
| nil, | ||
| nil, | ||
| newSigner, | ||
| cacheManager, | ||
| metrics, | ||
| config.DefaultConfig(), | ||
| gen, | ||
| nil, | ||
| nil, | ||
| zerolog.Nop(), | ||
| common.DefaultBlockOptions(), | ||
| make(chan error, 1), | ||
| nil, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| prevHeader := &types.SignedHeader{ | ||
| Header: types.Header{ | ||
| Version: types.InitStateVersion, | ||
| BaseHeader: types.BaseHeader{ | ||
| ChainID: gen.ChainID, | ||
| Height: 1, | ||
| Time: uint64(gen.StartTime.UnixNano()), | ||
| }, | ||
| AppHash: []byte("state-root-0"), | ||
| ProposerAddress: oldAddr, | ||
| DataHash: common.DataHashForEmptyTxs, | ||
| }, | ||
| Signature: types.Signature([]byte("sig-1")), | ||
| Signer: oldSignerInfo, | ||
| } | ||
| prevData := &types.Data{ | ||
| Metadata: &types.Metadata{ | ||
| ChainID: gen.ChainID, | ||
| Height: 1, | ||
| Time: prevHeader.BaseHeader.Time, | ||
| }, | ||
| Txs: nil, | ||
| } | ||
|
|
||
| batch, err := memStore.NewBatch(context.Background()) | ||
| require.NoError(t, err) | ||
| require.NoError(t, batch.SaveBlockData(prevHeader, prevData, &prevHeader.Signature)) | ||
| require.NoError(t, batch.SetHeight(1)) | ||
| require.NoError(t, batch.Commit()) | ||
|
|
||
| executor.setLastState(types.State{ | ||
| Version: types.InitStateVersion, | ||
| ChainID: gen.ChainID, | ||
| InitialHeight: gen.InitialHeight, | ||
| LastBlockHeight: 1, | ||
| LastBlockTime: prevHeader.Time(), | ||
| LastHeaderHash: prevHeader.Hash(), | ||
| AppHash: []byte("state-root-1"), | ||
| }) | ||
|
|
||
| header, data, err := executor.CreateBlock(context.Background(), 2, &BatchData{ | ||
| Batch: &coreseq.Batch{}, | ||
| Time: time.Now(), | ||
| }) | ||
| require.NoError(t, err) | ||
| require.Equal(t, newAddr, header.ProposerAddress) | ||
| require.Equal(t, newAddr, header.Signer.Address) | ||
| require.Equal(t, uint64(2), data.Height()) | ||
| } |
Contributor
There was a problem hiding this comment.
Use fixed times in the new executor fixtures.
These tests only need ordered/non-zero times, so wall-clock values make the fixtures less reproducible.
🧪 Proposed direction
+ fixedStartTime := time.Unix(1_700_000_000, 0).UTC()
+ fixedBatchTime := fixedStartTime.Add(time.Second)
gen := genesis.Genesis{
ChainID: "test-chain",
InitialHeight: 1,
- StartTime: time.Now().Add(-time.Second),
+ StartTime: fixedStartTime,
ProposerAddress: entry1.Address,
ProposerSchedule: []genesis.ProposerScheduleEntry{entry1, entry2},
DAEpochForcedInclusion: 1,
}
@@
header, data, err := executor.CreateBlock(context.Background(), 2, &BatchData{
Batch: &coreseq.Batch{},
- Time: time.Now(),
+ Time: fixedBatchTime,
})Apply the same pattern to the other new genesis and BatchData fixtures in this file.
As per coding guidelines, Ensure tests are deterministic.
Also applies to: 237-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@block/internal/executing/executor_test.go` around lines 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.
Comment on lines
+56
to
+73
| Each entry declares: | ||
|
|
||
| - `start_height` | ||
| - `address` | ||
| - `pub_key` | ||
|
|
||
| The active proposer for block height `h` is the last entry whose `start_height <= h`. | ||
|
|
||
| The legacy `proposer_address` field remains for backward compatibility. When no explicit schedule is present, | ||
| ev-node derives an implicit single-entry schedule beginning at `initial_height`. | ||
|
|
||
| When an explicit schedule is present: | ||
|
|
||
| - the first entry must start at `initial_height` | ||
| - entries must be strictly increasing by `start_height` | ||
| - each entry's `address` must match the configured `pub_key` | ||
| - `proposer_address`, when present, must match the first schedule entry | ||
|
|
Contributor
There was a problem hiding this comment.
Documentation currently contradicts optional pub_key behavior
Line 60/Line 71 and the JSON example imply pub_key is required for every entry, but runtime validation supports address-only schedule entries (entry.PubKey can be empty). Please align the ADR wording so operators don’t treat missing pub_key as invalid by default.
✏️ Suggested doc fix
Each entry declares:
- `start_height`
- `address`
-- `pub_key`
+- `pub_key` (optional; when present, it must match `address`)
@@
When an explicit schedule is present:
@@
-- each entry's `address` must match the configured `pub_key`
+- if `pub_key` is provided, the entry's `address` must match it
@@
{
"start_height": 1,
"address": "...",
- "pub_key": "..."
+ "pub_key": "..."
},
{
"start_height": 1250000,
- "address": "...",
- "pub_key": "..."
+ "address": "..."
}
]
@@
-This design improves safety over address-only pinning by allowing validation against the scheduled public key.
+This design improves safety by allowing validation against a scheduled public key when one is pinned.Also applies to: 80-91, 124-126
🤖 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` around lines 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).
| } | ||
|
|
||
| func TestGenesis_ValidateProposerSchedule(t *testing.T) { | ||
| validTime := time.Now().UTC() |
Contributor
There was a problem hiding this comment.
Use a fixed timestamp for this fixture.
This test only needs a non-zero time, so using wall-clock time adds unnecessary nondeterminism.
🧪 Proposed fix
- validTime := time.Now().UTC()
+ validTime := time.Unix(1_700_000_000, 0).UTC()As per coding guidelines, Ensure tests are deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/genesis/genesis_test.go` at 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.
Comment on lines
+227
to
+238
| 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", | ||
| }, |
Contributor
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.
Comment on lines
+271
to
+286
| func TestEffectiveProposerSchedule_LegacyFallback(t *testing.T) { | ||
| addr := []byte("some-address-bytes") | ||
| legacy := Genesis{ | ||
| ChainID: "c", | ||
| InitialHeight: 7, | ||
| ProposerAddress: addr, | ||
| } | ||
| schedule := legacy.EffectiveProposerSchedule() | ||
| require.Len(t, schedule, 1) | ||
| require.Equal(t, uint64(7), schedule[0].StartHeight) | ||
| require.Equal(t, addr, schedule[0].Address) | ||
| require.Empty(t, schedule[0].PubKey) | ||
|
|
||
| // mutating the derived slice must not affect the genesis backing data. | ||
| schedule[0].Address[0] ^= 0xFF | ||
| require.Equal(t, addr, legacy.ProposerAddress) |
Contributor
There was a problem hiding this comment.
Clone the expected address before mutating the returned schedule.
As written, a missing defensive copy can still pass because addr and legacy.ProposerAddress share the same backing slice.
🧪 Proposed fix
func TestEffectiveProposerSchedule_LegacyFallback(t *testing.T) {
addr := []byte("some-address-bytes")
+ origAddr := bytes.Clone(addr)
legacy := Genesis{
ChainID: "c",
InitialHeight: 7,
ProposerAddress: addr,
}
@@
// mutating the derived slice must not affect the genesis backing data.
schedule[0].Address[0] ^= 0xFF
- require.Equal(t, addr, legacy.ProposerAddress)
+ require.Equal(t, origAddr, legacy.ProposerAddress)
}🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Add key rotation for proposers
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
Documentation