From 612b14de3c999702eeafccee64e6a5ea18bc76dc Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Tue, 27 Jan 2026 22:25:18 +0100 Subject: [PATCH 1/7] added debug_assert! in evm/ext_bytecode.rs --- substrate/frame/revive/src/vm/evm/ext_bytecode.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/substrate/frame/revive/src/vm/evm/ext_bytecode.rs b/substrate/frame/revive/src/vm/evm/ext_bytecode.rs index c72b58194c6dd..6bb1783ebdc04 100644 --- a/substrate/frame/revive/src/vm/evm/ext_bytecode.rs +++ b/substrate/frame/revive/src/vm/evm/ext_bytecode.rs @@ -54,6 +54,11 @@ 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) }; } @@ -89,6 +94,11 @@ 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) } } } From 04da5f55207d19f3016ff2510d14d00def532f81 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Tue, 27 Jan 2026 22:32:15 +0100 Subject: [PATCH 2/7] get rid of transmute from instructions/arithmetic/i256.rs --- .../frame/revive/src/vm/evm/instructions/arithmetic/i256.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/revive/src/vm/evm/instructions/arithmetic/i256.rs b/substrate/frame/revive/src/vm/evm/instructions/arithmetic/i256.rs index 5cf1e38ba5ac2..e43de27cede3b 100644 --- a/substrate/frame/revive/src/vm/evm/instructions/arithmetic/i256.rs +++ b/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::(!val.is_zero()) } + Sign::Plus } } From f6dd782390b50e495e63bf47d3033eff828bf2a3 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Tue, 27 Jan 2026 22:51:41 +0100 Subject: [PATCH 3/7] add calltrace for terminate --- substrate/frame/revive/src/exec.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index f4ec66eb3bc17..f50fd0f11b30f 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1744,6 +1744,16 @@ where // we do this last as we cannot roll this back transaction_meter.terminate(contract_account.clone(), refund); + // emit selfdestruct trace + if_tracing(|t| { + t.terminate( + contract_address, + T::AddressMapper::to_address(&args.beneficiary), + 0, // gas is tracked separately; termination happens at end of call stack + balance, + ); + }); + Ok(()) }; @@ -1753,7 +1763,6 @@ where with_transaction(|| -> TransactionOutcome> { match delete_contract(&args.trie_id, &args.code_hash) { Ok(()) => { - // TODO: emit sucicide trace log::trace!(target: LOG_TARGET, "Terminated {contract_address:?}"); TransactionOutcome::Commit(Ok(())) }, From 717557bb984f04e0d51dcc29b28c96102f040f6d Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Tue, 27 Jan 2026 22:55:20 +0100 Subject: [PATCH 4/7] add gas to terminate calltrace call --- substrate/frame/revive/src/exec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index f50fd0f11b30f..dcb3aeac9946f 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1749,7 +1749,7 @@ where t.terminate( contract_address, T::AddressMapper::to_address(&args.beneficiary), - 0, // gas is tracked separately; termination happens at end of call stack + transaction_meter.eth_gas_left().unwrap_or_default().saturated_into::(), balance, ); }); From 70df786b5d95151144c5c1ee31e22ebc9cb184e9 Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 29 Jan 2026 13:16:48 +0100 Subject: [PATCH 5/7] small improvements --- substrate/frame/revive/src/exec.rs | 34 +++++++++++-------- .../frame/revive/src/vm/evm/ext_bytecode.rs | 4 +++ 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index dcb3aeac9946f..40c0909db8a7a 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -1744,34 +1744,38 @@ where // we do this last as we cannot roll this back transaction_meter.terminate(contract_account.clone(), refund); - // emit selfdestruct trace - if_tracing(|t| { - t.terminate( - contract_address, - T::AddressMapper::to_address(&args.beneficiary), - transaction_meter.eth_gas_left().unwrap_or_default().saturated_into::(), - balance, - ); - }); - - 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> { + let result = with_transaction(|| -> TransactionOutcome> { match delete_contract(&args.trie_id, &args.code_hash) { - Ok(()) => { + 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::(), + balance, + ); + }); + } + + result.map(|_| ()) } /// Reference to the current (top) frame. diff --git a/substrate/frame/revive/src/vm/evm/ext_bytecode.rs b/substrate/frame/revive/src/vm/evm/ext_bytecode.rs index 6bb1783ebdc04..2e197178ed7a4 100644 --- a/substrate/frame/revive/src/vm/evm/ext_bytecode.rs +++ b/substrate/frame/revive/src/vm/evm/ext_bytecode.rs @@ -66,6 +66,10 @@ impl ExtBytecode { /// 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) }; } From cb962a3874858448365f641838eb8942fe3029bd Mon Sep 17 00:00:00 2001 From: Robert van Eerdewijk Date: Thu, 29 Jan 2026 13:17:05 +0100 Subject: [PATCH 6/7] format --- .../frame/revive/src/vm/evm/ext_bytecode.rs | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/substrate/frame/revive/src/vm/evm/ext_bytecode.rs b/substrate/frame/revive/src/vm/evm/ext_bytecode.rs index 2e197178ed7a4..0815e3d73da69 100644 --- a/substrate/frame/revive/src/vm/evm/ext_bytecode.rs +++ b/substrate/frame/revive/src/vm/evm/ext_bytecode.rs @@ -54,11 +54,14 @@ 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"); + 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) }; } @@ -98,11 +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"); + 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) } } } From a55bdf2811fb460f7b0f743cf51076ad8c5b02b2 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 30 Jan 2026 10:46:12 +0000 Subject: [PATCH 7/7] Update from github-actions[bot] running command 'prdoc --audience runtime_dev --bump patch' --- prdoc/pr_10922.prdoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 prdoc/pr_10922.prdoc diff --git a/prdoc/pr_10922.prdoc b/prdoc/pr_10922.prdoc new file mode 100644 index 0000000000000..a4a4048763635 --- /dev/null +++ b/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