Upgrade to SDK v0.43+ #948

ancazamfir · 2021-05-15T07:41:55Z

Closes: #947
Closes: #918

Description

The proto files in this branch are currently aligned with:

ibc-go v1.0.0-alpha2: d70f49c8f612d60f1b7e2f1d1f160f28988962e1
cosmos-sdk v0.43.0-beta1: 8cfa2c26738276d895caf9eb98b3f70616218e17

For contributor use:

Updated the Unreleased section of CHANGELOG.md with the issue.
If applicable: Unit tests written, added test to CI.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments.
Re-reviewed Files changed in the Github PR explorer.

colin-axner · 2021-05-19T15:07:40Z

Did some testing using this branch between a gaia bin on v4.2.1 and the hub/cosmoshub-4.6 branch. Everything works great! I do get some "packet already received" errors when running hermes start-multi. See logs. Is this expected?

The main downside is lost fees for retrying these transactions

Edit: It works fine if I use gaiad to initiate the transfer. Must be related to hermes tx raw

ancazamfir · 2021-05-19T18:26:24Z

Did some testing using this branch between a gaia bin on v4.2.1 and the hub/cosmoshub-4.6 branch. Everything works great! I do get some "packet already received" errors when running hermes start-multi. See logs. Is this expected?

The main downside is lost fees for retrying these transactions

Edit: It works fine if I use gaiad to initiate the transfer. Must be related to hermes tx raw

This is not expected unless you had another relayer running in parallel. Could you give more detail of the setup? And the exact ft-transfer command you used? I use this command often and haven't seen this before.

colin-axner · 2021-05-20T11:34:32Z

I setup an old/new gaia using the gaiad manager. Created channels and sent tokens using ts-relayer. Then stopped the ts-relayer (I'm fairly certain)

Created channels using hermes, started hermes via start-multi. Sent using

hermes tx raw ft-transfer network2 network1 transfer channel-0 1 -o 1000 -n 1 -d localcoin

got the above logs.

Then sent using gaiad afterwards and it worked fine.

I will try to reproduce again later today

I realize that the transfer is being sent over channel-0. I created channel-0 with ts-relayer and channel-1 with hermes

colin-axner · 2021-05-20T13:53:56Z

I managed to reproduce. The problem scenario is when the ts-relayer is also running. Specifically:

start ts-relayer
send packet
wait for ts-relayer to log that it has relayed the packet
start hermes

Then hermes will actually beat the ts-relayer to the acknowledgement process but get stuck on trying to relay the packet

Does hermes rely on the ibc-go error codes at all? We are planning on making the error numbers as apart of our API for versioning so we don't change them often and when we do it is well documented. We can turn the unordered channel packet already received into an error code so that hermes could stop retries. Would this help?

ancazamfir · 2021-05-20T14:37:23Z

Does hermes rely on the ibc-go error codes at all? We are planning on making the error numbers as apart of our API for versioning so we don't change them often and when we do it is well documented. We can turn the unordered channel packet already received into an error code so that hermes could stop retries. Would this help?

Thanks @colin-axner ! We are working on better error filtering and definitely having the error code here will help.

BTW, this also happens with two hermes relayers running. We haven't really started doing these types of tests heavily and understand the issues. But it's coming.

colin-axner · 2021-05-21T12:09:15Z

opened a pr to update the error messages, also opened an issue on the spec repo

Please checkout the ibc-go pr and see if there is anything missing that'd be useful. The only scenario I could think of not covered is already timed out packets on ordered channels. This is tricky since by construction only 1 packet can be timed out at the moment (since the channel closes). Since ICS20 doesn't use ordered channels, I'd prefer to wait to fix until more than 1 packet can be timed out

ancazamfir · 2021-05-24T18:07:31Z

Opened #984 to check if the event has been previously handled by another relayer. This should reduce the number of retries to one. I tested with two hermes instances and the ft-transfer command.

Used 'merge master -X theirs' to favor master version. The problem was that there were numerous conflicts in the /proto crate, and wanted to avoid solving conflicts on a per-file basis. So I choose to favor master during the merge, and will re-generate the /proto files again.

…applications.transfer generated file

…s into adi/ibc-go-1-proto

adizere · 2021-08-12T11:10:53Z

Some context on this PR: Even if we build Hermes with these new proto files (SDK 0.43 and ibc-go v1), the relayer should still be able to submit chain upgrade proposals and upgrade clients with 0.42-based chains (gaiad 5.0.5) for instance.

~~So merging this PR should not break any CLI and we should retain backwards compat with 0.42-based chains. We need to test for that.~~

Update: Romain's tests reminded me that my statement above was wrong. Issue #1209 documents that we'll need an explicit --legacy flag to maintain backwards compatibility when it comes to the chain upgrade CLI.

Another thing is that we should update compatibility.rs to mark 0.43 as a compat. version.

romac · 2021-08-12T12:55:04Z

Running the following command using Hermes built from this branch and two v4.2.1 Gaia nodes setup via gm yields:

❯ hermes -c ~/.hermes/gm-config.toml tx raw upgrade-chain network1 network2 07-tendermint-0 10000000 3000

Aug 12 14:53:15.911  INFO Message UpgradePlanOptions { src_chain_config: ChainConfig { id: ChainId { id: "network2", version: 0 }, rpc_addr: Url { inner: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(27030), path: "/", query: None, fragment: None }, scheme: Http, host: "localhost", port: 27030 }, websocket_addr: Url { inner: Url { scheme: "ws", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(27030), path: "/websocket", query: None, fragment: None }, scheme: WebSocket, host: "localhost", port: 27030 }, grpc_addr: Url { inner: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(27032), path: "/", query: None, fragment: None }, scheme: Http, host: "localhost", port: 27032 }, rpc_timeout: 10s, account_prefix: "cosmos", key_name: "wallet", store_prefix: "ibc", max_gas: Some(500000), gas_adjustment: None, max_msg_num: MaxMsgNum(30), max_tx_size: MaxTxSize(2097152), clock_drift: 5s, trusting_period: 1209600s, trust_threshold: TrustThresholdFraction { numerator: 1, denominator: 3 }, gas_price: GasPrice { price: 0.001, denom: "stake" }, packet_filter: AllowAll }, dst_chain_config: ChainConfig { id: ChainId { id: "network1", version: 0 }, rpc_addr: Url { inner: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(27020), path: "/", query: None, fragment: None }, scheme: Http, host: "localhost", port: 27020 }, websocket_addr: Url { inner: Url { scheme: "ws", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(27020), path: "/websocket", query: None, fragment: None }, scheme: WebSocket, host: "localhost", port: 27020 }, grpc_addr: Url { inner: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(27022), path: "/", query: None, fragment: None }, scheme: Http, host: "localhost", port: 27022 }, rpc_timeout: 10s, account_prefix: "cosmos", key_name: "wallet", store_prefix: "ibc", max_gas: Some(500000), gas_adjustment: None, max_msg_num: MaxMsgNum(30), max_tx_size: MaxTxSize(2097152), clock_drift: 5s, trusting_period: 1209600s, trust_threshold: TrustThresholdFraction { numerator: 1, denominator: 3 }, gas_price: GasPrice { price: 0.001, denom: "stake" }, packet_filter: AllowAll }, src_client_id: ClientId("07-tendermint-0"), amount: 10000000, height_offset: 3000, upgraded_chain_id: ChainId { id: "network1", version: 0 }, upgraded_unbonding_period: None, upgrade_plan_name: "plan" }

Error:
   0: upgrade chain error
   1: tx response event consists of an error: deliver_tx on chain network1 for Tx hash 6FCF5B01B97296451F774BCD2DE8CEE47CA16E6F84CC3A6A5AE4F33249A1B05B reports error: code=Err(2), log=Log("unable to resolve type URL /ibc.core.client.v1.UpgradeProposal: tx parse error")

romac · 2021-08-12T12:58:40Z

The issue is likely that

SDK 0.42 expects a /cosmos.upgrade.v1beta1.SoftwareUpgradeProposal message
SDK 0.43 expects a /ibc.core.client.v1.UpgradeProposal message

UPDATE: This is already tracked in #1209 and will be addressed in a follow-up PR.

hu55a1n1 · 2021-08-16T07:55:35Z

I have tested this branch against all gaia versions v4.0.3 onwards, i.e. SDK version >=0.41.3. All E2E tests are passing.

* Add legacy upgrade support * Fix check for legacy * Update .changelog * Add TODO * Apply suggestion Co-authored-by: Adi Seredinschi <adi@informal.systems> Co-authored-by: Adi Seredinschi <adi@informal.systems>

adizere

Left a few ideas for improvements, none are blockers.

adizere · 2021-08-17T11:28:46Z

modules/src/application/mod.rs

+
+// TODO: These consts should move into the ICS27 namespace
+pub const ICS27_BANK_SEND_TYPE_URL: &str = "/cosmos.bank.v1beta1.MsgSend";
+pub const ICS27_SEND_TYPE_URL: &str = "/intertx.MsgSend";


FYI to share context: this is necessary for the Interchain Accounts module that @seantking is doing. Adding these lines is a temporary fix, so we'll need a permanent fix instead. Will open an issue for tracking that and detailing the problem.

Later edit: these constants should be deleted with PR https://github.com/informalsystems/ibc-rs/pull/1172/files.

proto-compiler/README.md

proto/src/prost/tendermint.crypto.rs

proto/src/prost/tendermint.p2p.rs

proto/src/prost/tendermint.version.rs

proto/src/prost/tendermint.types.rs

relayer/src/event/rpc.rs

…s into adi/ibc-go-1-proto

hu55a1n1 · 2021-08-18T09:39:11Z

Also ran the E2E tests on some heterogeneous setups ->

v4.0.3 <> colin-fee-event-testing (i.e. earliest v/s latest SDK versions supported)
v5.0.5 <> colin-fee-event-testing (latest release v/s latest supported)
v5.0.5 <> v4.0.3 (latest release v/s earliest supported)
v4.1.2 <> colin-fee-event-testing (possibly widely deployed)
v5.0.5 <> v5.0.2 (random)

All tests are passing. ✅

* Adapted proto compiler. Regenerated Rust files * Adapt to newer namespace for apps. * Manual fix for Staking erroneous compilation * Workaround: disabled chain upgrade * Added specific GRPC-web port to prevent conflict on 9091. * Upgrade protos * Add support for new message action strings * Improved output for one-chain script * CLI for chain upgrade proposal adapted to gaia v5. * Re-generated proto files with updated proto-compiler (fmt enabled). * Manual fix for staking::Validators erroneous compilation; added the .applications.transfer generated file * Quick feature for parametrizing upgraded chain * Clippy fix. Meta for params for better display. * Adapted query_upgraded_client_state * Adapted query_upgraded_consensus_state to ibc-go-v1 * Moved from gRPC to ABCI * Adapted send_tx_simulate to new def of SimulateRequest * Added parametrizable upgrade plan name * Fix unwraps. Retain unbonding period if none specified. * Missing post-merge bracket * Removed register and send match arms for rpc events * Support for new message.actions strings of ICS27 Based on discussion from cosmos/cosmos-sdk#9139 (comment) This is not yet tested! * Update protos sdk(v0.43.0-rc2) & ibc-go(v1.0.0-rc3) * Revert "Added specific GRPC-web port to prevent conflict on 9091." This reverts commit abf6dba. * Add missing fields in upgrade Plan * Fix clippy warnings * Added another action field to match intertx MsgSend * Update protos sdk(v0.43.0) & ibc-go(v1.0.0) * Update cosmos SDK module version requirement in compatibility.rs * Add legacy upgrade support (informalsystems#1289) * Add legacy upgrade support * Fix check for legacy * Update .changelog * Add TODO * Apply suggestion Co-authored-by: Adi Seredinschi <adi@informal.systems> Co-authored-by: Adi Seredinschi <adi@informal.systems> * Apply suggestions * Update .changelog * Fix .changelog entry Co-authored-by: Adi Seredinschi <adi@informal.systems> Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>

adizere and others added 8 commits May 10, 2021 16:29

Adapted proto compiler. Regenerated Rust files

f896b01

Adapt to newer namespace for apps.

33a0e19

Manual fix for Staking erroneous compilation

fc0eab5

Workaround: disabled chain upgrade

30688a4

Added specific GRPC-web port to prevent conflict on 9091.

abf6dba

Merge branch 'master' into adi/ibc-go-1-proto

2d1e51a

Upgrade protos

9c64b9f

Add support for new message action strings

b824869

ancazamfir mentioned this pull request May 15, 2021

Event parsing message.action preparation for new SDK version #918

Closed

5 tasks

adizere added 3 commits May 17, 2021 11:31

Improved output for one-chain script

0e9bdb8

Merge branch 'master' into adi/ibc-go-1-proto

c06fc0c

CLI for chain upgrade proposal adapted to gaia v5.

d4ec4a8

adizere added the backburner label May 19, 2021

colin-axner mentioned this pull request May 20, 2021

improve error message for identifier validation cosmos/ibc-go#152

Open

5 tasks

adizere added 9 commits June 11, 2021 11:38

Re-generated proto files with updated proto-compiler (fmt enabled).

b32e28b

Manual fix for staking::Validators erroneous compilation; added the .…

261e5ec

…applications.transfer generated file

Quick feature for parametrizing upgraded chain

03abf7a

Clippy fix. Meta for params for better display.

8592959

Merge branch 'master' into adi/ibc-go-1-proto

f103b96

Adapted query_upgraded_client_state

c306530

Adapted query_upgraded_consensus_state to ibc-go-v1

16228aa

Moved from gRPC to ABCI

75f3fc2

