refactor(blockchain): drop SyncStatusTracker::gate_proposer

[MegaRedHand]

Split out of #445 (proposer pre-build).

gate_proposer was a one-line wrapper over duties_allowed() with a single call site. This inlines it as scheduled_proposer.filter(|_| self.sync_status.duties_allowed()) and removes the method, so the sync gate reads the same way everywhere.

Behavior-preserving — proposer_validator_id is computed identically; every downstream use (the skip-while-syncing log, the store::on_tick proposal flag, the proposal call) is untouched.

The three gate_proposer-only unit tests collapse onto the duties_allowed() assertions they already made.

Stacked under the on_tick-regrouping PR; #445 will drop these hunks once both land.

[github-actions]

🤖 Kimi Code Review

Overall Assessment: This is a clean refactoring that removes unnecessary abstraction without changing behavior. No security or correctness issues introduced.

Detailed Feedback:

crates/blockchain/src/sync_status.rs

• Lines 48-50 (removed): Good removal of the gate_proposer helper. It was a thin wrapper around duties_allowed() that added no semantic value and required the caller to pass an Option<u64>. The new approach at the call site is more idiomatic.
• Lines 113-128 (tests): Test simplification is appropriate. Since gate_proposer was merely proposer.filter(|_| self.duties_allowed()), removing assertions on that method in favor of testing the underlying duties_allowed() logic is correct—Option::filter is standard library code that doesn't require unit testing.

crates/blockchain/src/lib.rs

• Lines 239-240: The inline use of filter is clearer than the previous abstraction:

  let proposer_validator_id =
      scheduled_proposer.filter(|_| self.sync_status.duties_allowed());

  This makes the dependency on sync status explicit at the point of use.

• Lines 242-248: Logic flow remains correct. The if let condition correctly identifies when we are the scheduled proposer but cannot fulfill duties due to syncing (i.e., scheduled_proposer is Some but proposer_validator_id is None).

Consensus Safety Check:

• The change preserves the critical safety property: validators cannot propose or attest while the node is syncing (!self.syncing). This prevents split-view attacks where a validator might sign messages on an outdated fork during catch-up sync.

Minor Style Note:

• The test rename from syncing_gates_proposals_and_attestations to syncing_blocks_duties is slightly less explicit (the verb "blocks" could be misread as a noun), but acceptable given the context of surrounding tests.

Verdict: LGTM. The refactoring reduces API surface area and improves code clarity.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Claude Code Review

Here is my review of PR #449:

---

PR #449: refactor(blockchain): drop SyncStatusTracker::gate_proposer

Overview: A small, focused refactor that removes a one-liner wrapper method (gate_proposer) and inlines its logic at the single call site. +5 / -11 across 2 files.

---

Correctness

The inlined expression is semantically identical to the removed method:

// Removed:
pub(crate) fn gate_proposer(&self, proposer: Option<u64>) -> Option<u64> {
    proposer.filter(|_| self.duties_allowed())
}

// Inlined at call site:
scheduled_proposer.filter(|_| self.sync_status.duties_allowed())

|_| correctly ignores the validator ID when evaluating the sync gate — the gating decision is independent of the contained value. Behavior is preserved exactly.

---

Test Changes

The three removed assertions (assert_eq!(tracker.gate_proposer(Some(3)), None/Some(3))) were directly exercising the deleted method and added no coverage beyond duties_allowed() itself. Removing them is the right call — the remaining assert!(tracker.duties_allowed()) still captures the intended invariant in each scenario.

The renamed test names (syncing_blocks_duties, caught_up_node_allows_duties, network_stall_keeps_duties_enabled) are cleaner and more general than the originals.

---

Minor Observations

• The gate_proposer name carried some domain meaning ("gating the proposer"). The inlined form is more mechanical. This is a reasonable tradeoff at a single call site — the surrounding context in lib.rs (variable named proposer_validator_id, followed immediately by a log for the skipped-while-syncing case) keeps intent readable.
• No behavior changes to on_tick flow, the skip-while-syncing log path, or store::on_tick proposal flag — all untouched as stated.

