Cache contract on deployment. #3859
Conversation
There was a problem hiding this comment.
In general, when every PR should contain:
- description explaining the problem
- explanation how this PR solves the problem
- Test Plan - what you did to test this change. E.g. why it doesn't break the protocol.|
For this particular change there are a few issues:
- We're modifying the behavior, so the costs of deploy should be adjusted already. We should keep the old behavior before the protocol version that adjusts fees. So we should guard this behind a compilation feature that will be used for rolling out the new fees config.
- This error handling shouldn't change the protocol, but it does.
- Need to add tests to verify that invalid contract doesn't break the protocol, but the new valid contract gets precompiled.
runtime/runtime/src/actions.rs
Outdated
| if let Some(err) = | ||
| precompile(code.get_code(), &code.get_hash(), vm_config, cache.deref(), VMKind::Wasmer) | ||
| { | ||
| return Err(StorageError::StorageInconsistentState(format!( |
There was a problem hiding this comment.
What if the contract doesn't compile? It looks like precompile can throw an error.
There was a problem hiding this comment.
Actually if contract doesn't compile we could do one of the following:
- disregard error, but it means we'll try to recompile it later on and spend CPU
- make deployment an error
Second behavior seems to be more correct, as it will also protect our runtime from attempts to recompile later on.
@evgenykuzyakov probably your question was not regarding if we shall return an error, but more liek what kind of error shall be returned here. In review feedback I introducedDataProcessingErrorwhich tries to distinguish it from the storage error.
There was a problem hiding this comment.
Regarding protocol change - that's an interesting question. You mean protocol changes because we now fail earlier than before in case of invalid contracts, right? Could you please recommend what else to be done to migrate to the new protocol, but just bumping PROTOCOL_VERSION in version.rs?
runtime/runtime/src/actions.rs
Outdated
| { | ||
| return Err(StorageError::StorageInconsistentState(format!( | ||
| "Cannot precompile contract {}: {:?}", | ||
| code.get_hash().to_string(), |
There was a problem hiding this comment.
| code.get_hash().to_string(), | |
| code.get_hash(), |
x.to_string is, by definition, format!("{}", x), so format!("{}", x.to_string()) is a tautology.
There was a problem hiding this comment.
Thanks, C still haunts my brain.
runtime/runtime/src/actions.rs
Outdated
| if let Some(err) = | ||
| precompile(code.get_code(), &code.get_hash(), vm_config, cache.deref(), VMKind::Wasmer) | ||
| { | ||
| return Err(StorageError::StorageInconsistentState(format!( |
There was a problem hiding this comment.
A more idiomatic (but not necessary more readable :) ) version would be `
precompile(...).map_err(|err| StorageError(...))?
There was a problem hiding this comment.
IIRC originally it was map_err, but IDE inspection suggested me to rewrite with 'let`.
There was a problem hiding this comment.
Ah, sorry, I was confused by the peculiar signature of precompile. It returns an Option<VMError>. I would expect it to return Result<(), VMError>. The tangible benefit there would be that it would be impossible to forget to check the error: compiler warns about unused results, but not about unused options. And map_err would also work then.
Problem: currently we blindly deploy the contract, and do not check if it is fully correct Wasm binary that could be linked with NEAR host functions. As result, our compilation caches could be attacked by incorrect contracts execution attempts both intentionally and non-intentionally. Also node on which deployment happened will get precompiled contract immediately. Solution: precompile contracts on deployment and return deployment error immediately. Test plan: cargo test --all --workspace
1419415 to
be526ca
Compare
It should increase deployment cost a bit, need to come up with measurement scheme. |
|
Obsolete by now, closing. |
Problem
Currently we blindly deploy the contract, and do not check if it is fully correct Wasm
binary that could be linked with NEAR host functions. As result, our compilation caches could
be attacked by incorrect contracts execution attempts both intentionally and non-intentionally.
Also node on which deployment happened will get precompiled contract immediately.
Solution
Precompile contracts on deployment and return deployment error immediately.
Test plan
cargo test --all --workspace
Considerations
As we store precompiled contracts in regular store, not in trie, propagation of compiled contracts across nodes happens gradually. We could come up with a mechanism to actually distribute precompiled contracts, but this looks like a next step.