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.

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

Clear packets on hermes start #1199

Closed
wants to merge 2 commits into from
Closed

Clear packets on hermes start #1199

wants to merge 2 commits into from

Conversation

ancazamfir
Copy link
Collaborator

@ancazamfir ancazamfir commented Jul 14, 2021

Closes: #1200
Partially closes: #1196

Description

Hermes does not clear packets on start if there are no IBC events and no clear_packets_interval trigger.
The e2e script masks this error because it sends some packets, starts hermes and then sends more packets (issue masked here because this triggers packet clearing) before checking that all packets are cleared.

  • Fixed script to catch the bug
  • Fixed the bug

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 marked this pull request as ready for review July 14, 2021 12:51
@ancazamfir ancazamfir requested review from adizere and romac as code owners July 14, 2021 12:51
@@ -261,12 +261,16 @@ impl RelayPath {
Err(LinkError::OldPacketClearingFailed)
}

pub fn clear_packets(&self) -> bool {
Copy link
Member

@romac romac Jul 14, 2021

Choose a reason for hiding this comment

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

How about we keep the clear_packets name for the method that does the clearing, and rename this to:

Suggested change
pub fn clear_packets(&self) -> bool {
pub fn will_clear_packets(&self) -> bool {

or

Suggested change
pub fn clear_packets(&self) -> bool {
pub fn packet_clearing_enabled(&self) -> bool {

Suggesting this because I find the clear_packets and do_clear_packets naming scheme a bit confusing, as I would expect either of those to actually clear the packets.

@ancazamfir ancazamfir closed this Jul 14, 2021
@ancazamfir ancazamfir deleted the anca/clear_on_start branch July 14, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hermes does not clear packets on start Hermes issues found by Dex scalability tests on v0.6.0
2 participants