Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add client workers for active channels, clear packets on start #872

Merged
merged 20 commits into from
May 4, 2021
Merged

Add client workers for active channels, clear packets on start #872

merged 20 commits into from
May 4, 2021

Conversation

ancazamfir
Copy link
Collaborator

@ancazamfir ancazamfir commented Apr 29, 2021 •
edited by adizere
Loading

Closes: #786
Closes: #784

Description

tbd

For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • 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 ancazamfir changed the title Anca/client refresh Add client workers for active channels, clear packets on start Apr 30, 2021
@ancazamfir ancazamfir marked this pull request as ready for review May 3, 2021 12:28
@ancazamfir ancazamfir requested a review from romac as a code owner May 3, 2021 12:28
@ancazamfir ancazamfir requested a review from adizere May 3, 2021 12:28
@codecov-commenter

This comment has been minimized.

Sign in to view
adizere
adizere reviewed May 3, 2021
View reviewed changes
relayer/src/chain/handle.rs
@@ -247,8 +255,6 @@ pub trait ChainHandle: DynClone + Send + Sync + Debug {

fn query_latest_height(&self) -> Result<Height, Error>;

fn query_clients(&self, request: QueryClientStatesRequest) -> Result<Vec<ClientId>, Error>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're removing here the ChainHandle::query_clients interface. The intention is to bring it back eventually, right?

This interface will be necessary when we want to implement a CLI to query clients for a non-CosmosSdk chain. (Currently this CLI only works for CosmosSdk.)

relayer/src/link.rs Show resolved Hide resolved
romac
romac reviewed May 4, 2021
View reviewed changes
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing stuff @ancazamfir!

Left some suggestions for further refactoring, but perhaps @adizere can do some of them in #881?

modules/src/timestamp.rs Outdated Show resolved Hide resolved
relayer/src/foreign_client.rs Outdated Show resolved Hide resolved
relayer/src/foreign_client.rs Outdated Show resolved Hide resolved
relayer/src/foreign_client.rs Outdated Show resolved Hide resolved
relayer/src/supervisor.rs Show resolved Hide resolved
relayer/src/supervisor.rs Outdated Show resolved Hide resolved
relayer/src/supervisor.rs Show resolved Hide resolved
relayer/src/supervisor.rs Show resolved Hide resolved
relayer/src/supervisor.rs Show resolved Hide resolved
relayer/src/supervisor.rs Outdated Show resolved Hide resolved
@romac
Copy link
Member

romac commented May 4, 2021

@ancazamfir What's the best way to test this?

@adizere
Copy link
Member

adizere commented May 4, 2021 •
edited
Loading

@ancazamfir What's the best way to test this?

Here's a basic approach: https://gist.github.com/adizere/3b522c20f7b487f398942bba9dabd2c8#file-instructions-md
There's additional scenarios that are not covered there, for instance:

  • reducing the trusting_period parameter for all three chains in the config
  • having multiple channels than just one
  • creating some ft-transfer packets prior to calling start-multi and thus testing the clearing packets functionality

adizere
adizere approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@adizere adizere left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into a few different scenarios testing this, and did not find anything suspicious. In any case, we'll continue working on this + addressing outstanding Romain's comments in PR #881.

Awesome progress with this, Anca!

@adizere adizere mentioned this pull request May 4, 2021
Merged
10 tasks
@romac romac merged commit 251a860 into master May 4, 2021
@romac romac deleted the anca/client_refresh branch May 4, 2021 21:01
@ancazamfir ancazamfir mentioned this pull request May 5, 2021
Merged
5 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@ancazamfir @adizere
Add client workers for active channels, clear packets on start (infor…
3e69119 
…malsystems#872)

* Initial changes

* Create client workers on startup for all existing clients

* Fix client worker creation, push domain type conversion at chain

* Spawn client workers only for active channels. Spawn path workers to clear packets on start.

* Get refresh time from client state, trusting_period for tendermint clients

* Remove last_update, get it from timestamp of consensus state at latest height

* Fix - client worker should not exit if some consensus states expired

* Cleanup

* FMT and typed error in ICS 02 deserializer

* Fix for stale doc comments. Cleaner imports

* Simplified refresh() timestamp comparison.

* Testing & error reporting improvements. Renamed Timestamp::subtract to duration_since.

* Better type & name for MAX_MISBEHAVIOUR_CHECK_DURATION

* More of Romain's comments

* Sync w/ Anca, Fixed the consensus_state bug

* Changelog

Co-authored-by: Adi Seredinschi <adi@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romac romac romac left review comments

@adizere adizere adizere approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prevent client expiration Clear packets when running hermes start-multi
4 participants
@ancazamfir @codecov-commenter @romac @adizere