Implement `verify_upgrade_and_update_state` for Tendermint Client #349

Farhad-Shabani · 2023-01-13T15:31:36Z

Closes: #19

Summary

This PR handles unimplemented verify_upgrade_and_update_state method for Tendermint clients.

Remarks

verify_upgrade_and_update_state has been kept the same as IBC-Go's so far, but the processes it perform do not match the ongoing ADR05 change and requires both updating and verifying in one operation. Here, it's broken down into verify_upgrade_client and update_state_with_upgrade_client.
Similarly, there are some other methods within ClientState trait like new_check_header_and_update_state that can be refactored to have clearer distinction. In particular, when it comes to implement pub(crate) fn validate() and pub(crate) fn execute() functions.
upgrade_client message tests are done with MockContext, which does not cover every aspect of this change. The main purpose of this handler is to serve Tendermint clients. A separate PR or this could be covered here ?!

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 · 2023-01-13T21:36:00Z

Codecov Report

Base: 58.77% // Head: 58.45% // Decreases project coverage by -0.32% ⚠️

Coverage data is based on head (3d76bfc) compared to base (0605dd3).
Patch coverage: 49.21% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
- Coverage   58.77%   58.45%   -0.32%     
==========================================
  Files         124      124              
  Lines       16111    16259     +148     
==========================================
+ Hits         9470     9505      +35     
- Misses       6641     6754     +113

Impacted Files	Coverage Δ
...s/ibc/src/clients/ics07_tendermint/client_state.rs	`40.55% <0.00%> (-2.83%)`	⬇️
crates/ibc/src/core/ics02_client/client_state.rs	`75.00% <ø> (ø)`
crates/ibc/src/core/ics02_client/error.rs	`4.76% <ø> (ø)`
...s/ibc/src/core/ics02_client/msgs/upgrade_client.rs	`84.53% <ø> (ø)`
crates/ibc/src/core/ics26_routing/handler.rs	`91.11% <ø> (ø)`
...bc/src/core/ics02_client/handler/upgrade_client.rs	`64.17% <70.00%> (-9.20%)`	⬇️
crates/ibc/src/mock/client_state.rs	`75.30% <95.83%> (+2.60%)`	⬆️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

crates/ibc/src/clients/ics07_tendermint/client_state.rs

plafer · 2023-01-31T16:27:17Z

crates/ibc/src/clients/ics07_tendermint/client_state.rs

+        let mut upgrade_path = upgraded_tm_client_state.clone().upgrade_path;
+        if upgrade_path.pop().is_none() {
+            return Err(ClientError::ClientSpecific {
+                description: "cannot upgrade client as no upgrade path has been set".to_string(),
+            });
+        };
+
+        let last_height = self.latest_height().revision_height();
+
+        // Construct the merkle path for the client state
+        let mut client_upgrade_path = upgrade_path.clone();
+        client_upgrade_path.push(ClientUpgradePath::UpgradedClientState(last_height).to_string());
+
+        let client_upgrade_merkle_path = MerklePath {
+            key_path: client_upgrade_path,
+        };


I'm concerned that we're not building the right path, comparing with ibc-go. Can you confirm/disconfirm this?

Quite understandable. Do you think we can keep this checked with issue #385?

If were that unsure about the current implementation, we should then put it behind a feature flag (similar to val_exec_ctx), which will allow us to test it with basecoin-rs, and at the same time, signal to users that this feature shouldn't be used yet.

crates/ibc/src/clients/ics07_tendermint/client_state.rs

plafer · 2023-01-31T16:44:55Z

crates/ibc/src/core/ics02_client/client_state.rs

-        upgrade_options: &dyn UpgradeOptions,
-        chain_id: ChainId,
-    );
+    fn zero_custom_fields(&mut self);


Assuming we need this (see previous comments), I don't think this should be in the ICS-2 interface. Can't it just be a private function in the tendermint client module?

Considering that in any chain and client state struct there will be some relayer customizable fields that are needed to be zeroed out for the upgrade process. I think it makes sense to keep it here

crates/ibc/src/core/ics02_client/handler/upgrade_client.rs

Signed-off-by: Farhad Shabani <[email protected]>

plafer · 2023-01-31T22:47:42Z

crates/ibc/Cargo.toml