adizere and others added 3 commits August 10, 2021 10:54

Added another action field to match intertx MsgSend

97360a0

Merge branch 'adi/ibc-go-1-proto' of github.com:informalsystems/ibc-r…

e173d74

…s into adi/ibc-go-1-proto

Update protos sdk(v0.43.0) & ibc-go(v1.0.0)

bd8cdb4

adizere marked this pull request as ready for review August 12, 2021 11:06

adizere requested a review from romac as a code owner August 12, 2021 11:06

Update cosmos SDK module version requirement in compatibility.rs

ac7f92d

romac self-assigned this Aug 12, 2021

hu55a1n1 added 2 commits August 13, 2021 14:45

Merge branch 'master' into adi/ibc-go-1-proto

a44193d

Merge branch 'master' into adi/ibc-go-1-proto

7e38aff

This was referenced Aug 16, 2021

Release 0.7 requirements #1279

Closed

Ethermint support #1295

Merged

Add legacy upgrade support (#1289)

4bf511c

* Add legacy upgrade support * Fix check for legacy * Update .changelog * Add TODO * Apply suggestion Co-authored-by: Adi Seredinschi <adi@informal.systems> Co-authored-by: Adi Seredinschi <adi@informal.systems>

adizere reviewed Aug 17, 2021

View reviewed changes

adizere mentioned this pull request Aug 17, 2021

Get events from RPC event's data instead of events #1172

Merged

6 tasks

hu55a1n1 added 4 commits August 18, 2021 03:53

Merge branch 'adi/ibc-go-1-proto' of github.com:informalsystems/ibc-r…

1b32a0c

…s into adi/ibc-go-1-proto

Apply suggestions

abca556

Update .changelog

147cc35

Fix .changelog entry

61642a5

romac mentioned this pull request Aug 18, 2021

Release Hermes v0.7.0 #1296

Merged

10 tasks

romac approved these changes Aug 19, 2021

View reviewed changes

romac merged commit c4b6e30 into master Aug 19, 2021

romac deleted the adi/ibc-go-1-proto branch August 19, 2021 13:26

ancazamfir mentioned this pull request Oct 4, 2021

Fix tx simulation for chains based on Cosmos SDK 0.42.x #1405

Merged

5 tasks

adizere removed the needs guide update label Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to SDK v0.43+ #948

Upgrade to SDK v0.43+ #948

ancazamfir commented May 15, 2021 •

edited by hu55a1n1

Loading

colin-axner commented May 19, 2021 •

edited

Loading

ancazamfir commented May 19, 2021 •

edited

Loading

colin-axner commented May 20, 2021 •

edited

Loading

colin-axner commented May 20, 2021

ancazamfir commented May 20, 2021

colin-axner commented May 21, 2021

ancazamfir commented May 24, 2021

adizere commented Aug 12, 2021 •

edited

Loading

romac commented Aug 12, 2021 •

edited

Loading

romac commented Aug 12, 2021 •

edited

Loading

hu55a1n1 commented Aug 16, 2021

adizere left a comment

adizere Aug 17, 2021

adizere Aug 17, 2021

hu55a1n1 commented Aug 18, 2021 •

edited

Loading

Upgrade to SDK v0.43+ #948

Upgrade to SDK v0.43+ #948

Conversation

ancazamfir commented May 15, 2021 • edited by hu55a1n1 Loading

Description

colin-axner commented May 19, 2021 • edited Loading

ancazamfir commented May 19, 2021 • edited Loading

colin-axner commented May 20, 2021 • edited Loading

colin-axner commented May 20, 2021

ancazamfir commented May 20, 2021

colin-axner commented May 21, 2021

ancazamfir commented May 24, 2021

adizere commented Aug 12, 2021 • edited Loading

romac commented Aug 12, 2021 • edited Loading

romac commented Aug 12, 2021 • edited Loading

hu55a1n1 commented Aug 16, 2021

adizere left a comment

Choose a reason for hiding this comment

adizere Aug 17, 2021

Choose a reason for hiding this comment

adizere Aug 17, 2021

Choose a reason for hiding this comment

hu55a1n1 commented Aug 18, 2021 • edited Loading

ancazamfir commented May 15, 2021 •

edited by hu55a1n1

Loading

colin-axner commented May 19, 2021 •

edited

Loading

ancazamfir commented May 19, 2021 •

edited

Loading

colin-axner commented May 20, 2021 •

edited

Loading

adizere commented Aug 12, 2021 •

edited

Loading

romac commented Aug 12, 2021 •

edited

Loading

romac commented Aug 12, 2021 •

edited

Loading

hu55a1n1 commented Aug 18, 2021 •

edited

Loading