Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Contracts module rejig #1358

Merged
merged 116 commits into from
Jan 17, 2019
Merged

Contracts module rejig #1358

merged 116 commits into from
Jan 17, 2019

Conversation

pepyakin
Copy link
Contributor

@pepyakin pepyakin commented Jan 7, 2019

I started this branch as working on PUTCODE model, described here. After some initial work was done I realized that I can reliably test that code, because all of the testing was done in top-down approach, similar how other modules are doing it. Tests were super fragile, they tested too much, even though the system was split in two parts (one was able to test wasm bindings separately). If that weren't enough all tests had asserts on how much a particular operation spent gas which was merely a single number which changed often.

Because of that I decided to go all in and do a few refactorings to the codebase.

Adding a Vm seam

First, I added a seam between vm and executive layers, so a few of new traits was introduced: Loader and Vm. In essense, they allow you to switch an implementation of a VM with something other that can call interact with the outside world via Ext.
It would be kind of cool to add a feature to run other kind of bytecode, say, EVM, but I didn't dig into. The main purpose of this change is to allow to use a closure executor as a VM (which uses closures instead of bytecode). This allows testing code for more closer and easier interaction with the environment.

For example, consider this test in the current version which is written in raw wasm:

const CODE_INPUT_DATA: &'static str = r#"
(module
(import "env" "ext_input_size" (func $ext_input_size (result i32)))
(import "env" "ext_input_copy" (func $ext_input_copy (param i32 i32 i32)))
(import "env" "memory" (memory 1 1))
(func (export "call")
(block $fail
;; fail if ext_input_size != 4
(br_if $fail
(i32.ne
(i32.const 4)
(call $ext_input_size)
)
)
(call $ext_input_copy
(i32.const 0)
(i32.const 0)
(i32.const 4)
)
(br_if $fail
(i32.ne
(i32.load8_u (i32.const 0))
(i32.const 0)
)
)
(br_if $fail
(i32.ne
(i32.load8_u (i32.const 1))
(i32.const 1)
)
)
(br_if $fail
(i32.ne
(i32.load8_u (i32.const 2))
(i32.const 2)
)
)
(br_if $fail
(i32.ne
(i32.load8_u (i32.const 3))
(i32.const 3)
)
)
(return)
)
unreachable
)
)
"#;

with this code in the proposed PR:

let input_data_ch = loader.insert(|ctx| {
    assert_eq!(ctx.input_data, &[1, 2, 3, 4]);
    VmExecResult::Ok
});

There is a one well-known gotcha with this mocking approach: when you mock too much you don't test anything. To avoid that I had to specify contract between vm and executive layer in more detail, and in particular encoded some invariants in type system (for example, instead of passing a mutable pointer to a &mut output_data for direct write of return data of the callee contract to the scratch buffer of the caller, we now use EmptyOutputBuf/OutputBuf/VmExecResult types which you pass by value now).

This is only the first step. I can see opportunities for upgrading this approach by adding some macro magic for initial setup (which is quite boilerplately right now).

Smart Gas Metering

It was really hard to test gas metering. The strategy was like this

// 11 - value sent with the transaction
// (3 * 129) - gas spent by the init_code.
// (3 * 175) - base gas fee for create (175) (top level) multipled by gas price (3)
// ((21 / 3) * 3) - price for contract creation
assert_eq!(
Balances::free_balance(&0),
100_000_000 - 11 - (3 * 129) - (3 * 175) - ((21 / 3) * 3)

after the execution of a contract you check how much gas was spent (or rather, how much balance units was refunded). You express the forumula as an arithmetic expression that describes how much each component consumed after adjust for gas.

This was really hard to work with because every time something changes you have to figure out which component exactly has changed. Changes from wasm code was especially hard. Changes from wasm code constructors (basically wasm code wrapped in a wasm code) was even harder.

The solution is to require every single charging for gas to provide token, purpose of which is two-fold:

  1. It provides a nominal type which you can use then for matching at the testing time.
  2. Exact gas calculation. So instead of calculating how much gas to charge you just use the right token and it will calculate the gas for you, thus achieving separation of concerns.

Instead of code like this littered every where

let data_len_in_gas = <<E::T as Trait>::Gas as As<u64>>::sa(data_len as u64);
let price = (ctx.schedule.return_data_per_byte_cost)
.checked_mul(&data_len_in_gas)
.ok_or(sandbox::HostError)?;

you declare token once

#[cfg_attr(test, derive(Debug, PartialEq, Eq))]
#[derive(Copy, Clone)]
pub enum RuntimeToken {
...
    /// The given number of bytes is read from the sandbox memory.
    ReadMemory(u32),
...
}

impl<T: Trait> Token<T> for RuntimeToken {
    type Metadata = Schedule<T::Gas>;

    fn calculate_amount(&self, metadata: &Schedule<T::Gas>) -> Option<T::Gas> {
        let value = match *self {
...
            ReadMemory(byte_count) => metadata
                .sandbox_data_read_cost
                .checked_mul(&<T::Gas as As<u32>>::sa(byte_count))?,
...
        };

        Some(value)
    }
}

and then use it like that

gas_meter.charge(&schedule, RuntimeToken::ReadMemory(bytes))?;

Other changes

# Conflicts:
#	Cargo.lock
#	node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm
# Conflicts:
#	core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm
#	node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm
#	srml/contract/src/account_db.rs
#	srml/contract/src/lib.rs
#	srml/contract/src/tests.rs
@pepyakin
Copy link
Contributor Author

@bkchr @gnunicorn I think this is ready for another look!

@pepyakin pepyakin added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 14, 2019
Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Minor code doc fixes needed (suggestions supplied)

srml/contract/src/exec.rs Outdated Show resolved Hide resolved
srml/contract/src/exec.rs Outdated Show resolved Hide resolved
srml/contract/src/lib.rs Outdated Show resolved Hide resolved
srml/contract/src/wasm/code_cache.rs Outdated Show resolved Hide resolved
srml/contract/src/wasm/code_cache.rs Outdated Show resolved Hide resolved
gnunicorn and others added 3 commits January 15, 2019 12:32
Co-Authored-By: pepyakin <s.pepyakin@gmail.com>
Co-Authored-By: pepyakin <s.pepyakin@gmail.com>
Co-Authored-By: pepyakin <s.pepyakin@gmail.com>
@pepyakin
Copy link
Contributor Author

@gnunicorn thanks for the suggestions, I really appreciate it!

# Conflicts:
#	node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm
@gavofyork
Copy link
Member

@bkchr any other remarks before i merge?

# Conflicts:
#	node/executor/src/lib.rs
#	node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm
#	srml/contract/src/lib.rs
#	srml/contract/src/tests.rs
@bkchr
Copy link
Member

bkchr commented Jan 17, 2019

I'm okay @pepyakin just update the wasm files and this can be merged.

# Conflicts:
#	Cargo.lock
#	core/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm
#	node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm
#	srml/contract/Cargo.toml
@pepyakin pepyakin merged commit 8ef1490 into master Jan 17, 2019
@pepyakin pepyakin deleted the ser-putcode-model branch January 17, 2019 11:01
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
* Move prepare under code.

* Schedule update

* CodeHash

* create takes code_hash

* pass mem def and use code in vm::execute

* Actually save and load code

* Use T::Hash as CodeHash

* Explicit entrypoint name

* Return code_hash and deposit an Event

* Charge for deployed code with gas.

* ImportSatisfyCheck and FunctionImplProvider

* Progress.

* Use new infrastructure for checking imports

* Rename entrypoint to entrypoint_name

* Use strings instead of a Error enum

* Clean

* WIP

* Fix macro_define_env test.

* Fix vm code tests.

* Remove tests for now.

* Fix borked merge

* Fix build for wasm

* fmt

* Scaffolding for abstracting vm.

* Hook up execution to exec layer.

* Fix vm tests.

* Use schedule directly in WasmLoader

* Implement test language.

* Add input_data test.

* Max depth test

* ext_caller

* Simplify test.

* Add TODO

* Some tests and todos.

* top_level

* Clean.

* Restore a couple of integration tests.

* Add a few comments.

* Add ext_address runtime call.

* Deduplicate caller/self_account

* Add not_exists test.

* Change bool to TransferCause.

* Add address tests.

* Remove output_buf from parameter.

* return from start fn.

* Smart gas meter

* Tracing

* Fix prepare tests.

* Code moving

* Add ExecFeeToken

* Use tokens everywhere.

* Make it compile in no_std.

* Lift all test requirements to TestAuxiliaries

* A minor clean

* First create tests

* Remove unneeded TODO

* Docs.

* Code shuffling

* Rename create → instantiate

* Add test address.

* Code shuffling

* Add base_fee tests.

* rejig the code

* Add some comments

* on_finalise comment

* Move event deposit further

* Update Cargo.lock

* Use crates.io version of pwasm-utils

* Format todo comments

* Fix formatting

* Comments

* EmptyOutputBuf and OutputBuf split.

* Restore code_hash

* Fix node-executor.

* Fix typo

* Fix fmt

* Update srml/contract/src/account_db.rs

Co-Authored-By: pepyakin <s.pepyakin@gmail.com>

* Update srml/contract/src/lib.rs

Co-Authored-By: pepyakin <s.pepyakin@gmail.com>

* Line wraps

* Wrapping macros

* Add _ prefix

* Grumbles

* Doc updates.

* Update srml/contract/src/wasm/mod.rs

Co-Authored-By: pepyakin <s.pepyakin@gmail.com>

* Update srml/contract/src/lib.rs

Co-Authored-By: pepyakin <s.pepyakin@gmail.com>

* Add comment

* Use saturation to signal overflow

* Add prepare_test! macro

* Require deploy function.

* Add entry point tests

* Add comment.

* Rename code → code_cache to better describe

* Get rid of weird match!

* Recompile binaries

* Add comments

* refuse_instantiate_with_value_below_existential_deposit

* Little fix

* Make test more complete

* Clean

* Add integration test for instantiation

* Rebuild runtime.

* Add some tests.

* Attach an issue to a TODO

* Attach another issue

* Apply suggestions from code review

Co-Authored-By: pepyakin <s.pepyakin@gmail.com>

* Update srml/contract/src/exec.rs

Co-Authored-By: pepyakin <s.pepyakin@gmail.com>

* Update srml/contract/src/exec.rs

Co-Authored-By: pepyakin <s.pepyakin@gmail.com>

* Recompile node_runtime
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

srml-contract: Reuse instrumented code
5 participants