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

Unambiguous packet query #613

Merged
merged 1 commit into from
Feb 6, 2021
Merged

Unambiguous packet query #613

merged 1 commit into from
Feb 6, 2021

Conversation

vitorenesduarte
Copy link
Contributor

@vitorenesduarte vitorenesduarte commented Feb 5, 2021

Closes: #614

Description

Before, a query_txs for a packet on a given chain would filter only by the packet source port, source channel and sequence.
With 2 chains, there can be at most a single packet matching such filter, and this fact was assumed in the code.

However, this assumption does not hold with more than 2 chains. To understand why, consider the following example with chains ibc-0, ibc-1 and ibc-2:

  • a channel handshake occurs between ibc-0 and ibc-1; the channel identifiers are channel-0 on both ends
  • a channel handshake occurs between ibc-1 and ibc-2; the channel identifier on ibc-1 is channel-1 and on ibc-2 is channel-0

If now both ibc-0 and ibc-2 send packets to ibc-1 and we do a packet query filtering only by the source, such query will return
packets from both ibc-0 and ibc-1 (as both have channel-0 as the source channel in their packets).

In this commit, we start filtering also by the destination port and destination channel. This makes the packet query unambiguous.

Thanks @ancazamfir for the help debugging this issue. It was a fun one!


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.

Before, a `query_txs` for a packet on a given chain would filter only
by the packet source port, source channel and sequence.
With 2 chains, there can be at most a single packet matching such
filter, and this fact was assumed in the code.

However, this assumption does not hold with more than 2 chains. To
understand why, consider the following example with chains `ibc-0`,
`ibc-1` and `ibc-2`:
- a channel handshake occurs between `ibc-0` and `ibc-1`; the channel
identifiers are `channel-0` on both ends
- a channel handshake occurs between `ibc-1` and `ibc-2`; the channel
identifier on `ibc-1` is `channel-1` and on `ibc-2` is `channel-0`

If now both `ibc-0` and `ibc-2` send packets to `ibc-1` and we do a
packet query filtering only by the source, such query will return
packets from both `ibc-0` and `ibc-1` (as both have `channel-0` as
the source channel in their packets).

In this commit, we start filtering also by the destination port and
destination channel. This makes the packet query unambiguous.
@vitorenesduarte
Copy link
Contributor Author

With 2 chains, there can be at most a single packet matching such filter, and this fact was assumed in the code.

I left a TODO to make this assumption explicit. It will be addressed in a following PR.

@codecov-io
Copy link

Codecov Report

Merging #613 (a6315e2) into master (b1b37f5) will increase coverage by 30.9%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #613      +/-   ##
=========================================
+ Coverage    13.6%   44.5%   +30.9%     
=========================================
  Files          69     144      +75     
  Lines        3752    9542    +5790     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4255    +3742     
- Misses       2618    5287    +2669     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
modules/src/address.rs 100.0% <ø> (ø)
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...pplication/ics20_fungible_token_transfer/events.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_def.rs 48.3% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 100.0% <ø> (ø)
modules/src/ics02_client/error.rs 100.0% <ø> (ø)
... and 246 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1b9dac...a6315e2. Read the comment docs.

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 again!

@ancazamfir ancazamfir merged commit d3d8a69 into master Feb 6, 2021
@ancazamfir ancazamfir deleted the vitor/query_tx branch February 6, 2021 11:43
@romac
Copy link
Member

romac commented Feb 6, 2021

@vitorenesduarte Awesome, thanks! Could you please create an issue and open a PR to add a changelog entry for this, mentioning said issue?

@romac
Copy link
Member

romac commented Feb 6, 2021

^ This can be done as part of your follow-up PR.

@vitorenesduarte
Copy link
Contributor Author

@vitorenesduarte Awesome, thanks! Could you please create an issue and open a PR to add a changelog entry for this, mentioning said issue?

Oh, right 😅 @romac Here it goes: #617

hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
Before, a `query_txs` for a packet on a given chain would filter only
by the packet source port, source channel and sequence.
With 2 chains, there can be at most a single packet matching such
filter, and this fact was assumed in the code.

However, this assumption does not hold with more than 2 chains. To
understand why, consider the following example with chains `ibc-0`,
`ibc-1` and `ibc-2`:
- a channel handshake occurs between `ibc-0` and `ibc-1`; the channel
identifiers are `channel-0` on both ends
- a channel handshake occurs between `ibc-1` and `ibc-2`; the channel
identifier on `ibc-1` is `channel-1` and on `ibc-2` is `channel-0`

If now both `ibc-0` and `ibc-2` send packets to `ibc-1` and we do a
packet query filtering only by the source, such query will return
packets from both `ibc-0` and `ibc-1` (as both have `channel-0` as
the source channel in their packets).

In this commit, we start filtering also by the destination port and
destination channel. This makes the packet query unambiguous.
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.

tx raw packet-ack sends wrong acknowledgments in a 3 chain setup
4 participants