Merge pull request #1894 from carver/net-sstore-original-value-bugfix

Lock in storage changes before transaction starts

Author: carver

eth/abc.py

@@ -1573,6 +1573,16 @@ def commit(self, checkpoint: JournalDBCheckpoint) -> None:
         """
         ...
 
+    @abstractmethod
+    def lock_changes(self) -> None:
+        """
+        Locks in changes to storage, typically just as a transaction starts.
+
+        This is used, for example, to look up the storage value from the start
+        of the transaction, when calculating gas costs in EIP-2200: net gas metering.
+        """
+        ...
+
     @abstractmethod
     def make_storage_root(self) -> None:
         """
@@ -2153,6 +2163,16 @@ def commit(self, snapshot: Tuple[Hash32, UUID]) -> None:
         """
         ...
 
+    @abstractmethod
+    def lock_changes(self) -> None:
+        """
+        Locks in all changes to state, typically just as a transaction starts.
+
+        This is used, for example, to look up the storage value from the start
+        of the transaction, when calculating gas costs in EIP-2200: net gas metering.
+        """
+        ...
+
     @abstractmethod
     def persist(self) -> None:
         """

eth/db/account.py

@@ -370,6 +370,10 @@ def commit(self, checkpoint: JournalDBCheckpoint) -> None:
         for _, store in self._dirty_account_stores():
             store.commit(checkpoint)
 
+    def lock_changes(self) -> None:
+        for _, store in self._dirty_account_stores():
+            store.lock_changes()
+
     def make_state_root(self) -> Hash32:
         for _, store in self._dirty_account_stores():
             store.make_storage_root()

eth/db/storage.py

@@ -157,7 +157,7 @@ def __init__(self, db: AtomicDatabaseAPI, storage_root: Hash32, address: Address
 
         .. code::
 
-            db -> _storage_lookup -> _storage_cache -> _journal_storage
+            db -> _storage_lookup -> _storage_cache -> _locked_changes -> _journal_storage
 
         db is the raw database, we can assume it hits disk when written to.
         Keys are stored as node hashes and rlp-encoded node values.
@@ -174,6 +174,10 @@ def __init__(self, db: AtomicDatabaseAPI, storage_root: Hash32, address: Address
         after a state root change. Otherwise, you will see data since the last
         storage root was calculated.
 
+        _locked_changes is a batch database that includes only those values that are
+        un-revertable in the EVM. Currently, that means changes that completed in a
+        previous transaction.
+
         Journaling batches writes at the _journal_storage layer, until persist is called.
         It manages all the checkpointing and rollbacks that happen during EVM execution.
 
@@ -183,11 +187,12 @@ def __init__(self, db: AtomicDatabaseAPI, storage_root: Hash32, address: Address
         self._address = address
         self._storage_lookup = StorageLookup(db, storage_root, address)
         self._storage_cache = CacheDB(self._storage_lookup)
-        self._journal_storage = JournalDB(self._storage_cache)
+        self._locked_changes = BatchDB(self._storage_cache)
+        self._journal_storage = JournalDB(self._locked_changes)
 
     def get(self, slot: int, from_journal: bool=True) -> int:
         key = int_to_big_endian(slot)
-        lookup_db = self._journal_storage if from_journal else self._storage_cache
+        lookup_db = self._journal_storage if from_journal else self._locked_changes
         try:
             encoded_value = lookup_db[key]
         except MissingStorageTrieNode:
@@ -237,9 +242,13 @@ def commit(self, checkpoint: JournalDBCheckpoint) -> None:
             #    then flatten all changes, without persisting
             self._journal_storage.flatten()
 
-    def make_storage_root(self) -> None:
+    def lock_changes(self) -> None:
         self._journal_storage.persist()
 
+    def make_storage_root(self) -> None:
+        self.lock_changes()
+        self._locked_changes.commit(apply_deletes=True)
+
     def _validate_flushed(self) -> None:
         """
         Will raise an exception if there are some changes made since the last persist.

eth/vm/base.py

@@ -148,6 +148,10 @@ def apply_transaction(self,
                           transaction: SignedTransactionAPI
                           ) -> Tuple[ReceiptAPI, ComputationAPI]:
         self.validate_transaction_against_header(header, transaction)
+
+        # Mark current state as un-revertable, since new transaction is starting...
+        self.state.lock_changes()
+
         computation = self.state.apply_transaction(transaction)
         receipt = self.make_receipt(header, transaction, computation, self.state)
         self.validate_receipt(receipt)

eth/vm/state.py

@@ -175,6 +175,9 @@ def commit(self, snapshot: Tuple[Hash32, UUID]) -> None:
         _, account_snapshot = snapshot
         self._account_db.commit(account_snapshot)
 
+    def lock_changes(self) -> None:
+        self._account_db.lock_changes()
+
     def persist(self) -> None:
         self._account_db.persist()
 

newsfragments/1893.bugfix.rst

@@ -0,0 +1,3 @@
+Fix for net gas metering (EIP-2200) in Istanbul. The "original value" used to calculate gas
+costs was incorrectly accessing the value at the start of the block, instead of the start of the
+transaction.

tests/core/vm/test_vm_state.py

@@ -101,3 +101,13 @@ def test_revert_selfdestruct(state, read_storage_before_snapshot):
     #       "starting" storage root hash would always be the empty one, which causes
     #       it to not be able to recover from a revert
     assert state.get_storage(ADDRESS, 1) == 2
+
+
+def test_lock_state(state):
+    assert state.get_storage(ADDRESS, 1, from_journal=False) == 0
+
+    state.set_storage(ADDRESS, 1, 2)
+    assert state.get_storage(ADDRESS, 1, from_journal=False) == 0
+
+    state.lock_changes()
+    assert state.get_storage(ADDRESS, 1, from_journal=False) == 2