I'm still a little confused by the logic here. update_approved_aggregate_key is invoked to refresh the signer's view of the approved aggregate key. Why should that trigger a load from saved state? Shouldn't the signer's state already be loaded into memory at this point? Why doesn't the signer just load state into memory during process startup (as I think was the general idea behind Jude's suggestion)?

The general logic seems like it should be:

On boot:
    self.refresh_approved_aggregate_key();
    if self.approved_aggregate_public_key.is_some():
         self.signer_state = self.load_saved_states()[self.aggregate_key];
         if self.signer_state.pending_dkg_results.is_some():
            send_dkg_results();
            self.signer_state.pending_dkg_results.clear();
            self.overwrite_saved_states()

On refresh aggregate public key:
     if self.approved_aggregate_public_key is changed:
         if self.signer_state.pending_dkg_results.is_some():
            send_dkg_results();
            self.signer_state.pending_dkg_results.clear();
            self.overwrite_saved_states()

That way, the signer sends a DkgResults message after the aggregate key is approved if they're running at the time, or if they wakeup after the aggregate key has been approved, they send the message then.

This avoids excessively reloading the signer states and it avoids resending the DkgResults message potentially every time a state reload occurs.

Hmm yeah I think we could do it that way. Two thoughts on the logic:

1. On boot the signer won't have any pending DKG results so we don't have to send DKG results at that point.
2. State writes happen now as soon as DKG is finished (i.e. a DkgEnd(_) message is received), which I think makes sense because you want to persist the shares as soon as you have confirmation that you have been able to create an aggregate key.

With these I think we can simplify the logic to

On boot:
    self.refresh_approved_aggregate_key();
    if self.approved_aggregate_public_key.is_some():
         self.signer_state = self.load_saved_states()[self.aggregate_key];

On refresh aggregate public key:
     if self.approved_aggregate_public_key is changed:
         if self.signer_state.pending_dkg_results.is_some():
            send_dkg_results();
            self.signer_state.pending_dkg_results.clear();

On DkgEnd received:
      self.save_signer_state()

What do you think about this?

Why should that trigger a load from saved state? Shouldn't the signer's state already be loaded into memory at this point?

It probably should be, but the rationale behind the current logic is to ensure that the saved state is always consistent with the approved aggregate key. If we for any reason observe a change in the approved aggregate key, a natural reaction would be to reload the state observed with the changed aggregate key.

I think its possible to have a pending dkg results on boot, because the signer may have gone offline between DkgEnd and the key being approved by the network. I think we should still be persisting these states after the pending_dkg_results is cleared as well, so I think my suggested logic is about as simplified as it can get. I do agree that we should be persisting on DkgEnd as well.

Actually, a problem with the proposed logic is if you boot the signer and an approved aggregate key is not yet set you would never load any persisted state. If you are unlucky with the timing, your signer might boot just before the last vote for an aggregate key arrives and sets that key in the contract.

From that perspective, I think it is safer to load the saved state when you observe a change in the aggregate key.

To prevent excessive state reloads I could add a condition to only load the saved state in case the approved aggregate key is different from the one already present in the state.

So I'd then propose considering something that looks like this:

On boot:
     Start without any approved_aggregate_key or loaded signer state

On refresh aggregate public key:
     if self.approved_aggregate_public_key is changed:

         if the new approved_aggregate_public_key is not the same as the one in our signer state:
             self.load_saved_state(approved_aggregate_key)

         if self.signer_state.pending_dkg_results.is_some():
            send_dkg_results();
            self.signer_state.pending_dkg_results.clear();

On DkgEnd received:
      self.save_signer_state()

This would avoid excessive state reloads, but ensure that the signer doesn't miss loading the state for a particular aggregate key.

The way we've addressed this in the past (albeit in very different contexts) is to have two structs: a "preload" struct and a "loaded" struct which is instantiated from a "preload" struct and some additional state. You'd have an UninitializedSigner struct, which represents a Signer that does not yet have its state loaded. Its impl would have a conversion method like into_signer(state), which fully instantiated the Signer from the loaded state once it became available.

Then, you'd make it impossible to accidentally have a partially-instantiated or corrupted Signer used somewhere. Given the importance of the signer to chain liveness, I think it's worth the effort to make invalid Signer states unrepresentable here.

I agree that it makes a lot of sense to differentiate between an UninitializedSigner and a Signer instead of having the current State field within the signer. It's not clear to me what the relation is between an initialized/uninitialized signer and which DKG shares the signer should have loaded. Afaik, a Signer may still be initialized but without having a known approved aggregate key or any DKG shares.