Make contract a separate runtime module #345

pepyakin · 2018-07-17T10:09:28Z

The main theme of this PR is extracting contract module from staking module, and lifting it to a standalone module which has it's own entrypoints: call and create.

Now, the module relations are inverted: now contract module uses staking module only for transfers. This is in contrast how it was before: staking invoked contract module whenever there is a code in account to which transfer is happening. This change allows us to avoid polluting staking module with contract concepts (such as gas limits, etc).

There are however a few complications:

Since plain transfers in staking can kill accounts, we need to provide a way for notifying other modules of such kills. For that reason OnAccountKill is added to staking::Trait.
Revertable state mechanisms are moved to the contract crate. However, we still need to have an ability to issue ephemeral (which can be reverted) transfers on the ephemeral storage. For that reason effect_transfer/effect_create logic is duplicated in the contract module.

Along with that numerous changes were made, such as:

Constructors and returning data were re-applied
Nested calls support with gas propogation was added
Additional fees per call and create were introduced

However, I would say there are still areas to improve, including:

implementing remaining futures to MVP
avoiding quadratic attacks
limiting maximal depth of the transaction stack
refactoring of gas mechanism
more convenient ways of testing the runtime
way more tests
more informative errors which e.g preserves transaction call stacks for ease of debugging.
etc

I'm planning to address these issues in the following PRs. This PR already got pretty big.

sorpaas

Tried to do a review. Overall LGTM. Just some small grumbles/questions/notes.

sorpaas · 2018-07-25T10:19:38Z

substrate/runtime/contract/src/exec.rs

+		dest: T::AccountId,
+		value: T::Balance,
+		gas_meter: &mut GasMeter<T>,
+		_data: Vec<u8>,


If I understood this, we don't pass _data to vm::execute or Ext yet because it's unimplemented, right?

And just a note on quadratic attacks -- it looks like we decide to put callstack directly on program stack, so in the future, using &[u8] might be more efficient and lifetime should work.

Yeah, exactly, it is untackled issue right now. And I didn't delve into it.
And yes, in theory, we can directly read from/write to the caller memory.

But there is a complication with writing though. Maybe we will have to make a temporary copy. The reason for that is caller can't know the size of the return buffer before calling the callee, and thus, might want to allocate space after the return data size is known. This is an open research question right now.

sorpaas · 2018-07-25T10:21:55Z

substrate/runtime/contract/src/exec.rs

+		endowment: T::Balance,
+		gas_meter: &mut GasMeter<T>,
+		ctor: &[u8],
+		_data: &[u8],


We're already using &[u8] here. So I think it may be better to change above L51 to use &[u8] as well to keep it consistent.

Also possibly:

L237 and L250.

substrate/runtime/contract/src/lib.rs: L120, L128 and L217.

substrate/runtime/contract/src/vm.rs: L65.

sorpaas · 2018-07-25T10:28:18Z

substrate/runtime/contract/src/exec.rs

+				).map_err(|_| "vm execute returned error while create")?
+			};
+
+			nested.overlay.set_code(&dest, exec_result.return_data().to_vec());


In runtime/consensus/src/lib.rs:89, we can change set_code to take only &[u8]. This will avoid the to_vec copy here. Or a temporary solution before we properly fix quadratic attacks -- use exec_result.return_data directly because we don't need exec_result after this point.

sorpaas · 2018-07-25T10:35:48Z

substrate/runtime/contract/src/exec.rs

+	}
+
+	let to_balance = overlay.get_balance(dest);
+	if to_balance + value <= to_balance {


Just a possible confusion here: it looks like we define staking::Trait::Balance to be wrapping (non-panic), but we're using std traits Add/Sub etc, which by default is non-wrapping (panic). So if someone accidentally set staking::Trait::Balance to be, for example, usize. The code would panic.

Maybe it's worth considering to define another trait (like WrappingAdd?).

You're right! Will use checked_add here and in the staking module.

sorpaas · 2018-07-25T10:49:43Z

substrate/runtime/contract/src/vm.rs

+/// to just terminate quickly in some cases.
+enum SpecialTrap {
+	/// Signals that trap was generated in response to call `ext_return` host function.
+	Return(Vec<u8>),


Just a note: we might want to avoid the extra copying here in the future. One way I can think of is to change this enum point to a region of the memory, and then like we do in parity-ethereum, directly wrap that memory as the return data.

Yeah, I will add TODO about this!

sorpaas · 2018-07-25T10:51:39Z

substrate/runtime/contract/src/vm.rs

+	runtime: Runtime<T, E>,
+	run_err: Option<sandbox::Error>,
+) -> Result<ExecutionResult, Error> {
+	let mut return_data = Vec::new();


Just a small grumble -- we can change this to:

let return_data = match (run_err, runtime.special_trap) { (None, None) => Vec::new(), (Some(sandbox::Error::Execution), Some(SpecialTrap::Return(rd))) => rd, ... }

This avoids the mut and an extra stack item.

Sure! The current construction used to make sense, but now direct assignemnt is a way to go.

pepyakin · 2018-07-25T14:22:18Z

@sorpaas thanks for your valuable feedback!

@gavofyork
I've changed the code from overflowing to checked_add, please check paritytech/polkadot@3afe59a

Also unfortunately my paritytech/polkadot#345 (comment) was hidden but still might be relevant.

# Conflicts: # demo/runtime/wasm/target/wasm32-unknown-unknown/release/demo_runtime.compact.wasm # demo/runtime/wasm/target/wasm32-unknown-unknown/release/demo_runtime.wasm # polkadot/runtime/wasm/target/wasm32-unknown-unknown/release/polkadot_runtime.compact.wasm # polkadot/runtime/wasm/target/wasm32-unknown-unknown/release/polkadot_runtime.wasm # substrate/executor/wasm/target/wasm32-unknown-unknown/release/runtime_test.compact.wasm # substrate/executor/wasm/target/wasm32-unknown-unknown/release/runtime_test.wasm # substrate/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm # substrate/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.wasm

gavofyork · 2018-07-29T11:43:30Z

On ice until we have runtime version incremented. #450

* master: (86 commits) Make contract a separate runtime module (#345) Version bump (#450) DB-based blockchain data cache for light nodes (#251) Update libp2p again (#445) Update version on git head change (#444) Fix the public key of bootnode 3 (#441) Update libp2p (#442) Switch to the master branch of libp2p (#427) Export ws port 9944 and add doc (#440) Iterate over overlay to decide which keys to purge (#436) Exit signal gets its own trait (#433) Add docker image (#375) Reset peers.json if the content is not loadable (#405) Limit number of incoming connections (#391) Fix memory leaks in libp2p (#432) Do not queue empty blocks set for import (#431) 5 random fixes (#1) (#435) Chore: fix typo (#434) Prevent building invalid blocks (#430) Better logging for public key mismatch (#429) ...

* decl_module and extract runtime mod * Invert dependency staking←→contract * Remove CodeOf * Remove StorageOf and move double_map * Comment staking test * Clean * Add gas_price and gas_limit * Commit. * Renames * Params * WIP * Rename transfer to call * WIP * Rebuild binaries. * WIP * Backport ctro changes * Call wiring * Commit overlay. * Rename merge → commit, into_state → ..._change_set * WIP * Contract creation routines * Set code of the created account. * Fix the ID of `create` Call * Fix most of the warning. * Add the simplest test in the contract crate * Transfers work! * Add contract_create test. * Clean * Add top-level create test * Clean a bit. * Pass gas_limit and data via create. * Introduce OnAccountKill callback in staking * Hook up OnAccountKill * Comments * Pay for gas. * Refund unused gas in call * Tests for zero call and zero endownment. * Add todo about rewriting docs * Pay for gas in create transactions * Fix refunds * Clean unrelevant comments * fixup! Fix refunds * fixup! Clean unrelevant comments * Move DetermineContractAddress to contract Also restore account removal test * fixup! Clean unrelevant comments * Inline effect_transfer, remove effect_create Remove account_db! * Use own new_test_ext. * Don't account for liability * Add some docs * Move contract_fee into contract module * Take GasMeter in vm::execute * Use GasMeter throughout contract module for meter * gas module refactoring * Clean * Add base call fee * note about gas price should be taken from storage * Add base fee for create * Rename send → call * Clean * Take fee expressed in dots in gas * Add Checked{Add,Sub,Mul,Div} to SimpleArithmetic * Make Gas generic * Store {call,create}_base_fee in storage * Clean * Rename buy_gas * Store gas_price in the storage * Remove unneeded comment. * Bail out if contract already has code. * Todos * Refund even if top-level contract fails. * Fix error msg * Fix caller issue * Extract tests module * Add max_depth var in storage * Remove left over gas_left * Refactor exec * Add test oog test. * set_free_balance_creating * Docs and comments. * Update storage roots because of ContractFee move * Rebuild binaries. * Simplify vm code. * Wrapping. * Refactor a bit. * Typo * UpdateBalanceOutcome enum * Style grumbles. * Rebuild binaries. * Always consume the given amount of gas. * [skip ci] endownment → endowment * Rename `AccountId` generic in on_account_kill * Fix Cargo.lock * Refine docs for gas meter. * [skip ci] Add comments for gas module * Directly assign to `return_data` at declaration * Use slices instead of vecs to pass the input data * Add todo about passing return data without copy * Use checked_add instead of add with overflow * Use return_data directly. * Rebuild binaries. * Rebuild binaries.

* Fix up session phase. * Version bump. * Version fix * Fix session rotation properly and add test * Make contract a separate runtime module (#345) * decl_module and extract runtime mod * Invert dependency staking←→contract * Remove CodeOf * Remove StorageOf and move double_map * Comment staking test * Clean * Add gas_price and gas_limit * Commit. * Renames * Params * WIP * Rename transfer to call * WIP * Rebuild binaries. * WIP * Backport ctro changes * Call wiring * Commit overlay. * Rename merge → commit, into_state → ..._change_set * WIP * Contract creation routines * Set code of the created account. * Fix the ID of `create` Call * Fix most of the warning. * Add the simplest test in the contract crate * Transfers work! * Add contract_create test. * Clean * Add top-level create test * Clean a bit. * Pass gas_limit and data via create. * Introduce OnAccountKill callback in staking * Hook up OnAccountKill * Comments * Pay for gas. * Refund unused gas in call * Tests for zero call and zero endownment. * Add todo about rewriting docs * Pay for gas in create transactions * Fix refunds * Clean unrelevant comments * fixup! Fix refunds * fixup! Clean unrelevant comments * Move DetermineContractAddress to contract Also restore account removal test * fixup! Clean unrelevant comments * Inline effect_transfer, remove effect_create Remove account_db! * Use own new_test_ext. * Don't account for liability * Add some docs * Move contract_fee into contract module * Take GasMeter in vm::execute * Use GasMeter throughout contract module for meter * gas module refactoring * Clean * Add base call fee * note about gas price should be taken from storage * Add base fee for create * Rename send → call * Clean * Take fee expressed in dots in gas * Add Checked{Add,Sub,Mul,Div} to SimpleArithmetic * Make Gas generic * Store {call,create}_base_fee in storage * Clean * Rename buy_gas * Store gas_price in the storage * Remove unneeded comment. * Bail out if contract already has code. * Todos * Refund even if top-level contract fails. * Fix error msg * Fix caller issue * Extract tests module * Add max_depth var in storage * Remove left over gas_left * Refactor exec * Add test oog test. * set_free_balance_creating * Docs and comments. * Update storage roots because of ContractFee move * Rebuild binaries. * Simplify vm code. * Wrapping. * Refactor a bit. * Typo * UpdateBalanceOutcome enum * Style grumbles. * Rebuild binaries. * Always consume the given amount of gas. * [skip ci] endownment → endowment * Rename `AccountId` generic in on_account_kill * Fix Cargo.lock * Refine docs for gas meter. * [skip ci] Add comments for gas module * Directly assign to `return_data` at declaration * Use slices instead of vecs to pass the input data * Add todo about passing return data without copy * Use checked_add instead of add with overflow * Use return_data directly. * Rebuild binaries. * Rebuild binaries. * Docs

* Erasure encoding availability initial commit * Modifications to availability store to keep chunks as well as reconstructed blocks and extrinsics. * Gossip messages containig signed erasure chunks. * Requesting eraure chunks with polkadot-specific messages. * Validation of erasure chunk messages. * Apply suggestions from code review Co-Authored-By: Luke Schoen <ltfschoen@users.noreply.github.com> * Fix build after a merge * Gossip erasure chunk messages under their own topic * erasure_chunks should use the appropriate topic * Updates Cargo.lock * Fixes after merge * Removes a couple of leftover pieces of code * Fixes simple stuff from review * Updates erasure and storage for more flexible logic * Changes validation and candidate receipt production. * Adds add_erasure_chunks method * Fixes most of the nits * Better validate_collation and validate_receipt functions * Fixes the tests * Apply suggestions from code review Co-Authored-By: Robert Habermeier <rphmeier@gmail.com> * Removes unwrap() calls * Removes ErasureChunks primitive * Removes redundant fields from ErasureChunk struct * AvailabilityStore should store CandidateReceipt * Changes the way chunk messages are imported and validated. * Availability store now stores a validator_index and n_validators for each relay_parent. * Availability store now also stores candidate receipts. * Removes importing chunks in the table and moves it into network gossip validation. * Validation of erasure messages id done against receipts that are stored in the availability store. * Correctly compute topics for erasure messages * Removes an unused parameter * Refactors availability db querying into a helper * Adds the apis described in the writeup * Adds a runtime api to extract erasure roots form raw extrinsics. * Adds a barebone BlockImport impl for avalability store * Adds the implementation of the availability worker * Fix build after the merge with master. * Make availability store API async * Bring back the default wasmtime feature * Lines width * Bump runtime version * Formatting and dead code elimination * some style nits (#1) * More nits and api cleanup * Disable wasm CI for availability-store * Another nit * Formatting

* Add script for generating the keys for genesis validators * Regenerate genesis validators keys * Split out genesis assets module * Rename to genesis_assets() * Add vesting key * Add mainnet genesis * Deduct the genesis endowed from vesting account * . * . * . . * Add FIXME * .

Documentation updates, primarily around consensus

pepyakin added 30 commits July 11, 2018 16:43

decl_module and extract runtime mod

8bb5143

Invert dependency staking←→contract

7fa574f

Remove CodeOf

8c06e98

Remove StorageOf and move double_map

f315ca0

Comment staking test

799e591

Clean

92922fd

Add gas_price and gas_limit

d43ab98

Commit.

92cfeeb

Renames

a4264f3

Params

9d07af9

WIP

00919a4

Rename transfer to call

900c859

WIP

593d51c

Rebuild binaries.

0b501c6

WIP

ce0e128

Backport ctro changes

8915d76

Call wiring

ca48073

Commit overlay.

3f2423f

Rename merge → commit, into_state → ..._change_set

32b0e55

WIP

8577fad

Contract creation routines

14ee2f4

Set code of the created account.

3a218d9

Fix the ID of create Call

e8157c6

Fix most of the warning.

43508cd

Add the simplest test in the contract crate

a4b6480

Transfers work!

a340742

Add contract_create test.

e3b906c

Clean

8a541a3

Add top-level create test

77ee25b

Clean a bit.

30a72d3

pepyakin added 6 commits July 23, 2018 16:23

Rename AccountId generic in on_account_kill

d91dfbe

Merge branch 'master' into ser-contract-ioc

ee83701

Fix Cargo.lock

f570351

Refine docs for gas meter.

4de767b

[skip ci] Add comments for gas module

58878a8

Merge branch 'master' into ser-contract-ioc

7a712e1

sorpaas reviewed Jul 25, 2018

View reviewed changes

pepyakin added 6 commits July 25, 2018 15:23

Directly assign to return_data at declaration

eeebaba

Use slices instead of vecs to pass the input data

595e534

Add todo about passing return data without copy

a846bb1

Use checked_add instead of add with overflow

3afe59a

Use return_data directly.

dae4d40

Rebuild binaries.

9e13c68

pepyakin added 2 commits July 27, 2018 16:26

Rebuild binaries.

af99a9d

gavofyork added A8-looksgood and removed A5-grumble labels Jul 29, 2018

gavofyork approved these changes Jul 29, 2018

View reviewed changes

gavofyork added the A1-onice label Jul 29, 2018

gavofyork removed the A1-onice label Jul 29, 2018

gavofyork merged commit 9d7a3c1 into master Jul 29, 2018

gavofyork deleted the ser-contract-ioc branch July 29, 2018 13:55

liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022

Merge pull request paritytech#345 from subspace/update-docs

464d260

Documentation updates, primarily around consensus

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make contract a separate runtime module #345

Make contract a separate runtime module #345

pepyakin commented Jul 17, 2018 •

edited

Loading

sorpaas left a comment

sorpaas Jul 25, 2018

pepyakin Jul 25, 2018

sorpaas Jul 25, 2018

sorpaas Jul 25, 2018

pepyakin Jul 25, 2018

sorpaas Jul 25, 2018

sorpaas Jul 25, 2018

pepyakin Jul 25, 2018

sorpaas Jul 25, 2018

pepyakin Jul 25, 2018

sorpaas Jul 25, 2018

pepyakin Jul 25, 2018

pepyakin commented Jul 25, 2018

gavofyork commented Jul 29, 2018 •

edited

Loading

Make contract a separate runtime module #345

Make contract a separate runtime module #345

Conversation

pepyakin commented Jul 17, 2018 • edited Loading

sorpaas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin commented Jul 25, 2018

gavofyork commented Jul 29, 2018 • edited Loading

pepyakin commented Jul 17, 2018 •

edited

Loading

gavofyork commented Jul 29, 2018 •

edited

Loading