[Merged by Bors] - Don't return errors when fork choice fails #3370
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paulhauner
force-pushed
the
invalid-head
branch
2 times, most recently
from
35d64c7 to
ab0f7f2
Compare
paulhauner
added
ready-for-review
michaelsproul
approved these changes
Jul 28, 2022
michaelsproul
added
waiting-on-author
Don't return an error from fork choice Fix more tests Add test Fix beacon processor test Improve test Fix block import Rebuild from finalized state if `get_head` fails Rebuild from finalized state if `get_head` fails Revert changes to `ForkChoice` Add failing test Revert "Rebuild from finalized state if `get_head` fails" This reverts commit 17cf507. Add `set_all_blocks_to_optimistic` Remove code fragment Fix tests
|
This looks ready to merge, I noticed that my corrections from above got reverted but that's no biggie, we can merge without them |
michaelsproul
added
ready-for-merge
Co-authored-by: Michael Sproul <[email protected]>
|
bors r+ |
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
bors bot
pushed a commit
that referenced
this pull request
Jul 28, 2022
## Issue Addressed
NA
## Proposed Changes
There are scenarios where the only viable head will have an invalid execution payload, in this scenario the `get_head` function on `proto_array` will return an error. We must recover from this scenario by importing blocks from the network.
This PR stops `BeaconChain::recompute_head` from returning an error so that we can't accidentally start down-scoring peers or aborting block import just because the current head has an invalid payload.
## Reviewer Notes
The following changes are included:
1. Allow `fork_choice.get_head` to fail gracefully in `BeaconChain::process_block` when trying to update the `early_attester_cache`; simply don't add the block to the cache rather than aborting the entire process.
1. Don't return an error from `BeaconChain::recompute_head_at_current_slot` and `BeaconChain::recompute_head` to defensively prevent calling functions from aborting any process just because the fork choice function failed to run.
- This should have practically no effect, since most callers were still continuing if recomputing the head failed.
- The outlier is that the API will return 200 rather than a 500 when fork choice fails.
1. Add the `ProtoArrayForkChoice::set_all_blocks_to_optimistic` function to recover from the scenario where we've rebooted and the persisted fork choice has an invalid head.
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Jul 30, 2022
## Issue Addressed NA ## Proposed Changes This PR will make Lighthouse return blocks with invalid payloads via the API with `execution_optimistic = true`. This seems a bit awkward, however I think it's better than returning a 404 or some other error. Let's consider the case where the only possible head is invalid (#3370 deals with this). In such a scenario all of the duties endpoints will start failing because the head is invalid. I think it would be better if the duties endpoints continue to work, because it's likely that even though the head is invalid the duties are still based upon valid blocks and we want the VC to have them cached. There's no risk to the VC here because we won't actually produce an attestation pointing to an invalid head. Ultimately, I don't think it's particularly important for us to distinguish between optimistic and invalid blocks on the API. Neither should be trusted and the only *real* reason that we track this is so we can try and fork around the invalid blocks. ## Additional Info - ~~Blocked on #3370~~
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
NA
Proposed Changes
There are scenarios where the only viable head will have an invalid execution payload, in this scenario the
get_headfunction onproto_arraywill return an error. We must recover from this scenario by importing blocks from the network.This PR stops
BeaconChain::recompute_headfrom returning an error so that we can't accidentally start down-scoring peers or aborting block import just because the current head has an invalid payload.Reviewer Notes
The following changes are included:
fork_choice.get_headto fail gracefully inBeaconChain::process_blockwhen trying to update theearly_attester_cache; simply don't add the block to the cache rather than aborting the entire process.BeaconChain::recompute_head_at_current_slotandBeaconChain::recompute_headto defensively prevent calling functions from aborting any process just because the fork choice function failed to run.ProtoArrayForkChoice::set_all_blocks_to_optimisticfunction to recover from the scenario where we've rebooted and the persisted fork choice has an invalid head.