CosmWasm / wasmd

Basic cosmos-sdk app with web assembly smart contracts
Other
364 stars 395 forks source link

Update to wasmvm 2.0.0-rc.1 #1804

Closed chipshort closed 6 months ago

chipshort commented 7 months ago

Very WIP, main goal is to get it to compile and pass current tests

TODOs:

chipshort commented 7 months ago

@webmaster128 Any idea why the CI docker-image libwasmvm check is failing? When I locally run (cd cmd/wasmd && go run . query wasm libwasmvm-version), I get 2.0.0-rc.1. Is the docker image an old version?

webmaster128 commented 7 months ago

@webmaster128 Any idea why the CI docker-image libwasmvm check is failing? When I locally run (cd cmd/wasmd && go run . query wasm libwasmvm-version), I get 2.0.0-rc.1. Is the docker image an old version?

I did not check the error yet but you need to update those lines for sure: https://github.com/CosmWasm/wasmd/blob/main/Dockerfile#L18-L21

webmaster128 commented 7 months ago

I think it would be valuable to do as many of the Probably in follow-up PRs items in here as well. This PR then serves as a go-to-place for every other user of wasmvm (which right now is primarily wasmd forks and the Wasm Light Client).

pinosu commented 7 months ago

Changes look good so far! To make the CI pass you need to update the Dockerfile both version and checksum for libwasmvm.

chipshort commented 6 months ago

@pinosu @webmaster128 Does one of you know the difference between the different reflect.wasm files and reflect_1_1.wasm used in tests? Because of the Stargate to Any rename, we either need to update them to the CW 2.0 version or we need to continue calling them with stargate json. Any opinions on that?

webmaster128 commented 6 months ago

I think it would be pretty cool to test both, the old (stargate) and the new (any) message type to ensure wasmd supports both. I don't know about the 1_1 one. it was added here: https://github.com/CosmWasm/wasmd/pull/1055/files

webmaster128 commented 6 months ago

