Repository navigation
feat(blockchain): add --disable-duty-sync-gate to ungate duties - #452
Conversation
The sync-gate suppresses block proposal, attestation production, and aggregate re-derivation whenever a node judges itself to be syncing (local head lagging wall clock while the network still progresses). In practice this feedback loop has driven finality stalls on the devnets: dead/slow proposers create empty slots, head lag crosses the threshold, nodes flap into Syncing and stop attesting, which widens the head-finalized gap further. Add a `--disable-duty-sync-gate` flag (default off, so gating stays on) that makes the gate observe-only: `SyncStatusTracker::update` still tracks the syncing state and drives `lean_node_sync_status`, but `duties_allowed()` always returns true. This lets us A/B the hypothesis on a devnet without a rebuild while keeping the metric for observability.
When --disable-duty-sync-gate is set, the node may believe it is syncing yet run duties anyway. Surface that counterfactual: at each of the three gate sites (propose, attest, reaggregate) emit a warn when the gate would have suppressed the duty had it been enabled. Makes the override visible and greppable during devnet experiments. Also drops the two unit tests added in the previous commit.
Replace the per-duty counterfactual logs with a single transition log in SyncStatusTracker::update: emit a debug line only when the syncing state actually flips, carrying the slot/lag context that drove the decision. Quieter than per-duty logging and applies whether or not the duty gate is enabled.
🤖 Codex Code Review
I did not find a correctness or security regression in fork choice, attestation validation, justification/finalization, STF, XMSS handling, or SSZ paths from this diff; the change is localized to duty gating and metric/logging behavior. I couldn’t run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewReview: PR #452 - Optional sync-gate for validator duties Summary: Well-implemented safety feature. Adds a CLI flag to disable duty suppression during sync while maintaining conservative defaults. No critical issues found. Code Quality & Correctness:
Observability:
Documentation:
Minor Suggestions:
Security Assessment: The implementation correctly prioritizes safety:
Approval: LGTM. No changes required. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR adds a
Confidence Score: 3/5Safe to experiment with on a devnet, but should not be merged to a production branch without the missing tests being added first. The gate logic in crates/blockchain/src/sync_status.rs — the test module needs the promised duties_allowed() tests before this is ready to merge.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/sync_status.rs | Core logic change: adds gate_duties field, new() constructor, manual Default impl, and transition debug logging. The duties_allowed() logic is correct, but the PR description claims two new tests for gate-on/gate-disabled behaviour that are absent from the diff — the test module has 6 tests total (unchanged) and none exercise duties_allowed(). |
| crates/blockchain/src/lib.rs | Adds gate_duties parameter to BlockChain::spawn and passes it to SyncStatusTracker::new. All three existing duties_allowed() call sites are unmodified and cover proposal, attestation, and reaggregation uniformly. |
| bin/ethlambda/src/main.rs | Adds --disable-duty-sync-gate CLI arg (default false, consistent with is_aggregator pattern) and passes !options.disable_duty_sync_gate to BlockChain::spawn. No startup log records whether the gate was disabled. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["--disable-duty-sync-gate\n(CLI flag)"] --> NEGATE["!options.disable_duty_sync_gate\n→ gate_duties: bool"]
NEGATE --> SPAWN["BlockChain::spawn(…, gate_duties)"]
SPAWN --> TRACKER["SyncStatusTracker::new(gate_duties)"]
TRACKER --> UPDATE["update(current_slot, head_slot, max_seen_slot)\n→ sets self.syncing\n→ drives lean_node_sync_status metric\n→ logs transition at debug level"]
UPDATE --> DA["duties_allowed()\n!gate_duties || !syncing"]
DA -->|gate_duties=true, syncing=true| BLOCK["false → duties suppressed"]
DA -->|gate_duties=true, syncing=false| ALLOW1["true → duties run"]
DA -->|gate_duties=false, any| ALLOW2["true → always run"]
ALLOW1 --> SITES
ALLOW2 --> SITES
BLOCK --> SKIP["info log: Skipping…"]
SITES["Gate check sites"]
SITES --> P["propose_block (interval 0)"]
SITES --> A["produce_attestations (interval 1)"]
SITES --> R["run_reaggregate_from_block (on block import)"]
%%{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"}}}%%
flowchart TD
CLI["--disable-duty-sync-gate\n(CLI flag)"] --> NEGATE["!options.disable_duty_sync_gate\n→ gate_duties: bool"]
NEGATE --> SPAWN["BlockChain::spawn(…, gate_duties)"]
SPAWN --> TRACKER["SyncStatusTracker::new(gate_duties)"]
TRACKER --> UPDATE["update(current_slot, head_slot, max_seen_slot)\n→ sets self.syncing\n→ drives lean_node_sync_status metric\n→ logs transition at debug level"]
UPDATE --> DA["duties_allowed()\n!gate_duties || !syncing"]
DA -->|gate_duties=true, syncing=true| BLOCK["false → duties suppressed"]
DA -->|gate_duties=true, syncing=false| ALLOW1["true → duties run"]
DA -->|gate_duties=false, any| ALLOW2["true → always run"]
ALLOW1 --> SITES
ALLOW2 --> SITES
BLOCK --> SKIP["info log: Skipping…"]
SITES["Gate check sites"]
SITES --> P["propose_block (interval 0)"]
SITES --> A["produce_attestations (interval 1)"]
SITES --> R["run_reaggregate_from_block (on block import)"]
Comments Outside Diff (1)
-
bin/ethlambda/src/main.rs, line 291-297 (link)The negation of the CLI flag is applied inline at the call site, making the relationship between
--disable-duty-sync-gateandgate_dutieseasy to miss during a future refactor. A named binding makes the inversion intentional and self-documenting.Prompt To Fix With AI
This is a comment left during a code review. Path: bin/ethlambda/src/main.rs Line: 291-297 Comment: The negation of the CLI flag is applied inline at the call site, making the relationship between `--disable-duty-sync-gate` and `gate_duties` easy to miss during a future refactor. A named binding makes the inversion intentional and self-documenting. How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/blockchain/src/sync_status.rs:83-86
**Claimed tests for `duties_allowed()` are absent from the diff**
The PR description states "8/8 pass, incl. two new tests covering gate-on (default) and gate-disabled," but the test module in this file ends at 6 tests (all exercising `update()` via `SyncStatusTracker::default()`) and no new test functions appear anywhere in the diff. `duties_allowed()` has zero test coverage in this changeset, so a future regression in the `!self.gate_duties || !self.syncing` expression — or an accidental revert of the `Default` impl's `gate_duties: true` — would go undetected.
### Issue 2 of 3
bin/ethlambda/src/main.rs:291-297
The negation of the CLI flag is applied inline at the call site, making the relationship between `--disable-duty-sync-gate` and `gate_duties` easy to miss during a future refactor. A named binding makes the inversion intentional and self-documenting.
```suggestion
// --disable-duty-sync-gate flips the sense: absent → gate on, present → gate off.
let gate_duties = !options.disable_duty_sync_gate;
let blockchain = BlockChain::spawn(
store.clone(),
validator_keys,
aggregator.clone(),
attestation_committee_count,
gate_duties,
);
```
### Issue 3 of 3
crates/blockchain/src/sync_status.rs:64-74
Sync-status transitions are logged at `debug` level. Given that this flag is intended specifically for live devnet diagnosis of the sawtooth/non-finality loop, `info!` would make transitions visible without needing to lower the log level — especially useful when the flag is disabled and you want to confirm that nodes are still correctly tracking (but not suppressing) their sync state.
```suggestion
if self.syncing != was_syncing {
info!(
current_slot,
head_slot,
max_seen_slot,
head_lag,
network_lag,
syncing = self.syncing,
gate_duties = self.gate_duties,
"Sync status changed"
);
}
```
Reviews (1): Last reviewed commit: "feat(blockchain): log sync status transi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub(crate) fn duties_allowed(&self) -> bool { | ||
| !self.syncing | ||
| // Gate disabled: the syncing state is observe-only, never suppresses duties. | ||
| !self.gate_duties || !self.syncing | ||
| } |
There was a problem hiding this comment.
duties_allowed() are absent from the diff
The PR description states "8/8 pass, incl. two new tests covering gate-on (default) and gate-disabled," but the test module in this file ends at 6 tests (all exercising update() via SyncStatusTracker::default()) and no new test functions appear anywhere in the diff. duties_allowed() has zero test coverage in this changeset, so a future regression in the !self.gate_duties || !self.syncing expression — or an accidental revert of the Default impl's gate_duties: true — would go undetected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/sync_status.rs
Line: 83-86
Comment:
**Claimed tests for `duties_allowed()` are absent from the diff**
The PR description states "8/8 pass, incl. two new tests covering gate-on (default) and gate-disabled," but the test module in this file ends at 6 tests (all exercising `update()` via `SyncStatusTracker::default()`) and no new test functions appear anywhere in the diff. `duties_allowed()` has zero test coverage in this changeset, so a future regression in the `!self.gate_duties || !self.syncing` expression — or an accidental revert of the `Default` impl's `gate_duties: true` — would go undetected.
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewHere is my review of PR 452. PR 452 —
|
2 participants
What
Adds a
--disable-duty-sync-gateCLI flag (defaultfalse, so current behavior is preserved). When set, the sync-gate becomes observe-only: it still tracks the syncing state and exportslean_node_sync_status, but no longer suppresses any validator duty.Why
The sync-gate suppresses three things while a node judges itself to be syncing (local head lagging wall clock while the network still progresses):
lib.rs:264:289:738On the devnets this gate has repeatedly driven a head-finalized sawtooth / non-finality feedback loop: dead or slow proposers create empty slots, head lag crosses
SYNC_LAG_THRESHOLD, nodes flap intoSyncingand stop attesting, which only widens the gap. This flag lets us A/B that hypothesis on a live devnet without a rebuild, keeping the metric intact for observability.How
SyncStatusTrackergains agate_dutiesfield (defaulttrue).update()(the metric path) is unchanged.duties_allowed()returnstrueunconditionally when gating is disabled.duties_allowed(), so the flag covers proposal, attestation, and reaggregation uniformly.Behavior matrix
--disable-duty-sync-gatelean_node_sync_statusTesting
cargo test -p ethlambda-blockchain sync_status— 8/8 pass, incl. two new tests covering gate-on (default) and gate-disabled.cargo build,cargo fmt,cargo clippyclean.Draft: intended for devnet experimentation, not immediate merge.