chore: remove `legacy_ibc_testing` folder #1003

tbruyelle · 2023-06-09T13:45:09Z

I apologize in advance for the length of this PR, but a thorough read will significantly aid the code review process.

Description

As indicated in the code comments of the legacy_ibc_testing folder, this folder was meant to be removed following the IBC upgrade. In the context of the IBC v7 upgrade, I decided to tackle this task.

The process was not as straightforward as anticipated, because the legacy_ibc_testing is not merely a copy of the ibc-go/testing folder. It contains numerous adaptations related to ICS. I managed to address all of them except one, which is why some tests still fail, all for the same reason (see the valset/header update order section for details).

Here is a list of changes, categorized into three sections for easier readability: Minor changes, Consumer genesis updates, Packet sniffing, and finally, Valset/header update order.

Minor changes

remove legacy_ibc_testing folder, replace all imports from github.com/cosmos/interchain-security/legacy_ibc_testing to github.com/cosmos/ibc-go/v7/testing.
ibctesting.NewTestChain no longer accepts the appIniter. This must be assigned to ibctesting.DefaultTestingAppInit before calling NewTestChain (see documentation).
endpoint.SendPacket API now requires packet data and timeout values instead of a packet (see related PR).
ibctesting.TestingApp.GetStakingKeeper() no longer gives access to UnbondingTime() method: replace with GetTestStakingKeeper().
replace ibcsimapp.CreateTestPubKeys by ibctesting.GenerateKeys
replace icstestingutil.ReconstructPacket by ibctesting.ParsePacketFromEvents()
ibctesting.GetChainID() now returns by default a chainID with a revision, which wasn't expected by the tests. Disable it by setting ibctesting.ChainIDSuffix = "".

Consumer genesis updates

In consumer chains, the validator set is not expected to be find in the staking module, but rather in the consumer module. This was previously handled in SetupWithGenesisValSet, which feeds the consumer genesis with the valset passed in parameters.

With the removal of legacy_ibc_testing, it's no longer possible to adapt SetupWithGenesisValSet in this way. Therefore, I adopted a different approach. I modified the consumer and consumer-democracy appIniter to accept a valset parameter (I introduced a valSetAppIniter), and they populate their genesis from this valset (since appIniter also returns the genesis).

When calling consumer appIniter, the provider valset is passed in parameters, while when calling consumer-democracy appIniter, the valset created in the test is passed.

Similarly, SetupWithGenesisValSet expects a staking module entry in the genesis, which is not the case for a consumer chain. This was previously handled by checking if a staking entry was present in the genesis (see here). For the same reason, I had to find a workaround, and so I also updated the consumer appIniter so its genesis contains a placeholder staking entry.

Packet sniffing

The legacy_ibc_testing was adapted to record all packets in a map field of TestChain (see here). This field doesn't exist in the ibc-go/v7/testing, so I updated the CCVTestSuite struct to add a StreamingService implementation called packetSniffer to all created chains. The packetSniffer listens to EndBlock and appends all packets parsed from events.

Valset/header update order

In legacy_ibc_testing, the TestChain.NextBlock looks like that :

