State Machine: Abstract function execution #19

rphmeier · 2017-11-12T18:09:28Z

Sub-calls all share the same prospective state. An error in any sub-call reverts all changes.

An in-memory storage backend is provided. This could also be implemented for disk or network-backed data.

Committing to a backend yields two updated trie roots:

code tree root is the root of a mapping address => code_hash.
storage tree root is the root of a mapping address => storage_root, where storage_root is the root of a storage tree for that account.

The motivation for separating these parts of the account record is that it will allow better light-client caching of contract code, although it also might become reasonable to combine them.

We might want an execute_static function at the root as well.

tomusdrw

Couple of style nitpicks and two questions:

Arbitrary size storage
Distinction between contract panic and backend error.

tomusdrw · 2017-11-12T21:48:11Z

state_machine/Cargo.toml


 [dependencies]
-polkadot-primitives = { path = "../primitives", version = "0.1" }
+polkadot-primitives = { path = "../primitives" }


Why ditch version?

merge issue

tomusdrw · 2017-11-12T21:49:33Z

state_machine/src/backend.rs

+use primitives::Address;
+use primitives::hash::H256;
+
+use triehash::sec_trie_root;


Would keep it together with primitives. We might try to organize imports into 3 blocks:

<std imports> <external libraries> <internal>

discussed it some time ago with @debris and he likes it as well.

tomusdrw · 2017-11-12T21:52:39Z

state_machine/src/ext.rs

+impl<'a, B: 'a, E: 'a> Externalities<E> for Ext<'a, B, E>
+	where B: Backend, E: Executor
+{
+


Unnecessary line.

tomusdrw · 2017-11-12T21:53:18Z

state_machine/src/ext.rs

+}
+
+impl<'a, B: 'a, E: 'a> StaticExternalities<E> for Ext<'a, B, E>
+	where B: Backend, E: Executor


nitpick: I'd prefer

impl ... for Ext<..> where B: Backend, E: Executor, {

tomusdrw · 2017-11-12T21:54:36Z

state_machine/src/lib.rs

+extern crate hashdb;
+extern crate memorydb;
+extern crate keccak_hash;
+


Redundant line

tomusdrw · 2017-11-12T22:01:55Z

state_machine/src/lib.rs

+	fn set_storage(&mut self, key: H256, value: Vec<u8>);
+
+	/// Make a sub-call to another contract.
+	fn call(&mut self, address: &Address, method: &str, data: &CallData) -> Result<OutData, Self::Error>;


Blank line after method missing.

tomusdrw · 2017-11-12T22:03:16Z

contracts/src/executor.rs

 		type Error = Error;
+
+		fn storage(&self, _key: &H256) -> Result<&[u8]> {


You sure we want to allow storage of data of arbitrary size?

tomusdrw · 2017-11-12T22:06:08Z

state_machine/src/lib.rs

+	call_data: &CallData,
+) -> Result<OutData, Box<Error>> {
+	let code = match overlay.code(address) {
+		Some(x) => x.to_owned(),


We could use bytes here to avoid allocation and copying.

Will log it as a separate issue.

tomusdrw · 2017-11-12T22:07:29Z

state_machine/src/lib.rs

+	address: &Address,
+	method: &str,
+	call_data: &CallData,
+) -> Result<OutData, Box<Error>> {


Shouldn't we be able to distinguish between a backend failure (user's machine fault) and actual execution error (contract panic)?

The first should most likely kill the entire process and the latter should just indicate transaction failure.

yeah, so I guess what needs returning is this same ext::Error<B, E> type.

but the executor error may still be a backend error (having wrapped it internally): https://github.com/paritytech/polkadot/pull/19/files#diff-e5f6b2fc846df5e06dad05931f099408L43

Let's merge it for now and figure out better error handling for that (we might need to add some bounds on Error type for both Executor and Externalities)

tomusdrw · 2017-11-12T22:08:28Z

cli/Cargo.toml

@@ -8,3 +8,4 @@ description = "Polkadot node implementation in Rust."
 clap = { version = "2.27", features = ["yaml"] }
 env_logger = "0.4"
 log = "0.3"
+polkadot-state-machine = { path = "../state_machine" }


Not needed?

just to get it in the workspace :)

You should put it to top-level Cargo.toml

rphmeier · 2017-11-13T14:25:45Z

Arbitrary size storage only seems to complicate things for the paritydb (which will store a flattened version of the balances contract storage trie). Since we're not at the stage of integrating paritydb we might want to consider our strategy for integration before imposing restrictions on the storage formats.

tomusdrw · 2017-11-13T15:28:07Z

cli/Cargo.toml

@@ -8,3 +8,4 @@ description = "Polkadot node implementation in Rust."
 clap = { version = "2.27", features = ["yaml"] }
 env_logger = "0.4"
 log = "0.3"
+polkadot-state-machine = { path = "../state_machine" }


You should put it to top-level Cargo.toml

tomusdrw · 2017-11-13T15:29:17Z

state_machine/src/lib.rs

+	address: &Address,
+	method: &str,
+	call_data: &CallData,
+) -> Result<OutData, Box<Error>> {


Let's merge it for now and figure out better error handling for that (we might need to add some bounds on Error type for both Executor and Externalities)

tomusdrw · 2017-11-13T15:31:24Z

state_machine/src/lib.rs

+	call_data: &CallData,
+) -> Result<OutData, Box<Error>> {
+	let code = match overlay.code(address) {
+		Some(x) => x.to_owned(),


Will log it as a separate issue.

remove balances

…tech#19)

rphmeier added 9 commits November 10, 2017 14:34

initial primitives

58363b2

add block primitives

f6fc716

state machine: backend

188be63

in-memory backend

267df08

tests for overlayed storage

c865aba

Merge branch 'master' into rh-state-machine

2511014

blanked impl for state machine error

da75812

abstract call execution

bd1686c

squash warnings temporarily

71dc101

rphmeier added the A0-please_review Pull request needs code review. label Nov 12, 2017

rphmeier requested a review from tomusdrw November 12, 2017 18:09

fix contracts crate

c29cfa3

tomusdrw approved these changes Nov 12, 2017

View reviewed changes

5chdn added M4-core and removed A0-please_review Pull request needs code review. labels Nov 13, 2017

address grumbles

daf5488

tomusdrw approved these changes Nov 13, 2017

View reviewed changes

tomusdrw added A8-looksgood and removed A6-mustntgrumble labels Nov 13, 2017

remove redundant state-machine dependency

0f03bb4

rphmeier merged commit 79a88a4 into master Nov 13, 2017

rphmeier deleted the rh-state-machine branch November 13, 2017 15:42

gilescope pushed a commit that referenced this pull request Dec 20, 2022

Merge pull request #19 from jsidorenko/giles-remove-balances

d6e5ee3

remove balances

helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023

add substrate-rpc-primitives to use NubmerOrHex in rpc.rs (parity…

f9ae14b

…tech#19)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State Machine: Abstract function execution #19

State Machine: Abstract function execution #19

rphmeier commented Nov 12, 2017 •

edited

Loading

tomusdrw left a comment

tomusdrw Nov 12, 2017

rphmeier Nov 13, 2017

tomusdrw Nov 12, 2017

tomusdrw Nov 12, 2017

tomusdrw Nov 12, 2017

tomusdrw Nov 12, 2017

tomusdrw Nov 12, 2017

tomusdrw Nov 12, 2017

tomusdrw Nov 12, 2017

tomusdrw Nov 13, 2017

tomusdrw Nov 12, 2017

rphmeier Nov 13, 2017

rphmeier Nov 13, 2017

tomusdrw Nov 13, 2017

tomusdrw Nov 12, 2017

rphmeier Nov 13, 2017

tomusdrw Nov 13, 2017

rphmeier commented Nov 13, 2017

tomusdrw Nov 13, 2017

tomusdrw Nov 13, 2017

tomusdrw Nov 13, 2017

		type Error = Error;

		fn storage(&self, _key: &H256) -> Result<&[u8]> {

State Machine: Abstract function execution #19

State Machine: Abstract function execution #19

Conversation

rphmeier commented Nov 12, 2017 • edited Loading

tomusdrw 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

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

rphmeier commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Nov 12, 2017 •

edited

Loading