test: add UpgradeChain and UpgradeClient endpoint test functions #1169

colin-axner · 2022-03-24T13:37:27Z

Description

Adds:

UpgradeChain
UpgradeClient

UpgradeChain is a helper function to test upgrading a chain to a different revision number. It will change the testing chain's revision number and set the associated client/consensus states in state. It does not use upgrade plan's, though we can modify this behaviour in the future

UpgradeClient submits a MsgUpgradeClient using the upgraded client and consensus states set on the counterparty chain

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

add UpgradeChain to upgrade a testing chain to a new revision number add UpgradeClient to submit a MsgUpgradeClient to the client associated with an endpoint

seantking · 2022-03-24T13:38:50Z

testing/endpoint.go

@@ -492,6 +493,85 @@ func (endpoint *Endpoint) TimeoutOnClose(packet channeltypes.Packet) error {
 	return endpoint.Chain.sendMsgs(timeoutOnCloseMsg)
 }

+// UpgradeChain performs a IBC client upgrade using the provided client state.


mega nit: an IBC

colin-axner · 2022-03-24T13:39:51Z

testing/endpoint.go

+	if endpoint.Counterparty.ClientID != "" {
+
+	}
+


whoops, forgot to add endpoint.UpgradeClient call

…tion

damiannolan

Nice! Super useful, just left one question :)

damiannolan · 2022-03-24T13:43:48Z

testing/endpoint.go

