Perform bisection when verifying headers using the light client #51

romac · 2020-04-09T21:27:13Z

Closes: #49

Description

This PR implements proposal B described in #49.

All tasks spawned in the start command are now spawned on a local task set which executes them all on the same thread, ie. the main thread.

This seems like an acceptable solution to me given that the overall architecture of the relayer and the light client will likely change quite a bit in the coming weeks.

For contributor use:

Unit tests written
Added test to CI if applicable
Updated CHANGELOG_PENDING.md
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments
Re-reviewed Files changed in the Github PR explorer

ancazamfir · 2020-04-10T08:39:13Z

I tried to run the relayer on this branch and I don't see the light client update header messages. Just these:
Apr 10 10:26:56.933 INFO relayer_cli::relayer: Relayer is running

If I remove the relay thread then the relayer finishes quietly after spawning light client... messages

ancas-macbook-pro:1ibc-rs ancaz$ cargo run --bin relayer -- -c simple_config.toml start
...
Apr 10 10:30:21.657  INFO relayer_cli::commands::start: spawning light client chain.id=ibc0
Apr 10 10:30:21.658  INFO relayer_cli::commands::start: spawning light client chain.id=ibc1
ancas-macbook-pro:1ibc-rs ancaz$

Maybe I am doing smthg wrong, it's the first time I try this; followed the instructions from: https://github.com/informalsystems/ibc-rs/blob/master/README.md

romac · 2020-04-10T08:57:01Z

Yeah, I get the same output too. My bad, I should have mentioned that and marked the PR as a draft. Will let you know once(if) it works and is actually ready for review.

ancazamfir · 2020-04-10T09:33:41Z

ah ok, thought is ready for review :)

romac · 2020-04-10T09:36:39Z

I had forgotten to .await the client task 🤦‍♂️

This now works as expected but has not been rebased on master yet.

I rebased locally, but the start command now fails because of a deserialization error when fetching the TrustOptions from the store. Might be an issue with tendermint-rs@v0.33. Will investigate further!

romac · 2020-04-10T10:07:44Z

Blocked by informalsystems/tendermint-rs#196 (comment).

romac · 2020-04-10T13:18:37Z

Unblocked now that informalsystems/tendermint-rs#210 has been merged.

romac · 2020-04-10T13:41:32Z

@ancazamfir Ready for review.

ancazamfir

Thanks! This looks good to me, a couple minor comments. There's a bit more cleanup imo that needs to be done, but we can do in a separate PR.

ancazamfir · 2020-04-12T11:47:11Z

relayer/cli/src/commands/start.rs

        interval.tick().await;
    }
 }

