Add `execution_optimistic` flag to responses #190

paulhauner · 2022-02-04T00:39:55Z

Overview

Adds an execution_optimistic: bool to the "metadata" of endpoints which might be "tainted" by an assumption about execution payload validity in an optimistic node.

This PR conflicts with the "Ethereum Beacon APIs" section of the optimistic sync spec. I propose that these changes should supersede the opt sync spec. That spec will need to be updated if this PR is merged.

I have not done a version bump (e.g., add a v2 and deprecate v1) for most endpoints. I am operating under the assumption that an optional field can be added to metadata without breaking the API. I'm interested to other opinions about bumping the version of all affected endpoints. I'm open to bumping all versions that's expected.

It should be mentioned that I'm not attached to this proposal, but I'm concerned that we're running out of time to standardise API behaviour with optimistic sync. I'm completely open to criticism and alternate proposals. I suggest reading the Optimistic Sync: HTTP API to understand the problem this PR is trying to address.

Notes

I've added help text for the execution_optimistic bool, but I'm not sure there's a way to view it from the UI. My Swagger skills are poor, perhaps someone could help understand how to access it or where to make that info more prominent.

When it comes to choosing if to set execution_optimsitic, I think clients should be free to return true whenever their head is optimistic. A lot of endpoints are based around an assumption about the head, for instance getting the block at slot 1 involves making an assumption about what the head is. Clients can also be free to set it on a more granular basis.

ajsutton · 2022-02-04T01:02:37Z

Shouldn't need to bump the version number - adding a field is backwards compatible. And we do have precedent for that - we added dependent_root for duties without bumping the version number.

arnetheduck · 2022-02-04T07:46:30Z

Shouldn't need to bump the version number - adding a field is backwards compatible.

this is only partially true: de-facto, clients have been strict on extra fields because adding a field may alter the semantic requirements of correctly handling the message - we've had cross-client compatibility problems with different parsing strictness requirements on this point. basically, this would mean that any existing versions of clients possibly will stop working, forcing an upgrade, when the new field is added, until the strictness is relaxed.

If we're going to start adding fields, we should also decide on a policy for where this is allowed, with which constraints and what "old" versions of clients should do when they encounter a field they do not understand: what happens for example if a field is added to a spec type (like BeaconBlock) that you didn't expect to be there? It seems trivial now, but with enough forks "out there" it will be hard to maintain compatibility.

Even the config endpoint, which is an "open experimentation ground" for random constants has seen exhibits strictness compatibility issues.

mcdee · 2022-02-04T11:41:03Z

I would argue that metadata changes should not fall under the requirement of requiring a version bump. One of the reasons we designed the API to return results under a data heading is to allow for additional fields at the top level without disrupting the data itself. An optimistic-sync-unaware client should be able to continue with this change without any alteration in its behavior.

arnetheduck · 2022-02-04T11:43:05Z

One of the reasons we designed

This is a useful distinction, and if this is the way to go, we should make it explicit so that clients can design compatible data validation routines (and appropriately scope future additions to such "optional" fields)

mcdee · 2022-02-04T11:45:18Z

This is a useful distinction, and if this is the way to go, we should make it explicit so that clients can design compatible data validation routines (and appropriately scope future additions to such "optional" fields)

Totally agree with that, unfortunately there are various pieces of the API that are still way too much implicit rather than clearly stated anywhere. I'll see if I put together a PR to make this clear in the appropriate place in the spec.

djrtwo · 2022-02-07T14:04:59Z

checking in on this -- There seem to be two things under discussion

the use of execution_optimistic
how to extend the spec with additional optional data

Are we blocked on (1) until a subsequent PR for (2)

Or can we move forward tackling (1) here and now

paulhauner · 2022-02-07T23:38:24Z

Are we blocked on (1) until a subsequent PR for (2)

IMO we should move ahead with discussing execution_optimsitic regardless if we need to bump API versions or not. It's an important enough change to warrant a version bump if need be.

paulhauner · 2022-02-18T04:50:37Z

I'm looking for some help on getting CI passing 🙏 @mpetrunic, perhaps you are familiar with the error I'm getting?

mpetrunic · 2022-02-18T08:29:35Z

I'm looking for some help on getting CI passing pray @mpetrunic, perhaps you are familiar with the error I'm getting?

I already put PR with fix for it: #191 but we need to nudge @djrtwo or somebody with write access to approve so I can merge 😅

