Various fixes to checkpoint logic (#1039)

* Stack of checkpoints * Fixes * Don't use macro in revert selfdestruct * All stZeroCallsTest, stNonZeroCallsTest, stZeroCallsRevert passing * Minor
2026-01-03 06:13:07 +00:00 · 2023-05-16 10:47:38 +02:00 · 2023-05-16 10:47:38 +02:00 · bde7fb5019
commit bde7fb5019
parent b116929f11
18 changed files with 83 additions and 27 deletions
--- a/evm/src/cpu/kernel/asm/core/call.asm
+++ b/evm/src/cpu/kernel/asm/core/call.asm
@ -11,7 +11,7 @@ global sys_call:
    MUL // Cheaper than AND
    %jumpi(fault_exception)

-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint

    %stack (kexit_info, gas, address, value, args_offset, args_size, ret_offset, ret_size) ->
        (args_size, args_offset, kexit_info, gas, address, value, args_offset, args_size, ret_offset, ret_size)
@ -36,7 +36,7 @@ global sys_call:
          (new_ctx, args_offset, args_size, new_ctx, kexit_info, callgas, address, value, args_offset, args_size, ret_offset, ret_size)
    %copy_mem_to_calldata
    // stack: new_ctx, kexit_info, callgas, address, value, args_offset, args_size, ret_offset, ret_size
-    DUP5 DUP5 %address %transfer_eth %jumpi(panic) // TODO: Fix this panic.
+    DUP5 DUP5 %address %transfer_eth %jumpi(call_insufficient_balance)
    DUP5 DUP5 %address %journal_add_balance_transfer
    DUP3 %set_new_ctx_gas_limit
    %set_new_ctx_parent_pc(after_call_instruction)
@ -59,7 +59,7 @@ global sys_call:
 // Creates a new sub context as if calling itself, but with the code of the
 // given account. In particular the storage remains the same.
 global sys_callcode:
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint

    // stack: kexit_info, gas, address, value, args_offset, args_size, ret_offset, ret_size
    %stack (kexit_info, gas, address, value, args_offset, args_size, ret_offset, ret_size) ->
@ -84,6 +84,8 @@ global sys_callcode:
          (new_ctx, args_offset, args_size, new_ctx, kexit_info, callgas, address, value, args_offset, args_size, ret_offset, ret_size)
    %copy_mem_to_calldata
    // stack: new_ctx, kexit_info, callgas, address, value, args_offset, args_size, ret_offset, ret_size
+    DUP5 %address %address %transfer_eth %jumpi(call_insufficient_balance)
+    // stack: new_ctx, kexit_info, callgas, address, value, args_offset, args_size, ret_offset, ret_size
    DUP3 %set_new_ctx_gas_limit
    %set_new_ctx_parent_pc(after_call_instruction)
    DUP9 DUP9 DUP4 DUP4 DUP8 // Duplicate address, new_ctx, kexit_info, ret_offset, and ret_size.
@ -109,7 +111,7 @@ global sys_callcode:
 // are CREATE, CREATE2, LOG0, LOG1, LOG2, LOG3, LOG4, SSTORE, SELFDESTRUCT and
 // CALL if the value sent is not 0.
 global sys_staticcall:
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint

    // stack: kexit_info, gas, address, args_offset, args_size, ret_offset, ret_size
    %stack (kexit_info, gas, address, args_offset, args_size, ret_offset, ret_size) ->
@ -160,7 +162,7 @@ global sys_staticcall:
 // given account. In particular the storage, the current sender and the current
 // value remain the same.
 global sys_delegatecall:
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint

    // stack: kexit_info, gas, address, args_offset, args_size, ret_offset, ret_size
    %stack (kexit_info, gas, address, args_offset, args_size, ret_offset, ret_size) ->
@ -209,6 +211,7 @@ global sys_delegatecall:
 global after_call_instruction:
    // stack: success, leftover_gas, new_ctx, kexit_info, ret_offset, ret_size
    DUP1 ISZERO %jumpi(after_call_instruction_failed)
+    %pop_checkpoint
 after_call_instruction_contd:
    SWAP3
    // stack: kexit_info, leftover_gas, new_ctx, success, ret_offset, ret_size
@ -222,9 +225,15 @@ after_call_instruction_contd:

 after_call_instruction_failed:
    // stack: success, leftover_gas, new_ctx, kexit_info, ret_offset, ret_size
-    %mload_context_metadata(@CTX_METADATA_CHECKPOINT) %revert_checkpoint
+    %revert_checkpoint
    %jump(after_call_instruction_contd)

+call_insufficient_balance:
+    %stack (new_ctx, kexit_info, callgas, address, value, args_offset, args_size, ret_offset, ret_size) ->
+        (callgas, kexit_info, 0)
+    %shl_const(192) SWAP1 SUB
+    EXIT_KERNEL
+
 // Set @CTX_METADATA_STATIC to 1. Note that there is no corresponding set_static_false routine
 // because it will already be 0 by default.
 %macro set_static_true
@ -313,7 +322,7 @@ after_call_instruction_failed:
    // Switch to the new context and go to usermode with PC=0.
    DUP1 // new_ctx
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    PUSH 0 // jump dest
    EXIT_KERNEL
    // (Old context) stack: new_ctx
--- a/evm/src/cpu/kernel/asm/core/precompiles/blake2_f.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/blake2_f.asm
@ -4,7 +4,7 @@ global precompile_blake2_f:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/bn_add.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/bn_add.asm
@ -4,7 +4,7 @@ global precompile_bn_add:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/bn_mul.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/bn_mul.asm
@ -4,7 +4,7 @@ global precompile_bn_mul:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/ecrec.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/ecrec.asm
@ -4,7 +4,7 @@ global precompile_ecrec:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/expmod.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/expmod.asm
@ -133,7 +133,7 @@ global precompile_expmod:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/id.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/id.asm
@ -4,7 +4,7 @@ global precompile_id:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/rip160.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/rip160.asm
@ -4,7 +4,7 @@ global precompile_rip160:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/sha256.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/sha256.asm
@ -4,7 +4,7 @@ global precompile_sha256:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/precompiles/snarkv.asm
+++ b/evm/src/cpu/kernel/asm/core/precompiles/snarkv.asm
@ -4,7 +4,7 @@ global precompile_snarkv:
    // stack: new_ctx, (old stack)
    DUP1
    SET_CONTEXT
-    %checkpoint %mstore_context_metadata(@CTX_METADATA_CHECKPOINT) // Checkpoint and store it in context metadata.
+    %checkpoint // Checkpoint
    // stack: (empty)
    PUSH 0x100000000 // = 2^32 (is_kernel = true)
    // stack: kexit_info
--- a/evm/src/cpu/kernel/asm/core/terminate.asm
+++ b/evm/src/cpu/kernel/asm/core/terminate.asm
@ -152,7 +152,7 @@ sys_revert_finish:
 // - state modification is attempted during a static call
 global fault_exception:
    // stack: (empty)
-    %mload_context_metadata(@CTX_METADATA_CHECKPOINT) %revert_checkpoint
+    %revert_checkpoint
    PUSH 0 // leftover_gas
    // Set the parent context's return data size to 0.
    %mstore_parent_context_metadata(@CTX_METADATA_RETURNDATA_SIZE, 0)
--- a/evm/src/cpu/kernel/asm/journal/account_destroyed.asm
+++ b/evm/src/cpu/kernel/asm/journal/account_destroyed.asm
@ -13,6 +13,20 @@ global revert_account_destroyed:
    %jump(remove_selfdestruct_list)
 revert_account_destroyed_contd:
    // stack: address, target, prev_balance, retdest
-    SWAP1 %transfer_eth %jumpi(panic)
+    SWAP1
+    // Remove `prev_balance` from `target`'s balance.
+    // stack: target, address, prev_balance, retdest
+    %mpt_read_state_trie
+    %add_const(1)
+    // stack: target_balance_ptr, address, prev_balance, retdest
+    DUP3
+    DUP2 %mload_trie_data
+    // stack: target_balance, prev_balance, target_balance_ptr, address, prev_balance, retdest
+    SUB SWAP1 %mstore_trie_data
+    // Set `address`'s balance to `prev_balance`.
+    // stack: address, prev_balance, retdest
+    %mpt_read_state_trie
+    %add_const(1)
+    %mstore_trie_data
    JUMP

--- a/evm/src/cpu/kernel/asm/journal/account_touched.asm
+++ b/evm/src/cpu/kernel/asm/journal/account_touched.asm
@ -4,7 +4,9 @@
    %journal_add_1(@JOURNAL_ENTRY_ACCOUNT_TOUCHED)
 %endmacro

-// Note: We don't need to remove touched addresses. In fact doing so leads to bugs because of the way we load accounts in the MPT.
 global revert_account_touched:
    // stack: entry_type, ptr, retdest
-    %pop2 JUMP
+    POP
+    %journal_load_1
+    // stack: address, retdest
+    %jump(remove_touched_addresses)
--- a/evm/src/cpu/kernel/asm/journal/balance_transfer.asm
+++ b/evm/src/cpu/kernel/asm/journal/balance_transfer.asm
@ -1,7 +1,15 @@
 // struct BalanceTransfer { from, to, balance }

 %macro journal_add_balance_transfer
+    // stack: from, to, balance
+    DUP3 ISZERO %jumpi(%%zero)
    %journal_add_3(@JOURNAL_ENTRY_BALANCE_TRANSFER)
+    %jump(%%after)
+%%zero:
+    // stack: from, to, balance
+    %pop3
+%%after:
+    // stack: (empty)
 %endmacro

 global revert_balance_transfer:
--- a/evm/src/cpu/kernel/asm/journal/journal.asm
+++ b/evm/src/cpu/kernel/asm/journal/journal.asm
@ -186,7 +186,21 @@
    // stack: journal_size, current_checkpoint
    DUP2 %mstore_kernel(@SEGMENT_JOURNAL_CHECKPOINTS)
    // stack: current_checkpoint
-    DUP1 %increment
-    %mstore_global_metadata(@GLOBAL_METADATA_CURRENT_CHECKPOINT)
+    %mload_context_metadata(@CTX_METADATA_CHECKPOINTS_LEN)
+    // stack: i, current_checkpoint
+    DUP2 DUP2 %mstore_current(@SEGMENT_CONTEXT_CHECKPOINTS)
+    // stack: i, current_checkpoint
+    %increment
+    %mstore_context_metadata(@CTX_METADATA_CHECKPOINTS_LEN)
    // stack: current_checkpoint
+    %increment
+    %mstore_global_metadata(@GLOBAL_METADATA_CURRENT_CHECKPOINT)
+    // stack: (empty)
+%endmacro
+
+%macro pop_checkpoint
+    %mload_context_metadata(@CTX_METADATA_CHECKPOINTS_LEN)
+    // stack: i
+    %decrement
+    %mstore_context_metadata(@CTX_METADATA_CHECKPOINTS_LEN)
 %endmacro
--- a/evm/src/cpu/kernel/asm/journal/revert.asm
+++ b/evm/src/cpu/kernel/asm/journal/revert.asm
@ -61,6 +61,9 @@ after_revert:


 global revert_checkpoint:
+    // stack: retdest
+    PUSH 1 %mload_context_metadata(@CTX_METADATA_CHECKPOINTS_LEN) SUB
+    %mload_current(@SEGMENT_CONTEXT_CHECKPOINTS)
    // stack: target_checkpoint, retdest
    %current_checkpoint
    // stack: current_checkpoint, target_checkpoint, retdest
@ -74,10 +77,11 @@ revert_checkpoint_done:
    // stack: current_checkpoint, target_checkpoint, retdest
    POP
    %mstore_global_metadata(@GLOBAL_METADATA_CURRENT_CHECKPOINT)
+    %pop_checkpoint
    JUMP

 %macro revert_checkpoint
-    %stack (target_checkpoint) -> (target_checkpoint, %%after)
+    PUSH %%after
    %jump(revert_checkpoint)
 %%after:
    // stack: (empty)
--- a/evm/src/cpu/kernel/constants/context_metadata.rs
+++ b/evm/src/cpu/kernel/constants/context_metadata.rs
@ -28,7 +28,7 @@ pub(crate) enum ContextMetadata {
    StackSize = 11,
    /// The gas limit for this call (not the entire transaction).
    GasLimit = 12,
-    Checkpoint = 13,
+    ContextCheckpointsLen = 13,
 }

 impl ContextMetadata {
@ -49,7 +49,7 @@ impl ContextMetadata {
            Self::MemWords,
            Self::StackSize,
            Self::GasLimit,
-            Self::Checkpoint,
+            Self::ContextCheckpointsLen,
        ]
    }

@ -69,7 +69,7 @@ impl ContextMetadata {
            ContextMetadata::MemWords => "CTX_METADATA_MEM_WORDS",
            ContextMetadata::StackSize => "CTX_METADATA_STACK_SIZE",
            ContextMetadata::GasLimit => "CTX_METADATA_GAS_LIMIT",
-            ContextMetadata::Checkpoint => "CTX_METADATA_CHECKPOINT",
+            ContextMetadata::ContextCheckpointsLen => "CTX_METADATA_CHECKPOINTS_LEN",
        }
    }
 }
--- a/evm/src/memory/segments.rs
+++ b/evm/src/memory/segments.rs
@ -56,10 +56,12 @@ pub enum Segment {
    JournalCheckpoints = 28,
    /// List of addresses that have been touched in the current transaction.
    TouchedAddresses = 29,
+    /// List of checkpoints for the current context. Length in `ContextMetadata`.
+    ContextCheckpoints = 30,
 }

 impl Segment {
-    pub(crate) const COUNT: usize = 30;
+    pub(crate) const COUNT: usize = 31;

    pub(crate) fn all() -> [Self; Self::COUNT] {
        [
@ -93,6 +95,7 @@ impl Segment {
            Self::JournalData,
            Self::JournalCheckpoints,
            Self::TouchedAddresses,
+            Self::ContextCheckpoints,
        ]
    }

@ -129,6 +132,7 @@ impl Segment {
            Segment::JournalData => "SEGMENT_JOURNAL_DATA",
            Segment::JournalCheckpoints => "SEGMENT_JOURNAL_CHECKPOINTS",
            Segment::TouchedAddresses => "SEGMENT_TOUCHED_ADDRESSES",
+            Segment::ContextCheckpoints => "SEGMENT_CONTEXT_CHECKPOINTS",
        }
    }

@ -165,6 +169,7 @@ impl Segment {
            Segment::JournalData => 256,
            Segment::JournalCheckpoints => 256,
            Segment::TouchedAddresses => 256,
+            Segment::ContextCheckpoints => 256,
        }
    }
 }