Could you add the cosmwasm_2_0 capability to the README (on top of #1805)?

chipshort commented 6 months ago

I rebased on main and added the capability to the readme. I also fixed some more tests and added an updated reflect contract that is now used in most tests, as well as one test that uses both the old and new reflect contract for a Stargate / Any msg (TestGroupWithContract / TestGroupWithNewReflectContract). That should cover the backwards compatibility with the old Stargate variant.

To get the tests to pass, this PR in wasmvm is needed because the Payload is currently serialized incorrectly on the Go side. After that, the tests should pass and I can start working on the other new features.

chipshort commented 6 months ago

Tests are passing 🎉

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 62.50000% with 75 lines in your changes are missing coverage. Please review.

Project coverage is 54.89%. Comparing base (9e44af1) to head (62eaa6e). Report is 2 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804/graphs/tree.svg?width=650&height=150&src=pr&token=rxXgFH3QTf&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm)](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm) ```diff @@ Coverage Diff @@ ## main #1804 +/- ## ========================================== - Coverage 55.01% 54.89% -0.13% ========================================== Files 64 64 Lines 9663 9779 +116 ========================================== + Hits 5316 5368 +52 - Misses 3821 3865 +44 - Partials 526 546 +20 ``` | [Files](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm) | Coverage Δ | | |---|---|---| | [app/app.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-YXBwL2FwcC5nbw==) | `85.64% <100.00%> (-0.03%)` | :arrow_down: | | [app/wasm.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-YXBwL3dhc20uZ28=) | `100.00% <100.00%> (ø)` | | | [tests/e2e/reflect\_helper.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-dGVzdHMvZTJlL3JlZmxlY3RfaGVscGVyLmdv) | `100.00% <100.00%> (ø)` | | | [x/wasm/client/cli/gov\_tx.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2NsaWVudC9jbGkvZ292X3R4Lmdv) | `12.69% <ø> (ø)` | | | [x/wasm/client/cli/query.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2NsaWVudC9jbGkvcXVlcnkuZ28=) | `0.00% <ø> (ø)` | | | [x/wasm/ibc.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2liYy5nbw==) | `68.71% <ø> (ø)` | | | [x/wasm/keeper/events.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2tlZXBlci9ldmVudHMuZ28=) | `100.00% <100.00%> (ø)` | | | [x/wasm/keeper/keeper\_cgo.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2tlZXBlci9rZWVwZXJfY2dvLmdv) | `94.11% <ø> (ø)` | | | [x/wasm/keeper/metrics.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL2tlZXBlci9tZXRyaWNzLmdv) | `50.00% <ø> (ø)` | | | [x/wasm/types/authz.go](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm#diff-eC93YXNtL3R5cGVzL2F1dGh6Lmdv) | `81.70% <ø> (ø)` | | | ... and [13 more](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm) | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/CosmWasm/wasmd/pull/1804/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CosmWasm)
webmaster128 commented 6 months ago

I added one point to the list: Make redactError call conditional in func (d MessageDispatcher) DispatchSubmessages. However, I do not know how to implement that since there are a lot of levels through which things propagate. The high level idea is that redactError is not called for the cases where we get an error from inside of the contract. In this case we know for sure the error contents is deterministic.

chipshort commented 6 months ago

I added some code for the redactError change. Not sure if this is the best way to do it or if it would be better to have a completely separate error type for that to match against (like it is done for the SystemError)

chipshort commented 6 months ago

The test-system job seems to fail sporadically. Is it just a timing thing?

pinosu commented 6 months ago

The test-system job seems to fail sporadically. Is it just a timing thing?

Yes, unfortunately since we added the system test for the upgrade, sporadically it fails for timeout

webmaster128 commented 6 months ago

If you look at the logs and the implementation of AwaitBlockHeight we see that we wait for block number 21, current block is 9 and block time is 1s with a timeout of 14s. Finalizing 12 one-second blocks within 14 seconds is very optimistic given that the blockTime parameter is used for "--commit-timeout=" + s.blockTime.String(),. But commit timeout is not the block time but a lower bound for the block time (https://docs.tendermint.com/v0.34/tendermint-core/configuration.html). For Nois we use e.g.

            // Consensus settings
            config.Consensus.TimeoutPropose = 2000 * time.Millisecond
            config.Consensus.TimeoutProposeDelta = 500 * time.Millisecond
            config.Consensus.TimeoutPrecommitDelta = 500 * time.Millisecond
            config.Consensus.TimeoutPrevote = 1 * time.Second
            config.Consensus.TimeoutPrecommit = 1 * time.Second
            config.Consensus.TimeoutPrecommitDelta = 500 * time.Millisecond
            config.Consensus.TimeoutCommit = 1750 * time.Millisecond

to get block times of 2.11s in production or

# Custom settings for very fast blocks in CI
# We target 250ms block times with one validator.
sed -i"" \
  -e 's/^timeout_propose =.*$/timeout_propose = "100ms"/' \
  -e 's/^timeout_propose_delta =.*$/timeout_propose_delta = "100ms"/' \
  -e 's/^timeout_prevote =.*$/timeout_prevote = "100ms"/' \
  -e 's/^timeout_prevote_delta =.*$/timeout_prevote_delta = "100ms"/' \
  -e 's/^timeout_precommit =.*$/timeout_precommit = "100ms"/' \
  -e 's/^timeout_precommit_delta =.*$/timeout_precommit_delta = "100ms"/' \
  -e 's/^timeout_commit =.*$/timeout_commit = "230ms"/' \
  "config/config.toml"

to aim for 250ms block times in CI. So I think the --commit-timeout should be something like 90% of the desired block time.

webmaster128 commented 6 months ago

Do we have an integration test somewhere that shows how a submessage error is returned to a contract unredacted? This is a major feature in CosmWasm 2.0 which we should highlight and test, even if creating such a test is a bunch of work.

chipshort commented 6 months ago

Do we have an integration test somewhere that shows how a submessage error is returned to a contract unredacted?

Good point, I added a test now that instantiates two reflect instances and makes one send a submessage that fails to the other one. It then checks the error message the first contract got.