Allow nodes to mark tipsets as checkpointed #3680
Conversation
| // SyncMarkBad marks a blocks as bad, meaning that it won't ever by synced. | ||
| // Use with extreme caution. | ||
| SyncMarkBad(ctx context.Context, bcid cid.Cid) error | ||
|
|
||
| // SyncUnmarkBad unmarks a blocks as bad, making it possible to be validated and synced again. | ||
| SyncUnmarkBad(ctx context.Context, bcid cid.Cid) error |
There was a problem hiding this comment.
A ListBad call would be really nice for complectess / debugging (for a followup PR)
chain/sync.go
Outdated
| return nil, xerrors.Errorf("failed to retrieve checkpoint tipset: %w", err) | ||
| } | ||
|
|
||
| if chkTs.Height() > nts.Height() { |
There was a problem hiding this comment.
nit: i would also explicitly check that we're leaving the checkpoint tipset behind for more robustness; ideally comparing heights reflects this but there could be some corner cases (e.g., direct manipulation of chain by plumbing commands) that could be caught with that
There was a problem hiding this comment.
Yeah, we definitely should be checking specific hashes here.
There was a problem hiding this comment.
One potentially bad edge case here is if we mark a block as our checkpoint, but are currently synced to a different chain. This would prevent us from switching to that checkpoint.
There was a problem hiding this comment.
One potentially bad edge case here is if we mark a block as our checkpoint, but are currently synced to a different chain.
That case is explicitly checked by the syncer when setting a checkpoint (that's why I marked this only as a nit, but still nice to have as an integrity check).
There was a problem hiding this comment.
changed to explicitly check the tipsetkeys we're forking away from
@whyrusleeping As Schomatis said, the potentially bad edge case should never happen, because we only allow you to mark tipsets currently in your heaviest chain as checkpointed -- we said that was okay for v0 of this, can you confirm it's still okay?
(pretty easy to change that, we can simply call SetHead when setting a checkpoint, but it's risky)
There was a problem hiding this comment.
@arajasek Thats sufficient for a first release of this, I guess the flow would be:
run a checkpoint import, then mark the checkpoint head as your checkpoint.
arajasek
force-pushed
the
asr/checkpoint
branch
from
b7c0cfc
to
f055467
Compare
Such tipsets are never forked away
Fixes #3290