-async fn update_headers<C: Chain, S: Store<C>>(mut client: Client<C, S>) {
+async fn update_client<C: Chain, S: Store<C>>(mut client: Client<C, S>) {
    debug!(chain.id = %client.chain().id(), "updating headers");

    let mut interval = tokio::time::interval(Duration::from_secs(3));


From the documentation: "Note that when it starts, it produces item too." So the interval.tick().await; should be the first in the loop. I looked at the logs when running with two chains and noticed two updates per chain without the wait when starting the relayer.

Good catch!

ancazamfir · 2020-04-12T11:48:08Z

relayer/cli/src/commands/start.rs

    debug!(chain.id = %client.chain().id(), "updating headers");

    let mut interval = tokio::time::interval(Duration::from_secs(3));

    loop {
+        info!(chain.id = %client.chain().id(), "updating client");


Suggested change

info!(chain.id = %client.chain().id(), "updating client");

interval.tick().await;

info!(chain.id = %client.chain().id(), "updating client");

and remove from end of loop.

ancazamfir · 2020-04-12T11:55:00Z

relayer/cli/src/commands/start.rs

    debug!(chain.id = %client.chain().id(), "updating headers");

    let mut interval = tokio::time::interval(Duration::from_secs(3));

    loop {
+        info!(chain.id = %client.chain().id(), "updating client");
+
        let result = client.update(SystemTime::now()).await;


I believe this is the only place we should upate client, i.e. remove from client.rs

--- a/relayer/relay/src/client.rs +++ b/relayer/relay/src/client.rs @@ -93,10 +93,6 @@ where client.init_with_trust_options(trust_options).await?; } - // Perform an update already, to make sure the client is up-to-date. - // TODO: Should we leave this up to the responsibility of the caller? - let _ = client.update(SystemTime::now()).await?; - Ok(client) }```

ancazamfir

After restarting the relayer that had previously sync-ed some header, there are some debug messages about hashes not matching and trusted options height < header height in store. I think we need to look into this a bit, not sure why they occur as this is seen with good providers/ chains. Raised #53 for this.

romac · 2020-04-14T09:15:22Z

Just addressed your comments :)

ancazamfir · 2020-04-14T10:14:31Z

Just addressed your comments :)

Looks good! Thanks @romac!

…rmalsystems#51) * Perform bisection when verifying new headers in client * Fix typo in README * Tick interval at the beginning of the client loop * Do not update the light client upon creation * Improve client logs

romac requested a review from ancazamfir as a code owner April 9, 2020 21:27

romac force-pushed the romac/light-client-bisection branch from a5ef581 to 05dc0d9 Compare April 10, 2020 09:16

Perform bisection when verifying new headers in client

a6a5473

informalsystems deleted a comment from codecov-io Apr 10, 2020

romac force-pushed the romac/light-client-bisection branch from 05dc0d9 to a6a5473 Compare April 10, 2020 13:18

informalsystems deleted a comment from codecov-io Apr 10, 2020

ancazamfir reviewed Apr 12, 2020

View reviewed changes

ancazamfir approved these changes Apr 12, 2020

View reviewed changes

romac added 4 commits April 14, 2020 11:13

Fix typo in README

5f4cc37

Tick interval at the beginning of the client loop

0019910

Do not update the light client upon creation

de03aee

Improve client logs

8a79415

romac merged commit 8cd5bd0 into master Apr 14, 2020

romac deleted the romac/light-client-bisection branch April 14, 2020 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform bisection when verifying headers using the light client #51

Perform bisection when verifying headers using the light client #51

romac commented Apr 9, 2020

ancazamfir commented Apr 10, 2020

romac commented Apr 10, 2020

ancazamfir commented Apr 10, 2020

romac commented Apr 10, 2020

romac commented Apr 10, 2020

romac commented Apr 10, 2020

romac commented Apr 10, 2020

ancazamfir left a comment

ancazamfir Apr 12, 2020

romac Apr 14, 2020

ancazamfir Apr 12, 2020

ancazamfir Apr 12, 2020

romac Apr 14, 2020

ancazamfir left a comment •

edited

Loading

romac commented Apr 14, 2020

ancazamfir commented Apr 14, 2020

	info!(chain.id = %client.chain().id(), "updating client");
	interval.tick().await;
	info!(chain.id = %client.chain().id(), "updating client");

Perform bisection when verifying headers using the light client #51

Perform bisection when verifying headers using the light client #51

Conversation

romac commented Apr 9, 2020

Description

ancazamfir commented Apr 10, 2020

romac commented Apr 10, 2020

ancazamfir commented Apr 10, 2020

romac commented Apr 10, 2020

romac commented Apr 10, 2020

romac commented Apr 10, 2020

romac commented Apr 10, 2020

ancazamfir left a comment

Choose a reason for hiding this comment

ancazamfir Apr 12, 2020

Choose a reason for hiding this comment

romac Apr 14, 2020

Choose a reason for hiding this comment

ancazamfir Apr 12, 2020

Choose a reason for hiding this comment

ancazamfir Apr 12, 2020

Choose a reason for hiding this comment

romac Apr 14, 2020

Choose a reason for hiding this comment

ancazamfir left a comment • edited Loading

Choose a reason for hiding this comment

romac commented Apr 14, 2020

ancazamfir commented Apr 14, 2020

ancazamfir left a comment •

edited

Loading