Light Client: Fork Detection v1

[melekes]

TODO:

• verification
• better errors

[melekes]

Looks like bisection algorithm is using a trusted store and not abstract enough for us to run it against a witness (i.e. check that we can bisect from latest trusted state to given header using a witness). This will require some changes.

[brapse]

Right. In the latest it iteration The we haven't solidified the API on the witnesses or the lightstore so it's great to surface these needs now. Our current thinking is for each witness primary or otherwise to have a verify_to_target method and it's own trusted store. We will have to expose an API on the witness's store to trust a header if it matches the verified header on the primary. In this case it might make sense for the latest trusted to be a method on the witness and abstract the storage details of the underlying storage, roughly thinking 🤔

[romac]

I have a very much WIP implementation of fork detection roughly based on the draft spec here:

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

Lines 33 to 75 in c1b6755

	impl ForkDetector for ProdForkDetector {
	fn detect_forks(
	&self,
	light_block: &LightBlock,
	primary: &LightClient,
	secondaries: Vec<&LightClient>,
	) -> ForkDetection {
	for secondary in secondaries {
	let mut state: State = todo();
	let secondary_block = secondary
	.get_or_fetch_block(light_block.height(), &mut state)
	.unwrap(); // FIXME: unwrap
	if light_block.signed_header.header == secondary_block.signed_header.header {
	// Header matches, we continue.
	continue;
	}
	let latest_trusted = primary
	.state
	.light_store
	.latest(VerifiedStatus::Verified)
	.unwrap(); // FIXME: unwrap
	state
	.light_store
	.update(latest_trusted, VerifiedStatus::Verified);
	state
	.light_store
	.update(secondary_block, VerifiedStatus::Unverified);
	let result = secondary.verify_to_target_with_state(light_block.height(), &mut state);
	// TODO: Handle case where block expired
	match result {
	Ok(_) => todo!(), // There is a fork, report `secondary_block`
	Err(_) => todo!(), // `secondary` is faulty, report it
	}
	}
	ForkDetection::NoFork
	}

I currently just expose the LightClient's LightStore but as you say, having a specific method on the LightClient which returns the latest state would be a lot better.

I also added the ability to specify a State (which includes the LightStore) to perform verification with. I am not so happy with that solution at the moment, since it requires a hack to work around the double borrow that arises when we want to work over the light client's current state.

Let's try to sync up tomorrow to discuss the current state of things, as well as the discrepancies between this PR's implementation and the specification?

PS: Great job by the way :)

[ebuchman]

In the prior version managing the store was a more external responsibility, so all the internal bisection/verification functions didn't depend on it - ie. the verify_bisection took a trusted state and returned a list of new trusted states that the caller could add to the store. This way bisection could easily be done against any peer, without reference to a store. Does it make sense to do something like that? Or maybe to use something like a fresh mem-store for each bisection run and then sync the mem-store to the underlying db store afterwards as necessary.

[romac]

I guess both solutions would work, but I would perhaps favor the one where we pass the store explicitly as it's a bit simpler and more versatile. That's basically what we are doing now in the supervisor branch, but for now we are still keeping the internal store around, hence the need for the mem::replace trick.

Nonetheless, I think it's still worth giving your second solution more thought. For example, we could keep a log of the operations performed on it so that we can reapply them in order on the disk-based store. But this wouldn't solve the issue of supplying the light client with a pre-populated store, like the fork detector requires.

[ebuchman]

But this wouldn't solve the issue of supplying the light client with a pre-populated store, like the fork detector requires.

Why does the fork detector require a prepopulated store?

[romac]

Why does the fork detector require a prepopulated store?

For fork detection, each witness needs a custom light store which only contains the trusted state (marked as verified) and the light block to verify (marked as unverified), you can see the relevant code here.

As you can see in the code, since my comment above I changed the light client to not maintain its own state at all, but to rather have it passed as an argument to verify_to_target. It's now the responsibility of the supervisor to maintain a mapping between the peers and their light client instance + state.

[romac]

I think we can close this PR since it's been integrated within the supervisor PR.

The one difference between both the two implementation is that this PR's version compares header hashes, while the supervisor one compares header for equality, as per the fork detection spec. We should discuss which is best somewhere.

[melekes]

thanks @romac!