Sharing the state between peers.

[OStevan]

Issue and how it solves the problem?
#420 , this PR proposes a solution in which the Supervisor has a state which would be updated with trusted LightBlocks after each successful pass of fork detection. Subsequent verification requests first read the highest trusted LightBlock in the supervisor state and use that LightBlock as the starting point for verification.

Concerns related to the solution?

• Core verification on the primary and witnesses is always using InMemoryVersion of LightStore. We could either simplify the instance and remove associated LightStores or try to always use the one associated with the instance
• Should we actually always use the one associated with an Instance so that for example we can examine the store in case of failure?

Other related discussions?
The solution from #479 deletes the light blocks higher that the previously trusted one in the shared state. The approach here just discards the light store used by the primary.

• Referenced an issue explaining the need for the change
• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Updated CHANGES.md

[liamsi]

Not sure I have the right context but doesn't this mean you can remove the separate stores in each light client?
See above:

/// LightClientConfig contains all options of a light client instance.
#[derive(Clone, Debug, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct LightClientConfig {
    /// Address of the Tendermint fullnode to connect to and
    /// fetch LightBlock data from.
    pub address: tendermint::net::Address,
    /// PeerID of the same Tendermint fullnode.
    pub peer_id: PeerId,
    /// The data base folder for this instance's store.
    pub db_path: PathBuf,
}

[OStevan]

This is something I wanted to discuss. In practice we can use in-memory state for each peer instance enabling us to remove the separate stores for each light client. I'm however unsure if we might still want to keep these stores to be able to inspect what each light client verified by itself(do we even want such a feature?). @liamsi

[liamsi]

I'm however unsure if we might still want to keep these stores to be able to inspect what each light client verified by itself(do we even want such a feature?).

Not sure tbh, but there might be some cases where this is required for the fork-detector!? @milosevic @ebuchman

[romac]

I don't believe we need the flexibility afforded by the LightStoreFactory trait/component, a plain method which converts a LightStoreConfig to a impl LightStore would likely do the job as well.

This line is also a bit weird because we are always casting to a concrete type, which defeats the purpose of the trait.

[OStevan]

Changed it to a function.

[romac]

Looks good to me 👍 Thanks for opening this draft PR for discussion!

I just have one comment on the need for a LightStoreFactory.

That said, I think the general idea of sharing the state between all peers should be carefully considered and reviewed by @josef-widder and @milosevic.

[OStevan]

ping @milosevic @josef-widder

[josef-widder]

In #479, we stated the invariant [LCD-INV-TRUSTED-AGREED.1]. It says that the primary and the secondary always agree on the lightblock for height lightStore.LatestTrusted().Height. This allows us to not store any blocks downloaded from secondaries after fork detection returned: More precisely, fork detection is started from this latest trusted block at all secondaries, and if there is no fork, all downloaded blocks can be dropped. If there is a fork, the downloaded blocks are used to generate "proof of fork".

So I am not sure we need to share the state between peers, but perhaps I didn't get the question...

(@milosevic is on vacations this week)

[xla]

The proposed solution has the character of a band-aid rather than addressing the issue. Seeing the Handle holding onto actual state violates the original design constraints. It would be more effective if we look closer at the problem and maybe even adjust our understanding as @josef-widder pointed out to remove the need for the state all together.

[brapse]

This is a great first step but should integrate the work in #479 and be preceded by an ADR outlining the approach. Might make sense to do this as a separate PR. Will leave the branch for reference.