@@ -17,7 +17,7 @@ description  = """
 all-features = true

 [features]
-default = ["std"]
+default = ["std", "disable_upgrade_client"]


we should instead have the feature be upgraded_client, and if enabled, we provide the unfinished implementation. This is a subtle but necessary change: no_std users will build with default-features=false, which would "enable" the upgrade client code.

Shouldn't be a big change; feature checks just need to be flipped

cfg!(not(feature = "upgrade_client"))

Done 83de030

plafer · 2023-01-31T22:49:08Z

crates/ibc/Cargo.toml

@@ -45,6 +45,9 @@ serde = ["dep:serde", "dep:serde_derive", "serde_json", "erased-serde"]
 # This feature guards the unfinished implementation of ADR 5.
 val_exec_ctx = []

+# This feature disables the upgrade client handler and return an error when a client upgrade is attempted.


Suggested change

# This feature disables the upgrade client handler and return an error when a client upgrade is attempted.

# This feature guards the unfinished implementation of the `UpgradeClient` handler.

Let's make it clear that this is not a feature users should be using.

plafer

👌

* Implement verify_upgrade_and_update_state * Construct new_client_state and new_consensus_state * Split off verification and execution steps * Revise some namings and comments * Comment out some upgrade tests * Update mock tests related to ugrade_client * Add changelog entry * Refactor verify_upgrade_client to use Path and Encode error type * Remove pub before UPGRADE const * Rewrite upgrade_client unit tests * Add unbonding period validation step * Set root of new consensus state with sentinel value * Revise some comments * Refactor upgrade method to zero_custom_fields * Mend fields of new client state * Disable upgrade client handler * Fix issue with cargo test * Flip upgrade_client feature flag * Remove unnecessary unbonding period check --------- Signed-off-by: Farhad Shabani <[email protected]>

Farhad-Shabani added 5 commits January 12, 2023 23:37

Implement verify_upgrade_and_update_state

9061610

Construct new_client_state and new_consensus_state

73e87c1

Split off verification and execution steps

d63fa4d

Revise some namings and comments

5b04088

Comment out some upgrade tests

869d8de

Farhad-Shabani added 3 commits January 13, 2023 14:06

Update mock tests related to ugrade_client

025a366

Merge branch 'main' into farhad/verify-upgrade-update-state

c968ee7

Add changelog entry

5f51c7c

Farhad-Shabani requested a review from plafer January 13, 2023 22:37

Farhad-Shabani marked this pull request as ready for review January 13, 2023 22:37

Farhad-Shabani added 3 commits January 16, 2023 17:02

Refactor verify_upgrade_client to use Path and Encode error type

a5b3f05

Remove pub before UPGRADE const

d0c050e

Rewrite upgrade_client unit tests

5dcf609

Farhad-Shabani self-assigned this Jan 17, 2023

plafer reviewed Jan 17, 2023

View reviewed changes

crates/ibc/src/clients/ics07_tendermint/client_state.rs Outdated Show resolved Hide resolved

Farhad-Shabani added 4 commits January 26, 2023 21:33

Add unbonding period validation step

8ebf323

Set root of new consensus state with sentinel value

9ea805a

Revise some comments

4155c75

Refactor upgrade method to zero_custom_fields

a9b3198

Farhad-Shabani mentioned this pull request Jan 27, 2023

Validate "upgrade_client" implementation by adding unit and integration tests #385

Closed

3 tasks

plafer reviewed Jan 31, 2023

View reviewed changes

Mend fields of new client state

5bc1a04

Farhad-Shabani added the O: ADR06 label Jan 31, 2023

Disable upgrade client handler

cba4c94

Farhad-Shabani mentioned this pull request Jan 31, 2023

Remaining concerns for upgrade_client implementation #391

Closed

Farhad-Shabani and others added 2 commits February 1, 2023 01:00

Merge branch 'main' into farhad/verify-upgrade-update-state

efff958

Signed-off-by: Farhad Shabani <[email protected]>

Fix issue with cargo test

355cb1d

plafer suggested changes Jan 31, 2023

View reviewed changes

Flip upgrade_client feature flag

83de030

Remove unnecessary unbonding period check

3d76bfc

plafer approved these changes Feb 1, 2023

View reviewed changes

plafer merged commit 1fb3547 into main Feb 1, 2023

plafer deleted the farhad/verify-upgrade-update-state branch February 1, 2023 17:58

Farhad-Shabani removed the O: ADR06 label Feb 2, 2023

Farhad-Shabani added this to the ADR06 milestone Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `verify_upgrade_and_update_state` for Tendermint Client #349

Implement `verify_upgrade_and_update_state` for Tendermint Client #349

Farhad-Shabani commented Jan 13, 2023 •

edited

Loading

codecov bot commented Jan 13, 2023 •

edited

Loading

plafer Jan 31, 2023

Farhad-Shabani Jan 31, 2023

plafer Jan 31, 2023

plafer Jan 31, 2023

Farhad-Shabani Jan 31, 2023

plafer Jan 31, 2023

Farhad-Shabani Jan 31, 2023

plafer Jan 31, 2023

Farhad-Shabani Jan 31, 2023

plafer left a comment

	# This feature disables the upgrade client handler and return an error when a client upgrade is attempted.
	# This feature guards the unfinished implementation of the `UpgradeClient` handler.

Implement verify_upgrade_and_update_state for Tendermint Client #349

Implement verify_upgrade_and_update_state for Tendermint Client #349

Conversation

Farhad-Shabani commented Jan 13, 2023 • edited Loading

Summary

Remarks

PR author checklist:

Reviewer checklist:

codecov bot commented Jan 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Implement `verify_upgrade_and_update_state` for Tendermint Client #349

Implement `verify_upgrade_and_update_state` for Tendermint Client #349

Farhad-Shabani commented Jan 13, 2023 •

edited

Loading

codecov bot commented Jan 13, 2023 •

edited

Loading