func (chain *TestChain) NextBlock() {
	res := chain.App.EndBlock(abci.RequestEndBlock{Height: chain.CurrentHeader.Height})

	chain.App.Commit()

	// val set changes returned from previous block get applied to the next validators
	// of this block. See tendermint spec for details.
	chain.Vals = chain.NextVals
	chain.NextVals = ApplyValSetChanges(chain.TB, chain.Vals, res.ValidatorUpdates)

	// set the last header to the current header
	// use nil trusted fields
	chain.LastHeader = chain.CurrentTMClientHeader()

In ibc-go/v7/testing, it's a little different :

func (chain *TestChain) NextBlock() {
	res := chain.App.EndBlock(abci.RequestEndBlock{Height: chain.CurrentHeader.Height})

	chain.App.Commit()

	// set the last header to the current header
	// use nil trusted fields
	chain.LastHeader = chain.CurrentTMClientHeader()

	// val set changes returned from previous block get applied to the next validators
	// of this block. See tendermint spec for details.
	chain.Vals = chain.NextVals
	chain.NextVals = ApplyValSetChanges(chain.TB, chain.Vals, res.ValidatorUpdates)

The validators are updated after the chain.LastHeader update, which makes the UpdateClient call fail, because the trusted validators hash doesn't match the consensus state next validators hash (can't give more explanations because I lack knowledge on IBC).

If we change the order like it was in legacy_ibc_testing the UpdateClient works well.

For this one I wasn't able to find a workaround, probably a PR should be submitted to ibc-go to change that order. I ran the ibc-go tests with that change and there's still green, so that looks fine. Or else maybe someone can find a workaround ?

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

- Ensure a minimal staking module is present - Ensure the consumer module contains the provider valset

MSalopek · 2023-06-12T13:27:56Z

@tbruyelle Thank you for your contribution.

As you have noted above, legacy_ibc_testing is not really "legacy" since there are useful changes to ibc/testing behaviour.

I will look further into this, but as far as I can tell, we will need to port over some of the changes in behaviour.

Do you mind if we merge this after we get feat/upgrade-ics-sdk47-ibc7 branch into main?

The reason being is that we still need to test if v47 integrates with main (we need a working test suite for that) and we will probably need to keep some of the changes to ibc/testing but rename the legacy_ibc_testing to something less confusing.

tbruyelle · 2023-06-12T16:57:47Z

Do you mind if we merge this after we get feat/upgrade-ics-sdk47-ibc7 branch into main?

@MSalopek no problem, because this PR is not merge-able until this diff is applied to ibc-go :

diff --git a/testing/chain.go b/testing/chain.go
index 928941d4..c4f21507 100644
--- a/testing/chain.go
+++ b/testing/chain.go
@@ -279,15 +279,15 @@ func (chain *TestChain) NextBlock() {
 
 	chain.App.Commit()
 
-	// set the last header to the current header
-	// use nil trusted fields
-	chain.LastHeader = chain.CurrentTMClientHeader()
-
 	// val set changes returned from previous block get applied to the next validators
 	// of this block. See tendermint spec for details.
 	chain.Vals = chain.NextVals
 	chain.NextVals = ApplyValSetChanges(chain.TB, chain.Vals, res.ValidatorUpdates)
 
+	// set the last header to the current header
+	// use nil trusted fields
+	chain.LastHeader = chain.CurrentTMClientHeader()
+
 	// increment the current header
 	chain.CurrentHeader = tmproto.Header{
 		ChainID: chain.ChainID,

shaspitz · 2023-06-13T20:52:38Z

@tbruyelle awesome stuff! This is a change I've been looking forward to for a while. This will be a draft PR for now

tbruyelle · 2023-06-13T20:59:37Z

@tbruyelle awesome stuff! This is a change I've been looking forward to for a while. This will be a draft PR for now

Thank you <3
Yes it's definitively draft, I forgot to set it 🙏

faddat · 2023-06-25T04:21:55Z

@tbruyelle have you had a chance to make a PR to ibc-go?

I agree that this is a great change!

tbruyelle · 2023-06-26T08:45:20Z

@tbruyelle have you had a chance to make a PR to ibc-go?

I agree that this is a great change!

No, I wait for approval here, then I'll submit the change to ibc-go.
I don't think it's a good idea to submit such change to ibc-go without the support of the ICS core team.

tbruyelle · 2023-08-04T12:33:20Z

@MSalopek closed in favor of the rebased version #1185

tbruyelle added 14 commits May 31, 2023 11:26

wip remove legacy_ibc_testing

a1fa0e2

use ibctesting.DefaultTestingAppInit

de1fbab

use latest SendPacket API

1177115

fix access to StakingKeeper.UnbondingTime

6660142

replace ibcsimapp.CreateTestPubKeys by ibctesting.GenerateKeys

f709e6e

wip: attempt to replace SentPackets

723f04c

replace ReconstructPacket by ibctesting one

338696c

fix testing consumer genesis

f31ea60

- Ensure a minimal staking module is present - Ensure the consumer module contains the provider valset

Disable ibctesting revision format

e28ae8c

fix access to sent packets

5857807

fix democracy genesis

42ab6d2

use latest SendPacket API

acca623

fix AddDemocracyConsumer with non-nil valset

429caf9

fix tests/differences

f06495e

tbruyelle requested a review from a team as a code owner June 9, 2023 13:45

remove TODO

ae4a979

MSalopek added scope: testing Code review, testing, making sure the code is following the specification. type: refactoring Code refactoring labels Jun 12, 2023

MSalopek linked an issue Jun 12, 2023 that may be closed by this pull request

Refactor or remove ibc_legacy_testing from ICS #1008

Closed

shaspitz marked this pull request as draft June 13, 2023 20:52

tbruyelle mentioned this pull request Aug 4, 2023

chore: remove legacy_ibc_testing (rebased on main) #1185

Merged

20 tasks

tbruyelle closed this Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove `legacy_ibc_testing` folder #1003

chore: remove `legacy_ibc_testing` folder #1003

tbruyelle commented Jun 9, 2023 •

edited

Loading

MSalopek commented Jun 12, 2023

tbruyelle commented Jun 12, 2023

shaspitz commented Jun 13, 2023

tbruyelle commented Jun 13, 2023

faddat commented Jun 25, 2023

tbruyelle commented Jun 26, 2023

tbruyelle commented Aug 4, 2023

chore: remove legacy_ibc_testing folder #1003

chore: remove legacy_ibc_testing folder #1003

Conversation

tbruyelle commented Jun 9, 2023 • edited Loading

Description

Minor changes

Consumer genesis updates

Packet sniffing

Valset/header update order

Author Checklist

Reviewers Checklist

MSalopek commented Jun 12, 2023

tbruyelle commented Jun 12, 2023

shaspitz commented Jun 13, 2023

tbruyelle commented Jun 13, 2023

faddat commented Jun 25, 2023

tbruyelle commented Jun 26, 2023

tbruyelle commented Aug 4, 2023

chore: remove `legacy_ibc_testing` folder #1003

chore: remove `legacy_ibc_testing` folder #1003

tbruyelle commented Jun 9, 2023 •

edited

Loading