From 00006d123531495c12fd5add19237a26068d39d8 Mon Sep 17 00:00:00 2001 From: mbaxter Date: Thu, 18 Oct 2018 11:28:22 -0400 Subject: [PATCH] [NC-1748] Clear contract storage on contract creation (#85) --- .../ethereum/core/AbstractWorldUpdater.java | 29 +++- .../ethereum/core/MutableAccount.java | 3 + .../MainnetContractCreationProcessor.java | 5 +- .../worldstate/DefaultMutableWorldState.java | 26 ++-- .../vm/BlockchainReferenceTestTools.java | 3 - .../vm/GeneralStateReferenceTestTools.java | 3 - .../DefaultMutableWorldStateTest.java | 146 +++++++++++++++++- 7 files changed, 185 insertions(+), 30 deletions(-) diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/AbstractWorldUpdater.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/AbstractWorldUpdater.java index 64c5eac86e..0cae0ae6c1 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/AbstractWorldUpdater.java +++ b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/AbstractWorldUpdater.java @@ -172,9 +172,10 @@ public abstract class AbstractWorldUpdater updatedStorage; + private boolean storageWasCleared = false; UpdateTrackingAccount(final Address address) { checkNotNull(address); @@ -293,6 +294,9 @@ public abstract class AbstractWorldUpdater {nonce: %s, balance:%s, code:%s, storage:%s }", - address, - nonce, - balance, - updatedCode == null ? "[not updated]" : updatedCode, - updatedStorage.isEmpty() ? "[not updated]" : updatedStorage); + address, nonce, balance, updatedCode == null ? "[not updated]" : updatedCode, storage); } } @@ -404,6 +418,9 @@ public abstract class AbstractWorldUpdater 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 updatedStorage = updated.getUpdatedStorage(); - Hash storageRoot; - MerklePatriciaTrie storageTrie; - if (updatedStorage.isEmpty()) { - storageRoot = origin == null ? Hash.EMPTY_TRIE_HASH : origin.storageRoot; - } else { - storageTrie = - origin == null + if (!updatedStorage.isEmpty()) { + // Apply any storage updates + final MerklePatriciaTrie storageTrie = + freshState ? 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(); } } } diff --git a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/BlockchainReferenceTestTools.java b/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/BlockchainReferenceTestTools.java index b43e11e5e6..25b7677018 100644 --- a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/BlockchainReferenceTestTools.java +++ b/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\\]"); } diff --git a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/GeneralStateReferenceTestTools.java b/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/GeneralStateReferenceTestTools.java index 570216764a..5a4d7b4b1d 100644 --- a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/vm/GeneralStateReferenceTestTools.java +++ b/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\\]"); } diff --git a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldStateTest.java b/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldStateTest.java index 107f7f6c35..04be0d8431 100644 --- a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldStateTest.java +++ b/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();