ajsutton · 2022-02-20T23:50:49Z

What does the block/state ID head mean during optimistic sync? Should it mean the optimistic head or the fully verified ancestor? I think given we're exposing optimistic data now I'd say it should be the optimistic head but we should make that clear.

Currently it's defined as "head" (canonical head in node's view)

mpetrunic · 2022-02-21T15:48:06Z

@paulhauner could you check "Allow edits from maintainers" or rebase latest master to fix ci 😄

paulhauner · 2022-02-22T02:18:11Z

@paulhauner could you check "Allow edits from maintainers" or rebase latest master to fix ci smile

Unfortunately I can't allow edits, the checkbox is missing.

I did rebase on master, but that doesn't seem to have helped much sorry.

mpetrunic · 2022-02-23T08:07:29Z

apis/debug/heads.v2.yaml

+                    slot:
+                      $ref: "../../beacon-node-oapi.yaml#/components/schemas/Uint64"
+                    execution_optimistic:
+                      $ref: "../../../beacon-node-oapi.yaml#/components/schemas/ExecutionOptimistic"


wrong reference, one too many ../

mpetrunic · 2022-02-23T08:07:36Z

apis/debug/state.v2.yaml

@@ -27,6 +27,8 @@ get:
                type: string
                enum: [ phase0, altair ]
                example: "phase0"
+              execution_optimistic:
+                $ref: "../../../beacon-node-oapi.yaml#/components/schemas/ExecutionOptimistic"


wrong reference, one too many ../

mpetrunic · 2022-02-23T08:08:36Z

@paulhauner looked closely and it seems you have some wrong refs :)

mpetrunic

@paulhauner duuude haha one more :)

apis/debug/heads.v2.yaml

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

mcdee · 2022-03-01T15:43:48Z

Is this good to merge now? Would be good to have the API locked down as much as possible before kiln is released.

arnetheduck · 2022-03-14T07:20:59Z

Sorry for the late idea, but one thing that makes the transition period easier here would be that the field is only added after the client has transitioned into the merge epoch - this allows existing mainnet and testnets to not be affected by the change until the upgrade has happened and the field becomes relevant.

This is a better approach compatibility-wise because the hard fork guarantees that only updated clients access the api:s this way - thoughts?

ajsutton · 2022-03-14T07:26:30Z

Sorry for the late idea, but one thing that makes the transition period easier here would be that the field is only added after the client has transitioned into the merge epoch - this allows existing mainnet and testnets to not be affected by the change until the upgrade has happened and the field becomes relevant.

This is doable but does make things more complex than I'd ideally like and means that any incompatibility is only discovered when Bellatrix is enabled.
Currently Teku is setup to only add the field if Bellatrix is enabled for a network (ie the fork epoch is not FAR_FUTURE_EPOCH) which has a similar effect of delaying the appearance of the field to give people time to ensure compatibility, just with a condition that doesn't change without a restart so makes things a bit easier. By the time we set a fork epoch for MainNet people will have had plenty of time to update so the new field is handled safely, but it's not so late as to be caught up in the actual fork activation.

## Issue Addressed #3031 ## Proposed Changes Updates the following API endpoints to conform with ethereum/beacon-APIs#190 and ethereum/beacon-APIs#196 - [x] `beacon/states/{state_id}/root` - [x] `beacon/states/{state_id}/fork` - [x] `beacon/states/{state_id}/finality_checkpoints` - [x] `beacon/states/{state_id}/validators` - [x] `beacon/states/{state_id}/validators/{validator_id}` - [x] `beacon/states/{state_id}/validator_balances` - [x] `beacon/states/{state_id}/committees` - [x] `beacon/states/{state_id}/sync_committees` - [x] `beacon/headers` - [x] `beacon/headers/{block_id}` - [x] `beacon/blocks/{block_id}` - [x] `beacon/blocks/{block_id}/root` - [x] `beacon/blocks/{block_id}/attestations` - [x] `debug/beacon/states/{state_id}` - [x] `debug/beacon/heads` - [x] `validator/duties/attester/{epoch}` - [x] `validator/duties/proposer/{epoch}` - [x] `validator/duties/sync/{epoch}` Updates the following Server-Sent Events: - [x] `events?topics=head` - [x] `events?topics=block` - [x] `events?topics=finalized_checkpoint` - [x] `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). Co-authored-by: Paul Hauner <paul@paulhauner.com>