+	endpoint.Chain.GetSimApp().UpgradeKeeper.SetUpgradedClient(endpoint.Chain.GetContext(), endpoint.Chain.GetContext().BlockHeight(), bz)
+
+	// set upgraded consensus state
+	consensusState := &ibctmtypes.ConsensusState{


Does this need to include a root/app hash?

Nope, the consensus state is set for the next block to be produced under the new chain id so the root hash cannot be known before hand

seantking · 2022-03-24T13:45:25Z

testing/endpoint.go

+// UpgradeChain performs a IBC client upgrade using the provided client state.
+// The chainID within the client state will have its revision number incremented by 1.
+// The counterparty client will be upgraded if it exists.
+func (endpoint *Endpoint) UpgradeChain(clientState *ibctmtypes.ClientState) error {


It might be useful to pass revision number as a parameter rather than incrementing 🤔

colin-axner · 2022-03-24T14:20:31Z

CHANGELOG.md

@@ -44,6 +44,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

 ### Improvements

+* (testing) [\#1169](https://github.com/cosmos/ibc-go/pull/1169) Add `UpgradeChain` and `UpgradeClient` helper functions to the testing `Endpoint` structure.


todo, update changelog entry

…rade panic

…s/ibc-go into colin/upgrade-testing-helper-funcs

colin-axner · 2022-03-30T21:50:54Z

It took me a while, but I mostly understand the issue here. It is very nuanced.

ref https://github.com/cosmos/cosmos-sdk/pull/8187/files#discussion_r545757275

Note: chain.App.Commit() != a commit to a block

So tendermint process is:

vote
precommit (checkTx happening somewhere around here)
commit to block (height x)
beginblock (height x)
delivertx
deilvertx
delivertx
endblock
app.Commit

the testing package does things out of order

begin (height x)
deliver
end
app.Commit 
commit to block (height x)
begin block (height x + 1)

in most situations it doesn't matter because we never verify functionality between commit to block and beginblock - that is until now.

Normally I'd fix this issue in my specific instance by actually doing tm process for the upgrade
That is

begin block (upgradeHeight - 1)
app.Commit()
// swap to tm process
commit to block height x
// perform client upgrades

The problem is app.Commit() wipes the deliverState (which is beginblock is always called first). Every call to GetContext() panics (since GetContext() says to create a context for a deliver tx)

In other words, the testing package in order to be useful needs to be able to modify state during block execution (beginblock must be called).

We could be more explicit with checktx vs delivertx usage for context (all queries use checktx)

Quickly modifying my code to do this, I still run into

failed to execute message; message index: 0: cannot upgrade client with ID 07-tendermint-0: client state proof failed. Path: /upgrade/upgradedIBCState/6/upgradedClient: proof did not commit to expected root: 6DF8EF50366AA416EF9A8A1676D5CABFC499499C18D143B9CE9EC29538EDD1E6, got: 565FD18B44E72D4CE76E18B3995569DC3C913B5DC6BE4DB37DE60E09086C92A1. Please ensure proof was submitted with correct proofHeight and to the correct chain.: invalid proof

Edit: I got the tests to pass

colin-axner · 2022-03-31T11:03:38Z

I fixed the tests, but it currently requires a few hacks:

mocking behaviour of SendMsgs minus call to begin block on all chains
usage of CheckTx Context after app.Commit but before BeginBlock

I think it might be best to change the testing package to more elegantly mock the TM process before merging these changes, but I think this pr is very useful in understanding how things work.

In the short term, on the 02-client-refactor branch we can add a testing helper function which simply sets the lastest client/consensus state to be after the upgrade (bypassing the actual verification of such an upgrade)

test: add UpgradeChain and UpgradeClient helper test functions

7c84b1e

add UpgradeChain to upgrade a testing chain to a new revision number add UpgradeClient to submit a MsgUpgradeClient to the client associated with an endpoint

colin-axner requested review from AdityaSripal, crodriguezvega, seantking, charleenfei and damiannolan as code owners March 24, 2022 13:37

seantking reviewed Mar 24, 2022

View reviewed changes

add changelog entry

7d0af37

colin-axner commented Mar 24, 2022

View reviewed changes

add call to upgrade counterparty client from within UpgradeChain func…

736f6a7

…tion

damiannolan approved these changes Mar 24, 2022

View reviewed changes

seantking reviewed Mar 24, 2022

View reviewed changes

colin-axner marked this pull request as draft March 24, 2022 14:13

restructure endpoint functions

8d55925

colin-axner commented Mar 24, 2022

View reviewed changes

seantking mentioned this pull request Mar 25, 2022

02-client refactor: Adding VerifyClientMessage helper fn #1119

Merged

13 tasks

Merge branch 'main' into colin/upgrade-testing-helper-funcs

44c52a3

colin-axner added the 02-client label Mar 29, 2022

colin-axner and others added 4 commits March 30, 2022 12:26

refactor UpgradeChain to schedule and upgrade plan and handle the upg…

3af455e

…rade panic

fix proof of non existence bug

12e0491

Merge branch 'colin/upgrade-testing-helper-funcs' of github.com:cosmo…

89b09e2

…s/ibc-go into colin/upgrade-testing-helper-funcs

height issue

4002fc1

fix tests, pause work

c5eeb20

colin-axner closed this Apr 26, 2022

colin-axner mentioned this pull request May 18, 2022

Emit event upon setting upgrade consensus state #270

Closed

3 tasks

colin-axner deleted the colin/upgrade-testing-helper-funcs branch August 3, 2022 13:31

colin-axner mentioned this pull request Mar 21, 2023

Write ADR for IBC testing package++ #1265

Closed

3 tasks

colin-axner mentioned this pull request May 18, 2023

testing/ - Fix chain.LastHeader callsite #1668

Closed

colin-axner mentioned this pull request Jul 3, 2023

Add performABCIHandler to testing pkg to replace internal logic of SendMsgs/NextBlock #3997

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add UpgradeChain and UpgradeClient endpoint test functions #1169

test: add UpgradeChain and UpgradeClient endpoint test functions #1169

colin-axner commented Mar 24, 2022

seantking Mar 24, 2022 •

edited

Loading

colin-axner Mar 24, 2022

damiannolan left a comment

damiannolan Mar 24, 2022 •

edited

Loading

colin-axner Mar 24, 2022

seantking Mar 24, 2022 •

edited

Loading

colin-axner Mar 24, 2022

colin-axner commented Mar 30, 2022 •

edited

Loading

colin-axner commented Mar 31, 2022

		@@ -44,6 +44,8 @@ Ref: https://keepachangelog.com/en/1.0.0/

		### Improvements

		* (testing) [\#1169](https://github.com/cosmos/ibc-go/pull/1169) Add `UpgradeChain` and `UpgradeClient` helper functions to the testing `Endpoint` structure.

test: add UpgradeChain and UpgradeClient endpoint test functions #1169

test: add UpgradeChain and UpgradeClient endpoint test functions #1169

Conversation

colin-axner commented Mar 24, 2022

Description

seantking Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

colin-axner Mar 24, 2022

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

colin-axner Mar 24, 2022

Choose a reason for hiding this comment

seantking Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

colin-axner Mar 24, 2022

Choose a reason for hiding this comment

colin-axner commented Mar 30, 2022 • edited Loading

colin-axner commented Mar 31, 2022

seantking Mar 24, 2022 •

edited

Loading

damiannolan Mar 24, 2022 •

edited

Loading

seantking Mar 24, 2022 •

edited

Loading

colin-axner commented Mar 30, 2022 •

edited

Loading