CIP-16 Collect suicide logic at the end of transaction processing (#1748)

* Remove fresh collateral and read substate for collateral settlement

Add comment

remove unneccessary mutable

Collect suicide logic at the end of transaction processing

update after refactoring settlement

Update suicide logic and refactor substate

Several fixes

add change log

Burn when invalid suicide refund

Refactor commit ownership

Merge with master

Author: ChenxingLi

changelogs/CHANGELOG-1.0.x.md

@@ -9,7 +9,6 @@ Use `mining_type` to allow start CPU mining or disable mining manually.
 - Fix bug where users need to restart node before they can use a newly created account to send transactions.
 - Fix code() return value for uninitialized contract.
 - Fix bug in kill_account after which the contract account is revived by simple transaction.
-- Fix the place of collateral refund for suicide contracts.
 - Fix missing StorageKey conversion from bytes of DepositList and VoteList.
 
 ## Incompatible changes
@@ -19,3 +18,4 @@ Use `mining_type` to allow start CPU mining or disable mining manually.
 - CIP-10 Base mining reward finalization.
 - CIP-12 Allow non-zero collateral contract to be killed.
 - CIP-13 Use Big-Endian MPT Keys.
+- CIP-16 Collect suicide logic at the end of transaction processing

core/src/executive/context.rs

@@ -16,7 +16,7 @@ use crate::{
         ReturnData, Spec, TrapKind,
     },
 };
-use cfx_types::{address_util::AddressUtil, Address, H256, U256};
+use cfx_types::{Address, H256, U256};
 use primitives::transaction::UNSIGNED_SENDER;
 use std::sync::Arc;
 
@@ -315,11 +315,10 @@ impl<'a> ContextTrait for Context<'a> {
                 let collateral_for_code =
                     U256::from(data.len()) * *COLLATERAL_PER_BYTE;
                 debug!("ret()  collateral_for_code={:?}", collateral_for_code);
-                *self
-                    .substate
-                    .storage_collateralized
-                    .entry(self.origin.storage_owner)
-                    .or_insert(0) += data.len() as u64;
+                self.substate.record_storage_occupy(
+                    &self.origin.storage_owner,
+                    data.len() as u64,
+                );
 
                 self.state.init_code(
                     &self.origin.address,
@@ -355,10 +354,6 @@ impl<'a> ContextTrait for Context<'a> {
             return Err(vm::Error::MutableCallInStaticContext);
         }
 
-        if !refund_address.is_valid_address() {
-            return Err(vm::Error::InvalidAddress(*refund_address));
-        }
-
         suicide_impl(
             &self.origin.address,
             refund_address,

core/src/executive/executive.rs

@@ -16,7 +16,9 @@ use crate::{
     hash::keccak,
     machine::Machine,
     parameters::staking::*,
-    state::{CallStackInfo, CleanupMode, State, Substate},
+    state::{
+        CallStackInfo, CleanupMode, CollateralCheckResult, State, Substate,
+    },
     statedb::Result as DbResult,
     verification::VerificationConfig,
     vm::{
@@ -1321,7 +1323,7 @@ impl<'a> Executive<'a> {
             ));
         }
 
-        let mut substate = Substate::new();
+        let mut tx_substate = Substate::new();
         if balance512 < sender_intended_cost {
             // Sender is responsible for the insufficient balance.
             // Sub tx fee if not enough cash, and substitute all remaining
@@ -1347,7 +1349,7 @@ impl<'a> Executive<'a> {
             self.state.sub_balance(
                 &sender,
                 &actual_gas_cost,
-                &mut substate.to_cleanup_mode(&spec),
+                &mut tx_substate.to_cleanup_mode(&spec),
             )?;
 
             return Ok(ExecutionOutcome::ExecutionErrorBumpNonce(
@@ -1372,7 +1374,7 @@ impl<'a> Executive<'a> {
             self.state.sub_balance(
                 &sender,
                 &U256::try_from(gas_cost).unwrap(),
-                &mut substate.to_cleanup_mode(&spec),
+                &mut tx_substate.to_cleanup_mode(&spec),
             )?;
         } else {
             self.state.sub_sponsor_balance_for_gas(
@@ -1382,6 +1384,7 @@ impl<'a> Executive<'a> {
         }
 
         self.state.checkpoint();
+        let mut substate = Substate::new();
 
         let res = match tx.action {
             Action::Create => {
@@ -1461,6 +1464,7 @@ impl<'a> Executive<'a> {
             let out = match &res {
                 Ok(res) => {
                     self.state.discard_checkpoint();
+                    tx_substate.accrue(substate);
                     res.return_data.to_vec()
                 }
                 Err(vm::Error::StateDbError(_)) => {
@@ -1483,14 +1487,81 @@ impl<'a> Executive<'a> {
 
         Ok(self.finalize(
             tx,
-            substate,
+            tx_substate,
             result,
             output,
             refund_receiver,
             storage_sponsored,
         )?)
     }
 
+    fn kill_process(
+        &mut self, suicides: &HashSet<Address>,
+    ) -> DbResult<Substate> {
+        let mut substate = Substate::new();
+        for address in suicides {
+            if let Some(code_size) = self.state.code_size(address)? {
+                // Only refund the code collateral when code exists.
+                // If a contract suicides during creation, the code will be
+                // empty.
+                let code_owner =
+                    self.state.code_owner(address)?.expect("code owner exists");
+                substate.record_storage_release(&code_owner, code_size as u64);
+            }
+
+            self.state
+                .record_storage_entries_release(address, &mut substate)?;
+        }
+
+        let res = self.state.settle_collateral_for_all(&substate)?;
+        // The storage recycling process should never occupy new collateral.
+        assert_eq!(res, CollateralCheckResult::Valid);
+
+        for contract_address in suicides {
+            let sponsor_for_gas =
+                self.state.sponsor_for_gas(contract_address)?;
+            let sponsor_for_collateral =
+                self.state.sponsor_for_collateral(contract_address)?;
+            let sponsor_balance_for_gas =
+                self.state.sponsor_balance_for_gas(contract_address)?;
+            let sponsor_balance_for_collateral = self
+                .state
+                .sponsor_balance_for_collateral(contract_address)?;
+
+            if sponsor_for_gas.is_some() {
+                self.state.add_balance(
+                    sponsor_for_gas.as_ref().unwrap(),
+                    &sponsor_balance_for_gas,
+                    substate.to_cleanup_mode(self.spec),
+                )?;
+                self.state.sub_sponsor_balance_for_gas(
+                    contract_address,
+                    &sponsor_balance_for_gas,
+                )?;
+            }
+            if sponsor_for_collateral.is_some() {
+                self.state.add_balance(
+                    sponsor_for_collateral.as_ref().unwrap(),
+                    &sponsor_balance_for_collateral,
+                    substate.to_cleanup_mode(self.spec),
+                )?;
+                self.state.sub_sponsor_balance_for_collateral(
+                    contract_address,
+                    &sponsor_balance_for_collateral,
+                )?;
+            }
+        }
+
+        for contract_address in suicides {
+            let burnt_balance = self.state.balance(contract_address)?
+                + self.state.staking_balance(contract_address)?;
+            self.state.remove_contract(contract_address)?;
+            self.state.subtract_total_issued(burnt_balance);
+        }
+
+        Ok(substate)
+    }
+
     /// Finalizes the transaction (does refunds and suicides).
     fn finalize(
         &mut self, tx: &SignedTransaction, mut substate: Substate,
@@ -1531,9 +1602,9 @@ impl<'a> Executive<'a> {
         };
 
         // perform suicides
-        for address in &substate.suicides {
-            self.state.kill_account(address)?;
-        }
+
+        let subsubstate = self.kill_process(&substate.suicides)?;
+        substate.accrue(subsubstate);
 
         // TODO should be added back after enabling dust collection
         // Should be executed once per block, instead of per transaction?
@@ -1567,36 +1638,24 @@ impl<'a> Executive<'a> {
                 let mut storage_released = Vec::new();
 
                 if r.apply_state {
-                    let affected_address1: HashSet<_> = substate
-                        .storage_collateralized
-                        .keys()
+                    let mut affected_address: Vec<_> = substate
+                        .keys_for_collateral_changed()
+                        .iter()
                         .cloned()
                         .collect();
-                    let affected_address2: HashSet<_> =
-                        substate.storage_released.keys().cloned().collect();
-                    let mut affected_address: Vec<_> =
-                        affected_address1.union(&affected_address2).collect();
                     affected_address.sort();
                     for address in affected_address {
-                        let inc = substate
-                            .storage_collateralized
-                            .get(address)
-                            .cloned()
-                            .unwrap_or(0);
-                        let sub = substate
-                            .storage_released
-                            .get(address)
-                            .cloned()
-                            .unwrap_or(0);
-                        if inc > sub {
+                        let (inc, sub) =
+                            substate.get_collateral_change(address);
+                        if inc > 0 {
                             storage_collateralized.push(StorageChange {
                                 address: *address,
-                                amount: inc - sub,
+                                amount: inc,
                             });
-                        } else if inc < sub {
+                        } else if sub > 0 {
                             storage_released.push(StorageChange {
                                 address: *address,
-                                amount: sub - inc,
+                                amount: sub,
                             });
                         }
                     }

core/src/executive/internal_contract/impls/admin.rs

@@ -7,7 +7,7 @@ use crate::{
     state::{State, Substate},
     vm::{self, ActionParams, CallType, Spec},
 };
-use cfx_types::{Address, U256};
+use cfx_types::{address_util::AddressUtil, Address, U256};
 use std::str::FromStr;
 
 lazy_static! {
@@ -35,48 +35,8 @@ pub fn suicide(
     substate.suicides.insert(contract_address.clone());
     let balance = state.balance(contract_address)?;
 
-    if let Some(code_size) = state.code_size(contract_address)? {
-        // Only refund the code collateral when code exists.
-        // If a contract suicides during creation, the code will be empty.
-        let code_owner = state
-            .code_owner(contract_address)?
-            .expect("code owner exists");
-        *substate.storage_released.entry(code_owner).or_insert(0) +=
-            code_size as u64;
-    }
-
-    let sponsor_for_gas = state.sponsor_for_gas(contract_address)?;
-    let sponsor_for_collateral =
-        state.sponsor_for_collateral(contract_address)?;
-    let sponsor_balance_for_gas =
-        state.sponsor_balance_for_gas(contract_address)?;
-    let sponsor_balance_for_collateral =
-        state.sponsor_balance_for_collateral(contract_address)?;
-
-    if sponsor_for_gas.is_some() {
-        state.add_balance(
-            sponsor_for_gas.as_ref().unwrap(),
-            &sponsor_balance_for_gas,
-            substate.to_cleanup_mode(spec),
-        )?;
-        state.sub_sponsor_balance_for_gas(
-            contract_address,
-            &sponsor_balance_for_gas,
-        )?;
-    }
-    if sponsor_for_collateral.is_some() {
-        state.add_balance(
-            sponsor_for_collateral.as_ref().unwrap(),
-            &sponsor_balance_for_collateral,
-            substate.to_cleanup_mode(spec),
-        )?;
-        state.sub_sponsor_balance_for_collateral(
-            contract_address,
-            &sponsor_balance_for_collateral,
-        )?;
-    }
-    if refund_address == contract_address {
-        // This is the corner case that the sponsor of the contract is itself.
+    if refund_address == contract_address || !refund_address.is_valid_address()
+    {
         // When destroying, the balance will be burnt.
         state.sub_balance(
             contract_address,

core/src/state/account_entry.rs

@@ -6,7 +6,8 @@ use crate::{
     bytes::Bytes,
     consensus::debug::ComputeEpochDebugRecord,
     hash::{keccak, KECCAK_EMPTY},
-    state::{AccountEntryProtectedMethods, State},
+    parameters::staking::BYTES_PER_STORAGE_KEY,
+    state::{AccountEntryProtectedMethods, State, Substate},
     statedb::{Result as DbResult, StateDb, StateDbExt},
 };
 use cfx_types::{address_util::AddressUtil, Address, H256, U256};
@@ -732,9 +733,8 @@ impl OverlayAccount {
     /// execution. The second value means the number of keys released by this
     /// account in current execution.
     pub fn commit_ownership_change(
-        &mut self, db: &StateDb,
-    ) -> DbResult<HashMap<Address, (u64, u64)>> {
-        let mut storage_delta = HashMap::new();
+        &mut self, db: &StateDb, substate: &mut Substate,
+    ) -> DbResult<()> {
         let ownership_changes: Vec<_> =
             self.ownership_changes.drain().collect();
         for (k, v) in ownership_changes {
@@ -755,16 +755,16 @@ impl OverlayAccount {
                 // If the current value is zero or the owner has changed for the
                 // key, it means the key has released from previous owner.
                 if cur_value_is_zero || ownership_changed {
-                    storage_delta
-                        .entry(original_ownership)
-                        .or_insert((0, 0))
-                        .1 += 1;
+                    substate.record_storage_release(
+                        &original_ownership,
+                        BYTES_PER_STORAGE_KEY,
+                    );
                 }
             }
             // If the current value is not zero and the owner has changed, it
             // means the owner has occupied a new key.
             if !cur_value_is_zero && ownership_changed {
-                storage_delta.entry(v).or_insert((0, 0)).0 += 1;
+                substate.record_storage_occupy(&v, BYTES_PER_STORAGE_KEY);
             }
             // Commit ownership change to `ownership_cache`.
             if cur_value_is_zero {
@@ -774,7 +774,7 @@ impl OverlayAccount {
             }
         }
         assert!(self.ownership_changes.is_empty());
-        Ok(storage_delta)
+        Ok(())
     }
 
     pub fn commit(