paulhauner mentioned this pull request Feb 4, 2022

Optimistic sync changes for HTTP API sigp/lighthouse#2946

Closed

ajsutton mentioned this pull request Feb 10, 2022

[Merge] Support execution_optimistic flag in REST API Consensys/teku#4968

Closed

paulhauner mentioned this pull request Feb 17, 2022

Add the execution_optimistic flag to HTTP responses sigp/lighthouse#3031

Closed

paulhauner added 2 commits February 22, 2022 12:26

Add execution_optimistic

49e5b1a

Remove required field

3020332

paulhauner force-pushed the opt-flag branch from 350118d to 3020332 Compare February 22, 2022 01:27

mpetrunic requested changes Feb 23, 2022

View reviewed changes

paulhauner added 2 commits February 25, 2022 01:31

Update refs

e71d484

Fix more refs

b1b03ce

mpetrunic requested changes Feb 25, 2022

View reviewed changes

apis/debug/heads.v2.yaml Outdated Show resolved Hide resolved

Update apis/debug/heads.v2.yaml

d8b12fb

Co-authored-by: Marin Petrunić <mpetrunic@users.noreply.github.com>

mcdee mentioned this pull request Mar 1, 2022

Additioinal information about API endpoint versions, data and metadata. #195

Merged

mpetrunic approved these changes Mar 2, 2022

View reviewed changes

mpetrunic merged commit a7439b0 into ethereum:master Mar 2, 2022

macladson mentioned this pull request Mar 10, 2022

[Merged by Bors] - Add execution_optimistic flag to HTTP responses sigp/lighthouse#3070

Closed

22 tasks

rkapka mentioned this pull request Mar 17, 2022

Add execution_optimistic field to API responses prysmaticlabs/prysm#10389

Merged

zah mentioned this pull request Jun 9, 2022

Beacon API Spec Conformance status-im/nimbus-eth2#3666

Closed

dB2510 mentioned this pull request Oct 19, 2022

Return metadata fields for relevant endpoints attestantio/go-eth2-client#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `execution_optimistic` flag to responses #190

Add `execution_optimistic` flag to responses #190

paulhauner commented Feb 4, 2022 •

edited

Loading

ajsutton commented Feb 4, 2022

arnetheduck commented Feb 4, 2022

mcdee commented Feb 4, 2022

arnetheduck commented Feb 4, 2022 •

edited

Loading

mcdee commented Feb 4, 2022

djrtwo commented Feb 7, 2022

paulhauner commented Feb 7, 2022

paulhauner commented Feb 18, 2022

mpetrunic commented Feb 18, 2022

ajsutton commented Feb 20, 2022

mpetrunic commented Feb 21, 2022

paulhauner commented Feb 22, 2022

mpetrunic Feb 23, 2022

mpetrunic Feb 23, 2022

mpetrunic commented Feb 23, 2022

mpetrunic left a comment

mcdee commented Mar 1, 2022

arnetheduck commented Mar 14, 2022

ajsutton commented Mar 14, 2022

Add execution_optimistic flag to responses #190

Add execution_optimistic flag to responses #190

Conversation

paulhauner commented Feb 4, 2022 • edited Loading

Overview

Notes

ajsutton commented Feb 4, 2022

arnetheduck commented Feb 4, 2022

mcdee commented Feb 4, 2022

arnetheduck commented Feb 4, 2022 • edited Loading

mcdee commented Feb 4, 2022

djrtwo commented Feb 7, 2022

paulhauner commented Feb 7, 2022

paulhauner commented Feb 18, 2022

mpetrunic commented Feb 18, 2022

ajsutton commented Feb 20, 2022

mpetrunic commented Feb 21, 2022

paulhauner commented Feb 22, 2022

mpetrunic Feb 23, 2022

Choose a reason for hiding this comment

mpetrunic Feb 23, 2022

Choose a reason for hiding this comment

mpetrunic commented Feb 23, 2022

mpetrunic left a comment

Choose a reason for hiding this comment

mcdee commented Mar 1, 2022

arnetheduck commented Mar 14, 2022

ajsutton commented Mar 14, 2022

Add `execution_optimistic` flag to responses #190

Add `execution_optimistic` flag to responses #190

paulhauner commented Feb 4, 2022 •

edited

Loading

arnetheduck commented Feb 4, 2022 •

edited

Loading