From 0cd4dbb288cc2207b27bca9ef1cb911850dfb0b9 Mon Sep 17 00:00:00 2001 From: Valentin Chaboche Date: Mon, 28 Oct 2024 13:52:19 +0100 Subject: [PATCH 1/3] EVM/Node: refactor check_address_boundaries The goal is to retrieve the reached boundary, we need to dinstinguish between max addresses from max transactions per address. --- etherlink/bin_node/lib_dev/tx_pool.ml | 61 ++++++++++++++------------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/etherlink/bin_node/lib_dev/tx_pool.ml b/etherlink/bin_node/lib_dev/tx_pool.ml index 5a67732d639d..82725fe46b1a 100644 --- a/etherlink/bin_node/lib_dev/tx_pool.ml +++ b/etherlink/bin_node/lib_dev/tx_pool.ml @@ -372,29 +372,22 @@ let tx_data_size_limit_reached ~max_number_of_chunks ~tx_data = let check_address_boundaries ~pool ~address ~tx_pool_addr_limit ~tx_pool_tx_per_addr_limit = let open Lwt_result_syntax in - let boundaries_are_not_reached = return (false, "") in match Pool.Pkey_map.find address pool.Pool.transactions with | None -> if Pool.Pkey_map.cardinal pool.Pool.transactions < tx_pool_addr_limit then - boundaries_are_not_reached + return_unit else let*! () = Tx_pool_events.users_threshold_reached () in - return - ( true, - "The transaction pool has reached its maximum threshold for user \ - transactions. Transaction is rejected." ) + fail `MaxUsers | Some txs -> if Pool.Nonce_map.cardinal txs < tx_pool_tx_per_addr_limit then - boundaries_are_not_reached + return_unit else let*! () = Tx_pool_events.txs_per_user_threshold_reached ~address:(Ethereum_types.Address.to_string address) in - return - ( true, - "Limit of transaction for a user was reached. Transaction is \ - rejected." ) + fail `MaxPerUser let insert_valid_transaction state tx_raw (transaction_object : Ethereum_types.transaction_object) = @@ -417,30 +410,40 @@ let insert_valid_transaction state tx_raw let*! () = Tx_pool_events.tx_data_size_limit_reached () in return @@ Error "Transaction data exceeded the allowed size." else - let* address_boundaries_are_reached, error_msg = + let*! res_boundaries = check_address_boundaries ~pool ~address:transaction_object.from ~tx_pool_addr_limit ~tx_pool_tx_per_addr_limit in - if address_boundaries_are_reached then return @@ Error error_msg - else - (* Add the transaction to the pool *) - (* Compute the hash *) - let hash = Ethereum_types.hash_raw_tx tx_raw in - (* This is a temporary fix until the hash computation is fixed on the - kernel side. *) - let transaction_object = {transaction_object with hash} in - let*? pool = - Pool.add pool transaction_object.from tx_raw transaction_object - in - let*! () = - Tx_pool_events.add_transaction - ~transaction:(Ethereum_types.hash_to_string hash) - in - state.pool <- pool ; - return (Ok hash) + match res_boundaries with + | Error `MaxUsers -> + return + (Error + "The transaction pool has reached its maximum threshold for user \ + transactions. Transaction is rejected.") + | Error `MaxPerUser -> + return + (Error + "Limit of transaction for a user was reached. Transaction is \ + rejected.") + | Ok () -> + (* Add the transaction to the pool *) + (* Compute the hash *) + let hash = Ethereum_types.hash_raw_tx tx_raw in + (* This is a temporary fix until the hash computation is fixed on the + kernel side. *) + let transaction_object = {transaction_object with hash} in + let*? pool = + Pool.add pool transaction_object.from tx_raw transaction_object + in + let*! () = + Tx_pool_events.add_transaction + ~transaction:(Ethereum_types.hash_to_string hash) + in + state.pool <- pool ; + return (Ok hash) let on_normal_transaction state transaction_object tx_raw = insert_valid_transaction state tx_raw transaction_object -- GitLab From 9d5cd6953cfca1c09f0d37ba84119576b9098b77 Mon Sep 17 00:00:00 2001 From: Valentin Chaboche Date: Mon, 28 Oct 2024 15:11:29 +0100 Subject: [PATCH 2/3] EVM/Node: allow to replace transactions even when limits are reached --- etherlink/CHANGES_NODE.md | 6 + etherlink/bin_node/lib_dev/tx_pool.ml | 168 ++++++++++++++++++-------- 2 files changed, 122 insertions(+), 52 deletions(-) diff --git a/etherlink/CHANGES_NODE.md b/etherlink/CHANGES_NODE.md index 224cb850a669..34ff363ed064 100644 --- a/etherlink/CHANGES_NODE.md +++ b/etherlink/CHANGES_NODE.md @@ -17,6 +17,12 @@ - Fixes the `callTracer`’s output containing invalid UTF8 string on reverted calls. (!15421) +- The transaction pool has a limit of transactions per user, if the + limit is reached, the submitted must replace an existing transaction, that is + either a transaction with the same nonce, or replace the "largest" nonce + found in the transaction pool. It fixes the issue where the limit is reached + and the user cannot unlock the situation by itself as the node refuses + all transactions. (!15465) ### Internals diff --git a/etherlink/bin_node/lib_dev/tx_pool.ml b/etherlink/bin_node/lib_dev/tx_pool.ml index 82725fe46b1a..c9be2bf15c51 100644 --- a/etherlink/bin_node/lib_dev/tx_pool.ml +++ b/etherlink/bin_node/lib_dev/tx_pool.ml @@ -81,45 +81,82 @@ module Pool = struct (Ethereum_types.AddressMap.empty, Ethereum_types.AddressMap.empty) (** Add a transaction to the pool. *) - let add {transactions; global_index} pkey raw_tx + let add ~must_replace {transactions; global_index} pkey raw_tx (transaction_object : Ethereum_types.transaction_object) = - let open Result_syntax in let (Qty nonce) = transaction_object.nonce in let (Qty gas_price) = transaction_object.gasPrice in let (Qty gas_limit) = transaction_object.gas in let inclusion_timestamp = Misc.now () in + (* Add the transaction to the user's transaction map *) - let transactions = - let transaction = - { - index = global_index; - raw_tx; - gas_price; - gas_limit; - inclusion_timestamp; - transaction_object; - } + let transaction = + { + index = global_index; + raw_tx; + gas_price; + gas_limit; + inclusion_timestamp; + transaction_object; + } + in + let add_transaction transactions = + let replaced = ref false in + let transactions = + Pkey_map.update + pkey + (function + | None -> + (* User has no transactions in the pool *) + Some (Nonce_map.singleton nonce transaction) + | Some user_transactions -> + Some + (Nonce_map.update + nonce + (function + | None -> Some transaction + | Some user_transaction -> + if gas_price > user_transaction.gas_price then ( + replaced := true ; + Some transaction) + else Some user_transaction) + user_transactions)) + transactions in - Pkey_map.update - pkey - (function - | None -> - (* User has no transactions in the pool *) - Some (Nonce_map.singleton nonce transaction) - | Some user_transactions -> - Some - (Nonce_map.update - nonce - (function - | None -> Some transaction - | Some user_transaction -> - if gas_price > user_transaction.gas_price then - Some transaction - else Some user_transaction) - user_transactions)) - transactions + (transactions, !replaced) in - return {transactions; global_index = Int64.(add global_index one)} + match must_replace with + | `Replace_existing -> + let transactions, replaced = add_transaction transactions in + if replaced then + Ok {transactions; global_index = Int64.(add global_index one)} + else + Error + "Limit of transactions for a user was reached. Transaction is \ + rejected as it did not replace an existing one." + | `No -> + let transactions, _replaced = add_transaction transactions in + Ok {transactions; global_index = Int64.(add global_index one)} + | `Replace_shift max_nonce -> + let transactions = + Pkey_map.update + pkey + (function + | None -> + (* The list of user transactions cannot be empty, + otherwise we wouldn't need to replace anything. *) + assert false + | Some user_transactions -> + (* Shift by removing the maximum nonce. *) + let user_transactions = + Nonce_map.remove max_nonce user_transactions + in + (* Add the new nonce. *) + Some (Nonce_map.add nonce transaction user_transactions)) + transactions + in + Ok {transactions; global_index = Int64.(add global_index one)} + + (* return {transactions; global_index = Int64.(add global_index one)} *) (** Returns all the addresses of the pool *) let addresses {transactions; _} = @@ -387,7 +424,7 @@ let check_address_boundaries ~pool ~address ~tx_pool_addr_limit Tx_pool_events.txs_per_user_threshold_reached ~address:(Ethereum_types.Address.to_string address) in - fail `MaxPerUser + fail (`MaxPerUser txs) let insert_valid_transaction state tx_raw (transaction_object : Ethereum_types.transaction_object) = @@ -410,6 +447,30 @@ let insert_valid_transaction state tx_raw let*! () = Tx_pool_events.tx_data_size_limit_reached () in return @@ Error "Transaction data exceeded the allowed size." else + let add_transaction ~must_replace = + (* Add the transaction to the pool *) + (* Compute the hash *) + let hash = Ethereum_types.hash_raw_tx tx_raw in + (* This is a temporary fix until the hash computation is fixed on the + kernel side. *) + let transaction_object = {transaction_object with hash} in + match + Pool.add + ~must_replace + pool + transaction_object.from + tx_raw + transaction_object + with + | Ok pool -> + let*! () = + Tx_pool_events.add_transaction + ~transaction:(Ethereum_types.hash_to_string hash) + in + state.pool <- pool ; + return (Ok hash) + | Error msg -> return (Error msg) + in let*! res_boundaries = check_address_boundaries ~pool @@ -423,27 +484,30 @@ let insert_valid_transaction state tx_raw (Error "The transaction pool has reached its maximum threshold for user \ transactions. Transaction is rejected.") - | Error `MaxPerUser -> - return - (Error - "Limit of transaction for a user was reached. Transaction is \ - rejected.") - | Ok () -> - (* Add the transaction to the pool *) - (* Compute the hash *) - let hash = Ethereum_types.hash_raw_tx tx_raw in - (* This is a temporary fix until the hash computation is fixed on the - kernel side. *) - let transaction_object = {transaction_object with hash} in - let*? pool = - Pool.add pool transaction_object.from tx_raw transaction_object - in - let*! () = - Tx_pool_events.add_transaction - ~transaction:(Ethereum_types.hash_to_string hash) + | Error (`MaxPerUser transactions) -> + let (Qty nonce) = transaction_object.nonce in + let max_nonce, nonce_exists = + Pool.Nonce_map.fold + (fun nonce' _tx (max_nonce, nonce_exists) -> + (Z.max max_nonce nonce', nonce_exists || nonce = nonce')) + transactions + (Z.zero, false) in - state.pool <- pool ; - return (Ok hash) + if nonce_exists then + (* It must replace an existing one, otherwise it'll be above + the limit. *) + add_transaction ~must_replace:`Replace_existing + else if nonce < max_nonce then + (* If the nonce is smaller than the mmax nonce, we must shift the + list and drop one transaction. *) + add_transaction ~must_replace:(`Replace_shift max_nonce) + else + (* Otherwise we have just reached the limit of transactions. *) + return + (Error + "Limit of transaction for a user was reached. Transaction is \ + rejected.") + | Ok () -> add_transaction ~must_replace:`No let on_normal_transaction state transaction_object tx_raw = insert_valid_transaction state tx_raw transaction_object -- GitLab From 7791a5e890e2413b5135c6e64a89d2998e55a3cc Mon Sep 17 00:00:00 2001 From: Valentin Chaboche Date: Mon, 28 Oct 2024 15:57:24 +0100 Subject: [PATCH 3/3] EVM/Tezt: test replace transactions --- etherlink/tezt/lib/helpers.ml | 7 +- etherlink/tezt/lib/helpers.mli | 1 + etherlink/tezt/tests/evm_sequencer.ml | 107 ++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 3 deletions(-) diff --git a/etherlink/tezt/lib/helpers.ml b/etherlink/tezt/lib/helpers.ml index d14f1a6f4392..18a0a3d70a42 100644 --- a/etherlink/tezt/lib/helpers.ml +++ b/etherlink/tezt/lib/helpers.ml @@ -395,8 +395,9 @@ let find_and_execute_withdrawal ?(outbox_lookup_depth = 10) ~withdrawal_level in return withdrawal_level -let init_sequencer_sandbox ?set_account_code ?da_fee_per_byte - ?minimum_base_fee_per_gas ?patch_config ?(kernel = Constant.WASM.evm_kernel) +let init_sequencer_sandbox ?tx_pool_tx_per_addr_limit ?set_account_code + ?da_fee_per_byte ?minimum_base_fee_per_gas ?patch_config + ?(kernel = Constant.WASM.evm_kernel) ?(bootstrap_accounts = List.map (fun account -> account.Eth_account.address) @@ -433,7 +434,7 @@ let init_sequencer_sandbox ?set_account_code ?da_fee_per_byte wallet_dir = Some wallet_dir; tx_pool_timeout_limit = None; tx_pool_addr_limit = None; - tx_pool_tx_per_addr_limit = None; + tx_pool_tx_per_addr_limit; } in Evm_node.init ?patch_config ~mode:sequencer_mode Uri.(empty |> to_string) diff --git a/etherlink/tezt/lib/helpers.mli b/etherlink/tezt/lib/helpers.mli index f266e7b2eab4..5eca76f4a85e 100644 --- a/etherlink/tezt/lib/helpers.mli +++ b/etherlink/tezt/lib/helpers.mli @@ -220,6 +220,7 @@ val find_and_execute_withdrawal : (** Runs a sequencer in mode sandbox, with no connection needed to a rollup node. *) val init_sequencer_sandbox : + ?tx_pool_tx_per_addr_limit:int -> ?set_account_code:(string * string) list -> ?da_fee_per_byte:Wei.t -> ?minimum_base_fee_per_gas:Wei.t -> diff --git a/etherlink/tezt/tests/evm_sequencer.ml b/etherlink/tezt/tests/evm_sequencer.ml index 3d572a79d60d..8ece6e4e5b89 100644 --- a/etherlink/tezt/tests/evm_sequencer.ml +++ b/etherlink/tezt/tests/evm_sequencer.ml @@ -204,6 +204,33 @@ let default_dal_registration = let ci_enabled_dal_registration = Register_both {extra_tags_with = []; extra_tags_without = []} +let register_sandbox ?tx_pool_tx_per_addr_limit ~title ?set_account_code + ?da_fee_per_byte ?minimum_base_fee_per_gas ~tags ?patch_config body = + Test.register + ~__FILE__ + ~title + ~tags + ~uses_admin_client:false + ~uses_client:false + ~uses_node:false + ~uses: + [ + Constant.octez_evm_node; + Constant.WASM.evm_kernel; + Constant.smart_rollup_installer; + ] + @@ fun () -> + let* sequencer = + Helpers.init_sequencer_sandbox + ?tx_pool_tx_per_addr_limit + ?set_account_code + ?da_fee_per_byte + ?minimum_base_fee_per_gas + ?patch_config + () + in + body sequencer + (* Register all variants of a test. *) let register_all ?block_storage_sqlite3 ?sequencer_rpc_port ?sequencer_private_rpc_port ?genesis_timestamp ?time_between_blocks @@ -6691,6 +6718,85 @@ let test_tx_pool_replacing_transactions = ~error_msg:"Second transaction should have replaced the previous one" ; unit +let test_tx_pool_replacing_transactions_on_limit () = + register_sandbox + ~tags:["evm"; "tx_pool"] + ~title:"Transactions can be replaced even when limit is reached" + ~tx_pool_tx_per_addr_limit:3 + @@ fun sequencer -> + let sender = Eth_account.bootstrap_accounts.(0) in + let* gas_price = Rpc.get_gas_price sequencer in + let gas_price = Int32.to_int gas_price in + let craft_tx ?(gas = 21_000) ?(gas_price = gas_price) nonce = + Cast.craft_tx + ~source_private_key:sender.private_key + ~chain_id:1337 + ~nonce + ~gas_price + ~gas + ~value:(Wei.of_eth_int 10) + ~address:"0x0000000000000000000000000000000000000000" + () + in + let txpool_content () = + let*@ pending, queued = Rpc.txpool_content sequencer in + let sender = String.lowercase_ascii sender.address in + let pending = + List.find_map + (fun Rpc.{address; transactions} -> + if String.lowercase_ascii address = sender then + Some (List.map fst transactions |> List.sort compare) + else None) + pending + in + let queued = + List.find_map + (fun Rpc.{address; transactions} -> + if String.lowercase_ascii address = sender then + Some (List.map fst transactions |> List.sort compare) + else None) + queued + in + return @@ Option.value ~default:[] pending @ Option.value ~default:[] queued + in + (* Craft transactions. *) + let* tx_0 = craft_tx 0 in + let* tx_2 = craft_tx 2 in + let* tx_4 = craft_tx 4 in + let* tx_6 = craft_tx 6 in + let* tx_8 = craft_tx 8 in + (* Inject 3 transactions. *) + let*@ _tx_hash_2 = Rpc.send_raw_transaction ~raw_tx:tx_2 sequencer in + let*@ _tx_hash_4 = Rpc.send_raw_transaction ~raw_tx:tx_4 sequencer in + let*@ _tx_hash_6 = Rpc.send_raw_transaction ~raw_tx:tx_6 sequencer in + let* txpool = txpool_content () in + Check.((txpool = [2L; 4L; 6L]) (list int64)) + ~error_msg:"Expected a transaction pool with %R, got %L" ; + (* Injecting tx_8 will fail as the limit is 3 transactions per address. *) + let*@? err = Rpc.send_raw_transaction ~raw_tx:tx_8 sequencer in + Check.( + (err.message + = "Limit of transaction for a user was reached. Transaction is rejected.") + string) + ~error_msg:"Expected error %R got %L" ; + (* Sending tx_0 should replace tx_6 as the nonce is smaller. *) + let*@ _tx_hash_0 = Rpc.send_raw_transaction ~raw_tx:tx_0 sequencer in + let* txpool = txpool_content () in + Check.((txpool = [0L; 2L; 4L]) (list int64)) + ~error_msg:"Expected a transaction pool with %R, got %L" ; + (* Re-sending tx_0 is legal as long as it replace a transaction. *) + let* tx_0 = craft_tx ~gas:30_000 0 in + let*@? err = Rpc.send_raw_transaction ~raw_tx:tx_0 sequencer in + Check.( + (err.message + = "Limit of transactions for a user was reached. Transaction is rejected as \ + it did not replace an existing one.") + string) + ~error_msg:"Expected error %R got %L" ; + let* tx_0 = craft_tx ~gas_price:(gas_price * 2) 0 in + let*@ _new_tx_0 = Rpc.send_raw_transaction ~raw_tx:tx_0 sequencer in + unit + let test_da_fees_after_execution = register_all ~time_between_blocks:Nothing @@ -6873,6 +6979,7 @@ let () = test_outbox_size_limit_resilience ~slow:false protocols ; test_proxy_node_can_forward_to_evm_endpoint protocols ; test_tx_pool_replacing_transactions protocols ; + test_tx_pool_replacing_transactions_on_limit () ; test_da_fees_after_execution protocols ; test_trace_transaction_calltracer_failed_create protocols ; test_configuration_service [Protocol.Alpha] ; -- GitLab