ICS4: Fix Sequencing logic in UpgradeTry

[AdityaSripal]

Description

While writing quint tests against the spec I noticed a few issues surrounding our sequencing logic. This PR fixes them.

Issues:

• Unexpected Behaviour: UpgradeTry persists state writes after returning errorReceipt. This will cause the TRY chain to (for example) initialize with the upgrade attempt of an out-of-date counterparty that it wants to abort
• Inefficiency: In the crossing hello case, we do not fastforward the sequence. This will create an issue in the following circumstance. Suppose two sides have the exact same upgrade but different sequences, our current logic will fail to allow the upgrade to proceed. Instead, we just move the fastforwarding logic outside the crossing hello check and do it regardless so long as the upgrades are compatible.

See the following issues on spec:
cosmos/ibc#1052
cosmos/ibc#1053
cosmos/ibc#1054

This PR on the spec address the issues:
cosmos/ibc#1055

Unit tests to write:

(this is already written)
Case 1: ChainA(Sequence: 1), ChainB(Sequence 5)
ChainA.INIT
ChainB.TRY(ERROR_RECEIPT(5))
ChainA.Cancel(5)
ChainA.INIT(6)
ChainB.TRY(6)
...

Case 2: Equal sequences should work. I believe this is already written

Case 3: ChainA(Sequence: 5), ChainB(Sequence: 1)
ChainA.INIT
ChainB.TRY // fastforward to 5
...

// CROSSING HELLO CHECK

Case 4: ChainA(Sequence: 1), ChainB(Sequence 5)
ChainA.INIT
ChainB.INIT
ChainB.TRY(ErrorReceipt: 4)
ChainA.Cancel(4)
ChainA.TRY(5)
...

Case 5: ChainA(Sequence: 1), ChainB(Sequence 5)
ChainA.INIT
ChainB.INIT
ChainA.TRY(5) // fastforward
ChainB.TRY(ErrorReceipt: 4)
ChainA.Cancel => Error
ChainB.Ack => Success
...

// This test may already be written
Case 6: Crossing hello with the same sequence and upgrade should work fine.

// INCOMPATIBLE CROSSING HELLO

Case 7: ChainA(Sequence: 1, ics20-2), ChainB(Sequence: 5, ics20-3)
ChainA.INIT
ChainB.INIT
ChainA.TRY => error
ChainB.TRY(ErrorReceipt: 4)
ChainA.Cancel(4)
ChainA.TRY(5)
....

Case 8: ChainA(Sequence: 1, ics20-2), ChainB(Sequence: 1, ics20-3)
ChainA.INIT
ChainB.INIT
ChainA.TRY => error
ChainB.TRY => error
ChainA.INIT(2, ics20-3)
ChainB.TRY(2, ics20-3)
....

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

---

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 and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/) or specification (x/<module>/spec/).
• Added relevant godoc comments.
• Provide a commit message to be used for the changelog entry in the PR description for review.
• Re-reviewed Files changed in the Github PR explorer.
• Review Codecov Report in the comment section below once CI passes.

[crodriguezvega]

The following test should cover some of the cases mentioned:

• Case 1 by TestChanUpgrade_UpgradeSucceeds_AfterCancel.
• Case 2 by success of TestChanUpgradeTry.
• Case 3 by success: upgrade sequence is fast forwarded to counterparty upgrade sequence of TestChanUpgradeTry.

And then case 6 by TestChanUpgradeCrossingHelloWithHistoricalProofs.

And I added tests for cases 4, 5 7, and 8.

[crodriguezvega]

I get "account sequence mismatch, expected 14, got 13: incorrect account sequence" in of the tests, if anybody can figure out why, that would be amazing. :)

[damiannolan]

The account sequence mismatch error is arising because we only increment the account sequence in the testing pkg if the tx goes through successfully.

In the test in which its failing, we call the upgrade cancel handler, and expect an error, but later try to send another msg to the chain with a stale signing sequence.

https://github.com/cosmos/ibc-go/blob/main/testing/chain.go#L373-L377

edit: we could probably wrap the above code in a defer to ensure that the account sequence is incremented when the func returns, whether there is an error present or not.

edit2: I modified the testing pkg code to use the defer and the test then passes!

[crodriguezvega]

Thank you for finding the problem, @damiannolan! Would you like to push the fix?

[damiannolan]

ACK, looks good to me, happy to move forward with this! LGTM

I can cherry-pick the commit to main for TestChain.SendMsgs() if needs be, but I think its probably fine to keep it in the feat branch and just let it get merged through there!

[damiannolan]

Its intensional that the channel and upgrade are returned here instead of empty structs, right?

This is the only place where an upgrade error is return from the try handler and it looks like the channel returned is used for emitting error receipt events in this case:
https://github.com/cosmos/ibc-go/blob/04-channel-upgrades/modules/core/keeper/msg_server.go#L818-L820

[DimitrisJim]

lgtm, cov is also clean, amazing! 💯

[DimitrisJim]

delicious documentation @AdityaSripal, very helpful

[DimitrisJim]

great find @damiannolan 👀

[damiannolan]

Added more assertions to tests and wrapped steps in suite.Run to try make the flow as clear as possible 👍🏻