From 6cbeb9aad69a0f9776ac79f8ba42aea09dc61f23 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 26 Jan 2024 08:50:32 -0800 Subject: [PATCH] fix: sync: atomically switch chains when checkpointing (#11595) We can now atomically switch chains when checkpointing. Previously, we'd call `SetHead` followed by `SetCheckpoint`. Unfortunately, that's not atomic and the "head" could have reverted before we called `SetCheckpoint` (causing the latter to fail). Now, we just call `SetCheckpoint` and let `SetCheckpoint` adjust our head. This changes the behavior of `ChainStore.SetCheckpoint`, but `Syncer.SyncCheckpoint` is the only caller anyways. --- chain/checkpoint.go | 32 +++++++++----------------------- chain/store/checkpoint_test.go | 21 ++++++--------------- chain/store/store.go | 15 ++++++++------- 3 files changed, 23 insertions(+), 45 deletions(-) diff --git a/chain/checkpoint.go b/chain/checkpoint.go index f9b0bb4eb..2810b1e4a 100644 --- a/chain/checkpoint.go +++ b/chain/checkpoint.go @@ -24,8 +24,15 @@ func (syncer *Syncer) SyncCheckpoint(ctx context.Context, tsk types.TipSetKey) e ts = tss[0] } - if err := syncer.switchChain(ctx, ts); err != nil { - return xerrors.Errorf("failed to switch chain when syncing checkpoint: %w", err) + hts := syncer.ChainStore().GetHeaviestTipSet() + if hts.Equals(ts) { + // Current head, no need to switch. + } else if anc, err := syncer.store.IsAncestorOf(ctx, ts, hts); err != nil { + return xerrors.Errorf("failed to walk the chain when checkpointing: %w", err) + } else if anc { + // New checkpoint is on the current chain, we definitely have the tipsets. + } else if err := syncer.collectChain(ctx, ts, hts, true); err != nil { + return xerrors.Errorf("failed to collect chain for checkpoint: %w", err) } if err := syncer.ChainStore().SetCheckpoint(ctx, ts); err != nil { @@ -34,24 +41,3 @@ func (syncer *Syncer) SyncCheckpoint(ctx context.Context, tsk types.TipSetKey) e return nil } - -func (syncer *Syncer) switchChain(ctx context.Context, ts *types.TipSet) error { - hts := syncer.ChainStore().GetHeaviestTipSet() - if hts.Equals(ts) { - return nil - } - - if anc, err := syncer.store.IsAncestorOf(ctx, ts, hts); err == nil && anc { - return nil - } - - // Otherwise, sync the chain and set the head. - if err := syncer.collectChain(ctx, ts, hts, true); err != nil { - return xerrors.Errorf("failed to collect chain for checkpoint: %w", err) - } - - if err := syncer.ChainStore().SetHead(ctx, ts); err != nil { - return xerrors.Errorf("failed to set the chain head: %w", err) - } - return nil -} diff --git a/chain/store/checkpoint_test.go b/chain/store/checkpoint_test.go index c5dff94a8..a30c4d860 100644 --- a/chain/store/checkpoint_test.go +++ b/chain/store/checkpoint_test.go @@ -44,22 +44,14 @@ func TestChainCheckpoint(t *testing.T) { head := cs.GetHeaviestTipSet() require.True(t, head.Equals(checkpointParents)) - // Try to set the checkpoint in the future, it should fail. + // Checkpoint into the future. err = cs.SetCheckpoint(ctx, checkpoint) - require.Error(t, err) - - // Then move the head back. - err = cs.SetHead(ctx, checkpoint) require.NoError(t, err) - // Verify it worked. + // And verify that it worked. head = cs.GetHeaviestTipSet() require.True(t, head.Equals(checkpoint)) - // And checkpoint it. - err = cs.SetCheckpoint(ctx, checkpoint) - require.NoError(t, err) - // Let the second miner miner mine a fork last = checkpointParents for i := 0; i < 4; i++ { @@ -85,11 +77,10 @@ func TestChainCheckpoint(t *testing.T) { head = cs.GetHeaviestTipSet() require.True(t, head.Equals(last)) - // Setting a checkpoint on the other fork should fail. + // We should switch back if we checkpoint again. err = cs.SetCheckpoint(ctx, checkpoint) - require.Error(t, err) - - // Setting a checkpoint on this fork should succeed. - err = cs.SetCheckpoint(ctx, checkpointParents) require.NoError(t, err) + + head = cs.GetHeaviestTipSet() + require.True(t, head.Equals(checkpoint)) } diff --git a/chain/store/store.go b/chain/store/store.go index f2826fc2f..b1431c2ee 100644 --- a/chain/store/store.go +++ b/chain/store/store.go @@ -793,9 +793,12 @@ func (cs *ChainStore) removeCheckpoint(ctx context.Context) error { return nil } -// SetCheckpoint will set a checkpoint past which the chainstore will not allow forks. +// SetCheckpoint will set a checkpoint past which the chainstore will not allow forks. If the new +// checkpoint is not an ancestor of the current head, head will be set to the new checkpoint. // -// NOTE: Checkpoints cannot be set beyond ForkLengthThreshold epochs in the past. +// NOTE: Checkpoints cannot be set beyond ForkLengthThreshold epochs in the past, but can be set +// arbitrarily far into the future. +// NOTE: The new checkpoint must already be synced. func (cs *ChainStore) SetCheckpoint(ctx context.Context, ts *types.TipSet) error { tskBytes, err := json.Marshal(ts.Key()) if err != nil { @@ -805,10 +808,6 @@ func (cs *ChainStore) SetCheckpoint(ctx context.Context, ts *types.TipSet) error cs.heaviestLk.Lock() defer cs.heaviestLk.Unlock() - if ts.Height() > cs.heaviest.Height() { - return xerrors.Errorf("cannot set a checkpoint in the future") - } - // Otherwise, this operation could get _very_ expensive. if cs.heaviest.Height()-ts.Height() > build.ForkLengthThreshold { return xerrors.Errorf("cannot set a checkpoint before the fork threshold") @@ -821,7 +820,9 @@ func (cs *ChainStore) SetCheckpoint(ctx context.Context, ts *types.TipSet) error } if !anc { - return xerrors.Errorf("cannot mark tipset as checkpoint, since it isn't in the main-chain: %w", err) + if err := cs.takeHeaviestTipSet(ctx, ts); err != nil { + return xerrors.Errorf("failed to switch chains when setting checkpoint: %w", err) + } } } err = cs.metadataDs.Put(ctx, checkpointKey, tskBytes)