-
Notifications
You must be signed in to change notification settings - Fork 896
Add extra data in /eth/v1/debug/fork_choice
#7845
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
base: unstable
Are you sure you want to change the base?
Conversation
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 have added most fields mentioned in #7829 , except for balance
from the response of Prysm. Initially I thought it is the weight
(which is already returned in the main response), but the data shows that weight
is not equal to balance
most of the time.
This is the field in the source code, which leads to this struct. I only see weight
in ProtoNode
in Lighthouse code.
Other than that, this PR is ready for review. I am happy to add/remove any fields that are required/not necessary, or rearrange the fields to make it more readable
beacon_node/http_api/src/lib.rs
Outdated
unrealized_finalized_epoch: node | ||
.unrealized_finalized_checkpoint | ||
.map(|checkpoint| checkpoint.epoch), | ||
timestamp: timestamp_now().as_secs(), |
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.
This field and best_child
and best_descendant
are returning numbers without quotes ""
. I see the response from Prysm is with the quotes, as they convert it to string. So wondering whether I can leave these fields as pure numbers?
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.
Usually quoted numbers are better for compatibility.
For best child and best descendant does Prysm use a numeric index or a block root? To me it seems like a block root would be clearer
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.
The code is here (also found the definition for balance
): https://github.com/OffchainLabs/prysm/blob/develop/beacon-chain/forkchoice/doubly-linked-tree/types.go#L60-L62
I think the *Node refers to the block root?
In Lighthouse, it is defined as Option<usize>
(index):
pub best_descendant: Option<usize>, |
Do we have to change this?
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.
You can convert the index into a block root by looking it up in the array of nodes. E.g. if the index for best child is 5, use the block root of the 5th node as best child
For the balance vs weight, I think the difference is the attestation weight for the single block, vs the weight for that block and all its descendants. I think we can leave out the balance for now because it's a bit more involved to calculate 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.
Changed timestamp
to String, and best_child
and best_descendant
to display block_root instead
Thanks!
Issue Addressed
Additional Info
Querying the endpoint
curl -X 'GET' 'http://localhost:5052/eth/v1/debug/fork_choice' | jq
yields:Before:
After: