light-client: in what scenarios can a primary have no trusted LightBlock

[xla]

In reference to the thread in #394 (comment) where the question came up when a primary can have no trusted LightBlock. What is referred to here is this method of the Instance:

tendermint-rs/light-client/src/supervisor.rs

Lines 59 to 61 in 72b2886

	pub fn latest_trusted(&self) -> Option<LightBlock> {
	self.state.light_store.highest(Status::Trusted)
	}

By returning an optional it indicates that its presence is not always guaranteed or expected. When can that be the case for the primary of the Supervisor? And if so is it okay to treat it as a Maybe? /cc @brapse @milosevic @romac

[romac]

We should probably ensure somewhere that the returned light block is still within the trusting period, otherwise it's not really a trusted block, right @milosevic?

Perhaps that should even be the responsibility of the light store? WDYT?

[OStevan]

A somewhat related question to this (I can open a separate issue if other thinks it is not related to the discussion started here).

I noticed that we are maintaining Instances with associated state for each LightClient. However when doing fork detection we are using InMemory state so we never update the state associated states. Moreover, when we pass the fork detection the only light block which becomes trusted is the one for the primary.

Now, given this behaviour, one could imagine a situation where the primary does not change for a sufficiently long period of time such that the initially trusted light block (which is also the only trusted light block for all witness states) moves out of the trusted period and becomes no longer valid. In this situation an error for any of the subsequent verification requests on the primary would trigger primary swap and would bring a Supervisor in a state where it simply can not verify any new light blocks as the state we are using has only expired light blocks.

(and finally the question 😄 ) @romac @brapse @milosevic Is this behaviour something which is correct or we expect to swap primaries relatively frequently so that the latest trusted light block never falls out of the trusted period?

[milosevic]

Light client starts with a trusted light block (subjective initialisation) that must be within trusted period. There should always be a trusted light block in the store that is within its trusted period; once this is not anymore the case we should exit and require user to re-initialise the light client. Therefore, a primary should always have trusted light block in the store, so no need for Option. @josef-widder We should check if we have clarified this in the spec, as introducing trusted light block recently maybe introduce some ambiguity.

[OStevan]

@milosevic I had a bit of different issue in mind, which more about how we deal with state of different peers. During the Supervisor verification the only instance which updates it's LightBlockStore is the primary, while during fork detection the witnesses are using freshly created in memory stores which after that get discarded. Therefore, even though we have stored a LightBlock which we consider trusted if at any point in time we find out the primary is faulty we swap it and use the State associated with the new primary instance and as soon as we do that we basically go back in time to the state before any verification. For me this behaviours is strange as the history of previously verified LightBlocks is lost and we might need to restart the light client just because the primary was faulty.

[josef-widder]

Light client starts with a trusted light block (subjective initialisation) that must be within trusted period. There should always be a trusted light block in the store that is within its trusted period; once this is not anymore the case we should exit and require user to re-initialise the light client. Therefore, a primary should always have trusted light block in the store, so no need for Option. @josef-widder We should check if we have clarified this in the spec, as introducing trusted light block recently maybe introduce some ambiguity.

Right now we have this covered in the English spec as follows:

[LCV-INV-TP.1]:

It is always the case that LightStore.LatestTrusted.Header.Time > now - trustingPeriod.

If the invariant is violated, the light client does not have a header it can trust. A trusted header must be obtained externally, its trust can only be based on social consensus.

[josef-widder]

Regarding the question of an option, there is a separate question: In Fastsync we first check the height of a peer and only request blocks less than that height. In the Lightclient specs, we gloss over that, and (implicitly) assume that the primary is always synchronized.
Similarly, there is the question what should happen if VerifyToTarget is called for a height that does not exist yet on the blockchain. In this case the primary cannot return a block obviously.
I am not sure how relevant these cases are, or whether they cannot happen in the use cases. But I guess we should clarify that? What do you think @milosevic ?

[josef-widder]

@milosevic I had a bit of different issue in mind, which more about how we deal with state of different peers. During the Supervisor verification the only instance which updates it's LightBlockStore is the primary, while during fork detection the witnesses are using freshly created in memory stores which after that get discarded. Therefore, even though we have stored a LightBlock which we consider trusted if at any point in time we find out the primary is faulty we swap it and use the State associated with the new primary instance and as soon as we do that we basically go back in time to the state before any verification. For me this behaviours is strange as the history of previously verified LightBlocks is lost and we might need to restart the light client just because the primary was faulty.

I think once a LightBlock is trusted, it should stay trusted forever, even if the primary fails later. Otherwise, it is not clear what is the meaning of "trusted".

[brapse]

@xla are we happy with the answer here or are their additional details that can be added before closing this issue?

[xla]

Follow up captured by @romac in #420 - thanks everyone for contributing to the conversation.

Do we need to capture this in any other place like the specs? @josef-widder @milosevic

[josef-widder]

I guess we said we miss a high-level supervisor spec of the light client, respectively and ADR. This needs to go into such a document. I am not sure whether we have an issue for that or what is the specific plan to move forward. @brapse

[brapse]

Yes we do lack a Supervisor spec as well as clarity around its utility.

For now I think #420 captures the conclusion of the discussion and we can perhaps explore the idea a bit more in #446.