This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# 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
@bkchr @gnunicorn I think this is ready for another look! |
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
gnunicorn
approved these changes
Jan 15, 2019
There was a problem hiding this 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)
Co-Authored-By: pepyakin <[email protected]>
Co-Authored-By: pepyakin <[email protected]>
Co-Authored-By: pepyakin <[email protected]>
@gnunicorn thanks for the suggestions, I really appreciate it! |
# Conflicts: # node/runtime/wasm/target/wasm32-unknown-unknown/release/node_runtime.compact.wasm
gavofyork
approved these changes
Jan 16, 2019
@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
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
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 <[email protected]> * Update srml/contract/src/lib.rs Co-Authored-By: pepyakin <[email protected]> * Line wraps * Wrapping macros * Add _ prefix * Grumbles * Doc updates. * Update srml/contract/src/wasm/mod.rs Co-Authored-By: pepyakin <[email protected]> * Update srml/contract/src/lib.rs Co-Authored-By: pepyakin <[email protected]> * 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 <[email protected]> * Update srml/contract/src/exec.rs Co-Authored-By: pepyakin <[email protected]> * Update srml/contract/src/exec.rs Co-Authored-By: pepyakin <[email protected]> * Recompile node_runtime
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
seamFirst, I added a seam between vm and executive layers, so a few of new traits was introduced:
Loader
andVm
. In essense, they allow you to switch an implementation of a VM with something other that can call interact with the outside world viaExt
.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:
substrate/srml/contract/src/tests.rs
Lines 800 to 853 in 754f28d
with this code in the proposed PR:
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 useEmptyOutputBuf
/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
substrate/srml/contract/src/tests.rs
Lines 604 to 610 in 754f28d
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:
Instead of code like this littered every where
substrate/srml/contract/src/vm/runtime.rs
Lines 342 to 345 in 754f28d
you declare token once
and then use it like that
Other changes