Wasm based light clients #208
Conversation
mkaczanowski
requested review from
AdityaSripal,
colin-axner and
fedekunze
as code owners
| CacheSizeMb: 8, | ||
| }, &wasmkeeper.ValidationConfig{ | ||
| MaxSizeAllowed: 1024 * 1024, // 1 MB | ||
| }) |
There was a problem hiding this comment.
Is there a way to read/accesss configuration from keeper.go? Right now I left hardcodes, but this definitely should be configurable
There was a problem hiding this comment.
It probably needs to be passed in as an argument from the app.go file
Who is doing the configuring here? This is a chain level parameter correct? Any reason for it not to be a parameter controlled by governance?
There was a problem hiding this comment.
Good question. I would propose:
-
Governance controlled:
- MemoryLimitMb
- MaxSizeAllowed
-
Node/Validator config:
- DataDir
- CacheSizeMb
- PrintDebug
I would leave SupportedFeatures empty and hardcoded. Is there an usecase for Light Client to use other modules?:
SupportedFeatures: []string{},
|
|
||
| encodedData, err := json.Marshal(payload) | ||
| if err != nil { | ||
| // TODO: Handle error |
There was a problem hiding this comment.
There are a few places in this file where I left the error handling marked with TODO. Some methods signature doesn't allow to return an error, so options are:
panicon error (not great)- modify the interface / method signature (broader change)
It would be great if you could advice here, thanks.
|
Thanks @mkaczanowski!! Amazing that the pr is ready for review so quickly! I will try to block some time out over the next week to review this In the meantime, one non-code related question I have is around code ownership. I'll have a better idea around the complexity of the Wasm client after code review, but in the case that maintenance of this client requires a deep understanding of Wasm, we should ensure someone is taking code ownership responsibility of this code in order to review future changes and ensure their correctness. Would you be willing to take code ownership of this code? If not, that's totally fine and understandable, but we will need to take some time to ensure we do find someone qualified to take long term code ownership. It may very well be the case that the Wasm internals are abstracted enough that the IBC team at Interchain Gmbh (currently @AdityaSripal and I) can take the responsibility. I would just like to point out early on that sufficiently complex code needs clear code ownership (and we currently have zero Wasm experience) |
| WasmClientsEnabled: wasmClientAllowed, | ||
| AllowedClients: allowedClients, |
There was a problem hiding this comment.
Shouldn't WasmClientsEnabled just be encoded inside the allowed clients?
There was a problem hiding this comment.
This functionality flag at this moment is not being used by anything (as far as I can tell). To me, it looks like this flag was intended to be a killswitch to prohibit the creation of wasm-based clients (i.e. critical bug found in the module).
@ParthDesai, can you give us a highlight of what this flag aimed to do in the first place?
Overall the auth model seems to be broken here, quick example:
genesis.allowed_clients: {"07-tendermint"}simd tx ibc wasm-manager push_wasm dummy_client ...- This request works:
let client_state = ClientState { r#type: "07-tendermint".to_string(), code_id: ... };
let msg = MsgCreateClient {
client_state: Some(Any {
type_url: "/ibc.lightclients.wasm.v1.ClientState".to_string(),
value: prost_serialize(&client_state)?,
});
The above example is broken because I register a new client type dummy_client and then create an instance of dummy_client while specifying 07-tendermint client type. In short, the codeID is superior to the client type.
I'll analyze it and come back with the fix.
There was a problem hiding this comment.
WasmClientsEnabled is specified in ADR (as AreWasmClientsEnabled):
https://github.com/cosmos/ibc-go/pull/163/files#diff-6e67f71bfd3b039c42b2c26c3f2d51430f78c348018096e99a7b4addbc042971R54
There was a problem hiding this comment.
@AdityaSripal it's fixed now (10ea5ce).
Shouldn't WasmClientsEnabled just be encoded inside the allowed clients?
The ADR explains that Wasm light clients are allowed as long as the WasmClientsEnabled is true (controlled by governance) and the client type starts with wasm/ prefix.
Question:
ICS-24 prohibits / character in the client type, and yet the wasm/ is specified in ADR. To mitigate the issue, I used wasm- prefix. Is that okay? Do I need to update the ADR now?
There was a problem hiding this comment.
@AdityaSripal sorry for a bit of chaos'y comments regarding the client type handling... Hopefully, the latest update is acceptable for everyone.
The ADR specifies that a new Wasm light client type is "dynamically defined" during the light client binary upload.
Example CLI invocation:
$ ./simd tx ibc wasm-manager push-wasm "wasm/example-chain" binary.wasm
Our initial implementation used the client type as a namespace/alias, where every time the new binary is being uploaded under the same client type, we would version it. For example, the storage key <client-type>/{latest, previous, next} would point to selected versions of the binary.
While this looks nice from a user perspective, it raises a governance/ownership question. What prevents "other people" from uploading malicious binary under selected "client-type"? Who owns the "namespace/alias/client-type"?
This change simplifies the client type management:
- all Wasm light clients are of
10-wasmclient type. This way we don't have to make any changes toIsAllowedClientmethod - every time Wasm blob is uploaded, the caller gets
code-idreference to uploaded code. This removes the governance requirement.
modules/core/28-wasm/handler.go
Outdated
| "github.com/cosmos/ibc-go/modules/core/28-wasm/types" | ||
| ) | ||
|
|
||
| func HandleMsgPushNewWASMCode(ctx sdk.Context, k keeper.Keeper, msg *types.MsgPushNewWASMCode) (*types.MsgPushNewWASMCodeResponse, error) { |
There was a problem hiding this comment.
It looks like anyone can create a new version of an existing wasm contract? Shouldn't there be some authentication in place?
There was a problem hiding this comment.
I think this comment explains the flow in more detail.
In short, anyone who is willing to pay transaction fees can upload the new light client code. The uploaded wasm-code is then identified by the code_id (sha256(code)) handle and must be passed by the client's call.
If someone pushes a new version of code, he'll get a new code_id handle, as if that was a new code
|
This would also benefit from a review by @ethanfrey if possible. Especially the wasm-specific logic |
There was a problem hiding this comment.
Amazingly fast work here! I think the primary question in my mind is still how we want the WASM code to interact with state - see cosmos/ibc#571 (comment) and responses thereafter. I would prefer to alter the ibc-go internal structure and IBC API if necessary and do this properly so that the WASM code can be really flexible, though that's just my two cents.
|
Sorry for the sluggish response... I've made minor changes and responded to comments.
@cwgoes the highest priority now is to settle down the design in the ICS, because this implementation may change depending on the discussion there, obviously. There are a few open questions (including state interaction) on the ICS, so I am prioritizing my work to resolve them asap.
Regardless, I am happy to assist if needed. |
|
Since the ICS has been merged, I'll be applying discussed changes in the upcoming days |
There was a problem hiding this comment.
One meta question: why don't you use the standard x/wasm module?
There is a sudo entry point you can use and just define the formats you pass in there (which can never be called by users).
You can just use the module as the backend and maybe enable 1 or 2 of the messages (storing code, maybe instantiation) so as not to expose the whole wasm engine to end users if the chain doesn't want to.
Also, note that you can configure x/wasm to be governance gated, so only chain governance could upload code, or only governance could instantiate code. We added that 1 year ago for the Cosmos Hub. Happy if you would use this (and maybe give feedback on how to shape it for these concrete needs).
If there is some limit of wasmd that is blocking you from building on it, please make an issue (and please point to this PR/project... Unless you say who you are and what you are building, I have no way to distinguish a large, important project with a clear need asking for a feature. And a random dev who played with wasmd for a few hours.)
The main one I can think of is the lack of 0.44 support currently. We should get that into main in less than 4 weeks (provenance has it on a fork, we are waiting the 1.0 release - soon - for them to prepare a formal PR into wasmd)
wasmd has been shaped by users for 2 years and there are a number of chains pledging commitment to support it. Also many (but not all) chains that will add the wasm light client support will also have wasm contracts, so I wonder about the separation.
|
|
||
| latestVersionKey := types.LatestWASMCode(clientType) | ||
| if store.Has(codeIDKey) { |
There was a problem hiding this comment.
You will remove this later. I had this in an old version of cosmwasm for consistency and got all kinds of complaints. I can this will happen here as well.
|
@mkaczanowski how are you going with applying the changes from the spec? Please let us know if there's anything we can help you with! |
|
Hey, @crodriguezvega, thanks for asking. I am planning to resume (full steam) work on the implementation this week. For the past weeks, we have been continuing the work on the actual bridge that utilizes the WASM Manager, this development led to fruitful discussions such as this: cosmos/ibc#571 (comment) On the implementation front, I think the most urgent are to:
|
|
@mkaczanowski Thanks for the update and the overview of the work you're planning next! |
Use prefix store for contract history
| panic(err) | ||
| } | ||
|
|
||
| WasmVM = vm |
There was a problem hiding this comment.
Does IBC specify or somehow guarantee that all code is executed non-concurrently?
If not, then the fact that this global variable is exported means that any code which accesses it from concurrent goroutines — via NewKeeper, or any other method, or in fact any user code whatsoever — results in a data race and memory model violation.
|
Closing since this branch/pr is stale. |
Description
Implementation of Wasm based light clients described in:
Example light client implementation (Celo <-> Cosmos) utilizing the module:
https://github.com/ChorusOne/celo-light-client/blob/main/src/contract/mod.rs
The above light client can be executed via Makefile (gaia + geth + LC setup):
https://github.com/ChorusOne/celo-cosmos-bridge
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.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes