Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make pivot update attempts infinite #5988

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Make pivot update attempts infinite #5988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e612e5b
make pivot update attempts infinite
marcindsobczak Aug 7, 2023
942af4a
remove remaining attempts from logs
marcindsobczak Aug 7, 2023
835526e
add counting to logs
marcindsobczak Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Nethermind/Nethermind.Blockchain/Synchronization/ISyncConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public interface ISyncConfig : IConfig
[ConfigItem(DisabledForCli = true, HiddenFromDocs = true)]
Keccak? PivotHashParsed => PivotHash is null ? null : new Keccak(Bytes.FromHexString(PivotHash));

[ConfigItem(Description = "Max number of attempts (seconds) to update pivot block basing on Forkchoice message from Consensus Layer. Only for PoS chains.", DefaultValue = "900")]
[ConfigItem(Description = "Max number of attempts (seconds) to update pivot block basing on Forkchoice message from Consensus Layer. Only for PoS chains. Infinite by default.", DefaultValue = "2147483647")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcindsobczak Why not some value like -1? I guess this is because setting Int.Max is quicker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just have logic which is checking if attemts left > 0 in few places, making support for -1 will be nice, but requires bigger refactoring and it is always some risk. If we are not planning to use values between disabled and infinite, we could make it bool as well. But doing it extremely high (int.Max = 68 years) works the same as infinite, is quick and safest, as doesn't require any changes in the code, just config value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that is what I expected to hear - we can go safe way but would be nice to leave some issue to refactor this in future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created low priority good first issue for refactoring it to be infinite instead of ~68 years: #5992

int MaxAttemptsToUpdatePivot { get; set; }

[ConfigItem(Description = "[EXPERIMENTAL] Defines the earliest body downloaded in fast sync when DownloadBodiesInFastSync is enabled. Actual values used will be Math.Max(1, Math.Min(PivotNumber, AncientBodiesBarrier))", DefaultValue = "0")]
Expand Down
2 changes: 1 addition & 1 deletion src/Nethermind/Nethermind.Blockchain/Synchronization/SyncConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public string? PivotHash
get => FastSync || SnapSync ? _pivotHash : null;
set => _pivotHash = value;
}
public int MaxAttemptsToUpdatePivot { get; set; } = 900;
public int MaxAttemptsToUpdatePivot { get; set; } = int.MaxValue;
public bool WitnessProtocolEnabled { get; set; } = false;
public bool SnapSync { get; set; } = false;
public int SnapSyncAccountRangePartitionCount { get; set; } = 8;
Expand Down
6 changes: 4 additions & 2 deletions src/Nethermind/Nethermind.Merge.Plugin/Synchronization/PivotUpdator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class PivotUpdator

private readonly CancellationTokenSource _cancellation = new();

private static int _maxAttempts;
private int _attemptsLeft;
private int _updateInProgress;
private Keccak _alreadyAnnouncedNewPivotHash = Keccak.Zero;
Expand All @@ -55,6 +56,7 @@ public PivotUpdator(IBlockTree blockTree,
_metadataDb = metadataDb ?? throw new ArgumentNullException(nameof(metadataDb));
_logger = logManager.GetClassLogger() ?? throw new ArgumentNullException(nameof(logManager));

_maxAttempts = syncConfig.MaxAttemptsToUpdatePivot;
_attemptsLeft = syncConfig.MaxAttemptsToUpdatePivot;

if (!TryUpdateSyncConfigUsingDataFromDb())
Expand Down Expand Up @@ -141,7 +143,7 @@ private async Task<bool> TrySetFreshPivot(CancellationToken cancellationToken)

if (finalizedBlockHash is null || finalizedBlockHash == Keccak.Zero)
{
if (_logger.IsInfo && _attemptsLeft % 10 == 0) _logger.Info($"Waiting for Forkchoice message from Consensus Layer to set fresh pivot block. {_attemptsLeft} attempts left");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attempts left was pretty nice but if we keep int.max approach it does not have sense indeed. If You would plan to add some value like -1 to make it infinite instead of this approach, please rollback this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it will be infinite, then it will have even less sense, because infinite number of attempt will be always left. We can add one more variable and just increment it - it will be something like:
Waiting for Forkchoice message from Consensus Layer to set fresh pivot block [{counter}s]

And for users
Waiting for Forkchoice message from Consensus Layer to set fresh pivot block [1s]
Waiting for Forkchoice message from Consensus Layer to set fresh pivot block [2s]
...
Waiting for Forkchoice message from Consensus Layer to set fresh pivot block [123s]

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice! :)

if (_logger.IsInfo && (_maxAttempts - _attemptsLeft) % 10 == 0) _logger.Info($"Waiting for Forkchoice message from Consensus Layer to set fresh pivot block [{_maxAttempts - _attemptsLeft}s]");

return null;
}
Expand Down Expand Up @@ -217,7 +219,7 @@ private async Task<bool> TrySetFreshPivot(CancellationToken cancellationToken)
}
}

if (_logger.IsInfo && _attemptsLeft % 10 == 0) _logger.Info($"Potential new pivot block hash: {finalizedBlockHash}. Waiting for pivot block header. {_attemptsLeft} attempts left");
if (_logger.IsInfo && (_maxAttempts - _attemptsLeft) % 10 == 0) _logger.Info($"Potential new pivot block hash: {finalizedBlockHash}. Waiting for pivot block header [{_maxAttempts - _attemptsLeft}s]");
return null;
}

Expand Down