Repository navigation
Conversation
|
not marking this as ready to review as the upstream PR isn't merged yet |
|
@chong-he The spec PR has been merged and this PR has been upto date with the spec hence this PR is ready for review. |
|
This pull request has merge conflicts. Could you please resolve them @SamAg19? 🙏 |
mergify
Bot
added
waiting-on-author
mergify
Bot
added
ready-for-review
chong-he
left a comment
chong-he
left a comment
There was a problem hiding this comment.
Some comments below. I notice for V2 debug endpoint, both the inner and outer-most extra_data is now required in the beacon API:
https://github.com/ethereum/beacon-APIs/blob/master/types/fork_choice.yaml
https://github.com/ethereum/beacon-APIs/blob/master/apis/debug/fork_choice.v2.yaml#L25
This wasn't the case for the V1 debug endpoint. So we now have both extra_data to have some content there
chong-he
added
waiting-on-author
chong-he
left a comment
chong-he
left a comment
There was a problem hiding this comment.
Looks pretty good now, just a bit more comments to make the test structure more organized
chong-he
left a comment
chong-he
left a comment
There was a problem hiding this comment.
Looks good now, just the following function that I am unsure of if it is necessary. We will wait for developers' opinion on this
|
|
||
| /// Fills in `parent_root` from the store for nodes whose parent was pruned. Stays zero if the | ||
| /// block can't be loaded. | ||
| fn fill_pruned_parent_roots<T: BeaconChainTypes>( |
There was a problem hiding this comment.
Not sure if this is necessary? We will wait for a dev's comment on this
There was a problem hiding this comment.
yeah this is not necessary or desired. if a node has had its parents pruned we can either leave it with null parents or exclude it from the dump
chong-he
added
ready-for-review
chong-he
left a comment
chong-he
left a comment
There was a problem hiding this comment.
The diff is much cleaner now, thanks for the PR
|
Tick the box to add this pull request to the merge queue (same as
|
|
This pull request has merge conflicts. Could you please resolve them @SamAg19? 🙏 |
mergify
Bot
added
waiting-on-author
|
|
Issue Addressed
Add the v2 fork choice debug endpoint proposed in ethereum/beacon-APIs#615 and deprecate v1
Proposed Changes
ForkChoiceV2,ForkChoiceExtraDataV2,ForkChoiceNodeV2andForkChoiceNodeExtraDataV2response types tocommon/eth2/src/types.rsfor the endpointGET v2/debug/fork_choiceendpointdebug_fork_choice.rsmodule that expands each proto-array node into the spec's(block_root, payload_status)nodesget_debug_fork_choice_v1and replaced fork choice v1 test with fork choice v2 test (following a similar pattern seen with theGET debug/beacon/headsendpoint)