feat: implement validator sync-lag duty gate - #1385
Conversation
a8ee1ff to
eab4692
Compare
zemse
changed the title
eab4692 to
b19214c
Compare
01a0e34 to
c944d1f
Compare
zemse
changed the title
|
I've pushed some commits on my branch that dont appear in this PR, so I'll try if close open works |
73f09ee to
2018266
Compare
73f09ee to
8c6bd22
Compare
Add the test surface with minimal stubs so the tests compile. The next commit adds the implementation that makes them pass.
When the validator's local head falls too far behind wall-clock, pause block production and attestation duties until it catches up. Skip the pause if the whole network is stalled so the chain can still recover.
The loop bound 0..=4_u64 was the literal value of SYNC_LAG_THRESHOLD; reference the constant directly so the test follows future tweaks.
Plain subtraction underflows if HYSTERESIS_BAND ever exceeds SYNC_LAG_THRESHOLD, turning the resume condition into a permanently-true check after wrap. const_assert catches it at compile time.
If the node crashes while behind and restarts, init=false fires one signing attempt on the very first tick before is_synced_for_duties has a chance to close the gate. Starting paused lets the first predicate call unpause immediately when lag is within threshold, so clean-start behaviour is unchanged. The sync-lag test helper resets the flag to false explicitly because those tests exercise predicate behaviour from a running-normally state.
Four review fixes bundled because they all touch the same predicate: - Introduce DutyKind enum + AsRef<str> so the predicate, handlers, and metric labels share one source of truth instead of stringly-typed "block" / "attestation" literals scattered across the file. A future pass can swap the hand-rolled AsRef impl for strum::AsRefStr. - Add LeanDB::snapshot_head_and_max_slot so the predicate body shrinks to a single let-else and the RwLock + Mutex pair is held only for the duration of the synchronous DB reads. - Fold the FieldNotInitilized pre-genesis case into the same helper, mirroring the StoreError match used in fork_choice/beacon/handlers.rs. - Extract LeanChainService::skip_for_lag to kill the duplicate ~25-line preambles in handle_produce_block / handle_build_attestation_data; the handlers now skip-or-continue in three lines.
Two review-driven comment moves in service.rs: - The strum::AsRefStr TODO sat above the DutyKind enum but actually describes the hand-rolled AsRef<str> impl beneath it, so move it there. - The "start paused" doc on the duties_paused field is really about the constructor's initial value, not the field declaration. Move it above duties_paused: true in LeanChainService::new where the invariant is set. No behaviour change, no public API change.
The snapshot_head_and_max_slot helper added in cfdea16 didn't actually shrink the lock window beyond what the inline form gave us. Both forms take the LeanStoreWriter read lock and the inner LeanDB mutex once and do the same three reads under that scope. Moving the reads into LeanDB just spread the sync-lag gate across two crates without a real win. Inline the three reads back into is_synced_for_duties and drop LeanDB::snapshot_head_and_max_slot from crates/storage/src/db/lean.rs. Keep the StoreError::FieldNotInitilized -> Ok(SyncedForDuties::Yes) match arm at the call site so the pre-genesis case is still handled (this part was a real fix in cfdea16, not just refactor noise). No behaviour change at runtime.
…vers restart The earlier change (bbeb4ed) initialized duties_paused to true to defend against a restart-while-behind firing one duty before is_synced_for_duties could close the gate. The defense is unnecessary, because the ProduceBlock and BuildAttestationData handlers short-circuit on sync_status == Syncing before is_synced_for_duties is ever called (service.rs:422 and :436), and a fresh node always boots into sync_status = Syncing. By the time the chain service has transitioned to Synced, update_sync_status has already decided the node is caught up, so a duties_paused = false start is no closer to firing a bad duty than duties_paused = true would be. The hysteresis logic at runtime correctly engages duties_paused = true when lag exceeds SYNC_LAG_THRESHOLD, so the pause defense still kicks in when genuinely behind. Drop the start-paused defense, remove the explicit service.duties_paused = false override in the sync-lag test helper (no longer needed once the default is false).
The pause and resume-from-stall logs include max_seen_slot and network_lag; the resume-from-local-catchup log was missing both. Add them so operators have the same diagnostic shape on every transition.
8c6bd22 to
5eaf9a5
Compare
Drop the sync-lag helpers and test cases this PR added to the ream-chain-lean test module. The duty-gate implementation is unchanged.
1d5b1d0 to
e6d3d3d
Compare
|
Please rebase |
|
I revisited this PR and had a thought that this issue might exist:
So I actually ran a large local devnet of 19 nodes with this PR. I had 4 consecutive nodes skip authoring block at a slot with a hacky switch, it tricked rest of the network into sync lag and for the next 4 blocks (i.e. 8 - 4) the validators did not author a block during their slot. The network resumed after the 9th slot validator authored the block since it (and rest of the network) exited the sync lag gate. Problem: 5th validator could author a block but it didn't. Good news: I added a ream master branch node (that does not have sync lag impl) at the 6th position, so it will author a block even when rest of the network thinks they are sync lagged, it closed the sync lag gate after a block import and 7th and 8th proposer authored a block (they would have not authored if 6th validator didn't propose its block). It means we want somebody to author a block in this situation but everyone that implements the sync lag gate does not author a block. Potential solution: Sync lag gate should not affect block authoring, the validator should author block regardless. It creates possibility of minority fork but fork choice should resolve that. Further I tried with ethlamda peers that implement the sync lag and this issue occurs with them as well, they also do not propose blocks until the Relevant lean spec source code. |
is your pr ready to review? also please rebase |
|
Converted to draft, keeping this PR on hold until my doubt is cleared. I believe there is a bug in validator sync-lag lean specs which I am discussing with @shariqnaiyer. TLDR of the bug: when 4 consecutive validators (5 in case of ethlambda coz they look at |
2 participants
What was wrong?
Fixes: #1376
The validator signs blocks and attestations even when it's catching up to the chain head, so it sends out useless or risky signatures.
How was it fixed?
Skip the duty when the local head lags wall-clock by too much. A small hysteresis band stops the state from flapping in and out, and if the whole network looks stalled the check lets duties run anyway so the chain can recover.
To-Do