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

Replace check_header_and_update_state with an architecture similar to ibc-go #584

Merged
merged 67 commits into from
Apr 13, 2023

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Mar 30, 2023

While implementing the new API, I found several bugs. I implicitly fix those by not re-introducing them in the new API implementation.

Main issue:
Closes: #535

Bugs fixed:
Closes: #583
Closes: #585
Closes: #589
Closes: #598
Closes: #601

Description

  • tendermint light client methods to implement:
    • verify_client_message
    • check_for_misbehaviour
    • update_state_on_misbehaviour
    • update_state
  • update_client::validate calls client_state.verify_client_message, similar to here in ibc-go
  • update_client::execute calls client_state.{check_for_misbehaviour, update_state_on_misbehaviour, update_state}, similar to here in ibc-go
  • cleanup
    • remove now unused ClientState::{check_header_and_update_state} and ClientState::{check_misbehaviour_and_update_state}, and all unused with_* functions
    • remove now unused misbehaviour::{validate, execute}
    • remove checks from TmMisbehaviour::new(); this should be a light constructor.
  • Adapt MsgUpdateClient
    • MsgSubmitMisbehaviour and MsgUpdateClient are merged into the "new" MsgUpdateClient
    • Add a TryFrom<RawMsgSubmitMisbehaviour> for MsgUpdateClient
    • Update TryFrom<Any> for MsgEnvelope
    • Rename MsgUpdateClient.header to MsgUpdateClient.client_message, and add update_kind enum field

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

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 81.10% and project coverage change: +0.26 🎉

Comparison is base (707d032) 72.93% compared to head (f6c375a) 73.19%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #584      +/-   ##
==========================================
+ Coverage   72.93%   73.19%   +0.26%     
==========================================
  Files         126      125       -1     
  Lines       15725    15660      -65     
==========================================
- Hits        11469    11463       -6     
+ Misses       4256     4197      -59     
Impacted Files Coverage Δ
crates/ibc/src/core/context.rs 85.05% <ø> (+0.96%) ⬆️
crates/ibc/src/core/handler.rs 74.64% <ø> (ø)
crates/ibc/src/core/ics02_client/client_state.rs 71.42% <ø> (ø)
crates/ibc/src/core/ics02_client/msgs.rs 100.00% <ø> (ø)
crates/ibc/src/core/ics26_routing/msgs.rs 1.11% <0.00%> (-0.07%) ⬇️
crates/ibc/src/mock/misbehaviour.rs 91.30% <ø> (+10.53%) ⬆️
...es/ibc/src/core/ics02_client/msgs/update_client.rs 59.57% <36.00%> (-29.72%) ⬇️
crates/ibc/src/core/ics02_client/error.rs 20.00% <57.14%> (+14.44%) ⬆️
...nts/ics07_tendermint/client_state/update_client.rs 74.07% <74.07%> (ø)
...ents/ics07_tendermint/client_state/misbehaviour.rs 76.08% <76.08%> (ø)
... and 7 more

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@plafer plafer changed the title Replace check_{header,misbehaviour}_and_update_state with a similar architecture of ibc-go Replace check_header_and_update_state with a similar architecture of ibc-go Mar 31, 2023
@plafer plafer changed the title Replace check_header_and_update_state with a similar architecture of ibc-go Replace check_header_and_update_state with an architecture similar to ibc-go Apr 3, 2023
@plafer
Copy link
Contributor Author

plafer commented Apr 3, 2023

Created #600; I won't implement in this PR but it's a very desirable feature which we should have.

plafer and others added 3 commits April 10, 2023 09:45
…our.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
…ient.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
…our.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

I left some suggestions. About the different checks discussed here in particular, I didn't go into much detail yet, I assumed there might be updates based on your discussion with Anca.
Greatly appreciate you handling this PR!

@plafer plafer requested a review from ancazamfir April 12, 2023 22:01
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

Great work 👌🏻

@plafer
Copy link
Contributor Author

plafer commented Apr 13, 2023

@ancazamfir I will wait for your approval as well before merging

@plafer plafer merged commit 8a75e3b into main Apr 13, 2023
@plafer plafer deleted the plafer/535-ibc-go-adr-6 branch April 13, 2023 19:12
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
…to ibc-go (#584)

* fix

* changelog

* verify_header

contains a FIXME to double check

* fix header.trusted_next_validator_set hash bug

* fmt

* remove bad comment

* add timestamp monotonicity checks to `verify_header()`

* clean up verify_header

* add revision number check in verify_header

* header height extra check

* clarifying comment

* check_misbehaviour_header scaffolding

* remove duplicate methods

* verify_misbehaviour done as ibc-go

* fix chain ID before commit verification

* finish verify_misbehavior

* `check_for_misbehaviour` header variant implementation

* check_for_misbehaviour, misbehaviour variant

* monotonicity of timestamps in update_client

* update_state_on_misbehaviour

* implement update_state in tendermint light client

* cleanup `map_err`s

* Rename MsgUpdateClient::client_message

* impl raw misbehaviour -> MsgUpdateClient

* change updateClientKind enum

* move misbehaviour tests to update_client

* move misbehaviour type url

* remove misbehaviour msg and handler

* update_client::validate

* implement part of update_client::execute

* separate events emitted

* finish update_client::execute

* remove TODO

* implement mock client state

* fix mockclientstate

* fix event created on update_client

* fix mock

* fix mock

* test var names

* fix parts of the problem with test

* fix test

* clippy

* remove old methods

* clean Misbehaviour::new()

* cleanup unused

* move `update_client` method to submodule

* move misbehaviour to submodule

* move implementation to function

* move implementation to function

* changelog

* add clarifying comment

* fmt

* Update crates/ibc/src/clients/ics07_tendermint/client_state/misbehaviour.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>

* Update crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>

* Update crates/ibc/src/clients/ics07_tendermint/client_state/misbehaviour.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>

* fix misbehaviour ctor

* improve documentation around `UpdateKind`

* fmt

* remove unused misbehaviour trait

* fix misbehaviour header checks

* Update crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Signed-off-by: Philippe Laferrière <plafer@protonmail.com>

* fix test after merge

---------

Signed-off-by: Philippe Laferrière <plafer@protonmail.com>
Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment