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

Clean up packets clearing logic in RelayPath #1879

Merged
merged 3 commits into from
Feb 16, 2022
Merged

Clean up packets clearing logic in RelayPath #1879

merged 3 commits into from
Feb 16, 2022

Conversation

adizere
Copy link
Member

@adizere adizere commented Feb 15, 2022

Closes: #1872

Description

The bug reported in #1872 exposed a regression: the a bug is that the clear_on_start option from Hermes config is ignored, and Hermes always clears packets when it starts up.

The bug appeared because we introduced duplicate logic for keeping track of the clear_on_start toggle:

  1. we kept track of this toggle in the RelayPath, which was incorrectly initialized to true https://github.com/informalsystems/ibc-rs/blob/1a3c1d2c50a8f207c93cce4b19b267ca6d7f3cfb/relayer/src/link/relay_path.rs#L125

  2. we kept track of this toggle additionally in the packet command worker task, which was correctly initialized with the clear_on_start value configured in the Hermes config.toml
    https://github.com/informalsystems/ibc-rs/blob/b65baa9456b25500c49bf34896e79d2939c2ef65/relayer/src/worker.rs#L114-L121

The fix in this PR is to remove the logic from (1) RelayPath, and proposes to keep only the logic from (2) packet command worker. The nice benefit of adopting this fix is that it also simplifies slightly the code in RelayPath (which is one of the most complex parts of the relayer).

How to test this PR

Reproduce the problem

First, please follow the "Steps to Reproduce" from the original issue #1872 to confirm you can reproduce the problem on your local setup.

Test the solution from the present PR

Test that disabling clear_on_start works

  1. Setup a two chain network using gm. Suppose the two networks and ibc-0 and ibc-1.
  2. Setup a channel-0 between the two networks.
  3. Trigger and ft-transfer on channel-0 of ibc-0
    • ./target/debug/hermes --json tx raw ft-transfer ibc-1 ibc-0 transfer channel-0 100 -d samoleans -t 1800 -n 1

    • take note of the packet sequence number provided in the output for example:
    • "sequence":9

  4. Configure clear_on_start to be false in the config.toml of Hermes.
    • Make sure your log level is set to log_level = 'debug'.
    • Also set clear_interval = 0 to completely disable periodic packet clearing.
  5. Do hermes start.
    • Notice the log output and it should be visible that Hermes will not clear the outstanding packets.
    • Hermes should not have any activity.
  6. In a parallel terminal, trigger and ft-transfer using a second Hermes instance (similar to step 3)
    • ./target/debug/hermes --json tx raw ft-transfer ibc-1 ibc-0 transfer channel-0 100 -d samoleans -t 1800 -n 1

    • take note of the packet sequence number provided in the output:
    • "sequence":10

  7. In the first terminal where Hermes start is running, observe the log output, we should now notice some Hermes activity
    • the important part is we should see that the second packet (sequence number 10) is being relayed.
    • the first packet (sequence number 9 triggered at step 3) should remain unrelayed.
    • example log output
    • 2022-02-15T10:20:02.962841Z DEBUG ThreadId(22) packet_cmd{src_chain=ibc-0 src_port=transfer src_channel=channel-0 dst_chain=ibc-1}:generate{id=tiLSMMUOjC}: /ibc.core.channel.v1.MsgRecvPacket from SendPacketEv(SendPacket - h:0-30551, seq:10, path:channel-0/transfer->channel-0/transfer, toh:0-0, tos:Timestamp(2022-02-15T10:49:52.478069Z)))

Test that enabling clear_on_start works

Redo the test from above, with the following adjustments:

  • at step 4: Configure clear_on_start to be true (not false)
  • at step 5: You should see that Hermes has activity and should be relaying the old packet (triggered at step 3).

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).

Copy link
Contributor

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

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

Oh great, one less should_clear_packets.

Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

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

The code changes look good to me and testing locally exhibits the expected behavior 👍

@romac romac changed the title Cleaned up clear_packets logic from RelayPath Cleaned up packets clearing logic in RelayPath Feb 16, 2022
@romac romac changed the title Cleaned up packets clearing logic in RelayPath Clean up packets clearing logic in RelayPath Feb 16, 2022
@adizere adizere merged commit 1f5e17d into master Feb 16, 2022
@adizere adizere deleted the adi/1872 branch February 16, 2022 10:05
@soareschen soareschen mentioned this pull request Feb 16, 2022
6 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Cleaned up clear_packets logic from RelayPath.

* changelog

* Better changelog
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 clear_on_start toggle does not work
4 participants