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

refactor: Move run_transaction params into fields of a TxInput struct and make doc comments of its fields #10769

Merged
merged 14 commits into from
Jul 9, 2019
Merged

refactor: Move run_transaction params into fields of a TxInput struct and make doc comments of its fields #10769

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8a6e644
Question
ltfschoen Jun 14, 2019
6bf9c05
Merge branch 'luke-ethjson-refactor' into luke-ethjson-refactor-test
ltfschoen Jun 20, 2019
8e09d1a
refactor: Further changes for doc comments to be part of public struct
ltfschoen Jun 21, 2019
1ad5e29
refactor: Rename InputData to TxInput for clarity in docs
ltfschoen Jun 21, 2019
2333fbb
docs: Change String to fixed length str
ltfschoen Jun 27, 2019
997d844
refactor: Update evmbin/src/info.rs moving mut into fn declaration
ltfschoen Jun 27, 2019
1a4e7cd
refactor: Update evmbin/src/info.rs moving mut into fn declaration pa…
ltfschoen Jun 27, 2019
ad18fc9
review-fix: Add missing docs to TxInput transaction and trie_spec
ltfschoen Jun 27, 2019
cb81ff9
docs: Improve grammar
ltfschoen Jun 27, 2019
ac83684
review-fix: Destructure tx_input
ltfschoen Jun 27, 2019
ed5e667
WIP
ltfschoen Jun 27, 2019
b828e33
review-fix: Rename variables of InputTx
ltfschoen Jun 27, 2019
2e17bb3
review-fix: Rename idx to tx_index
ltfschoen Jun 27, 2019
4b98474
review-fix: Destructure transaction and make destructured informant mut
ltfschoen Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 39 additions & 31 deletions evmbin/src/info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ pub fn run_action<T: Informant>(
params.code_hash = None;
}
}
run(spec, trie_spec, params.gas, spec.genesis_state(), |mut client| {
run(spec, &trie_spec, params.gas, spec.genesis_state(), |mut client| {
let result = match client.call(params, &mut trace::NoopTracer, &mut informant) {
Ok(r) => (Ok(r.return_data.to_vec()), Some(r.gas_left)),
Err(err) => (Err(err), None),
Expand All @@ -100,51 +100,58 @@ pub fn run_action<T: Informant>(
})
}

/// Input data to run transaction
#[derive(Debug)]
pub struct TxInput<'a, T> {
/// Chain specification name associated with the transaction.
pub name: &'a String,
ltfschoen marked this conversation as resolved.
Show resolved Hide resolved
/// Transaction index from list of transactions within a state root hashes corresponding to a chain.
pub idx: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need all the members to be pub? It is generally avoided so that the code is easier to change in the future. Maybe it could be a way to use the Default trait to avoid supplying all args all the time? It would also let you do fancy things like have MyChainTxInput types where e.g. the name member is pre-set and you can have logic to ensure users don't misuse the api (say, use a ForkSpec that is invalid for the given chain spec name).

Copy link
Contributor Author

@ltfschoen ltfschoen Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't figure out how to use the Default trait, but I've pushed an approach here
in this commit 426a438, but unfortunately I can't work out how to overcome these errors

error: expected expression, found `}` --> evmbin/./src/info.rs:139:3 | 139 | } | ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:146:3
|
146 | }
| ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:153:3
|
153 | }
| ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:160:3
|
160 | }
| ^ expected expression

error: expected expression, found }
--> evmbin/./src/info.rs:167:3
|
167 | }
| ^ expected expression

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:221:7
|
221 | tx_input::std_json_err_only();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:225:7
|
225 | tx_input::std_json_out_only();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:229:7
|
229 | tx_input::std_json_default();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:237:7
|
237 | tx_input::json_default();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0433]: failed to resolve: use of undeclared type or module tx_input
--> evmbin/./src/main.rs:241:7
|
241 | tx_input::simple_default();
| ^^^^^^^^ use of undeclared type or module tx_input

error[E0308]: mismatched types
--> evmbin/./src/info.rs:137:15
|
137 | informant: display::std_json::Informant::err_only(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::std_json::Informant
|
= note: expected type T
found type display::std_json::Informant<std::io::Stderr, std::io::Stderr>

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:136:3
|
136 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:144:15
|
144 | informant: display::std_json::Informant::out_only(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::std_json::Informant
|
= note: expected type T
found type display::std_json::Informant<std::io::Stdout, std::io::Stdout>

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:143:3
|
143 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:151:15
|
151 | informant: display::std_json::Informant::default(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::std_json::Informant
|
= note: expected type T
found type display::std_json::Informant<std::io::Stderr, std::io::Stdout>

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:150:3
|
150 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:158:15
|
158 | informant: display::json::Informant::default(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::json::Informant
|
= note: expected type T
found type display::json::Informant

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:157:3
|
157 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error[E0308]: mismatched types
--> evmbin/./src/info.rs:165:15
|
165 | informant: display::simple::Informant::default(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct display::simple::Informant
|
= note: expected type T
found type display::simple::Informant

error[E0063]: missing fields env_info, fork_spec_name, post_root and 5 other fields in initializer of info::TxInput<'_, _>
--> evmbin/./src/info.rs:164:3
|
164 | TxInput {
| ^^^^^^^ missing env_info, fork_spec_name, post_root and 5 other fields

error: aborting due to 20 previous errors

/// Fork specification (i.e. Constantinople, EIP150, EIP158, etc).
pub spec: &'a ethjson::spec::ForkSpec,
/// State of all accounts in the system that is a binary tree mapping of each account address to account data
/// that is expressed as Plain Old Data containing the account balance, account nonce, account code in bytes,
/// and the account storage binary tree map.
pub pre_state: &'a pod_state::PodState,
/// State root hash associated with the transaction.
pub post_root: H256,
/// Client environment information associated with the transaction's chain specification.
pub env_info: &'a client::EnvInfo,
pub transaction: transaction::SignedTransaction,
tomusdrw marked this conversation as resolved.
Show resolved Hide resolved
/// JSON formatting informant.
pub informant: T,
pub trie_spec: &'a TrieSpec,
tomusdrw marked this conversation as resolved.
Show resolved Hide resolved
}

/// Execute given transaction and verify resulting state root.
pub fn run_transaction<T: Informant>(
// Chain specification name associated with the transaction.
name: &str,
// Transaction index from list of transactions within a state root hashes corresponding to a chain.
idx: usize,
// Fork specification (i.e. Constantinople, EIP150, EIP158, etc).
spec: &ethjson::spec::ForkSpec,
// State of all accounts in the system that is a binary tree mapping of each account address to account data
// that is expressed as Plain Old Data containing the account balance, account nonce, account code in bytes,
// and the account storage binary tree map.
pre_state: &pod_state::PodState,
// State root hash associated with the transaction.
post_root: H256,
// Client environment information associated with the transaction's chain specification.
env_info: &client::EnvInfo,
transaction: transaction::SignedTransaction,
// JSON formatting informant.
mut informant: T,
trie_spec: TrieSpec,
tx_input: TxInput<T>
ltfschoen marked this conversation as resolved.
Show resolved Hide resolved
) {
let spec_name = format!("{:?}", spec).to_lowercase();
let spec = match EvmTestClient::spec_from_json(spec) {
let mut tx_input = tx_input;
ltfschoen marked this conversation as resolved.
Show resolved Hide resolved
let spec_name = format!("{:?}", tx_input.spec).to_lowercase();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should tx_input.spec perhaps be named fork_spec (and spec_name be fork_spec_name)?

Copy link
Contributor Author

@ltfschoen ltfschoen Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes great idea. I definitely think fork_spec is a better name than just spec. Yes I think we should rename spec_name to fork_spec_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed commits with those changes in commits 2e17bb3 and b828e33

let spec = match EvmTestClient::spec_from_json(&tx_input.spec) {
Some(spec) => {
informant.before_test(&format!("{}:{}:{}", name, spec_name, idx), "starting");
tx_input.informant.before_test(&format!("{}:{}:{}", &tx_input.name, &spec_name, tx_input.idx), "starting");
spec
},
None => {
informant.before_test(&format!("{}:{}:{}", name, spec_name, idx), "skipping because of missing spec");
tx_input.informant.before_test(&format!("{}:{}:{}", &tx_input.name, spec_name, &tx_input.idx), "skipping because of missing spec");
return;
},
};

informant.set_gas(env_info.gas_limit);
tx_input.informant.set_gas(tx_input.env_info.gas_limit);

let mut sink = informant.clone_sink();
let result = run(&spec, trie_spec, transaction.gas, pre_state, |mut client| {
let result = client.transact(env_info, transaction, trace::NoopTracer, informant);
let mut sink = tx_input.informant.clone_sink();
let result = run(&spec, tx_input.trie_spec, tx_input.transaction.gas, &tx_input.pre_state, |mut client| {
let result = client.transact(&tx_input.env_info, tx_input.transaction, trace::NoopTracer, tx_input.informant);
match result {
Ok(TransactSuccess { state_root, gas_left, output, vm_trace, end_state, .. }) => {
if state_root != post_root {
if state_root != tx_input.post_root {
(Err(EvmTestError::PostCondition(format!(
"State root mismatch (got: {:#x}, expected: {:#x})",
state_root,
post_root,
tx_input.post_root,
))), state_root, end_state, Some(gas_left), None)
} else {
(Ok(output), state_root, end_state, Some(gas_left), vm_trace)
Expand All @@ -168,14 +175,15 @@ fn dump_state(state: &state::State<state_db::StateDB>) -> Option<pod_state::PodS
/// Execute EVM with given `ActionParams`.
pub fn run<'a, F, X>(
spec: &'a spec::Spec,
trie_spec: TrieSpec,
trie_spec: &'a TrieSpec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change strictly required for something? It seems you have to clone the spec anyway, so maybe better take it by value than by reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried what you recommended but I haven't been able to implement it successfully as it opened a can of worms. This is where I got to ed5e667

Compiling evmbin v0.1.0 (/home/ltfschoen/code/github/paritytech/parity-ethereum-master/evmbin) error[E0504]: cannot move `params` into closure because it is borrowed --> evmbin/./src/info.rs:95:34 | 94 | run(spec, trie_spec, ¶ms.gas, spec.genesis_state(), |mut client| { | ---------- borrow of `params.gas` occurs here 95 | let result = match client.call(params, &mut trace::NoopTracer, &mut informant) { | ^^^^^^ move into closure occurs here

error[E0504]: cannot move tx_input into closure because it is borrowed
--> evmbin/./src/info.rs:153:43
|
152 | let result = run(&spec_checked, trie_spec, &tx_input.transaction.gas, &pre_state, |mut client| {
| -------------------- borrow of tx_input.transaction occurs here
153 | let result = client.transact(&env_info, tx_input.transaction, trace::NoopTracer, tx_input.informant);
| ^^^^^^^^ move into closure occurs here

error[E0382]: capture of partially moved value: tx_input
--> evmbin/./src/info.rs:153:43
|
136 | let TxInput { name, idx, spec, pre_state, post_root, env_info, trie_spec, .. } = tx_input;
| --------- value moved here
...
153 | let result = client.transact(&env_info, tx_input.transaction, trace::NoopTracer, tx_input.informant);
| ^^^^^^^^ value captured here after move
|
= note: move occurs because tx_input.trie_spec has type ethcore::TrieSpec, which does not implement the Copy trait

error: aborting due to 3 previous errors

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, if you destructure tx_input you can't really move it around. just destructure transaction as well and pass it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that worked, very slick! I've pushed changes in this commit 4b98474

initial_gas: U256,
pre_state: &'a pod_state::PodState,
run: F,
) -> RunResult<X> where
F: FnOnce(EvmTestClient) -> (Result<Vec<u8>, EvmTestError>, H256, Option<pod_state::PodState>, Option<U256>, Option<X>),
{
let do_dump = trie_spec == TrieSpec::Fat;
let do_dump = *trie_spec == TrieSpec::Fat;
let trie_spec = trie_spec.clone();

let mut test_client = EvmTestClient::from_pod_state_with_trie(spec, pre_state.clone(), trie_spec)
.map_err(|error| Failure {
Expand Down
94 changes: 86 additions & 8 deletions evmbin/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ use vm::{ActionParams, CallType};
mod info;
mod display;

use info::Informant;
use info::{Informant, TxInput};

const USAGE: &'static str = r#"
EVM implementation for Parity.
Expand Down Expand Up @@ -206,24 +206,79 @@ fn run_state_test(args: Args) {
// for CLI option `--std-dump-json` or `--std-json`.
if args.flag_std_dump_json || args.flag_std_json {
if args.flag_std_err_only {
let tx_input = TxInput {
name: &name,
idx,
spec: &spec,
pre_state: &pre,
post_root,
env_info: &env_info,
transaction,
informant: display::std_json::Informant::err_only(),
trie_spec: &trie_spec,
};
// Use Standard JSON informant with err only
info::run_transaction(&name, idx, &spec, &pre, post_root, &env_info, transaction, display::std_json::Informant::err_only(), trie_spec)
info::run_transaction(tx_input)
} else if args.flag_std_out_only {
let tx_input = TxInput {
name: &name,
idx,
spec: &spec,
pre_state: &pre,
post_root,
env_info: &env_info,
transaction,
informant: display::std_json::Informant::out_only(),
trie_spec: &trie_spec,
};
// Use Standard JSON informant with out only
info::run_transaction(&name, idx, &spec, &pre, post_root, &env_info, transaction, display::std_json::Informant::out_only(), trie_spec)
info::run_transaction(tx_input)
} else {
let tx_input = TxInput {
name: &name,
idx,
spec: &spec,
pre_state: &pre,
post_root,
env_info: &env_info,
transaction,
informant: display::std_json::Informant::default(),
trie_spec: &trie_spec,
};
// Use Standard JSON informant default
info::run_transaction(&name, idx, &spec, &pre, post_root, &env_info, transaction, display::std_json::Informant::default(), trie_spec)
info::run_transaction(tx_input)
}
} else {
// Execute the given transaction and verify resulting state root
// for CLI option `--json`.
if args.flag_json {
let tx_input = TxInput {
name: &name,
idx,
spec: &spec,
pre_state: &pre,
post_root,
env_info: &env_info,
transaction,
informant: display::json::Informant::default(),
trie_spec: &trie_spec,
};
// Use JSON informant
info::run_transaction(&name, idx, &spec, &pre, post_root, &env_info, transaction, display::json::Informant::default(), trie_spec)
info::run_transaction(tx_input)
} else {
let tx_input = TxInput {
name: &name,
idx,
spec: &spec,
pre_state: &pre,
post_root,
env_info: &env_info,
transaction,
informant: display::simple::Informant::default(),
trie_spec: &trie_spec,
};
// Use Simple informant
info::run_transaction(&name, idx, &spec, &pre, post_root, &env_info, transaction, display::simple::Informant::default(), trie_spec)
info::run_transaction(tx_input)
}
}
}
Expand Down Expand Up @@ -417,6 +472,7 @@ mod tests {
use types::transaction;

use info;
use info::{TxInput};
use display;

#[derive(Debug, PartialEq, Deserialize)]
Expand Down Expand Up @@ -537,7 +593,18 @@ mod tests {
let post_root = H256::from_str("99a450d8ce5b987a71346d8a0a1203711f770745c7ef326912e46761f14cd764").unwrap();
let trie_spec = TrieSpec::Secure; // TrieSpec::Fat for --std_dump_json
let transaction: transaction::SignedTransaction = multitransaction.select(&state.indexes).into();
info::run_transaction(&name, idx, &spec, &pre, post_root, &env_info, transaction, informant, trie_spec)
let tx_input = TxInput {
name: &name,
idx,
spec: &spec,
pre_state: &pre,
post_root,
env_info: &env_info,
transaction,
informant: informant,
trie_spec: &trie_spec,
};
info::run_transaction(tx_input)
}
}
}
Expand Down Expand Up @@ -572,7 +639,18 @@ mod tests {
let post_root = H256::from_str("0xde1d3953b508913c6e3e9bd412cd50daf60bb177517e5d1e8ccb0dab193aed03").unwrap();
let trie_spec = TrieSpec::Secure; // TrieSpec::Fat for --std_dump_json
let transaction: transaction::SignedTransaction = multitransaction.select(&state.indexes).into();
info::run_transaction(&name, idx, &spec, &pre, post_root, &env_info, transaction, informant, trie_spec)
let tx_input = TxInput {
name: &name,
idx,
spec: &spec,
pre_state: &pre,
post_root,
env_info: &env_info,
transaction,
informant: informant,
trie_spec: &trie_spec,
};
info::run_transaction(tx_input)
}
}
}
Expand Down