-
Notifications
You must be signed in to change notification settings - Fork 997
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
Optimistic Sync #2770
Changes from 62 commits
Commits
Show all changes
77 commits
Select commit
Hold shift + click to select a range
c99e48c
Add first efforts
paulhauner 38fffd3
Tidy, finish duties
paulhauner 8918823
Start adding p2p components
paulhauner 7f5b7d1
Remove attesting during opt sync
paulhauner 1a89d16
Remove recent valid ancestor
paulhauner 5a5f980
Add RPC responses
paulhauner 2c62ed3
Flip bool
paulhauner 497b5f8
Add EE assumptions
paulhauner 0ad6025
Remove valid roots set
paulhauner 3b67c33
Add note about removal from optimistic_roots
paulhauner fb520a8
Condense gossip topics
paulhauner d1851dc
Tidy tab formatting
paulhauner 4c4ffe7
Fix latest_valid_ancestor
paulhauner 3f6e5b9
Add checkpoint sync
paulhauner ffc2c40
Add section about API
paulhauner 9d7d4d0
Remove validate_merge_block check
paulhauner 4f1d815
Add note about full verification
paulhauner eb32e14
Fix typo
paulhauner 0842554
Add section about re-orgs
paulhauner d9a0d16
Tidy
paulhauner 538cc81
Flip bool
paulhauner 5c1fcaf
Add qualification for errors
paulhauner 2aa4edf
Move helpers
paulhauner e49685e
Add section for enabling opt sync
paulhauner ffba24f
Add failure recovery
paulhauner 26e934b
Remove merge transition section
paulhauner da6cad8
Tidy
paulhauner 9901cb3
Move optimsitic_roots definition
paulhauner 26431b7
Tidy
paulhauner a797ae4
Add qualification about fc store
paulhauner b287f65
Allow RPC blocks
paulhauner 91cad9b
Improve gossip wording
paulhauner aa9a296
Clarify API head condition
paulhauner 7837dc7
Tidy, add validator endpoints
paulhauner 451ae29
Specify no invalid parents
paulhauner 9421bf3
Tidy
paulhauner ff50bfe
Remove block production exception
paulhauner e696d11
Update gossip conditions
paulhauner 941531c
Update sync/optimistic.md
paulhauner 12293c9
Bump safe slots
paulhauner 50f526e
Fix typo
paulhauner 9e619f8
Apply suggestions from code review
paulhauner 6eba269
Update sync/optimistic.md
paulhauner 6c13e2e
Expand `should_optimistically_import_block`
paulhauner 2ce2aac
Address "yet to" paragraph
paulhauner 736f3ce
Remove `justified_block`
paulhauner 55d92ce
Fix typo
paulhauner 1228e01
Update p2p-networking
paulhauner e97335a
Merge branch 'dev' into opt-sync-2
paulhauner 60eab25
Add section about merge block
paulhauner 0ae80d9
Fix typo
paulhauner de1a6ca
Add comment about INVALID block
paulhauner ad7e924
Update links to bellatrix
paulhauner 90fb7f6
Add rationale
paulhauner 6d73b0a
Add poisoning prevention
paulhauner 0c2e416
Run doctoc
paulhauner 6d72038
Fix spelling mistakes
paulhauner 18c32e0
Fix indents
paulhauner 6af3d4c
Fix comment indents
paulhauner b7c332f
Tidy
paulhauner 8522f27
Modify "when" section
paulhauner 856eea4
Describe all fields in store
paulhauner 15ef2f3
Apply suggestions from @djrtwo review
paulhauner b1ec9bc
Update gossip conditions
paulhauner 6225236
Specify about EL/CL scoring rules
paulhauner 092f3e0
Propose -> Propagate
paulhauner 52caba6
Add section about backwards compat
paulhauner b50bee1
Rename Store -> OptimisticStore
paulhauner 4ccd38b
Tidy tracking note
paulhauner f4a125c
Remove reorg section
paulhauner 0ec61bd
Clarify liveness
paulhauner bfe4172
Define `current_slot`
paulhauner 24947be
Rename should_optimistically_import...
paulhauner be4319a
Rename `justified_is_verified`
paulhauner 50b236e
Address TERMINAL_BLOCK_HASH
paulhauner a5b3c91
build opimistic sync file and fix a minor lint/typing issue
djrtwo b50bea8
Merge pull request #2 from ethereum/build-opt-sync
paulhauner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
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.
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:
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.
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 😅
There was a problem hiding this comment.
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 asif negative_condition: REJECT/IGNORE
...