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

Generate distinct Channel/Connection identifiers when running tests #4062

Closed
2 of 3 tasks
DimitrisJim opened this issue Jul 11, 2023 · 5 comments · Fixed by #6748
Closed
2 of 3 tasks

Generate distinct Channel/Connection identifiers when running tests #4062

DimitrisJim opened this issue Jul 11, 2023 · 5 comments · Fixed by #6748
Assignees
Labels
channel-upgradability Channel upgradability feature needs discussion Issues that need discussion before they can be worked on nice-to-have testing Testing package and unit/integration tests

Comments

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jul 11, 2023

Summary

Subtle bugs can slip through when we use the same channel/connection (client?) identifiers since many of the verification functions expect those to verify the given proofs. These will work in our testing environment but would fail in the most common real-life case where identifiers are unique.

Case in point: #4052. A similar bug was also found in UpgradeCancel after forcing distinct identifiers for channels.

One possible hacky solution is forcing a channel/connection init in the coordinators functions before actually setting up the connection/channel (hack inspiration #4052 (comment)). This was we ensure distinct connection/channel identifiers for our internal tests.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@DimitrisJim DimitrisJim added channel-upgradability Channel upgradability feature nice-to-have needs discussion Issues that need discussion before they can be worked on labels Jul 11, 2023
@DimitrisJim
Copy link
Contributor Author

we currently make assumptions about channel/connection ids in the tests so maybe not do this for all tests atm.

@chatton
Copy link
Contributor

chatton commented Jul 12, 2023

Definitely like this one, even if we just did a channel id different since we usually do either chain A port/channel ID or counterparty port/channel ID.

@crodriguezvega
Copy link
Contributor

Maybe this is too hacky, but sounds simple to me: before the connection and channel handshakes complete, on one of the chains we write in the nextConnectionSequence and nextChannelSequence keys the value 1, so that when the connection and channel identifiers are generated in the handshake they use that number instead of 0.

@chatton
Copy link
Contributor

chatton commented Nov 22, 2023

Since the channelIDs are generated dynamically in core ibc and we parse them from events in our test, it sounds like @crodriguezvega 's solution seems reasonable to me. A little hacky but I think it is worth it to eliminate this class of error.

@DimitrisJim
Copy link
Contributor Author

added this to next iteration, both Carlos and Colin pointed out occurrences of this, best to take it on sooner rather than later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
channel-upgradability Channel upgradability feature needs discussion Issues that need discussion before they can be worked on nice-to-have testing Testing package and unit/integration tests
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants