Lock in storage changes before transaction starts

Currently, because we don't do this, there is a bug in EIP-2200, net gas
metering. When looking up the "original value", the EVM must get the
storage value at the beginning of the transaction, not the beginning of
the block. So we lock in changes at the beginning of every transaction,
to make sure we don't read values from older transactions, when reading
the original value.

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.