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 0cae0ae6c1..f3b6e40ccf 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 @@ -77,11 +77,6 @@ public abstract class AbstractWorldUpdater storageEntriesFrom( final Bytes32 startKeyHash, final int limit) { diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Account.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Account.java index 848087235d..031cc6b8d3 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/Account.java +++ b/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". * diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/WorldView.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/WorldView.java index 389f5e7d77..c6495f6ac9 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/core/WorldView.java +++ b/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); } diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ConstantinopleGasCalculator.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ConstantinopleGasCalculator.java index b13736c8f2..cbc7c077e6 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/ConstantinopleGasCalculator.java +++ b/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 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 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; diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/FrontierGasCalculator.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/FrontierGasCalculator.java index 2e0e333732..f2f939b696 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/mainnet/FrontierGasCalculator.java +++ b/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 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 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 diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/EVM.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/EVM.java index c7c9ec4ec5..47eba180b1 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/EVM.java +++ b/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 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)); diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/GasCalculator.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/GasCalculator.java index ea54b64937..fe0f3e5ebf 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/GasCalculator.java +++ b/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 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 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}. diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/operations/SStoreOperation.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/operations/SStoreOperation.java index a8a98615c5..dab9cb22e9 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/vm/operations/SStoreOperation.java +++ b/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; - } } diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DebuggableMutableWorldState.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DebuggableMutableWorldState.java index 45ac96cbc0..074ffcac21 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DebuggableMutableWorldState.java +++ b/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); - } } } diff --git a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java b/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java index 17dedde829..6a166f6808 100644 --- a/ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/worldstate/DefaultMutableWorldState.java +++ b/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 storageEntriesFrom( final Bytes32 startKeyHash, final int limit) { diff --git a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/ConstantinopleSstoreGasTest.java b/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/ConstantinopleSstoreGasTest.java index ada785466b..3007695a14 100644 --- a/ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/mainnet/ConstantinopleSstoreGasTest.java +++ b/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); } } 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 ad62e4d571..12b4d6ffbf 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 @@ -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 generateTestParametersForConfig(final String[] filePath) { 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 64a8292df3..b54d8c03ba 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 @@ -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 generateTestParametersForConfig(final String[] filePath) { 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 04be0d8431..b2c95ce5c8 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,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();