You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Explicitly states that store's finalized checkpoint must be used to obtain finalized_block_hash that is sent to EL. Apparently, all CL clients are already following this logic.
@hwwhww we would need to create tests checking that store.finalized used instead of head_state.finalized to obtain finalized_block_hash. These values can't diverge according to current specification, as tips with finalized checkpoint not matching to the store's one are filtered out and not eligible for the head. Not sure if we can implement such tests without changing the fork choice spec.
we would need to create tests checking that store.finalized used instead of head_state.finalized to obtain finalized_block_hash. These values can't diverge according to current specification, as tips with finalized checkpoint not matching to the store's one are filtered out and not eligible for the head. Not sure if we can implement such tests without changing the fork choice spec.
To provide test vectors, we need a new test format to mock notify_forkchoice_updated. I think it may be something like #2639 proposed, but probably in a simpler form. What do you think?
I pushed basic unit tests (270d930) to verify code logic of this PR.
To provide test vectors, we need a new test format to mock notify_forkchoice_updated. I think it may be something like #2639 proposed, but probably in a simpler form. What do you think?
Do you think that by mocking notify_forkchoice_updated we can bypass the requirement of store.finalized and head_state.finalized to diverge? Or you think this requirement is not needed to be satisfied to write tests covering proposed change?
This file contains hidden or 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
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.
Explicitly states that store's finalized checkpoint must be used to obtain
finalized_block_hashthat is sent to EL. Apparently, all CL clients are already following this logic.@hwwhww we would need to create tests checking that
store.finalizedused instead ofhead_state.finalizedto obtainfinalized_block_hash. These values can't diverge according to current specification, as tips with finalized checkpoint not matching to the store's one are filtered out and not eligible for the head. Not sure if we can implement such tests without changing the fork choice spec.