Merge pull request #172 from input-output-hk/feature/fix-suicide-refund

 EC-170: Fix gas refund for self destruct when using CALLs

Author: LukasGasior1

src/evmTest/resources/solidity/CallSelfDestruct.sol

@@ -0,0 +1,17 @@
+pragma solidity ^0.4.10;
+
+contract CallSelfDestruct {
+
+  function callDestruct() {
+    CallSelfDestruct firstCall = CallSelfDestruct(this);
+    firstCall.doSelfdestruct();
+
+    CallSelfDestruct secondCall = CallSelfDestruct(this);
+    secondCall.doSelfdestruct();
+  }
+
+  function doSelfdestruct() {
+    selfdestruct(msg.sender);
+  }
+
+}

src/evmTest/scala/io/iohk/ethereum/vm/CallSelfDestructSpec.scala

@@ -0,0 +1,20 @@
+package io.iohk.ethereum.vm
+
+import io.iohk.ethereum.vm.utils.EvmTestEnv
+import org.scalatest.{FreeSpec, Matchers}
+
+class CallSelfDestructSpec extends FreeSpec with Matchers {
+
+  "EVM running CallSelfDestruct contract" - {
+
+    "should refund only once" in new EvmTestEnv {
+      val (_, callSelfDestruct) = deployContract("CallSelfDestruct")
+
+      val callRes = callSelfDestruct.callDestruct().call()
+
+      callRes.error shouldBe None
+      callRes.gasRefund shouldBe 24000
+    }
+  }
+
+}

src/main/scala/io/iohk/ethereum/ledger/Ledger.scala

@@ -127,7 +127,7 @@ class LedgerImpl(vm: VM, blockchainConfig: BlockchainConfig) extends Ledger with
     val resultWithErrorHandling: PR =
       if(result.error.isDefined) {
         //Rollback to the world before transfer was done if an error happened
-        result.copy(world = checkpointWorldState, addressesToDelete = Nil, logs = Nil)
+        result.copy(world = checkpointWorldState, addressesToDelete = Set.empty, logs = Nil)
       } else
         result
 
@@ -314,7 +314,7 @@ class LedgerImpl(vm: VM, blockchainConfig: BlockchainConfig) extends Ledger with
     * @param worldStateProxy
     * @return a worldState equal worldStateProxy except that the accounts from addressesToDelete are deleted
     */
-  private[ledger] def deleteAccounts(addressesToDelete: Seq[Address])(worldStateProxy: InMemoryWorldStateProxy): InMemoryWorldStateProxy =
+  private[ledger] def deleteAccounts(addressesToDelete: Set[Address])(worldStateProxy: InMemoryWorldStateProxy): InMemoryWorldStateProxy =
     addressesToDelete.foldLeft(worldStateProxy){ case (world, address) => world.deleteAccount(address) }
 
 }

src/main/scala/io/iohk/ethereum/vm/OpCode.scala

@@ -696,7 +696,7 @@ case object CREATE extends OpCode(0xf0, 3, 1, _.G_create) {
       val availableGas = state.gas - (constGasFn(state.config.feeSchedule) + varGas(state))
       val startGas = state.config.gasCap(availableGas)
 
-      val context = ProgramContext[W, S](newEnv, newAddress, startGas, world2, state.config)
+      val context = ProgramContext[W, S](newEnv, newAddress, startGas, world2, state.config, state.addressesToDelete)
       val result = VM.run(context)
 
       val codeDepositGas = state.config.calcCodeDepositCost(result.returnData)
@@ -780,7 +780,8 @@ sealed abstract class CallOp(code: Int, delta: Int, alpha: Int) extends OpCode(c
         env = env,
         receivingAddr = toAddr,
         startGas = startGas,
-        world = world1)
+        world = world1,
+        initialAddressesToDelete = state.addressesToDelete)
 
       VM.run(context)
     }

src/main/scala/io/iohk/ethereum/vm/PrecompiledContracts.scala

@@ -48,7 +48,7 @@ object PrecompiledContracts {
         result,
         gasRemaining,
         context.world,
-        Nil,
+        Set.empty,
         Nil,
         0,
         error

src/main/scala/io/iohk/ethereum/vm/ProgramContext.scala

@@ -41,10 +41,12 @@ object ProgramContext {
   * @param startGas initial gas for the execution
   * @param world provides interactions with world state
   * @param config evm config
+  * @param initialAddressesToDelete contains initial set of addresses to delete (from lower depth calls)
   */
 case class ProgramContext[W <: WorldStateProxy[W, S], S <: Storage[S]](
   env: ExecEnv,
   receivingAddr: Address,
   startGas: BigInt,
   world: W,
-  config: EvmConfig)
+  config: EvmConfig,
+  initialAddressesToDelete: Set[Address] = Set.empty)

src/main/scala/io/iohk/ethereum/vm/ProgramResult.scala

@@ -16,7 +16,7 @@ case class ProgramResult[W <: WorldStateProxy[W, S], S <: Storage[S]](
   returnData: ByteString,
   gasRemaining: BigInt,
   world: W,
-  addressesToDelete: Seq[Address],
+  addressesToDelete: Set[Address],
   logs: Seq[TxLogEntry],
   gasRefund: BigInt,
   error: Option[ProgramError])

src/main/scala/io/iohk/ethereum/vm/ProgramState.scala

@@ -8,7 +8,8 @@ object ProgramState {
     ProgramState(
       context = context,
       gas = context.startGas,
-      world = context.world)
+      world = context.world,
+      addressesToDelete = context.initialAddressesToDelete)
 }
 
 /**
@@ -34,7 +35,7 @@ case class ProgramState[W <: WorldStateProxy[W, S], S <: Storage[S]](
   pc: Int = 0,
   returnData: ByteString = ByteString.empty,
   gasRefund: BigInt = 0,
-  addressesToDelete: Seq[Address] = Nil,
+  addressesToDelete: Set[Address] = Set.empty,
   logs: Vector[TxLogEntry] = Vector.empty,
   halted: Boolean = false,
   error: Option[ProgramError] = None
@@ -87,9 +88,9 @@ case class ProgramState[W <: WorldStateProxy[W, S], S <: Storage[S]](
     copy(returnData = data)
 
   def withAddressToDelete(addr: Address): ProgramState[W, S] =
-    copy(addressesToDelete = addressesToDelete :+ addr)
+    copy(addressesToDelete = addressesToDelete + addr)
 
-  def withAddressesToDelete(addresses: Seq[Address]): ProgramState[W, S] =
+  def withAddressesToDelete(addresses: Set[Address]): ProgramState[W, S] =
     copy(addressesToDelete = addressesToDelete ++ addresses)
 
   def withLog(log: TxLogEntry): ProgramState[W, S] =

src/test/scala/io/iohk/ethereum/Mocks.scala

@@ -27,7 +27,7 @@ object Mocks {
     returnData = ByteString.empty,
     gasRemaining = 1000000 - 25000,
     world = context.world,
-    addressesToDelete = Nil,
+    addressesToDelete = Set.empty,
     logs = Nil,
     gasRefund = 20000,
     error = None

src/test/scala/io/iohk/ethereum/ledger/DeleteAccountsSpec.scala

@@ -13,7 +13,7 @@ class DeleteAccountsSpec extends FlatSpec with Matchers {
   val ledger = new LedgerImpl(new Mocks.MockVM(), blockchainConfig)
 
   it should "delete no accounts when none of them should be deleted" in new TestSetup {
-    val newWorld = InMemoryWorldStateProxy.persistState(ledger.deleteAccounts(Nil)(worldState))
+    val newWorld = InMemoryWorldStateProxy.persistState(ledger.deleteAccounts(Set.empty)(worldState))
     accountAddresses.foreach{ a => assert(newWorld.getAccount(a).isDefined) }
     newWorld.stateRootHash shouldBe worldState.stateRootHash
   }
@@ -47,7 +47,7 @@ class DeleteAccountsSpec extends FlatSpec with Matchers {
     val validAccountAddress2 = Address(0xcdcdcd)
     val validAccountAddress3 = Address(0xefefef)
 
-    val accountAddresses = Seq(validAccountAddress, validAccountAddress2, validAccountAddress3)
+    val accountAddresses = Set(validAccountAddress, validAccountAddress2, validAccountAddress3)
 
     val worldStateWithoutPersist: InMemoryWorldStateProxy = InMemoryWorldStateProxy(
       storagesInstance.storages,

src/test/scala/io/iohk/ethereum/ledger/LedgerSpec.scala

@@ -35,7 +35,7 @@ class LedgerSpec extends FlatSpec with PropertyChecks with Matchers {
                    error: Option[ProgramError] = None,
                    returnData: ByteString = bEmpty,
                    logs: Seq[TxLogEntry] = Nil,
-                   addressesToDelete: Seq[Address] = Nil): PR =
+                   addressesToDelete: Set[Address] = Set.empty): PR =
     ProgramResult(
       returnData = returnData,
       gasRemaining = gasLimit - gasUsed,
@@ -164,11 +164,11 @@ class LedgerSpec extends FlatSpec with PropertyChecks with Matchers {
 
   it should "correctly run executeBlockTransactions for a block with one tx (that produces no errors)" in new BlockchainSetup {
 
-    val table = Table[BigInt, Seq[TxLogEntry], Seq[Address], Boolean](
+    val table = Table[BigInt, Seq[TxLogEntry], Set[Address], Boolean](
       ("gasLimit/gasUsed", "logs", "addressesToDelete", "txValidAccordingToValidators"),
-      (defaultGasLimit, Nil, Nil, true),
+      (defaultGasLimit, Nil, Set.empty, true),
       (defaultGasLimit / 2, Nil, defaultAddressesToDelete, true),
-      (2 * defaultGasLimit, defaultLogs, Nil, true),
+      (2 * defaultGasLimit, defaultLogs, Set.empty, true),
       (defaultGasLimit, defaultLogs, defaultAddressesToDelete, true),
       (defaultGasLimit, defaultLogs, defaultAddressesToDelete, false)
     )
@@ -711,7 +711,7 @@ class LedgerSpec extends FlatSpec with PropertyChecks with Matchers {
 
     val initialOriginNonce = defaultTx.nonce
 
-    val defaultAddressesToDelete = Seq(Address(Hex.decode("01")), Address(Hex.decode("02")), Address(Hex.decode("03")))
+    val defaultAddressesToDelete = Set(Address(Hex.decode("01")), Address(Hex.decode("02")), Address(Hex.decode("03")))
     val defaultLogs = Seq(defaultLog.copy(loggerAddress = defaultAddressesToDelete.head))
     val defaultGasPrice: UInt256 = 10
     val defaultGasLimit: UInt256 = 1000000

src/test/scala/io/iohk/ethereum/vm/CallOpcodesSpec.scala

@@ -316,13 +316,20 @@ class CallOpcodesSpec extends WordSpec with Matchers with PropertyChecks {
 
     "calling a program that executes a SELFDESTRUCT" should {
 
-      val context: PC = fxt.context.copy(world = fxt.worldWithSelfDestructProgram)
-      val call = CallResult(op = CALL, context)
-
       "refund the correct amount of gas" in {
+        val context: PC = fxt.context.copy(world = fxt.worldWithSelfDestructProgram)
+        val call = CallResult(op = CALL, context)
         call.stateOut.gasRefund shouldBe call.stateOut.config.feeSchedule.R_selfdestruct
       }
 
+      "not refund gas if account was already self destructed" in {
+        val context: PC = fxt.context.copy(
+          world = fxt.worldWithSelfDestructProgram,
+          initialAddressesToDelete = Set(fxt.extAddr))
+        val call = CallResult(op = CALL, context)
+        call.stateOut.gasRefund shouldBe 0
+      }
+
     }
 
     "calling a program that executes a SSTORE that clears the storage" should {