Optimistic Sync #2770
Optimistic Sync #2770
Conversation
specs/bellatrix/p2p-interface.md
Outdated
| - [IGNORE] The block's parent (defined by `block.parent_root`) passes all | ||
| validation, including verification of the `block.body.execution_payload`. |
There was a problem hiding this comment.
Would it be reasonable to phrase this as:
| - [IGNORE] The block's parent (defined by `block.parent_root`) passes all | |
| validation, including verification of the `block.body.execution_payload`. | |
| - [IGNORE] The block's parent (defined by `block.parent_root`) passes | |
| verification of the `block.body.execution_payload`. |
Given the reject rule above we already know that all verification except for the execution_payload passes so are just choosing between ignore and accept based on whether the execution payload is valid or not.
Although both versions still feel ambiguous about whether the result is ACCEPT or IGNORE in the case of the parent execution payload not yet being executed because the EL is syncing. I think it should be ACCEPT because the whole point is that an optimistic node can't validate the payload but also needs to avoid censoring blocks just because it can't validate the parent.
There was a problem hiding this comment.
These two conditions are a bit tough to parse in the way they are phrased in the positive.
Agreed that ACCEPT vs IGNORE is particularly hard to parse here.
Maybe:
- If `exection_payload` verification of block's parent is *not* complete:
- [REJECT] The block's parent (defined by `block.parent_root`) passes all
validation (excluding verification of the `block.body.execution_payload`).
- otherwise:
- [IGNORE] The block's parent (defined by `block.parent_root`) passes all
validation (including verification of the `block.body.execution_payload`).
I know it says the same thing, but I think the outer if/else might help parsing them
There was a problem hiding this comment.
I struggled to figure out the wording for this. The language for these propagation conditions is tricky!
I've applied @djrtwo's suggestion in b1ec9bc.
I was tempted to go off piste and use a different specification language for these conditions, since it would have been easier to parse (IMO). However, that breaks standard and might get a bit unruly if we have another spec that needs to override some of these rules.
I'm still open to suggestion here 😅
There was a problem hiding this comment.
yeah, I think I likely made a bad decision quite a while ago on the choice of if not positive_condition: REJECT/IGNORE. Might have been saner as if negative_condition: REJECT/IGNORE...
specs/bellatrix/p2p-interface.md
Outdated
| - [IGNORE] The block's parent (defined by `block.parent_root`) passes all | ||
| validation, including verification of the `block.body.execution_payload`. |
There was a problem hiding this comment.
These two conditions are a bit tough to parse in the way they are phrased in the positive.
Agreed that ACCEPT vs IGNORE is particularly hard to parse here.
Maybe:
- If `exection_payload` verification of block's parent is *not* complete:
- [REJECT] The block's parent (defined by `block.parent_root`) passes all
validation (excluding verification of the `block.body.execution_payload`).
- otherwise:
- [IGNORE] The block's parent (defined by `block.parent_root`) passes all
validation (including verification of the `block.body.execution_payload`).
I know it says the same thing, but I think the outer if/else might help parsing them
| Thankfully, if 2/3rds of validators are not poisoned, they can justify an | ||
| honest chain which will un-poison all other nodes. | ||
|
|
||
| Notably, this attack only exists for optimistic nodes. Nodes which fully verify |
There was a problem hiding this comment.
Probably worth making it even clearer that this means that if the vast majority of the network is "synced" going into the transition (which is generally the case for the network, and validator-nodes in particular), then the network cannot be poisoned
There was a problem hiding this comment.
Addressed in 0ec61bd
Co-authored-by: Danny Ryan <[email protected]>
sync/optimistic.md
Outdated
| ```python | ||
| def should_optimistically_import_block(store: Store, current_slot: Slot, block: BeaconBlock) -> bool: | ||
| justified_root = store.block_states[store.head_block_root].current_justified_checkpoint.root | ||
| justifed_is_verified = is_execution_block(store.blocks[justified_root]) |
There was a problem hiding this comment.
nit: Is justified_is_verified the right name here? We're effectively checking that the merge block is justified so may be merge_is_justified? or justified_is_execution_block.
To me the verified in the name is making me think it's about whether it's optimistic or fully verified.
There was a problem hiding this comment.
Ooh, good catch! Fixed in be4319a
build opimistic sync file
|
With the new status code ( |
Perhaps @djrtwo or @mkalinin have an opinion on if ethereum/execution-apis#165 will merge before this PR? I don't mind either way. |
|
Let's merge this now and patch the semantics in an updated PR. We're aiming to merge and release the engine api changes tomorrow (1/25). |
This PR introduces a specification for optimistic sync.
Optimistic sync provides a way for consensus engines (beacon nodes) to follow the head of the chain without verifying execution payloads. The requirement for this is based upon the premise that execution engines can sync most effectively if they are able to see the payload hashes from blocks at the immediate head of the beacon chain.
I have created an accompanying Opt. Sync Spec: Reading Companion to help explain some of my decisions.
Opt sync is a fairly tricky mechanism and it comes with some tangible risks to liveness. I would appreciate anyone who takes the time to cast a critical eye across this.
Thanks to @mkalinin, @ajsutton and @djrtwo for providing substance and feedback.