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.
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
Optimistic Sync #2770
Changes from 41 commits
c99e48c
38fffd3
8918823
7f5b7d1
1a89d16
5a5f980
2c62ed3
497b5f8
0ad6025
3b67c33
fb520a8
d1851dc
4c4ffe7
3f6e5b9
ffc2c40
9d7d4d0
4f1d815
eb32e14
0842554
d9a0d16
538cc81
5c1fcaf
2aa4edf
e49685e
ffba24f
26e934b
da6cad8
9901cb3
26431b7
a797ae4
b287f65
91cad9b
aa9a296
7837dc7
451ae29
9421bf3
ff50bfe
e696d11
941531c
12293c9
50f526e
9e619f8
6eba269
6c13e2e
2ce2aac
736f3ce
55d92ce
1228e01
e97335a
60eab25
0ae80d9
de1a6ca
ad7e924
90fb7f6
6d73b0a
0c2e416
6d72038
18c32e0
6af3d4c
b7c332f
8522f27
856eea4
15ef2f3
b1ec9bc
6225236
092f3e0
52caba6
b50bee1
4ccd38b
f4a125c
0ec61bd
bfe4172
24947be
be4319a
50b236e
a5b3c91
b50bea8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
justified_block
is unused in this spec.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.
Thanks, removed in 736f3ce.
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.
We might want to be more specific here as "yet to receive" is true for the case when a payload hasn't been sent to an EL client or in the case of error occurred during communication between the pair of clients.
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.
Good idea, I changed this in 2ce2aac. Let me know what you think about that wording.
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.
Something that confused me here is that a block that returned
INVALID
from the EL would pass this test and return hereblock
. Perhaps it's worth adding a note thatblock
is assumed valid 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.
Makes sense! I added a comment in de1a6ca.
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.
What do you think about moving both condition checks to
should_optimistically_import_block
for completeness?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've done this in 6c13e2e. I added a
Store
so we can access the blocks at states without undefinedget_block
andget_state
functions. Adding blocks/states to theStore
is undefined, but I guess that's OK.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.
We should add a note that
validate_merge_block
should be called retrospectively after a node is fully synced if a merge transition block has been processed optimistically.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.
Oh yep, I forgot this! Added here: 60eab25.
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.
It implies
store.latest_messages
is reverted to a state in which anINVALID
block doesn't exist, right? I think it worth noting explicitly.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.
It doesn't necessarily "revert" to an old state, since other aspects of the block tree may differ.
I agree that it should be as if the INVALID block doesn't exist, but I'm not sure how to specify that when in proto-array it will "exist" but be filtered out. Perhaps you could provide another suggestion, please?
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.
It should be enough for a node to import an honest chain justifying an epoch prior to malicious chain that justifies the same epoch to avoid poisoning the fork choice store and eventually finishing an optimistic sync which in this case should converge on the honest chain
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'm not sure I follow this sorry.
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.
Do we need a higher epoch to be justified by an honest chain? From my perspective the same epoch justified by an honest chain is enough to prevent poisoned store, with the condition that an honest justification is applied to the store before the malicious one. Simply:
N
N
VS
N
N
An honest chain justifying
N+1
does also prevent store poisoning. My point is that justifyingN
is also enough. And this is what we assume to get in 4 epochs: an honest chain with adversary power< 1/3
justifies epochN
. Then we allow for optimistically import any other chain without the risk of a store being poisoned.Am I missing anything?
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.
Ah yes, I see what you mean now. So a higher epoch always fixes it and a lower one fixes it sometimes, given a message ordering assumption.
I've mentioned this "poisoning prevention" in 6d73b0a :)
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.
Consider reflecting this change in the
p2p-interface
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.
Good idea, I did this in 1228e01.
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.
Do we want this to be reflected in the
p2p-interface
as well?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.
Also done in 1228e01.