fix(l1): release the syncer lock when waiting for a forkchoice head (sync wedge)#6924
fix(l1): release the syncer lock when waiting for a forkchoice head (sync wedge)#6924ilitteri wants to merge 2 commits into
Conversation
🤖 Kimi Code ReviewThis is a correct and important fix for a liveness issue. The change eliminates a potential deadlock scenario where the sync manager could wedge itself indefinitely. Key improvements:
Minor observations:
Verdict: Approve. The fix correctly addresses the race condition described in the detailed comment and follows Tokio best practices for lock management. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
Open question: I did not find a regression test covering “checkpoint present + FCU arrives while the bootstrap sync task is exiting because head is still zero”. This path should have a targeted async test, because it is timing-sensitive and easy to re-break. Other than that race, the intent of avoiding a permanently active spinner makes sense, and I did not see additional security or EVM/consensus-specific concerns in this diff. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR changes the sync manager's zero-head resume behavior. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| crates/networking/p2p/sync_manager.rs | Updates zero-head sync startup to return and release the syncer lock, with unused imports removed. |
Reviews (1): Last reviewed commit: "fix(l1): log the waiting-for-forkchoice ..." | Re-trigger Greptile
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewNow I have enough to synthesize a precise review. Let me write it up. PR Review:
|
start_sync spun on a zero forkchoice head (log + sleep + continue) while holding the syncer lock, so is_active() stayed true and every sync_to_head (from both forkchoiceUpdated and newPayload) became a silent no-op. A node that restarted before its first forkchoiceUpdate -- or whose fcu-head update was dropped by set_head's try_lock under contention -- could stay wedged indefinitely with zero sync logs. Return and release the syncer lock instead; the next forkchoiceUpdate re-triggers start_sync with the head populated. The non-zero-head path and the snap multi-cycle loop are unchanged.
Keep operator visibility into the restart-resume state (a node waiting for its first forkchoiceUpdate) at the default log level, now as a one-shot line since start_sync returns instead of spinning.
ilitteri
force-pushed
the
fix/sync-resume-lock-wedge
branch
from
53e1dfa to
6f8ce21
Compare
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
2 participants
Motivation
On glamsterdam-devnet-6, a node that fell behind (
buildoor-prysm-ethrex-1) sat wedged ~15h with zero sync logs while its consensus client drove it every slot — it only recovered after a manualremovedb+ resync. This is the recovery half of that incident (the trigger — peers not serving headers by hash — is #6921).Root cause
SyncManager::start_syncspawns a task thattry_lock()s thesyncermutex and holds it for the whole task. When the forkchoice head isH256::zero()(e.g. right after a restart, before the firstforkchoiceUpdated), the task entered an infinitelog + sleep(5s) + continueloop, never releasing the lock. Sinceis_active()isself.syncer.try_lock().is_err()andsync_to_head()only spawns when!is_active(), a task parked in that spin pinnedis_active() == trueand turned every latersync_to_head()— from bothforkchoiceUpdatedandnewPayload— into a silent no-op. Compounded byset_head()dropping the fcu-head ontry_lockcontention (leaving the head at zero), the node could never re-arm: a permanent wedge with no sync logs.Fix
In the zero-head branch, return and release the syncer lock instead of spinning while holding it. The next
forkchoiceUpdated/newPayloadcallssync_to_head(), finds the syncer idle, and re-spawnsstart_sync()with the head populated. The non-zero-head path and the snap multi-cycle loop (continuewhileget_header_download_checkpoint()isSome) are unchanged. The restart-resume state is still surfaced atinfolevel (now one-shot). Removed the now-unusedDuration/sleep/debugimports.Correctness / review
Adversarially reviewed (concurrency + regression lenses) — ship-it, no blockers:
try_lockguard is dropped synchronously onreturn(no.awaitbetween the zero-check and the return), sois_active()correctly flips back to false.is_active()check observes the boot task still holding the lock during the few instructions before itsreturn, that FCU's spawn is skipped — recovered on the next FCU (CLs resend every slot). This is a strict improvement over the baseline, which held the lock forever. It cannot leave a node permanently un-synced as long as the CL keeps sending forkchoice updates (if it doesn't, no sync design could proceed).Testing
cargo check/cargo clippy -p ethrex-p2pclean. This is a concurrency/lifetime fix (hard to unit-test deterministically); validated by reasoning + review. Empirically, the wedged node recovered after a manual reset, consistent with the diagnosis.Scope / follow-up
This is Q1 of the recovery work. Q2 — a node with no retained state pausing with "Full sync cannot resume … run
ethrex removedb" — is left as a separate change: the obvious "re-enable snap from the resume guard" loops on< MIN_FULL_BLOCKSdevnets (where full sync is force-enabled), so the real fix is an automatic reset-and-full-resync, which deserves its own design.