Fix termination

prdoc/pr_10302.prdoc

@@ -0,0 +1,35 @@
+title: Fix termination
+doc:
+- audience: Runtime Dev
+  description: |-
+    This PR fixes up termination by changing the behavior to:
+
+    - The free balance (without ed) should be send away right away to the beneficiary and not be delayed like the contract deletion.
+    - The ed and storage deposit will be send away only when terminating but to the origin (delayed).
+    - The scheduling of the terminate needs to be reverted if the scheduling frame reverts.
+    - `SELFDESTRUCT` should be allowed inside the constructor. The issuing contract will exist as account without code for the remainder of the transaction.
+    - The `terminate` pre-compile should revert if delegate called or its caller was delegate called. This is just my opinion but if we are changing semantics we can might as well add some security. We are increasing the attack surface by allowing the destruction of any contract (not only created in the current tx).
+
+
+    ## Other fixes
+    - Storage refunds should no longer use `BestEffort`. This is necessary to fail refunds in case some other locks (due to participation in gov for example) prevent sending them away. This is in anticipation of new pre-compiles.
+    - Moved pre-compile interfaces to sol files and made them available to fixtures
+    - Added some Solidity written tests to exercise error cases
+
+
+    ## Further tests needed
+
+    Those should all be written in Solidity to test both backends at the same time. No more Rust fixtures.
+
+    @0xRVE can you take those over as I am ooo.
+
+    - Test that checks that scheduled deletions do properly roll back if a frame fails
+    - Test that value send to a contract after scheduling for deletion is send to the beneficiary (different from Eth where this balance is lost)
+    - Add tests that use `SELFDESTRUCT` to `Terminate.sol`. Need https://github.com/paritytech/devops/issues/4508 but can be tested locally with newest `resolc`.
+crates:
+- name: pallet-revive-fixtures
+  bump: patch
+- name: pallet-revive
+  bump: patch
+- name: pallet-revive-uapi
+  bump: patch

substrate/frame/revive/fixtures/build.rs

@@ -18,6 +18,7 @@
 //! Compile text fixtures to PolkaVM binaries.
 use anyhow::{bail, Context, Result};
 use cargo_metadata::MetadataCommand;
+use pallet_revive_uapi::precompiles::INTERFACE_DIR;
 use std::{
 	env, fs,
 	io::Write,
@@ -232,6 +233,8 @@ fn compile_with_standard_json(
 	contracts_dir: &Path,
 	solidity_entries: &[&Entry],
 ) -> Result<serde_json::Value> {
+	let remappings = [format!("@revive/={INTERFACE_DIR}")];
+
 	let mut input_json = serde_json::json!({
 		"language": "Solidity",
 		"sources": {},
@@ -240,6 +243,7 @@ fn compile_with_standard_json(
 				"enabled": false,
 				"runs": 200
 			},
+			"remappings": remappings,
 			"outputSelection":
 
 		serde_json::json!({
@@ -264,6 +268,8 @@ fn compile_with_standard_json(
 
 	let compiler_output = Command::new(compiler)
 		.current_dir(contracts_dir)
+		.arg("--allow-paths")
+		.arg(INTERFACE_DIR)
 		.arg("--standard-json")
 		.stdin(std::process::Stdio::piped())
 		.stdout(std::process::Stdio::piped())

substrate/frame/revive/fixtures/contracts/Terminate.sol

@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.30;
+
+import "@revive/ISystem.sol";
+
+contract Terminate {
+	uint8 public constant METHOD_PRECOMPILE = 0;
+	uint8 public constant METHOD_DELEGATE_CALL = 1;
+	uint8 public constant METHOD_SYSCALL = 2;
+    receive() external payable {}
+	constructor(bool skip, uint8 method, address beneficiary) payable {
+		if (skip) {
+			return;
+		}
+		_terminate(method, beneficiary);
+	}
+
+	function echo(uint value) external pure returns (uint) {
+		return value;
+	}
+
+	function terminate(uint8 method, address beneficiary) external {
+		_terminate(method, beneficiary);
+	}
+
+	function indirectDelegateTerminate(address beneficiary) external {
+		bytes memory data = abi.encodeWithSelector(this.terminate.selector, METHOD_PRECOMPILE, beneficiary);
+		(bool success, bytes memory returnData) = address(this).delegatecall(data);
+		if (!success) {
+			assembly {
+				revert(add(returnData, 0x20), mload(returnData))
+			}
+		}
+	}
+	/// Call terminate and forward any revert.
+    /// Internal dispatcher: executes termination by
+    /// - delegatecall (METHOD_DELEGATE_CALL) to system precompile
+    /// - direct call (METHOD_PRECOMPILE) to system precompile
+    /// - selfdestruct (METHOD_SYSCALL) sending balance to beneficiary
+	function _terminate(uint8 method, address beneficiary) private {
+		bytes memory data = abi.encodeWithSelector(ISystem.terminate.selector, beneficiary);
+		(bool success, bytes memory returnData) = (false, "");
+
+		if (method == METHOD_DELEGATE_CALL) {
+			(success, returnData) = SYSTEM_ADDR.delegatecall(data);
+		} else if (method == METHOD_PRECOMPILE) {
+			(success, returnData) = SYSTEM_ADDR.call(data);
+		} else if (method == METHOD_SYSCALL) {
+			assembly {
+				selfdestruct(beneficiary)
+			}
+			// selfdestruct halts execution, so if we reach here, something went wrong.
+			revert("selfdestruct opcode returned");
+		} else {
+			revert("Invalid TerminateMethod");
+		}
+
+		if (!success) {
+			assembly {
+				revert(add(returnData, 0x20), mload(returnData))
+			}
+		}
+	}
+}
+

substrate/frame/revive/fixtures/contracts/TerminateCaller.sol

@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.30;
+
+import { Terminate } from "./Terminate.sol";
+import { TerminateDelegator } from "./TerminateDelegator.sol";
+
+contract TerminateCaller {
+    Terminate inner;
+    TerminateDelegator innerCaller;
+    receive() external payable {}
+
+    constructor() payable {}
+
+    function createAndTerminateTwice(uint value, uint8 method1, uint8 method2, address beneficiary) external returns (address) {
+        inner = new Terminate{value: value}(true, method1, beneficiary);
+        inner.terminate(method1, beneficiary);
+        inner.terminate(method2, beneficiary);
+        return address(inner);
+    }
+
+    function sendFundsAfterTerminateAndCreate(uint value, uint8 method, address beneficiary) external returns (address) {
+        inner = new Terminate(true, method, beneficiary);
+        inner.terminate(method, beneficiary);
+        (bool success, ) = address(inner).call{value: value}("");
+        require(success, "terminate reverted");
+        return address(inner);
+    }
+
+    function sendFundsAfterTerminate(address payable terminate_addr, uint value, uint8 method, address beneficiary) external {
+        terminate_addr.call(abi.encodeWithSelector(Terminate.terminate.selector, method, beneficiary));
+        (bool success, ) = terminate_addr.call{value: value}("");
+        require(success, "terminate reverted");
+    }
+
+    function revertAfterTerminate(address terminate_addr, uint8 method, address beneficiary) external {
+        terminate_addr.call(abi.encodeWithSelector(Terminate.terminate.selector, method, beneficiary));
+        revert("Deliberate revert");
+    }
+
+    function delegateCallTerminate(uint value, uint8 method, address beneficiary) external returns (address, address) {
+        inner = new Terminate(true, method, beneficiary);
+        innerCaller = new TerminateDelegator{value: value}();
+        bytes memory data = abi.encodeWithSelector(innerCaller.delegateCallTerminate.selector, address(inner), method, beneficiary);
+        (bool success, ) = address(innerCaller).call(data);
+        require(success, "delegatecall terminate reverted");
+        return (address(innerCaller), address(inner));
+    }
+
+    function callAfterTerminate(uint value, uint8 method) external returns (address, uint) {
+        inner = new Terminate(true, method, payable(address(this)));
+        inner.terminate(0, payable(address(this)));
+        bytes memory data = abi.encodeWithSelector(inner.echo.selector, value);
+        (bool success, bytes memory returnData) = address(inner).call(data);
+        require(success, "call after terminate reverted");
+        return (address(inner), returnData.length == 32 ? abi.decode(returnData, (uint)) : 0);
+    }
+}

substrate/frame/revive/fixtures/contracts/TerminateDelegator.sol

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.30;
+
+import { Terminate } from "./Terminate.sol";
+
+contract TerminateDelegator {
+    receive() external payable {}
+
+    constructor() payable {}
+
+    function delegateCallTerminate(address terminate_addr, uint8 method, address beneficiary) external {
+        bytes memory data = abi.encodeWithSelector(Terminate.terminate.selector, method, beneficiary);
+        (bool success, ) = terminate_addr.delegatecall(data);
+        require(success, "delegatecall terminate reverted");
+    }
+}

substrate/frame/revive/fixtures/contracts/self_destruct_factory.rs

@@ -17,7 +17,7 @@ pub extern "C" fn deploy() {}
 #[polkavm_derive::polkavm_export]
 pub extern "C" fn call() {
     input!(36, code_hash: [u8],);
-    
+
     let mut addr = [0u8; 20];
     let salt = [1u8; 32];
     api::instantiate(
@@ -30,18 +30,18 @@ pub extern "C" fn call() {
         None,
         Some(&salt),
     ).unwrap();
-    
+
     api::call(
         uapi::CallFlags::empty(),
         &addr,
         u64::MAX,
         u64::MAX,
-        &[0u8; 32],
+        &[u8::MAX; 32],
         &[0u8; 32],
         &[],
         None,
     ).unwrap();
-    
+
     // Return the address of the created (and destroyed) contract
     api::return_value(uapi::ReturnFlags::empty(), &addr);
 }

substrate/frame/revive/src/benchmarking.rs

@@ -51,10 +51,7 @@ use frame_support::{
 	self, assert_ok,
 	migrations::SteppedMigration,
 	storage::child,
-	traits::{
-		fungible::{InspectHold, UnbalancedHold},
-		Hooks,
-	},
+	traits::{fungible::InspectHold, Hooks},
 	weights::{Weight, WeightMeter},
 };
 use frame_system::RawOrigin;
@@ -134,7 +131,7 @@ mod benchmarks {
 	fn on_initialize_per_trie_key(k: Linear<0, 1024>) -> Result<(), BenchmarkError> {
 		let instance =
 			Contract::<T>::with_storage(VmBinaryModule::dummy(), k, limits::STORAGE_BYTES)?;
-		instance.info()?.queue_trie_for_deletion();
+		ContractInfo::<T>::queue_trie_for_deletion(instance.info()?.trie_id);
 
 		#[block]
 		{
@@ -1219,29 +1216,25 @@ mod benchmarks {
 
 	#[benchmark(pov_mode = Measured)]
 	fn seal_terminate_logic() -> Result<(), BenchmarkError> {
+		let caller = whitelisted_caller();
 		let beneficiary = account::<T::AccountId>("beneficiary", 0, 0);
+		T::AddressMapper::map_no_deposit(&beneficiary)?;
 
 		build_runtime!(_runtime, instance, _memory: [vec![0u8; 0], ]);
 		let code_hash = instance.info()?.code_hash;
 
 		assert!(PristineCode::<T>::get(code_hash).is_some());
 
-		// Set storage deposit to zero so terminate_logic can proceed.
-		T::Currency::set_balance_on_hold(
-			&HoldReason::StorageDepositReserve.into(),
-			&instance.account_id,
-			0u32.into(),
-		)
-		.unwrap();
-
-		T::Currency::set_balance(&instance.account_id, Pallet::<T>::min_balance() * 2u32.into());
+		T::Currency::set_balance(&instance.account_id, Pallet::<T>::min_balance() * 10u32.into());
 
 		let result;
 		#[block]
 		{
-			result = crate::storage::meter::terminate_logic_for_benchmark::<T>(
+			result = crate::exec::terminate_contract_for_benchmark::<T>(
+				caller,
 				&instance.account_id,
-				&beneficiary,
+				&instance.info()?,
+				beneficiary.clone(),
 			);
 		}
 		result.unwrap();
@@ -1255,7 +1248,7 @@ mod benchmarks {
 
 		// Check that the beneficiary received the balance
 		let balance = <T as Config>::Currency::balance(&beneficiary);
-		assert_eq!(balance, Pallet::<T>::min_balance() * 2u32.into());
+		assert_eq!(balance, Pallet::<T>::min_balance() + Pallet::<T>::min_balance() * 9u32.into());
 
 		Ok(())
 	}

substrate/frame/revive/src/evm/transfer_with_dust.rs

@@ -35,8 +35,9 @@ fn transfer_balance<T: Config>(
 	from: &AccountIdOf<T>,
 	to: &AccountIdOf<T>,
 	value: BalanceOf<T>,
+	preservation: Preservation,
 ) -> DispatchResult {
-	T::Currency::transfer(from, to, value, Preservation::Preserve)
+	T::Currency::transfer(from, to, value, preservation)
 			.map_err(|err| {
 				log::debug!(target: LOG_TARGET, "Transfer failed: from {from:?} to {to:?} (value: ${value:?}). Err: {err:?}");
 				Error::<T>::TransferFailed
@@ -93,27 +94,28 @@ pub(crate) fn transfer_with_dust<T: Config>(
 	from: &AccountIdOf<T>,
 	to: &AccountIdOf<T>,
 	value: BalanceWithDust<BalanceOf<T>>,
+	preservation: Preservation,
 ) -> DispatchResult {
 	let from_addr = <T::AddressMapper as AddressMapper<T>>::to_address(from);
 	let mut from_info = AccountInfoOf::<T>::get(&from_addr).unwrap_or_default();
 
-	if from_info.balance(from) < value {
-		log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} to {to:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from));
+	if from_info.balance(from, preservation) < value {
+		log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} to {to:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from, preservation));
 		return Err(Error::<T>::TransferFailed.into())
 	} else if from == to || value.is_zero() {
 		return Ok(())
 	}
 
 	let (value, dust) = value.deconstruct();
 	if dust == 0 {
-		return transfer_balance::<T>(from, to, value)
+		return transfer_balance::<T>(from, to, value, preservation)
 	}
 
 	let to_addr = <T::AddressMapper as AddressMapper<T>>::to_address(to);
 	let mut to_info = AccountInfoOf::<T>::get(&to_addr).unwrap_or_default();
 
 	ensure_sufficient_dust::<T>(from, &mut from_info, dust)?;
-	transfer_balance::<T>(from, to, value)?;
+	transfer_balance::<T>(from, to, value, preservation)?;
 	transfer_dust::<T>(&mut from_info, &mut to_info, dust)?;
 
 	let plank = T::NativeToEthRatio::get();
@@ -136,8 +138,8 @@ pub(crate) fn burn_with_dust<T: Config>(
 	let from_addr = <T::AddressMapper as AddressMapper<T>>::to_address(from);
 	let mut from_info = AccountInfoOf::<T>::get(&from_addr).unwrap_or_default();
 
-	if from_info.balance(from) < value {
-		log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from));
+	if from_info.balance(from, Preservation::Preserve) < value {
+		log::debug!(target: LOG_TARGET, "Insufficient balance: from {from:?} (value: ${value:?}). Balance: ${:?}", from_info.balance(from, Preservation::Preserve));
 		return Err(Error::<T>::TransferFailed.into())
 	} else if value.is_zero() {
 		return Ok(())