[pallet-revive] small improvements

prdoc/pr_10922.prdoc

@@ -0,0 +1,12 @@
+title: '[pallet-revive] small improvements'
+doc:
+- audience: Runtime Dev
+  description: |-
+    Small safety and completeness improvements for pallet-revive:
+
+    - Add selfdestruct call tracing: Emit terminate trace after successful SELFDESTRUCT
+    - Add debug assertions for unsafe bytecode operations: in relative_jump, absolute_jump, and read_slice
+    - Remove transmute in i256 sign detection: Replace unsafe { core::mem::transmute } with explicit conditional logic for determining Sign::Zero vs Sign::Plus
+crates:
+- name: pallet-revive
+  bump: patch

substrate/frame/revive/src/exec.rs

@@ -1744,25 +1744,38 @@ where
 			// we do this last as we cannot roll this back
 			transaction_meter.terminate(contract_account.clone(), refund);
 
-			Ok(())
+			Ok(balance)
 		};
 
 		// we cannot fail here as the contract that called `SELFDESTRUCT`
 		// is no longer on the call stack. hence we simply roll back the
 		// termination so that nothing happened.
-		with_transaction(|| -> TransactionOutcome<Result<_, DispatchError>> {
+		let result = with_transaction(|| -> TransactionOutcome<Result<_, DispatchError>> {
 			match delete_contract(&args.trie_id, &args.code_hash) {
-				Ok(()) => {
-					// TODO: emit sucicide trace
+				Ok(balance) => {
 					log::trace!(target: LOG_TARGET, "Terminated {contract_address:?}");
-					TransactionOutcome::Commit(Ok(()))
+					TransactionOutcome::Commit(Ok(balance))
 				},
 				Err(e) => {
 					log::debug!(target: LOG_TARGET, "Contract at {contract_address:?} failed to terminate: {e:?}");
 					TransactionOutcome::Rollback(Err(e))
 				},
 			}
-		})
+		});
+
+		// emit selfdestruct trace only after the transaction successfully commits
+		if let Ok(balance) = result {
+			if_tracing(|t| {
+				t.terminate(
+					contract_address,
+					T::AddressMapper::to_address(&args.beneficiary),
+					transaction_meter.eth_gas_left().unwrap_or_default().saturated_into::<u64>(),
+					balance,
+				);
+			});
+		}
+
+		result.map(|_| ())
 	}
 
 	/// Reference to the current (top) frame.

substrate/frame/revive/src/vm/evm/ext_bytecode.rs

@@ -54,13 +54,25 @@ impl ExtBytecode {
 	/// Relative jumps does not require checking for overflow.
 	pub fn relative_jump(&mut self, offset: isize) {
 		// SAFETY: The offset is validated by the caller to ensure it points within the bytecode
+		debug_assert!(
+			{
+				let bytes = self.base.bytes_ref();
+				let new_pc = self.pc().wrapping_add_signed(offset);
+				new_pc <= bytes.len()
+			},
+			"relative_jump would move instruction pointer out of bounds"
+		);
 		self.instruction_pointer = unsafe { self.instruction_pointer.offset(offset) };
 	}
 
 	/// Absolute jumps require checking for overflow and if target is a jump destination
 	/// from jump table.
 	pub fn absolute_jump(&mut self, offset: usize) {
 		// SAFETY: The offset is validated by the caller to ensure it points within the bytecode
+		debug_assert!(
+			offset <= self.base.bytes_ref().len(),
+			"absolute_jump would move instruction pointer out of bounds"
+		);
 		self.instruction_pointer = unsafe { self.base.bytes_ref().as_ptr().add(offset) };
 	}
 
@@ -89,6 +101,14 @@ impl ExtBytecode {
 	pub fn read_slice(&self, len: usize) -> &[u8] {
 		// SAFETY: The caller ensures that `len` bytes are available from the current instruction
 		// pointer position.
+		debug_assert!(
+			{
+				let bytes = self.base.bytes_ref();
+				let pc = self.pc();
+				pc.checked_add(len).map_or(false, |end| end <= bytes.len())
+			},
+			"read_slice would read out of bounds"
+		);
 		unsafe { core::slice::from_raw_parts(self.instruction_pointer, len) }
 	}
 }

substrate/frame/revive/src/vm/evm/instructions/arithmetic/i256.rs

@@ -27,7 +27,6 @@ pub enum Sign {
 	Minus = -1,
 	/// Zero value sign
 	Zero = 0,
-	#[allow(dead_code)] // "constructed" with `mem::transmute` in `i256_sign` below
 	/// Positive value sign
 	Plus = 1,
 }
@@ -43,9 +42,10 @@ const FLIPH_BITMASK_U64: u64 = 0x7FFF_FFFF_FFFF_FFFF;
 pub fn i256_sign(val: &U256) -> Sign {
 	if val.bit(255) {
 		Sign::Minus
+	} else if val.is_zero() {
+		Sign::Zero
 	} else {
-		// SAFETY: false == 0 == Zero, true == 1 == Plus
-		unsafe { core::mem::transmute::<bool, Sign>(!val.is_zero()) }
+		Sign::Plus
 	}
 }