Skip to content

kona-sp1-proposer: review follow-ups from the #22018 nits list - #22097

Merged
Inphi merged 12 commits into
developfrom
kona-sp1-proposer-nits
Jul 30, 2026
Merged

kona-sp1-proposer: review follow-ups from the #22018 nits list#22097
Inphi merged 12 commits into
developfrom
kona-sp1-proposer-nits

Conversation

@digorithm

Copy link
Copy Markdown
Contributor

Summary

Works through all 40 nit-level items from #22086 (wwared's #22018 review). Every item has an explicit verdict below: 37 fixed here, 2 deferred to their owning issues, 1 declined with reason. Four commits, grouped by risk (verdict-table references: commit 1 = 6cb46a3, commit 2 = 927955f, commit 3 = ec74e4b, commit 4 = 3bd005b; hashes change if the branch is rebased before opening the PR):

  1. kona-sp1-proposer: mechanical review nits from #22018: dead code (state snapshot trio, duplicate confirmation constants, backup-era serde derives and deps, devstack metrics block), PrestateCache newtype, startup logging for signer and resolved config with URL redaction, GamesCreated and GamesBondsClaimed double counting, tokio::fs artifact reads, named anchor root fields, missing-to signer error, TX_CONFIRMATION_TIMEOUT=0 rejection, required super-root timestamp fields, doc fixes.
  2. kona-sp1-proposer: test improvements from #22018 review: pending-exclusion contrast test, bond_claim_action ordering cases, ProposalStatus decode rejection, PROPOSAL_SAFETY field-selection test, revert-prefix pin test, super-root response timestamp verification, WithoutHonestChallenger honored in the non-ZK path, wait.For-based factory waits, opportunistic root check in WaitForAnchorRootAtLeast, race-free chaining test assertions.
  3. kona-sp1-proposer: contain per-game failures in sync_games: per-game error isolation (walk, pending re-validation, classification; new game_sync_error counter; cursor advance gated on a complete walk), one-sided pending eviction past the anchor deadline-lag cutoff, own-game super-root mismatches held re-checkable, DefenderWins-only parent resolution eligibility, retired/blacklisted-parent stall fix (pre-creation isGameBlacklisted/isGameRetired check against the registered registry).
  4. kona-sp1-proposer: harden tx submission (collision adoption, fee caps): a create tx landing after its confirmation timeout no longer leads to a duplicate bonded game (the collision arm consults gameCreator() and adopts our own game instead of advancing the timestamp), and MAX_FEE_PER_GAS / MAX_PRIORITY_FEE_PER_GAS env caps bound what the fee estimator may set (clamp-after-fill in both signer arms, unset = uncapped).

The sync_games decomposition (walk/step split, trait seams, multicall) deliberately stays in #22019 (blocked on #21463); every change here keeps the loop's current shape.

Verdicts (all 40 items)

Fixed items name the commit and the code; declined/deferred items carry their reason. Comment links are in #22086.

