Implement `clear packets` command #1834

mzabaluev · 2022-02-01T16:59:02Z

Closes: #1716

Description

Implement clear packets command as a combination of
the effects of tx raw packet-recv and tx raw packet-ack

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

Mutability does not seem to be required, and this guarantees we can reuse the same Link instance for receiving and acking packets in turn.

relayer-cli/src/commands.rs

relayer-cli/src/commands/clear.rs

relayer-cli/src/commands.rs

relayer/src/link.rs

relayer-cli/src/commands/clear.rs

adizere

Thanks Mikhail! I did some testing and I have 3 suggestions:

This was not clear enough in the issue description (User-facing CLI to clear packets #1716), but it would be ideal for clear packets to handle clearing in both directions, which means to: a. clear packets from source towards destination chain; b. clear packets from destination towards source chain.
- Currently we only handle case a. I believe.
- To reproduce, do the following:
- Setup a single channel with id channel-0 between ibc-0 and ibc-1
- Submit a packet at chain ibc-0
- $ hermes --json tx raw ft-transfer ibc-1 ibc-0 transfer channel-0 0184 -d samoleans -t 1800 -n 1
- Try to clear packets in direction ibc-1 (source) -> ibc-0 (dest), it will not find anything
- $ hermes clear packets ibc-0 ibc-1 transfer channel-0
  Success: []
Given my suggestion above to handle clearing in both directions, I think a reasonable way to do it as follows:
1. build_and_send_recv_packet_messages on direction source -> dest
2. build_and_send_recv_packet_messages on direction dest -> source
3. build_and_send_ack_packet_messages on direction source -> dest
4. build_and_send_ack_packet_messages on direction dest -> source

In the present implementation, I think we do 1 & 3.

This means we'll need to setup the Link objects for both directions, which means we need to find out the port+channel identifier on the destination chain (we only know the port+channel-id on the source chain). In other words, we'll need to query the channel port+channel end on the source chain and extract the counterparty info from there.

Testing an debugging this PR is actually difficult because the output is very noisy. It may be a good time to try and tackle issue hermes tx raw shows all the data bytes in a column #1559 as part of this PR, namely: to make the output for SendPacket, WriteAcknowledgement and AcknowledgePacket more concise. Just to emphasize: this scope creep is optional, I'm just suggesting what I would do to make debugging more ergonomic here.

mzabaluev · 2022-02-02T21:03:12Z

extract the counterparty info from there.

Would something like this be a good way to do it?

adizere · 2022-02-03T09:21:35Z

extract the counterparty info from there.

Would something like this be a good way to do it?

Yep, we need to do a chain.query_channel and from the result of this query we can extract the counterparty info.

mzabaluev · 2022-02-04T09:29:28Z

@adizere I have tested 4bb294c with the repro sequence you have provided, and it looks to be working.

mzabaluev · 2022-02-04T09:33:41Z

Adding an e2e test is a challenge, though: I need a clue on what the expected results of the command should be in the Python test classes. These are different for TxPacketRecv and TxPacketAck, and I'm not sure which data to extract from the combined result.

relayer-cli/src/commands/clear.rs

ancazamfir · 2022-02-04T12:31:01Z

relayer-cli/src/commands/clear.rs

+        run_and_collect_events(&mut ev_list, || link.build_and_send_recv_packet_messages());
+        run_and_collect_events(&mut ev_list, || link.build_and_send_ack_packet_messages());
+
+        // Clear packets in the dst -> src direction.
+        // Some of the checks in the Link constructor may be redundant;
+        // going slowly, but reliably.


Not sure the order is correct here, in the sense that it will leave pending packets. I believe we need to create the two links (one for A->B and the other for B->A), then build_and_send_recv_packet_messages on the two links and after build_and_send_ack_packet_messages on the two links. This is because ``build_and_send_recv_packet_messages` may create pending acks.

Also I believe that these methods end up waiting for commit response from the chain, which is what we want here. But could you please double check?

Thank you @ancazamfir for bringing this up. I'm still learning about these things. I guess the intent is to wait for commit in both directions, which I believe what the low-level tx raw commands also achieve. I will take care about interleaving the recv and ack phases as suggested.

When processing pending packets for the `clear packets` command, schedule RecvPacket in both directions before proceeding with AckPacket, also in both directions. This ensures we process also the acks created by the packets just received.

relayer-cli/src/commands/clear.rs

mzabaluev · 2022-02-04T13:44:23Z

relayer-cli/src/commands/clear.rs

+        let opts = LinkParameters {
+            src_port_id: fwd_link.a_to_b.dst_port_id().clone(),
+            src_channel_id: fwd_link.a_to_b.dst_channel_id().clone(),
+        };
+        let rev_link = match Link::new_from_opts(chains.dst, chains.src, opts, false) {
+            Ok(link) => link,
+            Err(e) => Output::error(format!("{}", e)).exit(),
+        };


This may be the case for a Link::reverse method, which would do this and maybe skip on some redundant checks and queries.

Sounds like a nice addition, would you mind doing that as part of this PR?

@romac Can you provide some clue on which of the checks done inside Link::new_from_opts can be bypassed, and which initializations reused, when creating the reverse Link?

ancazamfir · 2022-02-09T13:49:23Z

One more thought for a future PR maybe. Add a hermes query packets command that shows all pending packets for a channel at both ends.

There is no longer a "source" chain, but we need to clarify that both directions of the channel are cleared even though only one end is used for identification.

Use a span and KV pairs to provide the context in ChainEndpoint::build_packet_proofs, so the log event macro invocations are short and to the point.

relayer/src/chain/cosmos.rs

mzabaluev · 2022-02-09T14:27:39Z

As reported by @ancazamfir, this only works correctly on unordered channels.

ancazamfir

Great stuff!

romac · 2022-02-09T16:13:16Z

relayer-cli/src/commands/clear.rs

+
+        // Schedule RecvPacket messages for pending packets in both directions.
+        // This may produce pending acks which will be processed in the next phase.
+        run_and_collect_events(&mut ev_list, || {


ancazamfir · 2022-02-09T20:34:27Z

As reported by @ancazamfir, this only works correctly on unordered channels.

Actually this works well for ordered channels when tested with gaia v6.0.0 and ibc-go v2.0.0 so I think it's all good (we won't support ordered channels in older versions)

This is the slowest implementation, not optimizing on any shared state or eliminating redundant checks.

This is an option on Link::new_from_opts, so it seems logical to also make it optional on the same level API.

adizere · 2022-02-14T14:38:48Z

Tested & looks good, cool work here Mikhail, I will merge it ASAP!

* Make self immutable on some methods of Link Mutability does not seem to be required, and this guarantees we can reuse the same Link instance for receiving and acking packets in turn. * cli: Add `clear packets` command * Changelog for informalsystems#1834 * Improved help for `clear` and `clear packets` * cli: clear packets in reverse direction as well * fix clippy lints about diverging exit method * Interleave recv and ack in each direction When processing pending packets for the `clear packets` command, schedule RecvPacket in both directions before proceeding with AckPacket, also in both directions. This ensures we process also the acks created by the packets just received. * Rename positional parameters in `clear packets` * No need for counterparty * Clarify help on `clear packets` command There is no longer a "source" chain, but we need to clarify that both directions of the channel are cleared even though only one end is used for identification. * Fix a syntax error * Improve logging in proven_packet Use a span and KV pairs to provide the context in ChainEndpoint::build_packet_proofs, so the log event macro invocations are short and to the point. * Removed the extra debug logs * Implement Link::reverse naively This is the slowest implementation, not optimizing on any shared state or eliminating redundant checks. * Add tx confirmation flag to Link::reverse This is an option on Link::new_from_opts, so it seems logical to also make it optional on the same level API. * Extended error types when port/channel is incorrect Co-authored-by: Anca Zamfir <zamfiranca@gmail.com> Co-authored-by: Adi Seredinschi <adi@informal.systems>

mzabaluev added 2 commits February 1, 2022 18:45

Make self immutable on some methods of Link

3dffc2a

Mutability does not seem to be required, and this guarantees we can reuse the same Link instance for receiving and acking packets in turn.

cli: Add clear packets command

7fd297c

mzabaluev requested review from adizere and romac February 1, 2022 16:59

mzabaluev added I: CLI Internal: related to the relayer's CLI needs guide update O: usability Objective: cause to improve the user experience (UX) and ease using the product labels Feb 1, 2022

mzabaluev commented Feb 1, 2022

View reviewed changes

relayer-cli/src/commands.rs Outdated Show resolved Hide resolved

mzabaluev changed the title ~~Mikhail/clear-packets-cmd~~ Implement clear packets command Feb 1, 2022

Changelog for #1834

a67ec51

mzabaluev commented Feb 1, 2022

View reviewed changes

relayer-cli/src/commands/clear.rs Outdated Show resolved Hide resolved

adizere reviewed Feb 2, 2022

View reviewed changes

relayer-cli/src/commands.rs Outdated Show resolved Hide resolved

relayer/src/link.rs Show resolved Hide resolved

relayer-cli/src/commands/clear.rs Outdated Show resolved Hide resolved

adizere reviewed Feb 2, 2022

View reviewed changes

Improved help for clear and clear packets

5a80326

mzabaluev mentioned this pull request Feb 3, 2022

cli: change exit functions to never return #1839

Merged

3 tasks

cli: clear packets in reverse direction as well

4bb294c

mzabaluev marked this pull request as ready for review February 4, 2022 09:27

mzabaluev requested a review from ancazamfir as a code owner February 4, 2022 09:27

ancazamfir reviewed Feb 4, 2022

View reviewed changes

relayer-cli/src/commands/clear.rs Outdated Show resolved Hide resolved

ancazamfir reviewed Feb 4, 2022

View reviewed changes

relayer-cli/src/commands/clear.rs Outdated Show resolved Hide resolved

ancazamfir reviewed Feb 4, 2022

View reviewed changes

mzabaluev added 3 commits February 4, 2022 14:56

Merge branch 'master' into mikhail/clear-packets-cmd

2d05671

fix clippy lints about diverging exit method

c00696a

Interleave recv and ack in each direction

667faca

When processing pending packets for the `clear packets` command, schedule RecvPacket in both directions before proceeding with AckPacket, also in both directions. This ensures we process also the acks created by the packets just received.

mzabaluev commented Feb 4, 2022

View reviewed changes

Rename positional parameters in clear packets

248940d

mzabaluev requested a review from ancazamfir February 8, 2022 10:16

No need for counterparty

98b2a5c

mzabaluev added 2 commits February 9, 2022 15:57

Clarify help on clear packets command

df6c3d7

There is no longer a "source" chain, but we need to clarify that both directions of the channel are cleared even though only one end is used for identification.

Fix a syntax error

aef0e6a

mzabaluev mentioned this pull request Feb 9, 2022

Implement query packet pending command #1862

Closed

Improve logging in proven_packet

8ab1a72

Use a span and KV pairs to provide the context in ChainEndpoint::build_packet_proofs, so the log event macro invocations are short and to the point.

mzabaluev commented Feb 9, 2022

View reviewed changes

relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved

ancazamfir approved these changes Feb 9, 2022

View reviewed changes

Removed the extra debug logs

0970302

romac reviewed Feb 9, 2022

View reviewed changes

mzabaluev added 2 commits February 11, 2022 18:58

Implement Link::reverse naively

0521081

This is the slowest implementation, not optimizing on any shared state or eliminating redundant checks.

Add tx confirmation flag to Link::reverse

13869cf

This is an option on Link::new_from_opts, so it seems logical to also make it optional on the same level API.

adizere self-assigned this Feb 14, 2022

Extended error types when port/channel is incorrect

3f73fb9

adizere approved these changes Feb 14, 2022

View reviewed changes

adizere merged commit 1a3c1d2 into master Feb 14, 2022

adizere deleted the mikhail/clear-packets-cmd branch February 14, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `clear packets` command #1834

Implement `clear packets` command #1834

mzabaluev commented Feb 1, 2022 •

edited by adizere

Loading

adizere left a comment

mzabaluev commented Feb 2, 2022

adizere commented Feb 3, 2022

mzabaluev commented Feb 4, 2022

mzabaluev commented Feb 4, 2022

ancazamfir Feb 4, 2022

mzabaluev Feb 4, 2022

mzabaluev Feb 4, 2022 •

edited

Loading

romac Feb 9, 2022

mzabaluev Feb 9, 2022

ancazamfir commented Feb 9, 2022

mzabaluev commented Feb 9, 2022

ancazamfir left a comment

romac Feb 9, 2022

ancazamfir commented Feb 9, 2022

adizere commented Feb 14, 2022

Implement clear packets command #1834

Implement clear packets command #1834

Conversation

mzabaluev commented Feb 1, 2022 • edited by adizere Loading

Description

PR author checklist:

Reviewer checklist:

adizere left a comment

Choose a reason for hiding this comment

mzabaluev commented Feb 2, 2022

adizere commented Feb 3, 2022

mzabaluev commented Feb 4, 2022

mzabaluev commented Feb 4, 2022

ancazamfir Feb 4, 2022

Choose a reason for hiding this comment

mzabaluev Feb 4, 2022

Choose a reason for hiding this comment

mzabaluev Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

romac Feb 9, 2022

Choose a reason for hiding this comment

mzabaluev Feb 9, 2022

Choose a reason for hiding this comment

ancazamfir commented Feb 9, 2022

mzabaluev commented Feb 9, 2022

ancazamfir left a comment

Choose a reason for hiding this comment

romac Feb 9, 2022

Choose a reason for hiding this comment

ancazamfir commented Feb 9, 2022

adizere commented Feb 14, 2022

Implement `clear packets` command #1834

Implement `clear packets` command #1834

mzabaluev commented Feb 1, 2022 •

edited by adizere

Loading

mzabaluev Feb 4, 2022 •

edited

Loading