[NC-1748] Treat original storage as empty when a new contract is created (#95)

Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

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

@@ -77,11 +77,6 @@ public abstract class AbstractWorldUpdater<W extends WorldView, A extends Accoun
     return world.get(address);
   }
 
-  @Override
-  public Account getOriginalAccount(final Address address) {
-    return world.getOriginalAccount(address);
-  }
-
   @Override
   public MutableAccount getMutable(final Address address) {
     // We may have updated it already, so check that first.
@@ -303,6 +298,13 @@ public abstract class AbstractWorldUpdater<W extends WorldView, A extends Accoun
       return account == null ? UInt256.ZERO : account.getStorageValue(key);
     }
 
+    @Override
+    public UInt256 getOriginalStorageValue(final UInt256 key) {
+      return storageWasCleared || account == null
+          ? UInt256.ZERO
+          : account.getOriginalStorageValue(key);
+    }
+
     @Override
     public NavigableMap<Bytes32, UInt256> storageEntriesFrom(
         final Bytes32 startKeyHash, final int limit) {

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

@@ -105,6 +105,16 @@ public interface Account {
    */
   UInt256 getStorageValue(UInt256 key);
 
+  /**
+   * Retrieves the original value from before the current transaction in the account storage given
+   * its key.
+   *
+   * @param key the key to retrieve in the account storage.
+   * @return the original value associated to {@code key} in the account storage. Note that this is
+   *     never {@code null}, but 0 acts as a default value.
+   */
+  UInt256 getOriginalStorageValue(UInt256 key);
+
   /**
    * Whether the account is "empty".
    *

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

@@ -14,18 +14,7 @@ package tech.pegasys.pantheon.ethereum.core;
 
 /** Generic interface for a view over the accounts of the world state. */
 public interface WorldView {
-  WorldView EMPTY =
-      new WorldView() {
-        @Override
-        public Account get(final Address address) {
-          return null;
-        }
-
-        @Override
-        public Account getOriginalAccount(final Address address) {
-          return null;
-        }
-      };
+  WorldView EMPTY = address -> null;
 
   /**
    * Get an account provided it's address.
@@ -35,12 +24,4 @@ public interface WorldView {
    *     such account.
    */
   Account get(Address address);
-
-  /**
-   * Retrieve the original account, prior to any modifications made in this transaction.
-   *
-   * @param address the address of the account.
-   * @return the account {@code address} or {@code null} if the account does not exist.
-   */
-  Account getOriginalAccount(Address address);
 }

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

@@ -12,13 +12,12 @@
  */
 package tech.pegasys.pantheon.ethereum.mainnet;
 
+import tech.pegasys.pantheon.ethereum.core.Account;
 import tech.pegasys.pantheon.ethereum.core.Gas;
 import tech.pegasys.pantheon.ethereum.vm.MessageFrame;
 import tech.pegasys.pantheon.util.bytes.Bytes32;
 import tech.pegasys.pantheon.util.uint.UInt256;
 
-import java.util.function.Supplier;
-
 public class ConstantinopleGasCalculator extends SpuriousDragonGasCalculator {
 
   private static final Gas SSTORE_NO_OP_COST = Gas.of(200);
@@ -43,14 +42,13 @@ public class ConstantinopleGasCalculator extends SpuriousDragonGasCalculator {
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-1283
   public Gas calculateStorageCost(
-      final Supplier<UInt256> originalValueSupplier,
-      final UInt256 currentValue,
-      final UInt256 newValue) {
+      final Account account, final UInt256 key, final UInt256 newValue) {
 
+    final UInt256 currentValue = account.getStorageValue(key);
     if (currentValue.equals(newValue)) {
       return SSTORE_NO_OP_COST;
     } else {
-      final UInt256 originalValue = originalValueSupplier.get();
+      final UInt256 originalValue = account.getOriginalStorageValue(key);
       if (originalValue.equals(currentValue)) {
         return originalValue.isZero()
             ? SSTORE_FIRST_DIRTY_NEW_STORAGE_COST
@@ -64,14 +62,13 @@ public class ConstantinopleGasCalculator extends SpuriousDragonGasCalculator {
   @Override
   // As per https://eips.ethereum.org/EIPS/eip-1283
   public Gas calculateStorageRefundAmount(
-      final Supplier<UInt256> originalValueSupplier,
-      final UInt256 currentValue,
-      final UInt256 newValue) {
+      final Account account, final UInt256 key, final UInt256 newValue) {
 
+    final UInt256 currentValue = account.getStorageValue(key);
     if (currentValue.equals(newValue)) {
       return Gas.ZERO;
     } else {
-      final UInt256 originalValue = originalValueSupplier.get();
+      final UInt256 originalValue = account.getOriginalStorageValue(key);
       if (originalValue.equals(currentValue)) {
         if (originalValue.isZero()) {
           return Gas.ZERO;

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

@@ -24,8 +24,6 @@ import tech.pegasys.pantheon.util.bytes.Bytes32;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 import tech.pegasys.pantheon.util.uint.UInt256;
 
-import java.util.function.Supplier;
-
 public class FrontierGasCalculator implements GasCalculator {
 
   private static final Gas TX_DATA_ZERO_COST = Gas.of(4L);
@@ -419,16 +417,18 @@ public class FrontierGasCalculator implements GasCalculator {
 
   @Override
   public Gas calculateStorageCost(
-      final Supplier<UInt256> originalValue, final UInt256 currentValue, final UInt256 newValue) {
-    return !newValue.isZero() && currentValue.isZero()
+      final Account account, final UInt256 key, final UInt256 newValue) {
+    return !newValue.isZero() && account.getStorageValue(key).isZero()
         ? STORAGE_SET_GAS_COST
         : STORAGE_RESET_GAS_COST;
   }
 
   @Override
   public Gas calculateStorageRefundAmount(
-      final Supplier<UInt256> originalValue, final UInt256 currentValue, final UInt256 newValue) {
-    return newValue.isZero() && !currentValue.isZero() ? STORAGE_RESET_REFUND_AMOUNT : Gas.ZERO;
+      final Account account, final UInt256 key, final UInt256 newValue) {
+    return newValue.isZero() && !account.getStorageValue(key).isZero()
+        ? STORAGE_RESET_REFUND_AMOUNT
+        : Gas.ZERO;
   }
 
   @Override

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/EVM.java

@@ -68,7 +68,7 @@ public class EVM {
         currentGasCost,
         () -> {
           checkForExceptionalHalt(frame);
-          logState(frame);
+          logState(frame, currentGasCost);
           decrementRemainingGas(frame, currentGasCost);
           frame.getCurrentOperation().execute(frame);
           incrementProgramCounter(frame);
@@ -118,13 +118,15 @@ public class EVM {
     }
   }
 
-  private static void logState(final MessageFrame frame) {
+  private static void logState(final MessageFrame frame, final Optional<Gas> currentGasCost) {
     if (LOG.isTraceEnabled()) {
       final StringBuilder builder = new StringBuilder();
       builder.append("Depth: ").append(frame.getMessageStackDepth()).append("\n");
       builder.append("Operation: ").append(frame.getCurrentOperation().getName()).append("\n");
-      builder.append(" PC: ").append(frame.getPC()).append("\n");
+      builder.append("PC: ").append(frame.getPC()).append("\n");
+      currentGasCost.ifPresent(gas -> builder.append("Gas cost: ").append(gas).append("\n"));
       builder.append("Gas Remaining: ").append(frame.getRemainingGas()).append("\n");
+      builder.append("Depth: ").append(frame.getMessageStackDepth()).append("\n");
       builder.append("Stack:");
       for (int i = 0; i < frame.stackSize(); ++i) {
         builder.append("\n\t").append(i).append(" ").append(frame.getStackItem(i));

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/GasCalculator.java

@@ -38,8 +38,6 @@ import tech.pegasys.pantheon.ethereum.vm.operations.Sha3Operation;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 import tech.pegasys.pantheon.util.uint.UInt256;
 
-import java.util.function.Supplier;
-
 /**
  * Provides various gas cost lookups and calculations used during block processing.
  *
@@ -354,23 +352,22 @@ public interface GasCalculator {
   /**
    * Returns the cost for an SSTORE operation.
    *
-   * @param originalValue supplies the value from prior to this transaction
-   * @param currentValue the current value in the affected storage location
+   * @param account the account that storage will be changed in
+   * @param key the key the new value is to be stored under
    * @param newValue the new value to be stored
    * @return the gas cost for the SSTORE operation
    */
-  Gas calculateStorageCost(Supplier<UInt256> originalValue, UInt256 currentValue, UInt256 newValue);
+  Gas calculateStorageCost(Account account, UInt256 key, UInt256 newValue);
 
   /**
    * Returns the refund amount for an SSTORE operation.
    *
-   * @param originalValue supplies the value from prior to this transaction
-   * @param currentValue the current value in the affected storage location
+   * @param account the account that storage will be changed in
+   * @param key the key the new value is to be stored under
    * @param newValue the new value to be stored
    * @return the gas refund for the SSTORE operation
    */
-  Gas calculateStorageRefundAmount(
-      Supplier<UInt256> originalValue, UInt256 currentValue, UInt256 newValue);
+  Gas calculateStorageRefundAmount(Account account, UInt256 key, UInt256 newValue);
 
   /**
    * Returns the refund amount for deleting an account in a {@link SelfDestructOperation}.

ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/operations/SStoreOperation.java

@@ -37,12 +37,7 @@ public class SStoreOperation extends AbstractOperation {
     final UInt256 newValue = frame.getStackItem(1).asUInt256();
 
     final Account account = frame.getWorldState().get(frame.getRecipientAddress());
-    // Setting storage value to non-zero from zero (i.e. nothing currently at this location) vs.
-    // resetting an existing value.
-    final UInt256 currentValue = account.getStorageValue(key);
-
-    return gasCalculator()
-        .calculateStorageCost(() -> getOriginalValue(frame, key), currentValue, newValue);
+    return gasCalculator().calculateStorageCost(account, key, newValue);
   }
 
   @Override
@@ -54,10 +49,7 @@ public class SStoreOperation extends AbstractOperation {
     assert account != null : "VM account should exists";
 
     // Increment the refund counter.
-    final UInt256 currentValue = account.getStorageValue(key);
-    frame.incrementGasRefund(
-        gasCalculator()
-            .calculateStorageRefundAmount(() -> getOriginalValue(frame, key), currentValue, value));
+    frame.incrementGasRefund(gasCalculator().calculateStorageRefundAmount(account, key, value));
 
     account.setStorageValue(key.copy(), value.copy());
   }
@@ -71,10 +63,4 @@ public class SStoreOperation extends AbstractOperation {
         ? Optional.of(ExceptionalHaltReason.ILLEGAL_STATE_CHANGE)
         : Optional.empty();
   }
-
-  private UInt256 getOriginalValue(final MessageFrame frame, final UInt256 key) {
-    final Account originalAccount =
-        frame.getWorldState().getOriginalAccount(frame.getRecipientAddress());
-    return originalAccount != null ? originalAccount.getStorageValue(key) : UInt256.ZERO;
-  }
 }

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

@@ -168,11 +168,5 @@ public class DebuggableMutableWorldState extends DefaultMutableWorldState {
       record(address);
       return wrapped.get(address);
     }
-
-    @Override
-    public Account getOriginalAccount(final Address address) {
-      record(address);
-      return wrapped.getOriginalAccount(address);
-    }
   }
 }

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

@@ -100,11 +100,6 @@ public class DefaultMutableWorldState implements MutableWorldState {
         .orElse(null);
   }
 
-  @Override
-  public Account getOriginalAccount(final Address address) {
-    return get(address);
-  }
-
   private AccountState deserializeAccount(
       final Address address, final Hash addressHash, final BytesValue encoded) throws RLPException {
     final RLPInput in = RLP.input(encoded);
@@ -279,6 +274,11 @@ public class DefaultMutableWorldState implements MutableWorldState {
       return convertToUInt256(val.get());
     }
 
+    @Override
+    public UInt256 getOriginalStorageValue(final UInt256 key) {
+      return getStorageValue(key);
+    }
+
     @Override
     public NavigableMap<Bytes32, UInt256> storageEntriesFrom(
         final Bytes32 startKeyHash, final int limit) {

ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/ConstantinopleSstoreGasTest.java

@@ -13,12 +13,16 @@
 package tech.pegasys.pantheon.ethereum.mainnet;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 import static tech.pegasys.pantheon.util.uint.UInt256.ONE;
 import static tech.pegasys.pantheon.util.uint.UInt256.ZERO;
 
+import tech.pegasys.pantheon.ethereum.core.Account;
 import tech.pegasys.pantheon.ethereum.core.Gas;
 import tech.pegasys.pantheon.util.uint.UInt256;
 
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -84,16 +88,23 @@ public class ConstantinopleSstoreGasTest {
   @Parameter(value = 4)
   public Gas expectedGasRefund;
 
+  private final Account account = mock(Account.class);
+
+  @Before
+  public void setUp() {
+    when(account.getOriginalStorageValue(UInt256.ZERO)).thenReturn(originalValue);
+    when(account.getStorageValue(UInt256.ZERO)).thenReturn(currentValue);
+  }
+
   @Test
   public void shouldChargeCorrectGas() {
-    assertThat(gasCalculator.calculateStorageCost(() -> originalValue, currentValue, newValue))
+    assertThat(gasCalculator.calculateStorageCost(account, UInt256.ZERO, newValue))
         .isEqualTo(expectedGasCost);
   }
 
   @Test
   public void shouldRefundCorrectGas() {
-    assertThat(
-            gasCalculator.calculateStorageRefundAmount(() -> originalValue, currentValue, newValue))
+    assertThat(gasCalculator.calculateStorageRefundAmount(account, UInt256.ZERO, newValue))
         .isEqualTo(expectedGasRefund);
   }
 }

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

@@ -65,13 +65,6 @@ public class BlockchainReferenceTestTools {
 
     // Consumes a huge amount of memory
     params.blacklist("static_Call1MB1024Calldepth_d1g0v0_(Byzantium|Constantinople)");
-
-    // Needs investigation
-    params.blacklist("RevertInCreateInInit_d0g0v0_Byzantium");
-    params.blacklist("RevertInCreateInInit_d0g0v0_Constantinople");
-
-    // Constantinople failures to investigate
-    params.blacklist("RevertInCreateInInitCreate2_d0g0v0_Constantinople\\[Constantinople\\]");
   }
 
   public static Collection<Object[]> generateTestParametersForConfig(final String[] filePath) {

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

@@ -85,10 +85,6 @@ public class GeneralStateReferenceTestTools {
     params.blacklist("OverflowGasRequire");
     // Consumes a huge amount of memory
     params.blacklist("static_Call1MB1024Calldepth-(Byzantium|Constantinople)");
-
-    // Constantinople failures to investigate
-    params.blacklist("RevertInCreateInInitCreate2-Constantinople");
-    params.blacklist("RevertInCreateInInit-Constantinople");
   }
 
   public static Collection<Object[]> generateTestParametersForConfig(final String[] filePath) {

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

@@ -298,10 +298,41 @@ public class DefaultMutableWorldStateTest {
         worldState.rootHash());
   }
 
+  @Test
+  public void getOriginalStorageValue() {
+    final MutableWorldState worldState = createEmpty();
+    final WorldUpdater setupUpdater = worldState.updater();
+    final MutableAccount setupAccount = setupUpdater.createAccount(ADDRESS);
+    setupAccount.setStorageValue(UInt256.ONE, UInt256.of(2));
+    setupUpdater.commit();
+
+    final WorldUpdater updater = worldState.updater();
+    final MutableAccount account = updater.getOrCreate(ADDRESS);
+    assertThat(account.getOriginalStorageValue(UInt256.ONE)).isEqualTo(UInt256.of(2));
+
+    account.setStorageValue(UInt256.ONE, UInt256.of(3));
+    assertThat(account.getOriginalStorageValue(UInt256.ONE)).isEqualTo(UInt256.of(2));
+  }
+
+  @Test
+  public void originalStorageValueIsAlwaysZeroIfStorageWasCleared() {
+    final MutableWorldState worldState = createEmpty();
+    final WorldUpdater setupUpdater = worldState.updater();
+    final MutableAccount setupAccount = setupUpdater.createAccount(ADDRESS);
+    setupAccount.setStorageValue(UInt256.ONE, UInt256.of(2));
+    setupUpdater.commit();
+
+    final WorldUpdater updater = worldState.updater();
+    final MutableAccount account = updater.getOrCreate(ADDRESS);
+
+    account.clearStorage();
+    assertThat(account.getOriginalStorageValue(UInt256.ONE)).isEqualTo(UInt256.ZERO);
+  }
+
   @Test
   public void clearStorage() {
-    UInt256 storageKey = UInt256.of(1L);
-    UInt256 storageValue = UInt256.of(2L);
+    final UInt256 storageKey = UInt256.of(1L);
+    final UInt256 storageValue = UInt256.of(2L);
 
     // Create a world state with one account
     final MutableWorldState worldState = createEmpty();
@@ -333,8 +364,8 @@ public class DefaultMutableWorldStateTest {
 
   @Test
   public void clearStorage_AfterPersisting() {
-    UInt256 storageKey = UInt256.of(1L);
-    UInt256 storageValue = UInt256.of(2L);
+    final UInt256 storageKey = UInt256.of(1L);
+    final UInt256 storageValue = UInt256.of(2L);
 
     // Create a world state with one account
     final MutableWorldState worldState = createEmpty();
@@ -370,9 +401,9 @@ public class DefaultMutableWorldStateTest {
 
   @Test
   public void clearStorageThenEdit() {
-    UInt256 storageKey = UInt256.of(1L);
-    UInt256 originalStorageValue = UInt256.of(2L);
-    UInt256 newStorageValue = UInt256.of(3L);
+    final UInt256 storageKey = UInt256.of(1L);
+    final UInt256 originalStorageValue = UInt256.of(2L);
+    final UInt256 newStorageValue = UInt256.of(3L);
 
     // Create a world state with one account
     final MutableWorldState worldState = createEmpty();
@@ -405,9 +436,9 @@ public class DefaultMutableWorldStateTest {
 
   @Test
   public void clearStorageThenEditAfterPersisting() {
-    UInt256 storageKey = UInt256.of(1L);
-    UInt256 originalStorageValue = UInt256.of(2L);
-    UInt256 newStorageValue = UInt256.of(3L);
+    final UInt256 storageKey = UInt256.of(1L);
+    final UInt256 originalStorageValue = UInt256.of(2L);
+    final UInt256 newStorageValue = UInt256.of(3L);
 
     // Create a world state with one account
     final MutableWorldState worldState = createEmpty();