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

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Apr 3, 2018

Fixes #8158

The bug happens because PodState only records addresses that exist. In state diff, it uses the post PodState to populate the cache of pre-state, but missed killed addresses. So we simply collect all touched address directly from cache, and then use that to call query_pod.

// to work.
for key in pod_account.storage.keys() {
self.storage_at(address, key)?;
match pod.get(address) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if let Some(pod_account) = pod.get(address) ?

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.

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. B0-patch labels Apr 4, 2018
@debris debris merged commit 9aaedad into openethereum:master Apr 4, 2018
@5chdn 5chdn added this to the 1.11 milestone Apr 4, 2018
@tomusdrw tomusdrw mentioned this pull request Apr 10, 2018
tomusdrw pushed a commit that referenced this pull request Apr 10, 2018
* Include suicided accounts in state diff

* Shorten form match -> if let

* Test suicide trace diff in State
tomusdrw pushed a commit that referenced this pull request Apr 10, 2018
* Include suicided accounts in state diff

* Shorten form match -> if let

* Test suicide trace diff in State
@tomusdrw tomusdrw mentioned this pull request Apr 10, 2018
5chdn pushed a commit that referenced this pull request Apr 10, 2018
* Warp-only sync with warp-barrier [blocknumber] flag. (#8228)

* Warp-only sync with warp-after [blocknumber] flag.

* Fix tests.

* Fix configuration tests.

* Rename to warp barrier.

* Allow unsafe js eval on Parity Wallet. (#8204)

* Update musicoin spec in line with gmc v2.6.2 (#8242)

* Supress TemporaryInvalid verification failures. (#8256)

* Include suicided accounts in state diff (#8297)

* Include suicided accounts in state diff

* Shorten form match -> if let

* Test suicide trace diff in State

* replace_home for password_files, reserved_peers and log_file (#8324)

* replace_home for password_files, reserved_peers and log_file

* typo: arg_log_file is Option

* Enable UI by default, but only display info page.

* Fix test.

* Fix naming and remove old todo.

* Change "wallet" with "browser UI"
5chdn pushed a commit that referenced this pull request Apr 10, 2018
* Update musicoin spec in line with gmc v2.6.2 (#8242)

* Supress TemporaryInvalid verification failures. (#8256)

* Include suicided accounts in state diff (#8297)

* Include suicided accounts in state diff

* Shorten form match -> if let

* Test suicide trace diff in State

* replace_home for password_files, reserved_peers and log_file (#8324)

* replace_home for password_files, reserved_peers and log_file

* typo: arg_log_file is Option

* Bump version in util/version
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants