Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to wasmvm 2.0.0-rc.1 #1804

Merged
merged 45 commits into from
Mar 6, 2024
Merged

Update to wasmvm 2.0.0-rc.1 #1804

merged 45 commits into from
Mar 6, 2024

Conversation

chipshort
Copy link
Collaborator

@chipshort chipshort commented Feb 6, 2024

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

TODOs:

  • supported capabilities are now []string
  • nested Result changes
  • renaming
  • StoreCode gas
  • ValidateAddress
  • Array[C] type replacements
  • Adjust gas multiplier
  • TransferMsg.Memo
  • QueryRequest.Grpc
  • MsgResponses (see [idea, unfinished] Add flattened msgResponses to SubMsgResponse #1800)
  • IBCReceiveResponse.Acknowledgement == nil
  • Payload
  • Make redactError call conditional in func (d MessageDispatcher) DispatchSubmessages

go.sum Outdated Show resolved Hide resolved
x/wasm/keeper/handler_plugin_encoders.go Show resolved Hide resolved
x/wasm/keeper/keeper.go Outdated Show resolved Hide resolved
x/wasm/keeper/api.go Outdated Show resolved Hide resolved
@chipshort
Copy link
Collaborator Author

@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
Copy link
Member

@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
Copy link
Member

webmaster128 commented Feb 8, 2024

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
Copy link
Contributor

pinosu commented Feb 9, 2024

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

@chipshort
Copy link
Collaborator Author

@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
Copy link
Member

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
Copy link
Member

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

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! Not done reviwwing but here is a start

Dockerfile Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tests/e2e/reflect_helper.go Outdated Show resolved Hide resolved
x/wasm/keeper/api.go Outdated Show resolved Hide resolved
x/wasm/keeper/api.go Outdated Show resolved Hide resolved
x/wasm/keeper/api.go Outdated Show resolved Hide resolved
x/wasm/keeper/api.go Show resolved Hide resolved
@chipshort
Copy link
Collaborator Author

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

chipshort and others added 3 commits February 26, 2024 16:55
Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@pinosu
Copy link
Contributor

pinosu commented Feb 26, 2024

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
Copy link
Member

webmaster128 commented Feb 26, 2024

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.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very very nice solution

x/wasm/types/errors.go Outdated Show resolved Hide resolved
x/wasm/types/errors.go Show resolved Hide resolved
x/wasm/keeper/msg_dispatcher.go Show resolved Hide resolved
x/wasm/types/errors_test.go Show resolved Hide resolved
@webmaster128
Copy link
Member

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
Copy link
Collaborator Author

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.

Copy link
Contributor

@pinosu pinosu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! 💯 LGTM 👍

@chipshort chipshort merged commit ba3a59c into main Mar 6, 2024
16 checks passed
@chipshort chipshort deleted the update-to-wasmvm-2.0 branch March 6, 2024 11:40
@chipshort chipshort mentioned this pull request Mar 6, 2024
webmaster128 added a commit that referenced this pull request Mar 12, 2024
This is unused since #1804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants