[Merged by Bors] - Add execution_optimistic flag to HTTP responses

[macladson]

Issue Addressed

#3031

Proposed Changes

Updates the following API endpoints to conform with ethereum/beacon-APIs#190 and ethereum/beacon-APIs#196

• beacon/states/{state_id}/root
• beacon/states/{state_id}/fork
• beacon/states/{state_id}/finality_checkpoints
• beacon/states/{state_id}/validators
• beacon/states/{state_id}/validators/{validator_id}
• beacon/states/{state_id}/validator_balances
• beacon/states/{state_id}/committees
• beacon/states/{state_id}/sync_committees
• beacon/headers
• beacon/headers/{block_id}
• beacon/blocks/{block_id}
• beacon/blocks/{block_id}/root
• beacon/blocks/{block_id}/attestations
• debug/beacon/states/{state_id}
• debug/beacon/heads
• validator/duties/attester/{epoch}
• validator/duties/proposer/{epoch}
• validator/duties/sync/{epoch}

Updates the following Server-Sent Events:

• events?topics=head
• events?topics=block
• events?topics=finalized_checkpoint
• events?topics=chain_reorg

Backwards Incompatible

There is a very minor breaking change with the way the API now handles requests to beacon/blocks/{block_id}/root and beacon/states/{state_id}/root when block_id or state_id is the Root variant of BlockId and StateId respectively.

Previously a request to a non-existent root would simply echo the root back to the requester:

curl "http://localhost:5052/eth/v1/beacon/states/0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/root"
{"data":{"root":"0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}}

Now it will return a 404:

curl "http://localhost:5052/eth/v1/beacon/blocks/0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/root"
{"code":404,"message":"NOT_FOUND: beacon block with root 0xaaaa…aaaa","stacktraces":[]}

In addition to this is the block root 0x0000000000000000000000000000000000000000000000000000000000000000 previously would return the genesis block. It will now return a 404:

curl "http://localhost:5052/eth/v1/beacon/blocks/0x0000000000000000000000000000000000000000000000000000000000000000"
{"code":404,"message":"NOT_FOUND: beacon block with root 0x0000…0000","stacktraces":[]}

Additional Info

• execution_optimistic is always set, and will return false pre-Bellatrix. I am also open to the idea of doing something like #[serde(skip_serializing_if = "Option::is_none")].
• The value of execution_optimistic is set to false where possible. Any computation that is reliant on the head will simply use the ExecutionStatus of the head (unless the head block is pre-Bellatrix).

[macladson]

Note that this code will return execution_optimistic: None upon an error (which will then be skip_serialized out of the response).
I wanted to avoid the case where there are multiple heads but an error in one prevented any response from being shown. This felt like the lesser of two evils. The downside is that this behavior is not very clear. As this is a debug endpoint, this might be fine however.

It seems to me that there are two options here:

1. An error for 1 head is an error for all heads
2. No errors, but execution_optimistic: None means that there was an error in it's computation.
   (This should be distinct since to get this new behaviour you have to use the v2 endpoint)

Happy to hear alternate solutions.

[macladson]

I suspect there could be some duplicate/redundant code with #3140 regarding determining optimistic statuses. I'm happy to try and unify those different methods (assuming #3140 is merged in first, which seems likely)

[paulhauner]

Ok this is looking great! I have one more suggestion, but I've already written it up to try and be less annoying 😅

I propose that you merge this branch into yours: https://github.com/paulhauner/lighthouse/tree/ex-opt-api-atomic

You can see a diff here: paulhauner/lighthouse@0e6ffcd...c168704

I was concerned about the atomicity of "get the object" and then later "check to see if the object is optimistic". In particular, when we get an an object from the head and then later check to see if the head is optimistic. It's possible that the head was optimistic when we got the object and then we switch to a non-optimistic head in the meantime; that would result in reporting optimistic values an non-optimistic.

Have a look at my approach, I hope that you'll find that you like it. It manages to do away with some duplication, the HeaderComputationType enum and it also highlights a few methods that involve optimistic assumptions where we're not returning an optimistic indication (I don't think we need to worry about them).

[macladson]

Have a look at my approach, I hope that you'll find that you like it. It manages to do away with some duplication, the HeaderComputationType enum and it also highlights a few methods that involve optimistic assumptions where we're not returning an optimistic indication (I don't think we need to worry about them).

I think this makes a lot of sense. Thanks for writing this up! Happy to merge this in, I think it's a great improvement 👍

[paulhauner]

Let's do it!

bors r+

[paulhauner]

Halting bors so we can fix a simple issue with state root lookups.

bors r-

[bors]

Canceled.

[paulhauner]

Looks good, let's go!

bors r+

[bors]

Pull request successfully merged into unstable.

Build succeeded:

• arbitrary-check
• beacon-chain-tests
• cargo-audit
• cargo-fmt
• cargo-udeps
• cargo-vendor
• check-benchmarks
• check-consensus
• check-msrv
• clippy
• debug-tests-ubuntu
• dockerfile-ubuntu
• doppelganger-protection-test
• ef-tests-ubuntu
• eth1-simulator-ubuntu
• execution-engine-integration-ubuntu
• merge-transition-ubuntu
• no-eth1-simulator-ubuntu
• op-pool-tests
• release-tests-ubuntu
• release-tests-windows
• state-transition-vectors-ubuntu