-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add a optional json dump state to evm-bin #9706
Changes from 6 commits
b06a2ad
08f4d13
7dccffa
280e97b
fea5dd6
a6feb4e
8dcab6c
8e40d2e
c4e0ed6
a4f7c65
0f66e39
6a6e019
e9add15
f4d4700
78470ab
ab3c485
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Takes a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Takes a |
||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
} | ||
} | ||
|
@@ -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), | ||
}; | ||
} | ||
|
||
|
@@ -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, | ||
|
@@ -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, | ||
}, | ||
} | ||
} | ||
|
@@ -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>, | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. odd whitespace here, should be
|
||
serializer.collect_str(&format_args!("0x{}",opt_bytes.as_ref().map(|b|b.to_hex()).unwrap_or("".to_string()))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not implement it, alas There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider to replace |
||
} | ||
|
||
impl PodAccount { | ||
/// Convert Account to a PodAccount. | ||
/// NOTE: This will silently fail unless the account is fully cached. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not related to this PR, but there's an extra space between |
||
|
||
impl PodState { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this TODO still relevant after this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -952,8 +952,78 @@ impl<B: Backend> State<B> { | |
})) | ||
} | ||
|
||
#[cfg(feature="to-pod-full")] | ||
/// Populate a PodAccount map from this state. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems a bit rough to panic here, why not return an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
} | ||
} | ||
|
||
PodState::from(result) | ||
} | ||
|
||
fn account_to_pod_account(&self, account: &mut Account, address: &Address) -> PodAccount { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be |
||
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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this case possible? Previously we called Should we change this to an assert? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should use |
||
.map(|root| self.factories.trie.readonly(accountdb.as_hashdb(), &root) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't quite get why we need There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would you mind to try this:
Not sure whether that would solve the lifetime issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cheme Ah sorry. Yeah you're right. |
||
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`. | ||
|
@@ -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)) | ||
} | ||
|
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.
"Enables slow
to_pod_full
method for use in tests and evmbin"?