Skip to content

fix(challenger): call setAnchorState on resolved DEFENDER_WINS games#2372

Merged
leopoldjoy merged 13 commits intomainfrom
challenger-anchor-state-update
Apr 27, 2026
Merged

fix(challenger): call setAnchorState on resolved DEFENDER_WINS games#2372
leopoldjoy merged 13 commits intomainfrom
challenger-anchor-state-update

Conversation

@leopoldjoy
Copy link
Copy Markdown
Contributor

Summary

The AnchorStateRegistry was never being updated by any off-chain component. In the multiproof system (AggregateVerifier), claimCredit() does not call closeGame() — unlike FaultDisputeGame where claimCredit() triggers closeGame() which calls setAnchorState(). This meant the anchor root was never advancing after proposals finalized.

Root Cause

  • FaultDisputeGame.claimCredit() → calls closeGame() → calls setAnchorState()
  • AggregateVerifier.claimCredit() → does NOT call closeGame()
  • Zero references to closeGame, close_game, setAnchorState, or set_anchor_state exist in any .rs file in the repo

Fix

Integrates a best-effort setAnchorState() call into the BondManager lifecycle. After each successful advance_game() tick, if the game is past NeedsResolve and resolved as DEFENDER_WINS, we send a permissionless setAnchorState(game) tx to the ASR contract. The call is self-validating on-chain (checks finalization, airgap delay, DEFENDER_WINS status, and staleness) so premature calls revert harmlessly and are retried on the next tick.

Changes

  • anchor_state_registry.rs — Add setAnchorState(address) to IAnchorStateRegistry sol! bindings and encode_set_anchor_state_calldata() helper
  • lib.rs (contracts) — Re-export encode_set_anchor_state_calldata
  • bond.rs — Add anchor_state_registry_address to BondManager, anchor_update_complete flag to TrackedGame, and try_anchor_update() method called after each advance_game() success
  • cli.rs — Add --anchor-state-registry-address / BASE_CHALLENGER_ANCHOR_STATE_REGISTRY_ADDRESS CLI arg
  • config.rs — Wire through anchor_state_registry_address
  • service.rs — Pass config to BondManager::new()
  • metrics.rs — Add anchor_update_tx_outcome_total counter (success/error/skipped)
  • driver.rs (tests) — Update default_bond_manager() for new parameter

Testing

6 new unit tests covering all anchor update code paths:

  • anchor_update_skipped_when_no_asr_configured
  • anchor_update_skipped_for_needs_resolve_phase
  • anchor_update_skipped_for_challenger_wins
  • anchor_update_sends_tx_for_defender_wins
  • anchor_update_retries_on_failure
  • anchor_update_not_retried_after_success

All 39 tests pass, clippy clean with -D warnings.

Future Work

Consider adding closeGame() to AggregateVerifier.claimCredit() in a contract upgrade for defense-in-depth (mirrors the FaultDisputeGame pattern).

The AnchorStateRegistry was never being updated by any off-chain component.
In the multiproof system (AggregateVerifier), claimCredit() does not call
closeGame() — unlike FaultDisputeGame where claimCredit() triggers
closeGame() which calls setAnchorState(). This meant the anchor root
was never advancing after proposals finalized.

This fix integrates a best-effort setAnchorState() call into the
BondManager lifecycle. After each successful advance_game() tick,
if the game is past NeedsResolve and resolved as DEFENDER_WINS,
we send a permissionless setAnchorState(game) tx to the ASR contract.
The call is self-validating on-chain (checks finalization, airgap delay,
DEFENDER_WINS status, and staleness) so premature calls revert harmlessly
and are retried on the next tick.

Changes:
- Add setAnchorState(address) to IAnchorStateRegistry sol! bindings and
  encode_set_anchor_state_calldata() helper
- Add anchor_state_registry_address config to BondManager, CLI, and config
- Add try_anchor_update() method called after each advance_game() success
- Add anchor_update_tx_outcome_total metric (success/error/skipped)
- Add 6 unit tests covering all anchor update code paths
@cb-heimdall
Copy link
Copy Markdown
Collaborator

cb-heimdall commented Apr 24, 2026

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 24, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
base Ready Ready Preview, Comment Apr 27, 2026 8:32pm

Request Review

Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/service.rs Outdated
Address review feedback:
- Cache the on-chain game status on TrackedGame after the first
  successful RPC read, avoiding redundant status() calls on every
  poll tick during the airgap delay period (status is immutable
  after resolution).
- Warn at startup when anchor-state-registry-address is configured
  but bond-claim-addresses is empty, since the anchor updates
  require the bond manager to function.
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/bond.rs
… try_resolve

Address review feedback:
- Call try_anchor_update on the Ok(Some(_)) path before the game is
  removed from tracking. This gives a last-chance anchor update attempt
  when the bond lifecycle completes, preventing permanent loss of the
  update if the airgap delay is longer than the DelayedWETH delay.
- Cache the game status in try_resolve when the game is already resolved
  (status immutable), so try_anchor_update on the same tick can skip
  the redundant status() RPC round-trip.
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/bond.rs Outdated
Relax the NeedsResolve phase guard in try_anchor_update so that games
with a cached status (known to be resolved) can still attempt the
anchor update even when the phase hasn't advanced past NeedsResolve.

This fixes an edge case where a DEFENDER_WINS game is marked
NotClaimable (bond recipient not in our claim set after resolution)
and removed before the anchor update is attempted — the phase guard
was blocking the last-chance try_anchor_update call because set_phase
was never reached.
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/bond.rs
Cache the game status after we successfully submit a resolve() tx,
not just on the 'already resolved' path. Without this, self-resolved
DEFENDER_WINS games that are NotClaimable would miss their anchor
update because cached_status was None and the NeedsResolve phase
guard would trigger.
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/cli.rs Outdated
…f config

Remove the --anchor-state-registry-address CLI parameter and all
associated config plumbing. The AnchorStateRegistry address is now
read from each game contract via anchorStateRegistry() and cached
per-game on TrackedGame, eliminating the risk of misconfiguration.
@leopoldjoy leopoldjoy requested a review from jackchuma April 24, 2026 18:01
Comment thread crates/proof/challenge/src/bond.rs Outdated
Comment thread crates/proof/challenge/src/bond.rs
Comment thread crates/proof/challenge/src/bond.rs
@github-actions
Copy link
Copy Markdown
Contributor

Review Summary

The approach — bolting a best-effort setAnchorState() call onto the existing BondManager lifecycle — is sound. The on-chain self-validation means incorrect calls are harmless, and the caching strategy for immutable status/ASR values is a good design. However, there are a few correctness gaps in the interaction between the anchor update logic and the existing bond lifecycle that could cause DEFENDER_WINS games to permanently miss their anchor update.

Key issues (already raised inline)

1. Anchor update permanently lost when airgap delay > DelayedWETH delay
The "last-chance" try_anchor_update at line 600 is called right before the game is removed from self.tracked. If the airgap delay hasn't elapsed, the on-chain call reverts, anchor_update_complete stays false, and the game is immediately removed with no future retries. Consider retaining DEFENDER_WINS games with anchor_update_complete == false in tracked (with a TTL to prevent unbounded growth).

2. Status not cached on the success resolve path
When this node resolves the game itself (lines 690-716), cached_status is populated via a re-read. But if that re-read fails (line 708), cached_status remains None. Combined with the phase guard at line 1005-1006 (cached_status.is_none() && phase == NeedsResolve), this can cause try_anchor_update to skip eligible games. More critically, the NotClaimable return (line 744) happens before set_phase (line 747), so the phase is still NeedsResolve when the last-chance anchor update runs.

3. send_bond_tx semantics mismatch
The BondTransactionSubmitter::send_bond_tx trait documents its first parameter as game_address and the production ChallengeSubmitter logs it as game = %game_address. Passing the ASR address here produces misleading structured logs. This should be addressed either by generalizing the trait parameter name or by adding explicit ASR-address log fields in try_anchor_update.

Minor observations

  • The PR description mentions 6 tests but only 5 are in the diff (anchor_update_skipped_when_no_asr_configured is missing). The description also lists changes to cli.rs, config.rs, and service.rs that are not in the diff — the ASR address is instead read from the game contract directly, which is a cleaner design.
  • The contract bindings and re-exports in anchor_state_registry.rs and lib.rs follow existing patterns correctly.
  • The new AggregateVerifierClient::anchor_state_registry trait method and its mock implementations are well-structured.

@leopoldjoy leopoldjoy added this pull request to the merge queue Apr 27, 2026
Merged via the queue into main with commit 4b389f0 Apr 27, 2026
25 of 28 checks passed
@leopoldjoy leopoldjoy deleted the challenger-anchor-state-update branch April 27, 2026 22:09
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.

3 participants