feat(sync): support infinite pivot-update attempts via -1 (#5992)#11600
Conversation
Fixes NethermindEth#5992. MaxAttemptsToUpdatePivot = -1 (StartingSyncPivotUpdater.InfiniteAttempts) now means retry pivot acquisition forever without ever falling back to the static config pivot. Previously the cap was int.MaxValue (~68 years), which approximates infinity but is not actually infinite. Behaviour for 0 (disabled) and positive values is unchanged. MultiSyncModeSelector's UpdatingPivot gate widened to treat any non-zero value as active so the -1 path keeps the sync mode live.
Adds two cases to StartingSyncPivotUpdaterTests: with a finite cap and no finalized hash, the updater falls back to the static pivot after exhausting attempts; with InfiniteAttempts it never falls back even after 100 unsuccessful sync-mode changes.
0xDevNinja
requested review from
LukaszRozmej,
Marchhill,
MarekM25,
asdacap,
benaadams,
flcl42 and
marcindsobczak
as code owners
| if (_logger.IsInfo) | ||
| { | ||
| int attemptsMade = _maxAttempts == InfiniteAttempts ? InfiniteAttempts - _attemptsLeft : _maxAttempts - _attemptsLeft; | ||
| if (attemptsMade % 10 == 0) _logger.Info($"Waiting for Forkchoice message from Consensus Layer to set fresh pivot block [{attemptsMade}s]"); | ||
| } |
There was a problem hiding this comment.
this is equivalent to code before, please revert
There was a problem hiding this comment.
Reverted in 3d2a391 — the ternary was equivalent for the InfiniteAttempts case.
| { | ||
| _syncModeSelector.Changed -= OnSyncModeChanged; | ||
| } | ||
| else if (_attemptsLeft-- > 0) |
There was a problem hiding this comment.
I would prefer previous code design just making it here:
else if (_attemptsLeft-- > 0 || _maxAttempts == InfiniteAttempts)
easier to read
| // 0 means pivot updating is finished/disabled; any non-zero value (including -1 for infinite) keeps it active. | ||
| bool updateRequestedAndNotFinished = _syncConfig.MaxAttemptsToUpdatePivot != 0; |
There was a problem hiding this comment.
what about other negatives? This will be inconsistent
There was a problem hiding this comment.
Good catch. Tightened in 3d2a391 to > 0 || == InfiniteAttempts and moved InfiniteAttempts to ISyncConfig so it can be shared. Other negatives now consistently fall back to the static pivot, matching the behavior in OnSyncModeChanged.
- revert equivalent logging change in TrySetFreshPivot - restore original early-return shape in OnSyncModeChanged with `|| _maxAttempts == InfiniteAttempts` added to the retry condition - tighten ShouldBeInUpdatingPivot to accept only positives or the -1 sentinel, so other negatives no longer keep the mode active - move `InfiniteAttempts` to `ISyncConfig` so it can be shared between `MultiSyncModeSelector` and `StartingSyncPivotUpdater` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- merge `Finite_attempts_fall_back_to_static_pivot_after_exhaustion` and `Infinite_attempts_never_fall_back_to_static_pivot` into one parameterized test (`TestCase` with `TestName`), since they differ only by input and expected outputs - rewrite `ShouldBeInUpdatingPivot` check as `is > 0 or ISyncConfig.InfiniteAttempts` for terser intent Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
marcindsobczak
left a comment
marcindsobczak
left a comment
There was a problem hiding this comment.
The current default _syncConfig.MaxAttemptsToUpdatePivot means about 68 years of retries at one per second, so it's already infinite for any node in practice. Adding -1 doesn't change real behavior.
So in this context I'm not sure if this change is worth the extra complexity.
On the other hand, modifying a config value at runtime (as we do now) is bad practice, and this is a good opportunity to fix that tech debt. Maybe introduce a bool flag PivotUpdateCompleted and stop modifying _syncConfig.MaxAttemptsToUpdatePivot at runtime? Then introducing "real" infinity would make more sense.
Also, these changes require careful testing before merging to master, as they could break the node (obstruct the whole sync process).
| UInt256 PivotTotalDifficultyParsed => UInt256.Parse(PivotTotalDifficulty ?? "0"); | ||
|
|
||
| [ConfigItem(Description = "The max number of attempts to update the pivot block based on the FCU message from the consensus client.", DefaultValue = "2147483647")] | ||
| [ConfigItem(Description = "The max number of attempts to update the pivot block based on the FCU message from the consensus client. Set to `-1` to retry forever (recommended for nodes that may start before the consensus client is available).", DefaultValue = "2147483647")] |
There was a problem hiding this comment.
Worth to set default to -1 now as the current number is a workaround to achieve (almost) infinity
marcindsobczak
left a comment
marcindsobczak
left a comment
There was a problem hiding this comment.
Generally approving, it's fine also in the current shape. Worth changing the default to -1
Earlier versions defaulted to int.MaxValue (~68 years), which is indistinguishable from infinite in practice but quietly drifts toward the static-pivot fallback. Switch the default to the explicit InfiniteAttempts sentinel so the recommended behavior — keep retrying until the consensus client provides a finalized hash — is the default out of the box. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
Fixes #5992
Changes
StartingSyncPivotUpdater.InfiniteAttempts = -1as a typed sentinel for theMaxAttemptsToUpdatePivotconfig value.MaxAttemptsToUpdatePivot == -1, the updater retries forever and never falls back to the static config pivot. Existing semantics for0(disabled) and positive values are unchanged.MultiSyncModeSelector.ShouldBeInUpdatingPivotnow widens the gate from> 0to!= 0, so the-1value keeps theUpdatingPivotsync mode active.-1opt-in.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Two new cases in
StartingSyncPivotUpdaterTests:Finite_attempts_fall_back_to_static_pivot_after_exhaustion— with a finite cap and no finalized hash, the updater eventually callsAllowBeaconHeaderSyncand setsMaxAttemptsToUpdatePivot = 0.Infinite_attempts_never_fall_back_to_static_pivot— withInfiniteAttempts, 100 failed sync-mode changes neither triggerAllowBeaconHeaderSyncnor mutate the config back to0.The pre-existing happy-path tests continue to pass.
Documentation
Requires documentation update
The config description on
ISyncConfig.MaxAttemptsToUpdatePivotis updated in this PR to mention the-1sentinel; that surfaces automatically in the generated docs.Requires explanation in Release Notes
Remarks
Default stays at
int.MaxValue(~68 years) — this PR is purely opt-in. Earlier attempts (#5988, #8107) either changed the default unconditionally or went stale before tests were added; this iteration narrows the surface area to a single sentinel value and adds explicit coverage for both branches.