don't delete account and storage entries on commit #126

mattsse · 2022-06-10T10:12:32Z

As discovered here foundry-rs/foundry#1894

deleting storage or account entries does not play nice with fork mode which basically does:

if !db.contains(acc) {
    // fetch from remote 
}

So deleting accounts/slots (when empty) would result in the fork's account/slot being served and not 0 or the empty account.

Solution

on CacheDb::commit don't delete AccountInfo or storage entries in the CacheDb's maps
also bumps to v1.6.0

mattsse · 2022-06-10T10:27:27Z

hmm, perhaps keeping destroyed accounts has some side-effects?

rakita

selfdestruct, what else to say. :)
I am merkelizing everything in the state when running tests. I think i am skipping empty/zero values but I am not sure tbh, will check later.

rakita · 2022-06-10T11:01:44Z

crates/revm/src/db/in_memory_db.rs

@@ -87,26 +87,12 @@ impl<ExtDB: DatabaseRef> CacheDB<ExtDB> {
 impl<ExtDB: DatabaseRef> DatabaseCommit for CacheDB<ExtDB> {
    fn commit(&mut self, changes: Map<H160, Account>) {
        for (add, acc) in changes {
-            if acc.is_empty() || matches!(acc.filth, Filth::Destroyed) {
-                self.cache.remove(&add);
-                self.storage.remove(&add);


Here, you need to set cache to Account::default and iterate over storage.get(add).iter() and set them all to zero

@rakita you said set "cache to Account::default" but we're currently setting the changes? is that right, or a mistake?

@gakonst changes are added after, because of this: #126 (comment)

rakita · 2022-06-10T11:05:18Z

crates/revm/src/db/in_memory_db.rs

-                self.insert_cache(add, acc.info);
-                let storage = self.storage.entry(add).or_default();
-                if acc.filth.abandon_old_storage() {
-                    storage.clear();


here iterate over all storage and set all values to zero

rakita · 2022-06-10T11:06:13Z

crates/revm/src/db/in_memory_db.rs

-                for (index, value) in acc.storage {
-                    if value.is_zero() {
-                        storage.remove(&index);
-                    } else {
-                        storage.insert(index, value);
-                    }
-                }
-                if storage.is_empty() {
-                    self.storage.remove(&add);
-                }


for this for and if use extend

rakita · 2022-06-10T12:34:24Z

@mattsse in state_merkle_trie_root skip AccountInfos that are is_empty

mattsse · 2022-06-10T12:58:42Z

I guess we'd also need to skip 0 in storage now when computing sec_trie_root, right?

rakita · 2022-06-10T14:09:16Z

I guess we'd also need to skip 0 in storage now when computing sec_trie_root, right?

zero storage slots are already skipped. I am not sure why it is not working. I vaguely remember there are some funky things with merkelization in tests where some empty accounts get merkelized I will need to manually run it and compare it.

rakita · 2022-06-10T20:30:53Z

@mattsse
The problem was with state cleanup that was introduced way back in eth: https://eips.ethereum.org/EIPS/eip-161
The state can contain "empty" accounts, with this EIP, only accounts that are accessed are going to be removed from state.

For tests that means that there are preloaded empty accounts and not all of them are touched (called) and those empty accounts need to appear in Merkle trie.

I added changes field to CacheDB and I am open to modifying this to a more flexible form needed, but at least we know what is going on.

gakonst · 2022-06-11T17:07:23Z

crates/revm/src/db/in_memory_db.rs

@@ -86,26 +100,25 @@ impl<ExtDB: DatabaseRef> CacheDB<ExtDB> {
 // TODO It is currently only committing to cached in-memory DB
 impl<ExtDB: DatabaseRef> DatabaseCommit for CacheDB<ExtDB> {
    fn commit(&mut self, changes: Map<H160, Account>) {
+        // clear storage by setting all values to `0`
+        fn clear_storage(storage: &mut HashMap<U256, U256>) {
+            storage.values_mut().for_each(|val| {


something to consider in the future, maybe we want to convert these to rayon parallel iters? maybe not worth it

For this particular thing, it would be best to introduce storage incarnation: https://github.com/ledgerwatch/erigon/blob/devel/docs/programmers_guide/db_walkthrough.MD#table-plainstate
In general, for introducing rayon, we need to be careful that we have a good amount of data and processing that we want to iterate/Parallelize on.

gakonst · 2022-06-11T17:09:17Z

crates/revm/src/db/in_memory_db.rs

@@ -87,26 +87,12 @@ impl<ExtDB: DatabaseRef> CacheDB<ExtDB> {
 impl<ExtDB: DatabaseRef> DatabaseCommit for CacheDB<ExtDB> {
    fn commit(&mut self, changes: Map<H160, Account>) {
        for (add, acc) in changes {
-            if acc.is_empty() || matches!(acc.filth, Filth::Destroyed) {
-                self.cache.remove(&add);
-                self.storage.remove(&add);


@rakita you said set "cache to Account::default" but we're currently setting the changes? is that right, or a mistake?

gakonst · 2022-06-11T17:09:29Z

crates/revm/src/db/in_memory_db.rs

            } else {
-                self.insert_cache(add, acc.info);
+                self.insert_change(add, acc.info);


same q here, what's the diff between cache / change? should we add a note that this is a temp fix?

It is because of this: #126 (comment)
the cache is loaded from the external DB.
the changes is what is applied from commit.

gakonst · 2022-06-11T17:12:36Z

LGTM - couple comments above, should we try to release this ASAP to fix the downstream bugs in Foundry?

gakonst · 2022-06-14T10:23:40Z

@mattsse @rakita do we intend to merge this or are there any blockers?

mattsse · 2022-06-14T10:57:50Z

I think this is good to merge, don't think this has any downside

rakita · 2022-06-14T13:09:22Z

I think this is good to merge, don't think this has any downside

Hey i will merge it as it is, but check in forge if it breaks something as in using cache to check data but it now need changes

gakonst · 2022-06-14T13:11:35Z

Good point - will keep it in mind before we update^ thanks!

mattsse added 2 commits June 10, 2022 12:06

don't delete account and storage entries on commit

ecec2e9

bump version v1.6.0

770665d

rakita reviewed Jun 10, 2022

View reviewed changes

fix: clear storage by setting entries to 0

c0a1da4

skip empty accounts

6a962cf

track account change set for CacheDB

e54c50d

gakonst reviewed Jun 11, 2022

View reviewed changes

rakita merged commit a95390f into bluealloy:main Jun 14, 2022

This was referenced Jun 15, 2022

chore: use revm head foundry-rs/foundry#1979

Merged

fix: insert in cache map #132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't delete account and storage entries on commit #126

don't delete account and storage entries on commit #126

mattsse commented Jun 10, 2022

mattsse commented Jun 10, 2022

rakita left a comment

rakita Jun 10, 2022

gakonst Jun 11, 2022

rakita Jun 11, 2022

rakita Jun 10, 2022

rakita Jun 10, 2022

rakita commented Jun 10, 2022

mattsse commented Jun 10, 2022 •

edited

rakita commented Jun 10, 2022

rakita commented Jun 10, 2022 •

edited

gakonst Jun 11, 2022

rakita Jun 11, 2022

gakonst Jun 11, 2022

gakonst Jun 11, 2022 •

edited

rakita Jun 11, 2022

gakonst commented Jun 11, 2022

gakonst commented Jun 14, 2022

mattsse commented Jun 14, 2022

rakita commented Jun 14, 2022

gakonst commented Jun 14, 2022 •

edited

don't delete account and storage entries on commit #126

don't delete account and storage entries on commit #126

Conversation

mattsse commented Jun 10, 2022

Solution

mattsse commented Jun 10, 2022

rakita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakita commented Jun 10, 2022

mattsse commented Jun 10, 2022 • edited

rakita commented Jun 10, 2022

rakita commented Jun 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst Jun 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst commented Jun 11, 2022

gakonst commented Jun 14, 2022

mattsse commented Jun 14, 2022

rakita commented Jun 14, 2022

gakonst commented Jun 14, 2022 • edited

mattsse commented Jun 10, 2022 •

edited

rakita commented Jun 10, 2022 •

edited

gakonst Jun 11, 2022 •

edited

gakonst commented Jun 14, 2022 •

edited