---

Verdict

Clean, correct, behavior-preserving simplification. The removed method was a textbook case of an unnecessary indirection. No issues found — good to merge.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

No findings.

crates/blockchain/src/lib.rs:239-245 is a behavior-preserving inline replacement of the removed gate_proposer helper: scheduled_proposer.filter(|_| self.sync_status.duties_allowed()) is equivalent to the old wrapper, and the “Skipping block proposal while syncing” log still fires under the same condition. I do not see any impact on fork choice, attestation validation, justification/finalization, state transition, XMSS, or SSZ logic because those paths are untouched by this PR.

crates/blockchain/src/sync_status.rs:45-46 keeps a single sync predicate for validator duties, and the renamed tests at crates/blockchain/src/sync_status.rs:113-134 still cover the meaningful state transitions (syncing, recovered, network stall). No security or performance concerns stood out.

I could not run the targeted Rust tests in this environment: cargo is blocked by read-only ~/.cargo / ~/.rustup paths and restricted dependency fetching.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[greptile-apps]

Greptile Summary

This PR inlines the single-line SyncStatusTracker::gate_proposer helper directly at its one call site in on_tick, replacing self.sync_status.gate_proposer(scheduled_proposer) with scheduled_proposer.filter(|_| self.sync_status.duties_allowed()). No behavior changes — the three tests that previously asserted on gate_proposer are simplified to assert only on duties_allowed(), which they already covered.

• crates/blockchain/src/lib.rs: The gate_proposer call is replaced with an equivalent inline filter expression; all downstream uses of proposer_validator_id are untouched.
• crates/blockchain/src/sync_status.rs: gate_proposer method removed; three test names updated to reflect that they now only assert the duties_allowed() gate.

Confidence Score: 5/5

Safe to merge — pure mechanical inline of a one-line wrapper with no logic changes.

The change replaces a single method call with the exact body of that method at the only call site. Both the removed method and the inlined expression are identical (proposer.filter(|_| self.duties_allowed())), and every downstream consumer of proposer_validator_id is untouched. The test simplifications correctly reflect that gate_proposer had no independent behaviour beyond duties_allowed().

No files require special attention.

Important Files Changed

Filename	Overview
crates/blockchain/src/lib.rs	One-line substitution: gate_proposer(scheduled_proposer) → `scheduled_proposer.filter(
crates/blockchain/src/sync_status.rs	Removes gate_proposer method and simplifies the three tests that used it; duties_allowed() and all other logic are unchanged.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant OT as on_tick
    participant SS as SyncStatusTracker

    Note over OT: interval == 0 && slot > 0
    OT->>OT: "scheduled_proposer = get_our_proposer(slot)"
    OT->>SS: duties_allowed()
    SS-->>OT: bool
    OT->>OT: "proposer_validator_id = scheduled_proposer.filter(duties_allowed)"
    alt proposer_validator_id is None (syncing)
        OT->>OT: log Skipping block proposal while syncing
    else proposer_validator_id is Some
        OT->>OT: store::on_tick / propose block
    end

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant OT as on_tick
    participant SS as SyncStatusTracker

    Note over OT: interval == 0 && slot > 0
    OT->>OT: "scheduled_proposer = get_our_proposer(slot)"
    OT->>SS: duties_allowed()
    SS-->>OT: bool
    OT->>OT: "proposer_validator_id = scheduled_proposer.filter(duties_allowed)"
    alt proposer_validator_id is None (syncing)
        OT->>OT: log Skipping block proposal while syncing
    else proposer_validator_id is Some
        OT->>OT: store::on_tick / propose block
    end

Loading

_{Reviews (1): Last reviewed commit: "refactor(blockchain): drop SyncStatusTra..." | Re-trigger Greptile}