Add new endpoint to fetch fork choice context from a client.

[rolfyone]

Fixes #231

[potuz]

Just a couple of quick comments: The API says justified_epoch but uses $ref: './misc.yaml#/Checkpoint'.

Prysm cannot know the justified checkpoints of the nodes, it does know the justified epoch though. Finalized checkpoint is fine cause we do not keep any node beyond finalized and all nodes are descendant of the same finalized checkpoint.

Edit: actually due to an implementation detail we may not even know the finalized checkpoint of older nodes if forkchoice is small enough

[potuz]

I feel like other store specifics are universal: the best justified checkpoint, the proposer boost root (perhaps even the previous proposer boost root),

[rolfyone]

Prysm cannot know the justified checkpoints of the nodes, it does know the justified epoch though. Finalized checkpoint is fine cause we do not keep any node beyond finalized and all nodes are descendant of the same finalized checkpoint.

The prysm structure from the issue suggested you had the checkpoint I thought?

message ForkChoiceResponse {
    // Latest justified checkpoint in forkchoice store.
    Checkpoint justified_checkpoint = 1;
    // Latest finalized checkpoint in forkchoice store.
    Checkpoint finalized_checkpoint = 2;

[potuz]

The prysm structure from the issue suggested you had the checkpoint I thought?

message ForkChoiceResponse {
    // Latest justified checkpoint in forkchoice store.
    Checkpoint justified_checkpoint = 1;
    // Latest finalized checkpoint in forkchoice store.
    Checkpoint finalized_checkpoint = 2;

That's the store's checkpoint, not the node's ones

[rolfyone]

I'll change that to epoch and we can always embed the extra in extra_data if we think we need it... @tbenr hopefully epoch is sufficient for what you were thinking...

[tbenr]

I'm currently thinking about something like:

{
    "data_source": "teku", // self consistency, no API call to version required to get the context for interpreting extra_data
    "effective_ballance": "112228712638716", // effective balance for the current epoch
    "weight_mode": "COMULATIVE_TO_ROOT",  // NON_COMULATIVE | COMULATIVE_TO_ROOT | COMULATIVE_TO_HEAD
    "fork_choice_store": [
        {
            "slot": "123",
            "block_root": "0x...",
            "parent_root": "0x...",
            "justified_epoch": "1",
            "finalized_epoch": "2",
            "execution_payload_root": "0x...",
            "validity": "VALID", // VAILD | INVALID | OPTIMISTIC
            "weight": "678976",
            "extra_data": {
                "best_descendant_index": "3" // teku specific
            }
        },
...
    ]
}

@potuz does weight_mode make sense to you?

effective_ballance is something very useful IMO, for giving percentages to nodes\heads

[potuz]

@potuz does weight_mode make sense to you?

I assume the "cumulative-to-root" weight of a node means the sum of all the non-cummulative weights of every descendant (including itself)? Except perhaps for proposer boost.
If that's the case then we're happy to include this but it's not necessary if this is only because of us: we track both the non-cumulative and the cumulative to root weights of nodes.

Cumulative to head seems to be the right thing for visualization. You can derive any one from any other, so if teams use majoritarily cumulative to root, we can easily adapt to that.

In addition to the list of nodes (that I would call something like fork_choice_nodes instead of store) I think it's useful to include store level information like the diverse justified/finalized checkpoints, the proposer boost root, the proposer boost score the head root, etc.

Also if possible it would be nicer to teams to avoid the extra_data field and just let them include other fields as long as they don't have name collision with the required ones. But I'm ignorant in API handling I don't know how intrusive this is to tooling

[potuz]

effective_ballance is something very useful IMO, for giving percentages to nodes\heads

I think it should be for the justified checkpoint which is what's used in computations and not the current epoch. It's easy for us to include this one, but not sure if it would be for other teams

[potuz]

I'm wondering if it's not worth to have timestamp as a mandatory field in all of the reorgs we've had from mainnet having the timestamp was instrumental to quickly assess that the cause was simply a late block

[ajsutton]

I'm wondering if it's not worth to have timestamp as a mandatory field in all of the reorgs we've had from mainnet having the timestamp was instrumental to quickly assess that the cause was simply a late block

Teku doesn't currently track this and it's not a necessary thing to track (also there's a lot of variation in what you might want to track as a client - arrival time vs actual import time etc). So I'd be keen to keep it as an optional field so we don't lock ourselves into tracking unnecessary data, but it could be provided if available. I'm not against tracking it in teku if it's useful for debugging just don't want to be locked into it forever by this API.

The other way to see if a block is late would be to request the fork choice state at 4s into the slot - if the block's not there then, it must have been late. Teku also provides info in logs about late blocks etc so the data is available if needed.

[potuz]

The one big advantage of having the timestamp on the dump is when using @tbenr viewer: if you order the nodes by timestamp instead of by slot number, you get a much more clear picture of the forkchoice context in the event of reorgs. Before we added timestamps to our node, we would have to go to the logs for each slot individually to find out when the block was received. I do recognize that there's a difference between receival time, forkchoice insertion time, etc. But I think these are minor issues in front of the better UX

[ajsutton]

Yeah, I acknowledge it's useful, but I wouldn't want to require fork choice tracks it just for this API, hence I think it should be optional. Clients can then choose if its worth tracking or not and if they don't support it they just won't be able to sort by arrival time in the viewer.

[rolfyone]

I'm not sure whether we decided on finalized_checkpoint or finalized_epoch. The previous code had the label epoch, but the checkpoint structure... @tbenr this has the changes you sent me earlier I think, and lints fixed...

[tbenr]

thanks @rolfyone for proxying the PR and linting it :)

I'm not sure whether we decided on finalized_checkpoint or finalized_epoch

At the beginning of the thread @potuz says prysm doesn't track the full justified checkpoint but only the epoch. So I erroneously downgraded all checkpoint to epoch. Having finalized_checkpoint and justified_epoch seems fitting what prysm can do. An option would be to assume that justified_checkpoint \ justified_epoch goes to extra_data.

Or we could have them defined as

CheckpointOrEpoch:
  oneOf:
    - $ref: '../../beacon-node-oapi.yaml#/components/schemas/Checkpoint'
    - $ref: './primitive.yaml#/Uint64'

So I added an extra_data field in the topmost object so clients can add more info as a general context for the dump.

I was thinking about adding primitive.yaml/Version to have context for the extra_data

[michaelsproul]

LGTM!