-
Notifications
You must be signed in to change notification settings - Fork 834
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.
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
[POC] Comment out backwards sync for newPayload #5666
Conversation
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
|
This is a significant improvement for nimbus nodes. I tried two besu restarts and it recovered with a brief backwards sync.
|
I also tested with teku and it worked fine, i.e. got into sync which uses backwards sync. Restarting either or both clients doesn't really exercise this PR since it uses in-order lock-step rather than backwards sync. |
Tested nimbus and teku with both clients offline for 1.5 hours, both working. |
// LOG.atDebug() | ||
// .setMessage("Parent of block {} is not present, append it to backward sync") | ||
// .addArgument(block::toLogString) | ||
// .log(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should still log at debug, just change the message to indicate we have dropped it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't be merging this PR. If we want to progress this, we should create an issue and prioritise it.
I think we should take a more considered approach to ensure we align with the spec.
Also, there's more code we can rip out/refactor if we remove this code path.
closing, we will revisit #5411 as needed |
PR description
Running a quick feasibility test based on #5411 (comment)
Fixed Issue(s)