# Item Verdict
1 sync loop error bubbling Fixed (commit 3): sync_games contains per-game failures at the walk (cursor advance gated on a complete walk), pending re-validation, and classification; new kona_sp1_proposer_game_sync_error counter
2 chaining onto a possibly-invalid parent (#21445) Deferred to #21463: same design question as the is_owned widening already deferred there; reviewer suggested delegating to the prove PR
3 harden tx failure scenarios Fixed (commits 1+4): TX_CONFIRMATION_TIMEOUT=0 rejected at config parse; collision arm adopts our own landed game via gameCreator() instead of bonding a duplicate; 60s-default-vs-3-confirmations tightness left as-is (no observed failure)
4 evict permanently-failing pending entries Fixed (commit 3): Pending carries the game deadline; one-sided pending_evictable evicts entries whose deadline fell behind the anchor beyond MAX_GAME_DEADLINE_LAG; ahead-of-anchor games stay re-checkable under a stalled anchor
5 pin sync snapshot by block hash Deferred to #18411 (reviewer: nothing to do in this PR)
6 SYNC_L1_CONFIRMATIONS larger than chain height pins to genesis Declined as safe (reviewer: perfectly safe and sensible; devnet-only condition; genesis pinning only means everything at that height is confirmed)
7 use tokio::fs in the prestate artifact loader Fixed (commit 1): fetch_artifact file arm
8 is_parent_resolved treats ChallengerWins parents as resolved Fixed (commit 3): parent_status_resolves restricts eligibility to DefenderWins; ChallengerWins subtrees are removed by sync instead
9 proposer may get stuck on a retired or blacklisted parent Fixed (commit 3): confirmed real by contract investigation (initialize reverts InvalidParentGame; nothing pruned such a parent; stall survived restarts); pre-creation check of the chosen parent via isGameBlacklisted/isGameRetired (not isGameProper, whose paused() clause would mass-evict during a superchain pause) against the currently registered registry (the one a new child binds); on a hit the subtree is dropped and head selection falls back
10 own games should not be terminally invalidated on bad supernode data Fixed (commit 3): own-game claim mismatch is held Pending (re-checked each sync, aged out by the eviction cutoff); third-party mismatches stay terminal
11 log the error in the restore else branch Fixed (commit 1): try_init retry warn gains error = %e
12 tautological test: improve or remove Fixed (commit 2): replaced with an insertion-contrast test proving exclusion from state.games is load-bearing
13 bond_claim_action test: missing ordering cases Fixed (commit 2): unlock-precedes-payout and unreachable unfinalized-matured cases pinned
14 unused item at proposer.rs:275 Fixed (commit 1): ProposerStateSnapshot deleted
15 unused item at proposer.rs:377 Fixed (commit 1): state_snapshot deleted
16 unused pub item at proposer.rs:390 Fixed (commit 1): get_game deleted
17 stale doc comment on the sync walk Fixed (commit 3): rewritten once to describe the final post-robustness behavior
18 GamesCreated increments on collision-bound no-op creation Fixed (commit 1): increment moved into handle_game_creation's creation-success arm
19 name the unnamed getAnchorRoot output fields Fixed (commit 1): destructured into (anchor_root, anchor_sequence_number)
20 operator-tunable fee and gas price caps Fixed (commit 4): MAX_FEE_PER_GAS / MAX_PRIORITY_FEE_PER_GAS (wei, unset = uncapped) clamp the estimator's output after fill in both signer arms, with the priority <= max_fee invariant restored; five clamp unit tests
21 BondsClaimed double-increments across the two-phase claim Fixed (commit 1): only phase-2 payouts increment, classified from the pre-flight reads
22 drop serde defaults on the super-root timestamp fields Fixed (commit 1): the three fields are required; missing fields fail deserialization loudly; fixture extended plus a negative test
23 log which signer is used at startup Fixed (commit 1): both paths log the selected signer (URL redacted); half-set Web3Signer config is now a hard error instead of a silent PRIVATE_KEY fallback
24 error on missing to instead of the anvil contract-creation hack Fixed (commit 1): LocalSigner arm rejects requests without a to; the proposer only sends contract calls (verified end to end by the acceptance suite)
25 verify response timestamp matches the request, plus tests Fixed (commit 2): super_root_at takes the expected timestamp and errors on mismatch before the hash check; both callsites updated; two tests
26 WithoutHonestChallenger option only honored in the ZK path Fixed (commit 2): attachSuperChallengerAndProposer now honors SkipHonestChallenger (all L2ChallengerConfig consumers nil-check)
27 dead metrics block in the devstack launcher Fixed (commit 1): METRICS_PORT plumbing, freeTCPPort, and the post-startup metrics block removed
28 superroot decode testing gap Fixed (commit 2): max_proposable_selects_safety_field pins safe/finalized selection (the Finalized arm is the untested production default)
29 game status decode test asymmetry Fixed (commit 2): ProposalStatus::try_from(5).is_err() added
30 dead NUM_CONFIRMATIONS constant Fixed (commit 1): dead crate-root NUM_CONFIRMATIONS/TIMEOUT_SECONDS and signer TIMEOUT_SECONDS removed; the live signer NUM_CONFIRMATIONS kept
31 MaturesAt doc comment off by one from the impl Fixed (commit 1): impl drops the +1; first payable timestamp is timestamp+delay per DelayedWETH's <= check
32 SkipHonestProposer doc says op-proposer Fixed (commit 1): both docs now name op-proposer or kona-sp1-proposer
33 WaitForZKGameAtIndex test improvement (also WaitForGame) Fixed (commit 2): wait.For + contractio.Read; transient read errors retry instead of hanging the full timeout; last read error surfaced
34 possibly flaky chaining test assertion Fixed (commit 2): racy live-anchor comparison replaced with race-free structural assertions
35 log resolved config at binary startup Fixed (commit 1): single INFO covering every ProposerConfig field once; URLs userinfo-redacted (including the pre-existing log_startup_info leak)
36 typed revert error instead of string matching Fixed in part (commit 2): a pin test locks the outermost-rendering prefix semantics; the thiserror enum is deferred to #22019, where upstream diffability is already sacrificed
37 prestates cache type cleanup Fixed (commit 1): PrestateCache newtype owns the lock and base URL
38 remove unused proposer crate dependencies Fixed (commit 1): serde, serde_json, alloy-signer dropped; alloy-primitives serde feature dropped; alloy-contract kept (sol! expansion)
39 drop leftover Serialize/Deserialize derives Fixed (commit 1): Game, GameStatus, ProposalStatus
40 DSL function asserting a specific game anchored Fixed (commit 2): WaitForAnchorRootAtLeast restores the root-equality check when the observed anchor sits exactly at the game's height; the wider (root, minHeight) DSL shape stays unnecessary while this is the only caller

Test plan

  • Rust: cargo nextest run -p kona-sp1-proposer -p kona-sp1-super-range-executor --all-features: 55 tests (46 + 9), all passing; clippy --all-features --all-targets --locked clean; just lint-docs clean.
  • Go: gofmt/vet clean; op-golangci-lint run ./op-devstack/... ./op-acceptance-tests/tests/proofs/zk/...: 0 issues.
  • Acceptance (sysgo, RUST_JIT_BUILD): full ZK suite in stub mode, 9/9 PASS (both op-node and supernode variants); real-ELF mode (KONA_SP1_ELF_DIR) TestProposerClaimsBondAfterResolution PASS, covering the two-phase claim, MaturesAt change, and metrics paths end to end.
  • Coverage audit: an independent reviewer pass reconciled all 40 checklist items against the final diff; zero scope creep, every hunk attributed.

Notes for reviewers

  • Behavioral changes are isolated in commits 3 and 4; commits 1 and 2 are mechanical and test-only.
  • Two deliberate behavior changes beyond the letter of the nits, both flagged inline: half-set SIGNER_URL/SIGNER_ADDRESS now errors instead of silently using PRIVATE_KEY (the misconfiguration the logging nit describes), and pending eviction is one-sided (behind-the-anchor only), which follows the "nothing is lost by dropping them" premise of the original comment.

Dead code (state snapshot trio, duplicate confirmation constants, backup-era
serde derives and deps, devstack metrics block), PrestateCache newtype,
startup logging for signer and resolved config with URL redaction,
GamesCreated and GamesBondsClaimed double counting, tokio::fs artifact reads,
named anchor root fields, missing-to signer error, TX_CONFIRMATION_TIMEOUT=0
rejection, required super-root timestamp fields, doc fixes.

Part of #22086.
Pending-exclusion contrast test, bond_claim_action ordering cases,
ProposalStatus decode rejection, PROPOSAL_SAFETY field selection test,
revert-prefix pin test, super-root response timestamp verification,
WithoutHonestChallenger honored in the non-ZK path, wait.For-based factory
waits, opportunistic root check in WaitForAnchorRootAtLeast, race-free
chaining test assertions.

Part of #22086.
One unreadable game no longer aborts the sync cycle (walk, pending
re-validation, and status classification each contain per-game errors and
surface them via a new game_sync_error counter; the cursor only advances
past a complete walk). Pending entries whose deadline fell behind the anchor
beyond the lag cutoff are evicted (one-sided; fresh games under a stalled
anchor stay re-checkable). Own-game super-root mismatches stay re-checkable
instead of terminally dropping our bond on bad supernode data.
is_parent_resolved only counts DefenderWins parents. Creation checks the
chosen parent against the registered registry's isGameBlacklisted and
isGameRetired (a retired or blacklisted parent reverts child creation
forever and nothing pruned it; the stall survived restarts).

Structural decomposition of sync_games stays in #22019 (blocked on #21463).

Part of #22086.
A create tx that lands after its confirmation timeout left the duplicate
creation guard unarmed; the next tick treated the collision as third-party,
advanced the timestamp, and bonded a duplicate game on the same parent. The
collision arm now consults gameCreator(): our own game is adopted (guard
re-armed), third-party collisions still advance.

MAX_FEE_PER_GAS and MAX_PRIORITY_FEE_PER_GAS (wei, unset = uncapped) bound
what the fee estimator may set: fees are clamped after fill in both signer
arms, restoring the priority <= max_fee invariant. The local signer fills on
a wallet-less provider first, since a wallet provider signs during fill and
a signed envelope cannot be clamped.

Part of #22086.
@digorithm
digorithm requested a review from a team as a code owner July 29, 2026 03:39
@digorithm
digorithm requested review from Inphi and wwared July 29, 2026 03:45
@pauldowman pauldowman linked an issue Jul 29, 2026 that may be closed by this pull request
Comment thread rust/kona/sp1/crates/proposer/src/bin/kona-sp1-proposer.rs Outdated
Comment thread op-devstack/dsl/anchor_state_registry.go
Comment thread op-acceptance-tests/tests/proofs/zk/zk_proposer_test.go
Comment thread rust/kona/sp1/crates/proposer/src/lib.rs Outdated
Comment thread rust/kona/sp1/crates/proposer/src/proposer.rs Outdated
Comment thread rust/kona/sp1/crates/proposer/src/proposer.rs
Comment thread rust/kona/sp1/crates/proposer/src/proposer.rs
Comment thread op-devstack/dsl/proofs/dispute_game_factory.go Outdated
digorithm and others added 3 commits July 29, 2026 15:24
Co-authored-by: Inphi <mlaw2501@gmail.com>
Inline the parent_status_resolves helper into is_parent_resolved and drop
its tautological test (review feedback: PartialEq comparison needs no
helper). Add a TODO to strengthen the chaining test's anchor assertion via
the start-proposer-mid-test hook once #22105 lands.

Part of #22086.

@wwared wwared left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM once we solve the remaining open review threads

Review follow-up (#22097): acting on a partially discovered topology could
select a parent whose unfetched ancestry is invalid, so walk errors bubble
again and skip the whole cycle (cursor stays put; the range is re-walked
next tick). Per-game containment remains only in the status-classification
and pending re-validation phases, which operate on already-cached games.

To keep the walk immune to malformed games by construction, decode and
conversion failures on game-sourced data now classify the game invalid
instead of erroring, starting with the l2SequenceNumber u64 conversion
(unreachable today: the contract sources it from an 8-byte field). New
sync_cursor and factory_latest_game_index gauges make a stuck cursor
directly alertable alongside game_sync_error.

Part of #22086.
…path

Review agreement on #22097: exempting owned games (and their ancestors,
foreign included) from pending eviction is deferred to #21463, where
ownership widens to own-plus-ancestors. The TODO carries the requirement
and the current limitation (an evicted own game loses tracking until a
restart after the supernode heals).

Part of #22086.
Review follow-up (#22097): a create whose confirmation timed out may still
land, and the next tick could propose at a fresh timestamp, a different
uuid the factory cannot dedup, bonding a sibling on the same parent.

Record the exact rootClaim+extraData before sending. While the record is
unresolved, creation retries that uuid instead of proposing anew: the
factory finds the game (adopt ours, or recognize a foreign copy as spending
the uuid), or the resend succeeds, or it reverts (the pre-send lookup rules
out GameAlreadyExists and the remaining create-revert causes are permanent,
so a floating original would revert identically) and the record clears.
Transport errors and repeat timeouts keep the record for the next tick. A
landed revert of the original clears immediately (the tx is consumed).

Also document the fee-cap liveness tradeoff on both config knobs (review
consensus: keep them, default off).

Part of #22086.
Review catch (#22097, Inphi): the pre-send lookup only rules out
GameAlreadyExists at lookup time. The stuck original can be included
between our lookup and the resend reaching a builder, making the resend
revert with GameAlreadyExists; clearing on that mistook an adoptable game
for a permanent failure and left the duplicate guard unarmed (sibling risk
in the sync-lag window).

A landed revert is final chain state with exactly two causes, so re-running
the lookup disambiguates: the uuid's game now exists (adopt it; games are
never deleted) or the cause is permanent (clear; a floating original would
revert identically).

Part of #22086.
…resends

Adversarial review of the uuid-retry design found two defects: a resend
whose create would revert dies at fee estimation, which never yields the
landed-revert prefix, wedging the record forever (and the in-flight path
bypasses the parent-standing check); and pending-count nonces queue each
resend behind the stuck original, stacking same-uuid duplicates that all
revert once fees decay. Closing both properly required a parent
adjudication helper and pool checks, at which point the resend itself was
the complexity.

Drop the resend. The record still holds new proposals; each tick the
factory lookup adopts our landed game, and the record clears only when the
signer's pool drains (pending == latest), meaning nothing of ours can land
anymore. A visibly stuck transaction keeps holding; there is no time-based
clearing. Residual, documented: a transaction alive only in another node's
pool can land after the clear, re-creating the sibling case, which is
benign and self-healing (a valid own game whose bond the normal resolution
and claiming flow recovers).

Part of #22086.
@Inphi
Inphi added this pull request to the merge queue Jul 30, 2026
Merged via the queue into develop with commit 2ae10d6 Jul 30, 2026
131 checks passed
@Inphi
Inphi deleted the kona-sp1-proposer-nits branch July 30, 2026 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

proposer: update for new contracts API/ABI

3 participants