Skip to content
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

Optimistic Sync #2770

Merged
merged 77 commits into from
Jan 25, 2022
Merged

Optimistic Sync #2770

merged 77 commits into from
Jan 25, 2022

Conversation

paulhauner
Copy link
Contributor

@paulhauner paulhauner commented Dec 20, 2021

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.

Comment on lines 102 to 103
- [IGNORE] The block's parent (defined by `block.parent_root`) passes all
validation, including verification of the `block.body.execution_payload`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be reasonable to phrase this as:

Suggested change
- [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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for taking so long on review here.

This is awesome!

Comment on lines 102 to 103
- [IGNORE] The block's parent (defined by `block.parent_root`) passes all
validation, including verification of the `block.body.execution_payload`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

specs/bellatrix/p2p-interface.md Outdated Show resolved Hide resolved
specs/bellatrix/p2p-interface.md Outdated Show resolved Hide resolved
specs/bellatrix/p2p-interface.md Outdated Show resolved Hide resolved
specs/bellatrix/p2p-interface.md Outdated Show resolved Hide resolved
sync/optimistic.md Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 0ec61bd

sync/optimistic.md Show resolved Hide resolved
sync/optimistic.md Outdated Show resolved Hide resolved
sync/optimistic.md Outdated Show resolved Hide resolved
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
```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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, good catch! Fixed in be4319a

@paulhauner
Copy link
Contributor Author

I believe I have addressed all comments from @ajsutton and @djrtwo. Thanks for your review!

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent!

@terencechain
Copy link
Contributor

With the new status code (ACCEPTED and IGNORED) introduced in ethereum/execution-apis#165, should we update them here as well?

@paulhauner
Copy link
Contributor Author

With the new status code (ACCEPTED and IGNORED) introduced in ethereum/execution-apis#165, should we update them here as well?

Perhaps @djrtwo or @mkalinin have an opinion on if ethereum/execution-apis#165 will merge before this PR? I don't mind either way.

@djrtwo
Copy link
Contributor

djrtwo commented Jan 25, 2022

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).
@paulhauner, can you cut a minor PR with the semantics changes to match?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants