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

Include suicided accounts in state diff #8297

Merged
merged 3 commits into from
Apr 4, 2018
Merged

Include suicided accounts in state diff #8297

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34f46b9
Include suicided accounts in state diff
sorpaas Apr 3, 2018
dc0190a
Shorten form match -> if let
sorpaas Apr 4, 2018
d9b32e8
Test suicide trace diff in State
sorpaas Apr 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 51 additions & 7 deletions ethcore/src/state/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,16 +855,26 @@ impl<B: Backend> State<B> {
}))
}

fn query_pod(&mut self, query: &PodState) -> trie::Result<()> {
for (address, pod_account) in query.get() {
// Return a list of all touched addresses in cache.
fn touched_addresses(&self) -> Vec<Address> {
assert!(self.checkpoints.borrow().is_empty());
self.cache.borrow().iter().map(|(add, _)| *add).collect()
}

fn query_pod(&mut self, query: &PodState, touched_addresses: &[Address]) -> trie::Result<()> {
let pod = query.get();

for address in touched_addresses {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe use filter_map instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you explain how filter_map would work in this case? Here we need to call ensure_cached for all items in touched_addresses first, so I think it might not save us LOC or code clarity. Do you mean something like this?

let cached_pod_accounts = touched_addresses.filter_map(|address| {
  if !self.ensure_cached(address, RequireCache::Code, true, |a| a.is_some())? {
    None
  } else {
    pod.get(address)
  }
});

for pod_account in cached_pod_accounts {
  for key in pod_account.storage.keys() {
    self.storage_at(address, key)?;
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I think I get it now. Sorry for the confusion, had to go through the code to understand what's going on.

So if I get that correctly:

  1. For every touched_address we need to call ensure_cached to populate the pre-state cache.
  2. Additionally for every touched_address that has an account in pre-state we need to query all storage keys (from the post-state).

The current loop is the most clear way to do it then.

That said, can we get a test for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay I'll try to add a test for this.

Just a minor clarification, for (2), we query all storage keys that has been cached due to state change, but not all of the available keys. Otherwise it can be huge. :p

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added should_trace_diff_suicided_accounts that will test touched_addresses and State::diff_from.

if !self.ensure_cached(address, RequireCache::Code, true, |a| a.is_some())? {
continue
}

// needs to be split into two parts for the refcell code here
// to work.
for key in pod_account.storage.keys() {
self.storage_at(address, key)?;
if let Some(pod_account) = pod.get(address) {
// needs to be split into two parts for the refcell code here
// to work.
for key in pod_account.storage.keys() {
self.storage_at(address, key)?;
}
}
}

Expand All @@ -874,9 +884,10 @@ impl<B: Backend> State<B> {
/// Returns a `StateDiff` describing the difference from `orig` to `self`.
/// Consumes self.
pub fn diff_from<X: Backend>(&self, orig: State<X>) -> trie::Result<StateDiff> {
let addresses_post = self.touched_addresses();
let pod_state_post = self.to_pod();
let mut state_pre = orig;
state_pre.query_pod(&pod_state_post)?;
state_pre.query_pod(&pod_state_post, &addresses_post)?;
Ok(pod_state::diff_pod(&state_pre.to_pod(), &pod_state_post))
}

Expand Down Expand Up @@ -2202,4 +2213,37 @@ mod tests {
assert!(state.exists(&d).unwrap());
assert!(!state.exists(&e).unwrap());
}

#[test]
fn should_trace_diff_suicided_accounts() {
use pod_account;

let a = 10.into();
let db = get_temp_state_db();
let (root, db) = {
let mut state = State::new(db, U256::from(0), Default::default());
state.add_balance(&a, &100.into(), CleanupMode::ForceCreate).unwrap();
state.commit().unwrap();
state.drop()
};

let mut state = State::from_existing(db, root, U256::from(0u8), Default::default()).unwrap();
let original = state.clone();
state.kill_account(&a);

assert_eq!(original.touched_addresses(), vec![]);
assert_eq!(state.touched_addresses(), vec![a]);

let diff = state.diff_from(original).unwrap();
let diff_map = diff.get();
assert_eq!(diff_map.len(), 1);
assert!(diff_map.get(&a).is_some());
assert_eq!(diff_map.get(&a),
pod_account::diff_pod(Some(&PodAccount {
balance: U256::from(100),
nonce: U256::zero(),
code: Some(Default::default()),
storage: Default::default()
}), None).as_ref());
}
}