vsr: sync uses correct view to go into recovering_head

[matklad]

It might be the case that op_checkpoint:

• is prepared in view X
• is truncated in view X+1
• is committed in view X+2

When deciding whether to go into recovering_head state, the replica
currently considers prepared view (checkpoint.header.view), and not the
committed view.

Use the correct view by:

• Adding checkpoint's view into VSR State (but not checkpoint state:
  different replicas might commit prepares at different views)

• Populating that view from the message that informed us about the
  checkpoint target

  • this requires some intricate logic on pings, to make sure they
    indeed propagate correct view for checkpoint --- a replica accepts a
    checkpoint before it transitions to its view, and it should
    subsequently correctly propagate this higher view.

    This works because checkpoint view is also durable.

Seed: 8593423301425288917
Closes: #1703

[matklad]

last commit fixes #1702 -- a semi related vopr seed that also trips on main

[sentientwaffle]

Should we update sync_view during superblock.checkpoint()? Then we would have the invariant that sync_view >= checkpoint.header.view.

[sentientwaffle]

Should this be > self.view_durable()? We might have just called transition_to_normal_from_recovering_head_status() so our view_durable is not yet updated, but our self.view is updated.

[matklad]

I don't think so. I believe that

            if (message.header.view > self.view_durable()) {
                assert(self.superblock.working.vsr_state.sync_view >
                    self.superblock.working.vsr_state.view);
            }

would hold, but that's an almost tautological assertion.

In contrast

            if (message.header.view > self.view_durable()) {
                assert(self.status == .recovering_head);
            }

would not hold -- transition_to_normal_from_recovering_head_status changes status, but leaves self.superblock.working.vsr_state intact until durable update finishes.

And

            if (message.header.view > self.view) {
                assert(self.status == .recovering_head);

Is the interesting case -- it ensures that we can't get out out .recovering_head state earlier than self.superblock.working.vsr_state.view

[sentientwaffle]

Ahh I get it; thank you.

[matklad]

Should we update sync_view during superblock.checkpoint()? Then we would have the invariant that sync_view >= checkpoint.header.view.

Excellent point! We should at least reset it to zero once the sync is done. But I think it's best not to update it, and to keep it scoped strictly to state sync, rather than mix in both sync and non-sync code paths in a single field.

[sentientwaffle]

(A few lines above this) we can assert that update.sync_view >= update.checkpoint.header.view.

[sentientwaffle]

We could assert that update.sync_view == 0 or update.sync_view == superblock.staging.vsr_state.sync_view.