[NC-1748] Clear contract storage on contract creation (#85)

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/AbstractWorldUpdater.java

@@ -172,9 +172,10 @@ public abstract class AbstractWorldUpdater<W extends WorldView, A extends Accoun
     @Nullable private BytesValue updatedCode; // Null if the underlying code has not been updated.
     @Nullable private Hash updatedCodeHash;
 
-    // Only contains update storage entries, but may contains entry with a value of 0 to signify
+    // Only contains updated storage entries, but may contains entry with a value of 0 to signify
     // deletion.
     private final SortedMap<UInt256, UInt256> updatedStorage;
+    private boolean storageWasCleared = false;
 
     UpdateTrackingAccount(final Address address) {
       checkNotNull(address);
@@ -293,6 +294,9 @@ public abstract class AbstractWorldUpdater<W extends WorldView, A extends Accoun
       if (value != null) {
         return value;
       }
+      if (storageWasCleared) {
+        return UInt256.ZERO;
+      }
 
       // We haven't updated the key-value yet, so either it's a new account and it doesn't have the
       // key, or we should query the underlying storage for its existing value (which might be 0).
@@ -327,15 +331,25 @@ public abstract class AbstractWorldUpdater<W extends WorldView, A extends Accoun
       updatedStorage.put(key, value);
     }
 
+    @Override
+    public void clearStorage() {
+      storageWasCleared = true;
+      updatedStorage.clear();
+    }
+
+    public boolean getStorageWasCleared() {
+      return storageWasCleared;
+    }
+
     @Override
     public String toString() {
+      String storage = updatedStorage.isEmpty() ? "[not updated]" : updatedStorage.toString();
+      if (updatedStorage.isEmpty() && storageWasCleared) {
+        storage = "[cleared]";
+      }
       return String.format(
           "%s -> {nonce: %s, balance:%s, code:%s, storage:%s }",
-          address,
+          address, nonce, balance, updatedCode == null ? "[not updated]" : updatedCode, storage);
-          nonce,
-          balance,
-          updatedCode == null ? "[not updated]" : updatedCode,
-          updatedStorage.isEmpty() ? "[not updated]" : updatedStorage);
     }
   }
 
@@ -404,6 +418,9 @@ public abstract class AbstractWorldUpdater<W extends WorldView, A extends Accoun
         if (update.codeWasUpdated()) {
           existing.setCode(update.getCode());
         }
+        if (update.getStorageWasCleared()) {
+          existing.clearStorage();
+        }
         update.getUpdatedStorage().forEach(existing::setStorageValue);
       }
     }

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/MutableAccount.java

@@ -92,6 +92,9 @@ public interface MutableAccount extends Account {
    */
   void setStorageValue(UInt256 key, UInt256 value);
 
+  /** Clears out an account's storage. */
+  void clearStorage();
+
   /**
    * Returns the storage entries that have been set through the updater this instance came from.
    *

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/MainnetContractCreationProcessor.java

@@ -88,10 +88,6 @@ public class MainnetContractCreationProcessor extends AbstractMessageProcessor {
     final MutableAccount sender = frame.getWorldState().getMutable(frame.getSenderAddress());
     sender.decrementBalance(frame.getValue());
 
-    // TODO: Fix when tests are upstreamed or remove from test suit.
-    // EIP-68 mandates that contract creations cannot collide any more.
-    // While that EIP has been deferred, the General State reference tests
-    // incorrectly include this even in early hard forks.
     final MutableAccount contract = frame.getWorldState().getOrCreate(frame.getContractAddress());
     if (accountExists(contract)) {
       LOG.trace(
@@ -101,6 +97,7 @@ public class MainnetContractCreationProcessor extends AbstractMessageProcessor {
     } else {
       contract.incrementBalance(frame.getValue());
       contract.setNonce(initialContractNonce);
+      contract.clearStorage();
       frame.setState(MessageFrame.State.CODE_EXECUTING);
     }
   }

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java

@@ -349,11 +349,6 @@ public class DefaultMutableWorldState implements MutableWorldState {
         wrapped.updatedAccountCode.remove(address);
       }
 
-      // TODO: this is inefficient with a real persistent world state as doing updates one by one
-      // might create a lot of garbage nodes that will be persisted without being needed. Also, if
-      // the state is big, doing update one by one is not algorithmically optimal in general. We
-      // should create a small in-memory trie of the updates first, and then apply this in bulk
-      // to the real world state as a merge of trie.
       for (final UpdateTrackingAccount<AccountState> updated : updatedAccounts()) {
         final AccountState origin = updated.getWrappedAccount();
 
@@ -364,15 +359,16 @@ public class DefaultMutableWorldState implements MutableWorldState {
           wrapped.updatedAccountCode.put(updated.getAddress(), updated.getCode());
         }
         // ...and storage in the account trie first.
-        // TODO: same remark as above really, this could be make more efficient by "batching"
+        final boolean freshState = origin == null || updated.getStorageWasCleared();
+        Hash storageRoot = freshState ? Hash.EMPTY_TRIE_HASH : origin.storageRoot;
+        if (freshState) {
+          wrapped.updatedStorageTries.remove(updated.getAddress());
+        }
         final SortedMap<UInt256, UInt256> updatedStorage = updated.getUpdatedStorage();
-        Hash storageRoot;
+        if (!updatedStorage.isEmpty()) {
-        MerklePatriciaTrie<Bytes32, BytesValue> storageTrie;
+          // Apply any storage updates
-        if (updatedStorage.isEmpty()) {
+          final MerklePatriciaTrie<Bytes32, BytesValue> storageTrie =
-          storageRoot = origin == null ? Hash.EMPTY_TRIE_HASH : origin.storageRoot;
+              freshState
-        } else {
-          storageTrie =
-              origin == null
                   ? wrapped.newAccountStorageTrie(Hash.EMPTY_TRIE_HASH)
                   : origin.storageTrie();
           wrapped.updatedStorageTries.put(updated.getAddress(), storageTrie);
@@ -394,6 +390,10 @@ public class DefaultMutableWorldState implements MutableWorldState {
 
         wrapped.accountStateTrie.put(updated.getAddressHash(), account);
       }
+
+      // After committing, clear data
+      deletedAccounts().clear();
+      updatedAccounts().clear();
     }
   }
 }

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/BlockchainReferenceTestTools.java

@@ -78,9 +78,6 @@ public class BlockchainReferenceTestTools {
     params.blacklist("RevertInCreateInInit_d0g0v0_Constantinople");
 
     // Constantinople failures to investigate
-    params.blacklist("create2collisionStorage_d0g0v0_Constantinople\\[Constantinople\\]");
-    params.blacklist("create2collisionStorage_d1g0v0_Constantinople\\[Constantinople\\]");
-    params.blacklist("create2collisionStorage_d2g0v0_Constantinople\\[Constantinople\\]");
     params.blacklist("RevertInCreateInInitCreate2_d0g0v0_Constantinople\\[Constantinople\\]");
   }
 

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/GeneralStateReferenceTestTools.java

@@ -146,9 +146,6 @@ public class GeneralStateReferenceTestTools {
 
     // Constantinople failures to investigate
     params.blacklist("RevertInCreateInInitCreate2-Constantinople");
-    params.blacklist("create2collisionStorage-Constantinople\\[0\\]");
-    params.blacklist("create2collisionStorage-Constantinople\\[1\\]");
-    params.blacklist("create2collisionStorage-Constantinople\\[2\\]");
     params.blacklist("RevertInCreateInInit-Constantinople");
     params.blacklist("ecmul_0-3_5616_28000_96-Constantinople\\[3\\]");
   }

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldStateTest.java

@@ -298,6 +298,149 @@ public class DefaultMutableWorldStateTest {
         worldState.rootHash());
   }
 
+  @Test
+  public void clearStorage() {
+    UInt256 storageKey = UInt256.of(1L);
+    UInt256 storageValue = UInt256.of(2L);
+
+    // Create a world state with one account
+    final MutableWorldState worldState = createEmpty();
+    final WorldUpdater updater = worldState.updater();
+    MutableAccount account = updater.createAccount(ADDRESS);
+    account.setBalance(Wei.of(100000));
+    account.setStorageValue(storageKey, storageValue);
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(storageValue);
+
+    // Clear storage
+    account = updater.getMutable(ADDRESS);
+    assertThat(account).isNotNull();
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(storageValue);
+    account.clearStorage();
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+
+    // Check storage is cleared after committing
+    updater.commit();
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+
+    // And after persisting
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+  }
+
+  @Test
+  public void clearStorage_AfterPersisting() {
+    UInt256 storageKey = UInt256.of(1L);
+    UInt256 storageValue = UInt256.of(2L);
+
+    // Create a world state with one account
+    final MutableWorldState worldState = createEmpty();
+    final WorldUpdater updater = worldState.updater();
+    MutableAccount account = updater.createAccount(ADDRESS);
+    account.setBalance(Wei.of(100000));
+    account.setStorageValue(storageKey, storageValue);
+    updater.commit();
+    worldState.persist();
+    assertNotNull(worldState.get(ADDRESS));
+    assertNotEquals(MerklePatriciaTrie.EMPTY_TRIE_ROOT_HASH, worldState.rootHash());
+
+    // Clear storage
+    account = updater.getMutable(ADDRESS);
+    assertThat(account).isNotNull();
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(storageValue);
+    account.clearStorage();
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(storageValue);
+
+    // Check storage is cleared after committing
+    updater.commit();
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+
+    // And after persisting
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(UInt256.ZERO);
+  }
+
+  @Test
+  public void clearStorageThenEdit() {
+    UInt256 storageKey = UInt256.of(1L);
+    UInt256 originalStorageValue = UInt256.of(2L);
+    UInt256 newStorageValue = UInt256.of(3L);
+
+    // Create a world state with one account
+    final MutableWorldState worldState = createEmpty();
+    final WorldUpdater updater = worldState.updater();
+    MutableAccount account = updater.createAccount(ADDRESS);
+    account.setBalance(Wei.of(100000));
+    account.setStorageValue(storageKey, originalStorageValue);
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(originalStorageValue);
+
+    // Clear storage then edit
+    account = updater.getMutable(ADDRESS);
+    assertThat(account).isNotNull();
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(originalStorageValue);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(originalStorageValue);
+    account.clearStorage();
+    account.setStorageValue(storageKey, newStorageValue);
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(newStorageValue);
+
+    // Check storage is cleared after committing
+    updater.commit();
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+
+    // And after persisting
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+  }
+
+  @Test
+  public void clearStorageThenEditAfterPersisting() {
+    UInt256 storageKey = UInt256.of(1L);
+    UInt256 originalStorageValue = UInt256.of(2L);
+    UInt256 newStorageValue = UInt256.of(3L);
+
+    // Create a world state with one account
+    final MutableWorldState worldState = createEmpty();
+    final WorldUpdater updater = worldState.updater();
+    MutableAccount account = updater.createAccount(ADDRESS);
+    account.setBalance(Wei.of(100000));
+    account.setStorageValue(storageKey, originalStorageValue);
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(originalStorageValue);
+    updater.commit();
+    worldState.persist();
+
+    // Clear storage then edit
+    account = updater.getMutable(ADDRESS);
+    assertThat(account).isNotNull();
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(originalStorageValue);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(originalStorageValue);
+    account.clearStorage();
+    account.setStorageValue(storageKey, newStorageValue);
+    assertThat(account.getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(originalStorageValue);
+
+    // Check storage is cleared after committing
+    updater.commit();
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+
+    // And after persisting
+    assertThat(updater.getMutable(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(updater.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+    assertThat(worldState.get(ADDRESS).getStorageValue(storageKey)).isEqualTo(newStorageValue);
+  }
+
   @Test
   public void replaceAccountCode() {
     final MutableWorldState worldState = createEmpty();
@@ -343,7 +486,8 @@ public class DefaultMutableWorldStateTest {
     final WorldUpdater updater1 = worldState.updater();
     final MutableAccount account1 = updater1.createAccount(ADDRESS);
     updater1.commit();
-    assertThat(updater1.get(ADDRESS)).isEqualTo(account1);
+    assertThat(updater1.get(ADDRESS))
+        .isEqualToComparingOnlyGivenFields(account1, "address", "nonce", "balance", "codeHash");
     updater1.deleteAccount(ADDRESS);
 
     final WorldUpdater updater2 = updater1.updater();