This repository has been archived by the owner on Mar 24, 2023. It is now read-only.
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.
Add fork choice rule with DAS analysis #179
Add fork choice rule with DAS analysis #179
Changes from all commits
6fb3479
e68aff9
df507db
8d4a58a
d76e65b
72cc379
b7b3488
4e0092a
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.
@liamsi this is one point that might warrant a closer look. I suggest here just doing re-downloading/DAS continuously because it's simpler. But you could also do it by triggering a downloading/DAS attempt when you see a new block with a commit. I figured the latter would be more complicated for full nodes since you'd have to fall back to light client mode, but maybe I'm wrong.
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.
Isn't that basically against the assumption that at least one validator (e.g. the block proposer) has the data and has gossiped it to the network of validators (and fullnodes). How else would there be a commit?
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.
If that is a purely theoretical case, then this does not matter much.
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.
No, this case isn't theoretical; it could happen if 2/3 of stake is malicious. Even in that case, we want full nodes and light nodes to agree on the same head of the chain. Note that there won't be a fork, since this scenario involves no equivocation, but the two nodes could still be on different heads without continuous re-downloading/DAS attempts: different heights on the same fork.
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.
For this scenario to occur you'd need:
It seems to me that this means that this is an extreme edge-case (and basically is equivalent to the data not being available).
IMO, in practice if a node does not get the data in a few minutes (or max hours), it should not not continue attempting to download the block simply. Most likely this would rather require human interaction instead.
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.
Does the time of this matter at all? Do they download and execute blocks as they are produced or could that also be at a much later point in time?
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.
Hmm. This paragraph only really covers what happens in the consensus reactor, or more specifically what happens to a node that keeps up with consensus within the weak subjectivity window. IBD isn't covered, but that's because there's not much to cover: just get a trusted checkpoint and that's your fork choice rule. Uninteresting. If the node wants to do IBD by first downloading all blocks, then executing them, that's an implementation detail.
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 does IBD stand for?
IIRC, trusted checkpoints are only used in combination with state sync (state sync requires a checkpoint), full nodes that sync from genesis and replay all blocks do not require a checkpoint.
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.
Initial Block Download, i.e. what happens in the blockchain reactor.
This is actually a problem. The weak subjectivity trusted checkpoint informs the fork choice rule when doing any sync (full, state, only headers) when offline for longer than the unbonding window. It does not affect block validity. We absolutely need to have a trusted checkpoint even when point full sync.
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 think in the case of IBD, it will switch to the consensus reactor once it caught up and any full node would at least quickly detect that it was fed a fork. In the case that 2/3 of validators are byzantine, the network has to resolve this outside of the protocol anyways.
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.
Over a long enough time scale, the probability of there being a long-range fork in the distant past approaches 100%. But as was previously discussed in a call, it doesn't actually matter if you feed in a trusted checkpoint before starting in expectation of at least one fork or after halting on fork detection. It's an implementation detail.