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

Fork choice specs and implementation issues: blocks #191

Open
ericsson49 opened this issue Sep 17, 2019 · 0 comments
Open

Fork choice specs and implementation issues: blocks #191

ericsson49 opened this issue Sep 17, 2019 · 0 comments
Labels
question Further information is requested spec Changes related to spec

Comments

@ericsson49
Copy link
Contributor

List of differences, that I found, between Fork choice specs and our implementation. This doesn't necessarily mean it's a bug.

1 Blocks are not delayed and time check is different
Spec says:

# Blocks cannot be in the future. If they are, their consideration must be delayed until the are in the past.
assert store.time >= pre_state.genesis_time + block.slot * SECONDS_PER_SLOT

However, DefaultBeaconChain::insert simply rejects blocks too far the future, using a bit different time check:
assert block.slot <= get_current_slot(store.time) + 1

2 Blocks are not checked against finalized checkpoint
Spec says:

# Check block is a descendant of the finalized block

And also

# Check that block is later than the finalized epoch slot

The time check seems to be redundant, given the block passes the descendance check.
Our implementation performs a similar check against justified checkpoint LMDGhostProcessor::isJustifiedAncestor.
The other issue is that it's performed after the block is imported.
While, according to the spec, it's not imported if the finality-descendance check is failed.

3 Finality updates

Spec says:

# Update justified checkpoint
if state.current_justified_checkpoint.epoch > store.justified_checkpoint.epoch:
    store.justified_checkpoint = state.current_justified_checkpoint
# Update finalized checkpoint
if state.finalized_checkpoint.epoch > store.finalized_checkpoint.epoch:
    store.finalized_checkpoint = state.finalized_checkpoint

However, DefaultBeaconChain::updateFinality implementation is somewhat different:

    if (!previous.getFinalizedCheckpoint().equals(current.getFinalizedCheckpoint())) {
      chainStorage.getFinalizedStorage().set(current.getFinalizedCheckpoint());
      finalizedCheckpointStream.onNext(current.getFinalizedCheckpoint());
    }
    if (!previous.getCurrentJustifiedCheckpoint().equals(current.getCurrentJustifiedCheckpoint())) {
      // store new justified checkpoint if its epoch greater than previous one
      if (current
          .getCurrentJustifiedCheckpoint()
          .getEpoch()
          .greater(fetchJustifiedCheckpoint().getEpoch())) {
        chainStorage.getJustifiedStorage().set(current.getCurrentJustifiedCheckpoint());
      }

While the justified checkpoint update looks like giving an equivalent result, I'm not so sure about the finalized checkpoint update.

@ericsson49 ericsson49 added question Further information is requested spec Changes related to spec labels Sep 17, 2019
@ericsson49 ericsson49 changed the title Fork choice specs and implementation issues Fork choice specs and implementation issues: blocks Sep 17, 2019
eustimenko added a commit that referenced this issue Sep 19, 2019
eustimenko added a commit that referenced this issue Sep 20, 2019
eustimenko added a commit that referenced this issue Sep 20, 2019
eustimenko added a commit that referenced this issue Sep 20, 2019
eustimenko added a commit that referenced this issue Sep 20, 2019
eustimenko added a commit that referenced this issue Sep 20, 2019
eustimenko added a commit that referenced this issue Sep 20, 2019
ericsson49 added a commit that referenced this issue Nov 4, 2019
ericsson49 added a commit that referenced this issue Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested spec Changes related to spec
Projects
None yet
Development

No branches or pull requests

1 participant