Fix panic in conn open try when no connection id is provided #615
Conversation
There was a problem hiding this comment.
Looks good as a quick fix. But I think we should have a consistent approach with client and channel handling. I thought we decided when we re-wrote the client handler to return an id from process(). @adizere ? Looking at channels now, we do the same as in connection.
Currently only ICS02 performs identifier selection and returns that from We should adapt both ICS03 (and ICS04) to return identifiers from |
Sounds good. I will leave it up to you @vitorenesduarte if you want to do it in this PR or we do it in a separate one. I opened cosmos/ibc-rs#91, please mention in |
Let's do it in another PR. |
|
@vitorenesduarte Great stuff! Can you please also create an issue, mention it in the body of this PR and update the CHANGELOG accordingly in a follow-up PR? |
|
* Add ICS02 model * Add MBT test driver * Add ICS02TestExecutor * Add another apalache counterexample * Fix ICS02.tla: client counter starts at 0 * Check for errors in MockContext.deliver * Handle errors in MBT tests * Remove SyntheticTendermint mock context * More idiomatic check_next_state * Buffered file reads * Make extract_handler_error_kind generic over the IBC handler * Support multiple chains in MBT * Make eyre a dev-dependency * s/ICS0/IBC on TLA files * Initial support for conn open init * Start connection and channel identifiers at 0 * Add conn open init to TLA spec * Represent clients with a record in TLA spec * Finish connection open init * s/state/step * Minimize diff * Modularize TLA spec * TLA format convention * s/Null/None * Sketch conn open try; Model chain's height * Bound model space * Only update chain height upon success * Check that chain heights match the ones in the model * Sketch conn open try * Sketch conn open try * Model missing connections and connection mismatches in conn open try * Trigger bug in conn open try * Go back to previous way of generating connection and channel ids * Disable failing MBT test * Fix panic in conn open try when no connection id is provided * ICS02TestExecutor -> IBCTestExecutor * Failing MBT test now passes with #615 * Add notes on MBT * Remove ICS02 * Add README * Improve README * Remove MBT intro * new lines * Make MBT README more easily discoverable * IBCTestExecutor: Map from ChainId (instead of String) to MockContext * s/epoch/revision * Move from eyre to thiserror * Improve README * Improve README * Improve arguments order in modelator::test * Improve chain identifiers * Improve README
Closes: #626
Description
Currently, a conn open try can only succeed if some
previous_connection_idis provided in theMsgConnectionOpenTry:https://github.com/informalsystems/ibc-rs/blob/b1b9dac6132a2fd2a86fa1c6f0179f47db3e3454/modules/src/ics03_connection/msgs/conn_open_try.rs#L26-L35
However, to have a valid
previous_connection_id, one has to have a successful conn open try. This circular dependency makes it so that it's never possible to have successful conn open try.This issue was found during the MBT work (#601). I tried to add a regression test but didn't find an easy way to do it. The best candidate was the
routing_module_and_keeperstest inics26_routing/handler.rs: https://github.com/informalsystems/ibc-rs/blob/b1b9dac6132a2fd2a86fa1c6f0179f47db3e3454/modules/src/ics26_routing/handler.rs#L152-L316There's a single chain here, which I think means it's not possible to have a successful conn open try. I've also noticed that the tests here don't check that the returned error matches the expected error. This is done in the
IBCTestExecutorintroduced in #601. Maybe at some point it will be possible to reuse this test executor to write these simple unit tests.For contributor use:
docs/) and code comments.Files changedin the Github PR explorer.