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

Set counterparty_connection_id to None in conn_open_init #272

Merged
merged 1 commit into from
Dec 1, 2022
Merged

Set counterparty_connection_id to None in conn_open_init #272

merged 1 commit into from
Dec 1, 2022

Conversation

kevinji
Copy link
Contributor

@kevinji kevinji commented Nov 29, 2022

Closes: #174
Closes: #274

Description

In addition to setting the connection ID to None in conn_open_init, this PR also fixes a check in conn_open_ack that expected the counterparty's connection_id to be Some(_) before the Ack message is processed.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, and great catch in the ConnOpenAck handler! Can you just add another changelog entry and fix the merge conflicts?

.changelog/unreleased/bug-fixes/174-fix-conn-open-cpty.md Outdated Show resolved Hide resolved
@kevinji
Copy link
Contributor Author

kevinji commented Nov 30, 2022

Just rebased and updated the changelog as requested.

@kevinji
Copy link
Contributor Author

kevinji commented Dec 1, 2022

Should be updated.

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

🙏

@plafer plafer merged commit 509c9b9 into cosmos:main Dec 1, 2022
@kevinji kevinji deleted the fix-conn-open-init-cpty branch December 1, 2022 19:18
plafer added a commit that referenced this pull request Dec 2, 2022
plafer added a commit that referenced this pull request Dec 6, 2022
…S-3) (#257)

* `ConnOpenInit::validate`

* conn_open_init: execute

* conn_open_try `validate` and `execute`

* conn_open_ack::validate

* conn_open_ack::execute

* conn_open_confirm::validate

* conn_open_confirm::execute

* changelog

* LocalVars

* validate_impl and execute_impl

* Remove useless clone

* fix ConnOpenInit::validate

* fix conn_open_ack (as in #272)

* conn_open_try: LocalVars

* conn_open_try validate/execute impl

* conn_open_ack LocalVars

* conn_open_ack impl

* Track code coverage with `cargo-llvm-cov` and codecov.io (#277)

* Track code coverage with `cargo-llvm-cov`

* Add changelog entry

* Add coverage badge to the README

* Misbehaviour handling implementation (#215)

* Add ClientState::check_misbehaviour_and_update_state()

* Implement misbehaviour handler

* impl Protobuf<Any> for Misbehaviour

* Remove redundant definition of decode_header()

* Implement ChainId::with_version()

* Getters for Tm Misbehaviour

* Add missing checks for conversion from RawMisbehaviour

* Make TmClientState::with_frozen_height() infallible

* Implement TmClientState::check_misbehaviour_and_update_state()

* Cleanup inner functions

* Cleanup errors

* Clippy fix

* Ctor for TmMisbehaviour

* Use git dependencies for tendermint crates

* Add VerifyCommitLightTrusting check

* Add VerifyCommit check

* Clippy fix

* Patch tendermint deps for no-std-check

* Convert Tendermint VerificationError

* Add helpers `Header::as_{un}trusted_block_state()`

* Reorder untrusted verification logic

* Reorder trusted verification logic

* Misbehavior -> misbehaviour

* Check for matching chain-ids

* cargo update ci/no-std-check

* Fix build failure after merge with main

* Update for API changes in tm PR

* Delete ci/no-std-check/Cargo.lock

* Add changelog entry

* Cleanup (naming & comments)

* Rename check_trusted_header() -> check_header_validator_set()

* Rename check_misbehaviour_header() -> check_header_and_validator_set()

* Rename         MisbehaviourConsensusStateTimestampGteTrustingPeriod
 -> ConsensusStateTimestampGteTrustingPeriod

* Rename verify_misbehaviour_header_commit() -> verify_header_commit_against_trusted()

* Remove redundant client state expired check

* Impl Protobuf conversions for mock Misbehaviour

* Impl check_misbehaviour_and_update_state() for mock Misbehaviour

* Remove cargo patches

* Fixes after tendermint-rs bump

* Fix typo

* Add tests

* MockClientState::with_frozen_height()

* Provide MockContext helper to set client chain-id

* Conversions from HostBlock -> TmLightBlock -> TmHeader

* Fix tests

* Clippy fix

* Cleanup tests

* Add comments for tests

* Clippy fix

* Rustfmt

* Fix wrong main branch name in code coverage job (#280)

* implement `ValidationContext` for `MockContext`

* conn_open_init: test validate()

* Add `execute` entrypoint

* re-export validate and execute

* test validate() in connection handlers

* Use into() instead of ContextError directly

* reexport ContextError

* fmt

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
…S-3) (#257)

* `ConnOpenInit::validate`

* conn_open_init: execute

* conn_open_try `validate` and `execute`

* conn_open_ack::validate

* conn_open_ack::execute

* conn_open_confirm::validate

* conn_open_confirm::execute

* changelog

* LocalVars

* validate_impl and execute_impl

* Remove useless clone

* fix ConnOpenInit::validate

* fix conn_open_ack (as in #272)

* conn_open_try: LocalVars

* conn_open_try validate/execute impl

* conn_open_ack LocalVars

* conn_open_ack impl

* Track code coverage with `cargo-llvm-cov` and codecov.io (#277)

* Track code coverage with `cargo-llvm-cov`

* Add changelog entry

* Add coverage badge to the README

* Misbehaviour handling implementation (#215)

* Add ClientState::check_misbehaviour_and_update_state()

* Implement misbehaviour handler

* impl Protobuf<Any> for Misbehaviour

* Remove redundant definition of decode_header()

* Implement ChainId::with_version()

* Getters for Tm Misbehaviour

* Add missing checks for conversion from RawMisbehaviour

* Make TmClientState::with_frozen_height() infallible

* Implement TmClientState::check_misbehaviour_and_update_state()

* Cleanup inner functions

* Cleanup errors

* Clippy fix

* Ctor for TmMisbehaviour

* Use git dependencies for tendermint crates

* Add VerifyCommitLightTrusting check

* Add VerifyCommit check

* Clippy fix

* Patch tendermint deps for no-std-check

* Convert Tendermint VerificationError

* Add helpers `Header::as_{un}trusted_block_state()`

* Reorder untrusted verification logic

* Reorder trusted verification logic

* Misbehavior -> misbehaviour

* Check for matching chain-ids

* cargo update ci/no-std-check

* Fix build failure after merge with main

* Update for API changes in tm PR

* Delete ci/no-std-check/Cargo.lock

* Add changelog entry

* Cleanup (naming & comments)

* Rename check_trusted_header() -> check_header_validator_set()

* Rename check_misbehaviour_header() -> check_header_and_validator_set()

* Rename         MisbehaviourConsensusStateTimestampGteTrustingPeriod
 -> ConsensusStateTimestampGteTrustingPeriod

* Rename verify_misbehaviour_header_commit() -> verify_header_commit_against_trusted()

* Remove redundant client state expired check

* Impl Protobuf conversions for mock Misbehaviour

* Impl check_misbehaviour_and_update_state() for mock Misbehaviour

* Remove cargo patches

* Fixes after tendermint-rs bump

* Fix typo

* Add tests

* MockClientState::with_frozen_height()

* Provide MockContext helper to set client chain-id

* Conversions from HostBlock -> TmLightBlock -> TmHeader

* Fix tests

* Clippy fix

* Cleanup tests

* Add comments for tests

* Clippy fix

* Rustfmt

* Fix wrong main branch name in code coverage job (#280)

* implement `ValidationContext` for `MockContext`

* conn_open_init: test validate()

* Add `execute` entrypoint

* re-export validate and execute

* test validate() in connection handlers

* Use into() instead of ContextError directly

* reexport ContextError

* fmt

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants