From bde7fb5019036c2c449f31f43da7ea425f0a8a85 Mon Sep 17 00:00:00 2001 From: wborgeaud Date: Tue, 16 May 2023 10:47:38 +0200 Subject: [PATCH] Various fixes to checkpoint logic (#1039) * Stack of checkpoints * Fixes * Don't use macro in revert selfdestruct * All stZeroCallsTest, stNonZeroCallsTest, stZeroCallsRevert passing * Minor --- evm/src/cpu/kernel/asm/core/call.asm | 23 +++++++++++++------ .../kernel/asm/core/precompiles/blake2_f.asm | 2 +- .../kernel/asm/core/precompiles/bn_add.asm | 2 +- .../kernel/asm/core/precompiles/bn_mul.asm | 2 +- .../cpu/kernel/asm/core/precompiles/ecrec.asm | 2 +- .../kernel/asm/core/precompiles/expmod.asm | 2 +- .../cpu/kernel/asm/core/precompiles/id.asm | 2 +- .../kernel/asm/core/precompiles/rip160.asm | 2 +- .../kernel/asm/core/precompiles/sha256.asm | 2 +- .../kernel/asm/core/precompiles/snarkv.asm | 2 +- evm/src/cpu/kernel/asm/core/terminate.asm | 2 +- .../kernel/asm/journal/account_destroyed.asm | 16 ++++++++++++- .../kernel/asm/journal/account_touched.asm | 6 +++-- .../kernel/asm/journal/balance_transfer.asm | 8 +++++++ evm/src/cpu/kernel/asm/journal/journal.asm | 18 +++++++++++++-- evm/src/cpu/kernel/asm/journal/revert.asm | 6 ++++- .../cpu/kernel/constants/context_metadata.rs | 6 ++--- evm/src/memory/segments.rs | 7 +++++- 18 files changed, 83 insertions(+), 27 deletions(-) diff --git a/evm/src/cpu/kernel/asm/core/call.asm b/evm/src/cpu/kernel/asm/core/call.asm index 736ed27e..1acb715b 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/blake2_f.asm b/evm/src/cpu/kernel/asm/core/precompiles/blake2_f.asm index 1810d992..4d3e46b5 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/bn_add.asm b/evm/src/cpu/kernel/asm/core/precompiles/bn_add.asm index fc838916..2452b004 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/bn_mul.asm b/evm/src/cpu/kernel/asm/core/precompiles/bn_mul.asm index b772d1c4..24115988 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/ecrec.asm b/evm/src/cpu/kernel/asm/core/precompiles/ecrec.asm index 1f143068..67292e67 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/expmod.asm b/evm/src/cpu/kernel/asm/core/precompiles/expmod.asm index 66e65011..d48dd369 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/id.asm b/evm/src/cpu/kernel/asm/core/precompiles/id.asm index 5d68bd61..348decea 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/rip160.asm b/evm/src/cpu/kernel/asm/core/precompiles/rip160.asm index 8444088c..7991ab42 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/sha256.asm b/evm/src/cpu/kernel/asm/core/precompiles/sha256.asm index 61be27fc..8739ec84 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/precompiles/snarkv.asm b/evm/src/cpu/kernel/asm/core/precompiles/snarkv.asm index 7c6df1a4..dcb4e64d 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/core/terminate.asm b/evm/src/cpu/kernel/asm/core/terminate.asm index c77c31bf..b90b14c5 100644 --- 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) diff --git a/evm/src/cpu/kernel/asm/journal/account_destroyed.asm b/evm/src/cpu/kernel/asm/journal/account_destroyed.asm index 691331b4..3806a891 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/journal/account_touched.asm b/evm/src/cpu/kernel/asm/journal/account_touched.asm index d157464d..33f3b62b 100644 --- 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) diff --git a/evm/src/cpu/kernel/asm/journal/balance_transfer.asm b/evm/src/cpu/kernel/asm/journal/balance_transfer.asm index a6173daa..a9a58941 100644 --- 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: diff --git a/evm/src/cpu/kernel/asm/journal/journal.asm b/evm/src/cpu/kernel/asm/journal/journal.asm index 938894a1..a0e5502d 100644 --- 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 diff --git a/evm/src/cpu/kernel/asm/journal/revert.asm b/evm/src/cpu/kernel/asm/journal/revert.asm index f1f50f72..143f51ba 100644 --- 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) diff --git a/evm/src/cpu/kernel/constants/context_metadata.rs b/evm/src/cpu/kernel/constants/context_metadata.rs index ef4023c5..fd710eec 100644 --- 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", } } } diff --git a/evm/src/memory/segments.rs b/evm/src/memory/segments.rs index 04c6cf35..0e5e8d34 100644 --- 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, } } }