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

feat: allow checkpointing to forks #6107

Merged
merged 3 commits into from
Apr 29, 2021
Merged

feat: allow checkpointing to forks #6107

merged 3 commits into from
Apr 29, 2021

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Apr 26, 2021

Previously, lotus sync checkpoint would only checkpoint on the current
chain. Now, it can switch to a new fork.

Alternatives considered:

  1. Follow a peer: sketchy.
  2. Mess with the sync manager to only sync this chain: very
    invasive. (but I have a WIP patch if desired).
  3. Introduce a new lotus sync fetch command to fetch a chain (to be
    paired with lotus chain sethead followed by a lotus sync checkpoint.

Changes:

  1. Move the checkpointing logic into the chainstore so setting a checkpoint and syncing a different head can't race.
  2. Re-enable checkpoint tests, fix, and expand them.
  3. Rename Sync.SetCheckpoint to Sync.SyncCheckpoint and allow it to sync to a new fork.
  4. Make Chain.SetHead unset any set checkpoints.
  5. Reject checkpoints more than the fork limit in the past.

Known issues:

  1. The current checkpoint mechanism is racy because we only check before we start syncing a chain. We need to push checkpoints down into the chainstore to be safe/atomic. fixed
  2. Unlike lotus chain sethead, this code doesn't "invalidate" the current fork. I'm not entirely sure why we need to do that. not an issue here

@arajasek
Copy link
Contributor

@Stebalien This will need an update to the lotus sync checkpoint CLI command, which currently only takes epochs as inputs (since it's basically only ever used after lotus chain sethead).

@arajasek
Copy link
Contributor

  1. Unlike lotus chain sethead, this code doesn't "invalidate" the
    current fork. I'm not entirely sure why we need to do that.

Because if not you might (likely will) immediately sync away from your new "set" head if you're setting far back in the past.

Previously, `lotus sync checkpoint` would only checkpoint on the current
chain. Now, it can switch to a new fork.
@Stebalien
Copy link
Member Author

This will need an update to the lotus sync checkpoint CLI command, which currently only takes epochs as inputs (since it's basically only ever used after lotus chain sethead).

It can take a tipset or a height.

We need to wait until the node _has_ the tipset, not until it doesn't.

This probably only worked before due to race conditions.

fixes #4716 (probably)
That way, checkpoints can be enforced by the chainstore, removing a
potential race where an in-progress sync of a fork could bypass a sync
checkpoint.
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Yup, LGTM

@Stebalien Stebalien merged commit eb10918 into master Apr 29, 2021
@Stebalien Stebalien deleted the feat/checkpoint-sync branch April 29, 2021 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants