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

Add a optional json dump state to evm-bin #9706

Merged
merged 16 commits into from
Nov 25, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions ethcore/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ unexpected = { path = "../util/unexpected" }
journaldb = { path = "../util/journaldb" }
keccak-hasher = { path = "../util/keccak-hasher" }
kvdb-rocksdb = "0.1.3"
serde = "1.0"
serde_derive = "1.0"
tempdir = {version="0.3", optional = true}

[target.'cfg(any(target_os = "linux", target_os = "macos", target_os = "windows", target_os = "android"))'.dependencies]
Expand Down Expand Up @@ -110,3 +112,5 @@ test-heavy = []
benches = []
# Compile test helpers
test-helpers = ["tempdir"]
# Slow method to-pod-full for evmbin or test only.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Enables slow to_pod_full method for use in tests and evmbin"?

to-pod-full = []
54 changes: 45 additions & 9 deletions ethcore/src/client/evm_test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ use ethjson::spec::ForkSpec;
pub struct EvmTestClient<'a> {
state: state::State<state_db::StateDB>,
spec: &'a spec::Spec,
dump_state: fn(&state::State<state_db::StateDB>) -> Option<pod_state::PodState>,
}

fn no_dump_state(_: &state::State<state_db::StateDB>) -> Option<pod_state::PodState> {
None
}

impl<'a> fmt::Debug for EvmTestClient<'a> {
Expand All @@ -92,32 +97,51 @@ impl<'a> EvmTestClient<'a> {
}
}

/// Change default function for dump state (default does not dump)
pub fn set_dump_state_fn(&mut self, dump_state: fn(&state::State<state_db::StateDB>) -> Option<pod_state::PodState>) {
self.dump_state = dump_state;
}

/// Creates new EVM test client with in-memory DB initialized with genesis of given Spec.
pub fn new(spec: &'a spec::Spec) -> Result<Self, EvmTestError> {
let factories = Self::factories();
/// You can specify a specific trie kind.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Takes a TrieSpec to set the type of trie: Generic, Fat, Secure"

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 am skipping the different type of trie (chance of leading to outdated comment in case of change on the variants).

pub fn new_with_trie(spec: &'a spec::Spec, trie_spec: trie::TrieSpec) -> Result<Self, EvmTestError> {
let factories = Self::factories(trie_spec);
let state = Self::state_from_spec(spec, &factories)?;

Ok(EvmTestClient {
state,
spec,
dump_state: no_dump_state,
})
}

/// Creates new EVM test client with in-memory DB initialized with genesis of given Spec.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/with in-memory/with an in-memory/

("…of given kind": but the code says it's using the Secure kind so I think the doc needs to change a bit)

pub fn new(spec: &'a spec::Spec) -> Result<Self, EvmTestError> {
Self::new_with_trie(spec, trie::TrieSpec::Secure)
}

/// Creates new EVM test client with in-memory DB initialized with given PodState.
pub fn from_pod_state(spec: &'a spec::Spec, pod_state: pod_state::PodState) -> Result<Self, EvmTestError> {
let factories = Self::factories();
/// You can specify a specific trie kind.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"Takes a TrieSpec to set the type of trie: Generic, Fat, Secure"

pub fn from_pod_state_with_trie(spec: &'a spec::Spec, pod_state: pod_state::PodState, trie_spec: trie::TrieSpec) -> Result<Self, EvmTestError> {
let factories = Self::factories(trie_spec);
let state = Self::state_from_pod(spec, &factories, pod_state)?;

Ok(EvmTestClient {
state,
spec,
dump_state: no_dump_state,
})
}

fn factories() -> Factories {
/// Creates new EVM test client with in-memory DB initialized with given PodState.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/with in-memory/with an in-memory/

pub fn from_pod_state(spec: &'a spec::Spec, pod_state: pod_state::PodState) -> Result<Self, EvmTestError> {
Self::from_pod_state_with_trie(spec, pod_state, trie::TrieSpec::Secure)
}

fn factories(trie_spec: trie::TrieSpec) -> Factories {
Factories {
vm: factory::VmFactory::new(VMType::Interpreter, 5 * 1024),
trie: trie::TrieFactory::new(trie::TrieSpec::Secure),
trie: trie::TrieFactory::new(trie_spec),
accountdb: Default::default(),
}
}
Expand Down Expand Up @@ -223,6 +247,7 @@ impl<'a> EvmTestClient<'a> {
return TransactResult::Err {
state_root: *self.state.root(),
error: error.into(),
end_state: (self.dump_state)(&self.state),
};
}

Expand All @@ -247,12 +272,17 @@ impl<'a> EvmTestClient<'a> {
&None,
false
).ok();

self.state.commit().ok();

let state_root = *self.state.root();

let end_state = (self.dump_state)(&self.state);

match result {
Ok(result) => {
TransactResult::Ok {
state_root: *self.state.root(),
state_root,
gas_left: initial_gas - result.receipt.gas_used,
outcome: result.receipt.outcome,
output: result.output,
Expand All @@ -263,12 +293,14 @@ impl<'a> EvmTestClient<'a> {
Some(executive::contract_address(scheme, &transaction.sender(), &transaction.nonce, &transaction.data).0)
} else {
None
}
},
end_state,
}
},
Err(error) => TransactResult::Err {
state_root: *self.state.root(),
state_root,
error,
end_state,
},
}
}
Expand All @@ -295,12 +327,16 @@ pub enum TransactResult<T, V> {
logs: Vec<log_entry::LogEntry>,
/// outcome
outcome: receipt::TransactionOutcome,
/// end state if needed
end_state: Option<pod_state::PodState>,
},
/// Transaction failed to run
Err {
/// State root
state_root: H256,
/// Execution error
error: ::error::Error,
/// end state if needed
end_state: Option<pod_state::PodState>,
},
}
2 changes: 1 addition & 1 deletion ethcore/src/json_tests/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ pub fn json_chain_test<H: FnMut(&str, HookType)>(json_data: &[u8], start_stop_ho
flushln!("{} fail", info);
failed.push(name.clone());
},
Ok(TransactResult::Err { state_root, ref error }) if state_root != post_root => {
Ok(TransactResult::Err { state_root, ref error, .. }) if state_root != post_root => {
println!("{} !!! State mismatch (got: {}, expect: {}", info, state_root, post_root);
println!("{} !!! Execution error: {:?}", info, error);
flushln!("{} fail", info);
Expand Down
4 changes: 4 additions & 0 deletions ethcore/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ extern crate vm;
extern crate wasm;
extern crate memory_cache;
extern crate journaldb;
extern crate serde;
#[cfg(any(test, feature = "json-tests", feature = "test-helpers"))]
extern crate tempdir;

Expand All @@ -135,6 +136,8 @@ extern crate macros;
extern crate rlp_derive;
#[macro_use]
extern crate trace_time;
#[macro_use]
extern crate serde_derive;

#[cfg_attr(test, macro_use)]
extern crate evm;
Expand Down Expand Up @@ -185,3 +188,4 @@ pub use types::*;
pub use executive::contract_address;
pub use evm::CreateContractAddress;
pub use blockchain::{BlockChainDB, BlockChainDBHandler};
pub use trie::TrieSpec;
10 changes: 9 additions & 1 deletion ethcore/src/pod_account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,29 @@ use state::Account;
use ethjson;
use types::account_diff::*;
use rlp::{self, RlpStream};
use serde::Serializer;
use rustc_hex::ToHex;

#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
/// An account, expressed as Plain-Old-Data (hence the name).
/// Does not have a DB overlay cache, code hash or anything like that.
pub struct PodAccount {
/// The balance of the account.
pub balance: U256,
/// The nonce of the account.
pub nonce: U256,
#[serde(serialize_with="opt_bytes_to_hex")]
/// The code of the account or `None` in the special case that it is unknown.
pub code: Option<Bytes>,
/// The storage of the account.
pub storage: BTreeMap<H256, H256>,
}

fn opt_bytes_to_hex<S>(opt_bytes: &Option<Bytes>, serializer: S) -> Result<S::Ok, S::Error>
where S: Serializer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

odd whitespace here, should be

where
    S: Serializer
{

serializer.collect_str(&format_args!("0x{}",opt_bytes.as_ref().map(|b|b.to_hex()).unwrap_or("".to_string())))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bytes does not impl ToLowerHex (so you can use {:#x})? If not that's something we should fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not implement it, alas Bytes is only a type alias for Vec<u8>, so it should be quite a refact to fix it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider to replace map().unwrap_or() with map_or_else!

}

impl PodAccount {
/// Convert Account to a PodAccount.
/// NOTE: This will silently fail unless the account is fully cached.
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/pod_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use types::state_diff::StateDiff;
use ethjson;

/// State of all accounts in the system expressed in Plain Old Data.
#[derive(Debug, Clone, PartialEq, Eq, Default)]
#[derive(Debug, Clone, PartialEq, Eq, Default, Serialize)]
pub struct PodState (BTreeMap<Address, PodAccount>);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this PR, but there's an extra space between PodState and (BTreeMap.


impl PodState {
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/state/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ mod tests {
assert!(raw.len() > compact_vec.len());
let again_raw = decompress(&compact_vec, snapshot_swapper());
assert_eq!(raw, again_raw.into_vec());
}
}

#[test]
fn storage_at() {
Expand Down
76 changes: 73 additions & 3 deletions ethcore/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,7 @@ impl<B: Backend> State<B> {
}

/// Populate a PodAccount map from this state.
pub fn to_pod(&self) -> PodState {
fn to_pod_cache(&self) -> PodState {
assert!(self.checkpoints.borrow().is_empty());
// TODO: handle database rather than just the cache.
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 TODO still relevant after this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, the new method and renaming makes it somehow explicit. (in fact I should have read it, it was a good comment because it spotted the fact that fatdb would be need).

// will need fat db.
Expand All @@ -952,8 +952,78 @@ impl<B: Backend> State<B> {
}))
}

#[cfg(feature="to-pod-full")]
/// Populate a PodAccount map from this state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add docs here that this requires FatDB.

/// Warning this is not for real time use.
/// Use of this method requires FatDB mode to be able
/// to iterate on accounts.
pub fn to_pod_full(&self) -> PodState {

assert!(self.checkpoints.borrow().is_empty());
assert!(self.factories.trie.is_fat());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a bit rough to panic here, why not return an error?

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'd rather panic here, unless/untill we got coding rules that force error checking (I did not check it currently), forcing some 'clippy' rules in ci could really be an idea.
In the context of this function (very specific use, not in critical context), I think the assert is ok (the function should never be called if not 'fat' and if it is called it is a bug to fix).


let mut result = BTreeMap::new();

let trie = match self.factories.trie.readonly(self.db.as_hashdb(), &self.root) {
Ok(trie) => trie,
_ => {
trace!(target: "state", "to_pod_full: Couldn't open the DB");
return PodState::from(result);
}
};

let iter = match trie.iter() {
Ok(iter) => iter,
_ => return PodState::from(result),
};

// put trie in cache
for item in iter {
if let Ok((addr, _dbval)) = item {
let address = Address::from_slice(&addr);
let _ = self.require(&address, true);
}
}

// Resolve missing part
for (add, opt) in self.cache.borrow_mut().iter_mut() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to borrow a mutable reference here? I don't think we're changing anything in the cache from this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing, it is from previous code that did not use qrequire', will be removed.

if let Some(ref mut acc) = opt.account {
let pod_account = self.account_to_pod_account(acc, add);
result.insert(add.clone(), pod_account);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Address is copy, so we can just do *add here.

}
}

PodState::from(result)
}

fn account_to_pod_account(&self, account: &mut Account, address: &Address) -> PodAccount {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be #[cfg(feature="to-pod-full")] as well. Also would be great if we can include a short-line docs describing its behavior (merging all storage items into pod account).

let mut pod_storage = BTreeMap::new();
let addr_hash = account.address_hash(address);
let accountdb = self.factories.accountdb.readonly(self.db.as_hashdb(), addr_hash);

if account.code_size().is_none() {
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 case possible? Previously we called State::require with require_code to be true. So code_size cannot be None?

Should we change this to an assert?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like if this ever happens, then it means previously we had a DB failure caching the code. So below we shouldn't call cache_code again. Would suggest we either change this to just skip code and print an warning or return an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, seems impossible, it is a remnant from my previous code where I did not use 'require'.

let _ = account.cache_code(accountdb.as_hashdb());
}

account.storage_root()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use base_storage_root here.

.map(|root| self.factories.trie.readonly(accountdb.as_hashdb(), &root)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't quite get why we need map here. I think we can use and_then or similar if we're not interested in the return value.

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 will change to returning Result, it makes things looks a bit nicer.

.map(|trie| trie.iter()
.map(|iter| iter.for_each(|o_kv|{
if let Ok((key, val)) = o_kv {
pod_storage.insert(H256::from(&key[..]), H256::from(&val[..]));
}
}
))));

let mut pod_account = PodAccount::from_account(&account);
// cached one first
pod_storage.append(&mut pod_account.storage);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use self.storage_at instead. The issue is that what we have in trie and what we have in cache might differ, and we need to use the value in cache.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Reuse of storage_at was not easy in this case (could not iterate and
use the method at the same time (refcell mutable borrow panics) so
keeping code as is.

Would you mind to try this:

  • Collect all storage items from trie first in a Vec<H256>.
  • Call storage_at at the end of the round.

Not sure whether that would solve the lifetime issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this will works (but I kinda wanted to avoid that :) ), I will switch to this version for the sake of using similar code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please use self.storage_at instead. The issue is that what we have in trie and what we have in cache might differ, and we need to use the value in cache.

Doesn't the merge ('append') of both storage do that? (pod_account got values from storage_change replacing value read from trie). Can value from 'original_storage_cache' of 'Account' differs from trie value (this field is lost in the process) ?

Switching to 'storage_at' anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cheme Ah sorry. Yeah you're right. PodAccount::from would only print things on storage_changes, so using State::storage_at won't be sufficient for our case. I think the current method works.

pod_account.storage = pod_storage;
pod_account
}

/// Populate a PodAccount map from this state, with another state as the account and storage query.
pub fn to_pod_diff<X: Backend>(&mut self, query: &State<X>) -> TrieResult<PodState> {
fn to_pod_diff<X: Backend>(&mut self, query: &State<X>) -> TrieResult<PodState> {
assert!(self.checkpoints.borrow().is_empty());

// Merge PodAccount::to_pod for cache of self and `query`.
Expand Down Expand Up @@ -1008,7 +1078,7 @@ impl<B: Backend> State<B> {
/// Returns a `StateDiff` describing the difference from `orig` to `self`.
/// Consumes self.
pub fn diff_from<X: Backend>(&self, mut orig: State<X>) -> TrieResult<StateDiff> {
let pod_state_post = self.to_pod();
let pod_state_post = self.to_pod_cache();
let pod_state_pre = orig.to_pod_diff(self)?;
Ok(pod_state::diff_pod(&pod_state_pre, &pod_state_post))
}
Expand Down
1 change: 1 addition & 0 deletions ethcore/src/trace/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ pub trait VMTracer: Send {

/// Consumes self and returns the VM trace.
fn drain(self) -> Option<Self::Output>;

}

/// `DbExtras` provides an interface to query extra data which is not stored in tracesdb,
Expand Down
2 changes: 1 addition & 1 deletion ethcore/wasm/run/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl Fail {
}

impl fmt::Display for Fail {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
use self::Fail::*;
match *self {
Return { ref expected, ref actual } =>
Expand Down
2 changes: 1 addition & 1 deletion evmbin/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ path = "./src/main.rs"
[dependencies]
docopt = "0.8"
env_logger = "0.5"
ethcore = { path = "../ethcore", features = ["test-helpers", "json-tests"] }
ethcore = { path = "../ethcore", features = ["test-helpers", "json-tests", "to-pod-full"] }
ethjson = { path = "../json" }
parity-bytes = "0.1"
ethcore-transaction = { path = "../ethcore/transaction" }
Expand Down
6 changes: 5 additions & 1 deletion evmbin/src/display/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ impl Informant {
}

impl vm::Informant for Informant {
type Sink = ();

fn before_test(&mut self, name: &str, action: &str) {
println!("{}", json!({"action": action, "test": name}));
}
Expand All @@ -66,7 +68,9 @@ impl vm::Informant for Informant {
self.gas_used = gas;
}

fn finish(result: vm::RunResult<Self::Output>) {
fn clone_sink(&self) -> Self::Sink { () }

fn finish(result: vm::RunResult<Self::Output>, _sink: &mut Self::Sink) {
match result {
Ok(success) => {
for trace in success.traces.unwrap_or_else(Vec::new) {
Expand Down
7 changes: 6 additions & 1 deletion evmbin/src/display/simple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,16 @@ use info as vm;
pub struct Informant;

impl vm::Informant for Informant {

type Sink = ();

fn before_test(&mut self, name: &str, action: &str) {
println!("Test: {} ({})", name, action);
}

fn finish(result: vm::RunResult<Self::Output>) {
fn clone_sink(&self) -> Self::Sink { () }

fn finish(result: vm::RunResult<Self::Output>, _sink: &mut Self::Sink) {
match result {
Ok(success) => {
println!("Output: 0x{}", success.output.to_hex());
Expand Down
Loading