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

Fix the connection delay logic to use the header update block time #1917

Merged
merged 51 commits into from
Apr 5, 2022
Merged

Fix the connection delay logic to use the header update block time #1917

merged 51 commits into from
Apr 5, 2022

Conversation

hu55a1n1
Copy link
Member

@hu55a1n1 hu55a1n1 commented Feb 23, 2022 •
edited
Loading

Closes: #1772

Description

Fix the connection delay logic to use the target chain's latest block time and header update block time.

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@hu55a1n1 hu55a1n1 changed the title Return update height from update_client_{dst,src} methods Fix the connection delay logic to use the target chain's latest block time and header update block time. Feb 24, 2022
@hu55a1n1 hu55a1n1 changed the title Fix the connection delay logic to use the target chain's latest block time and header update block time. Fix the connection delay logic to use the header update block time. Feb 24, 2022
romac
romac reviewed Feb 24, 2022
View reviewed changes
relayer/src/link/operational_data.rs Outdated Show resolved Hide resolved
relayer/src/link/relay_path.rs Outdated Show resolved Hide resolved
hu55a1n1
hu55a1n1 commented Feb 24, 2022
View reviewed changes
relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
@romac romac changed the title Fix the connection delay logic to use the header update block time. Fix the connection delay logic to use the header update block time Feb 25, 2022
@hu55a1n1 hu55a1n1 force-pushed the hu55a1n1/1772-fix-conn-delay-check branch from fc3b986 to 6181fb9 Compare March 8, 2022 12:37
@soareschen soareschen marked this pull request as ready for review March 8, 2022 14:30
@soareschen soareschen self-assigned this Mar 8, 2022
soareschen
soareschen requested changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@soareschen soareschen left a comment

Choose a reason for hiding this comment

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

Can we have an integration test to ensure that the connection delay logic is working correctly?

relayer/src/link/relay_path.rs Show resolved Hide resolved
@hu55a1n1 hu55a1n1 marked this pull request as draft March 11, 2022 09:06
@soareschen soareschen mentioned this pull request Mar 11, 2022
Closed
6 tasks
@hu55a1n1 hu55a1n1 mentioned this pull request Mar 29, 2022
Merged
6 tasks
@adizere adizere mentioned this pull request Mar 29, 2022
Merged
6 tasks
@adizere adizere requested a review from soareschen March 29, 2022 13:19
soareschen
soareschen requested changes Mar 30, 2022
View reviewed changes
relayer/src/link/relay_path.rs Outdated Show resolved Hide resolved
soareschen
soareschen requested changes Apr 1, 2022
View reviewed changes
relayer/src/link/relay_path.rs Outdated Show resolved Hide resolved
relayer/src/link/relay_path.rs Outdated Show resolved Hide resolved
ancazamfir
ancazamfir approved these changes Apr 5, 2022
View reviewed changes
Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Looks good!! Thanks @hu55a1n1 !

@hu55a1n1 hu55a1n1 merged commit 3e34b4b into master Apr 5, 2022
@hu55a1n1 hu55a1n1 deleted the hu55a1n1/1772-fix-conn-delay-check branch April 5, 2022 08:45
@adizere adizere mentioned this pull request May 9, 2022
Closed
5 tasks
hu55a1n1 added a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@hu55a1n1 @soareschen @ancazamfir
Fix the connection delay logic to use the header update block time (i…
4b5fe24 
…nformalsystems#1917)

* Return update height from update_client_{dst,src} methods

* Add query_host_consensus_state()

* Set scheduled time appropriately

* Handle connection delay only if batch contains packet events

* Apply suggestion

* Fix mock impl

* Fix schedule_time calculation

* Update comment

* Fix comment

* Add .changelog entry

* Improve has_packet_msgs()

* Implement query_host_consensus_state() for wrapper chain handles

* Apply suggestion

* Fix clippy errors

* Rework update methods

* Minor refactoring

* Avoid partitioning events

* Handle misbehaviour case explicitly

* Add connection delay test

* Use first UpdateClient event to determine processed_height

* Add comment

* Check for frozen clients in case of misbehavior during client update

* Adjust connection delay for avg block time

* Add TODO for moving to /header endpoint

* Make update_client_dst() similar to update_client_src()

* Fix integration test

* Compare scheduled time against latest chain time

* Allow query_host_consensus_state to return latest state when Height is zero

* Fix conn delay elapsed calculation

* Cleanup

* Check for block delay

* Add config comment for max_expected_time_per_block

* Wait for conn delay

* More comments

* Extract all connection-delay logic from RelayPath

* Address review feedback

* Address review feedback

* Closures for conn-delay specific lazy eval

* Minor refactoring

* Add helpers has_conn_delay_elapsed() and conn_delay_remaining()

* Handle connection block delay

* Cast block delay to u32

* Extract out CLI specific link code

* Make opdata methods priv

* Apply suggestions from review

Co-authored-by: Soares Chen <soares.chen@gmail.com>
Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>
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

@seanchen1991 seanchen1991 seanchen1991 left review comments

@soareschen soareschen soareschen approved these changes

@ancazamfir ancazamfir ancazamfir approved these changes

@adizere adizere Awaiting requested review from adizere

Assignees

@soareschen soareschen

@ancazamfir ancazamfir

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Connection delay period is checked against relayer's local time
6 participants
@hu55a1n1 @soareschen @romac @adizere @seanchen1991 @ancazamfir