From 0e4e9fee758a387ca86a5b2f1658edf0d6c8390c Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Thu, 8 Jun 2023 16:26:36 +0200 Subject: [PATCH 1/9] Plugins & shell: expose Conflict_map instead of find_manager and fee_needed_to_replace_by_fee --- src/lib_shell/prevalidation.ml | 66 ++++----- src/lib_shell/shell_plugin.ml | 35 +++-- src/lib_shell/shell_plugin.mli | 56 ++++--- src/lib_shell/test/test_prevalidation.ml | 9 -- src/proto_016_PtMumbai/lib_plugin/mempool.ml | 137 ++++++++++++++---- src/proto_016_PtMumbai/lib_plugin/mempool.mli | 69 +++++---- .../test/test_fee_needed_to_replace_by_fee.ml | 8 +- src/proto_017_PtNairob/lib_plugin/mempool.ml | 137 ++++++++++++++---- src/proto_017_PtNairob/lib_plugin/mempool.mli | 69 +++++---- .../test/test_fee_needed_to_replace_by_fee.ml | 8 +- src/proto_alpha/lib_plugin/mempool.ml | 137 ++++++++++++++---- src/proto_alpha/lib_plugin/mempool.mli | 69 +++++---- .../test/test_fee_needed_to_replace_by_fee.ml | 8 +- 13 files changed, 554 insertions(+), 254 deletions(-) diff --git a/src/lib_shell/prevalidation.ml b/src/lib_shell/prevalidation.ml index f1e466788282..d70c8510877f 100644 --- a/src/lib_shell/prevalidation.ml +++ b/src/lib_shell/prevalidation.ml @@ -129,8 +129,6 @@ module MakeAbstract type operation = protocol_operation Shell_operation.operation - module Manager = Signature.Public_key_hash - type t = { validation_info : Proto.Mempool.validation_info; (** Static information needed by [Proto.Mempool.add_operation]. *) @@ -139,10 +137,11 @@ module MakeAbstract bounding_state : Bounding.state; (** Representation of currently valid operations used to enforce mempool bounds. *) - manager_map : protocol_operation Manager.Map.t; - (** Valid manager operations in the mempool, indexed by manager. *) filter_info : Filter.Mempool.filter_info; (** Static information needed by [Filter.Mempool.pre_filter]. *) + conflict_map : Filter.Mempool.Conflict_map.t; + (** State needed by + [Filter.Mempool.Conflict_map.fee_needed_to_replace_by_fee]. *) } let create_aux ?old_state chain_store head timestamp = @@ -166,8 +165,8 @@ module MakeAbstract | Some old_state -> Filter.Mempool.flush old_state.filter_info ~head in let bounding_state = Bounding.empty in - let manager_map = Manager.Map.empty in - return {validation_info; mempool; bounding_state; manager_map; filter_info} + let conflict_map = Filter.Mempool.Conflict_map.empty in + return {validation_info; mempool; bounding_state; filter_info; conflict_map} let create chain_store ~head ~timestamp = create_aux chain_store head timestamp @@ -278,37 +277,26 @@ module MakeAbstract in return (mempool, bounding_state, replacements) - (* Update the [manager_map] by removing the [replacements] then - adding [op] if it is a manager operation. Non-manager operations - are ignored. - - Note that it is important to remove before adding, otherwise we - risk removing the newly added operation if it has just replaced - an operation from the same manager. - - [mempool_before] is the protocol's mempool representation before - calling [Proto.Mempool.add_operation], so that it still contains - the replaced operations. Indeed, it is used to retrieve these - operations from their hash. *) - let update_manager_map manager_map mempool_before op replacements = - let manager_map = - (* No need to call [Proto.Mempool.operations] when the list is empty. *) - if List.is_empty replacements then manager_map + let update_conflict_map conflict_map ~mempool_before op replacements = + (* [mempool_before] is the protocol's mempool representation + **before calling [Proto.Mempool.add_operation]**, so that it + still contains the replaced operations. Indeed, it is used to + retrieve these operations from their hash. *) + let replacements = + if List.is_empty replacements then [] + (* No need to call [Proto.Mempool.operations] when the list is empty. *) else let ops = Proto.Mempool.operations mempool_before in - let remove_replacement mmap (oph, _error_classification) = - match Operation_hash.Map.find oph ops with - | None -> (* This case should never happen. *) mmap - | Some op -> ( - match Filter.Mempool.find_manager op with - | None -> (* Not a manager operation: ignore it. *) mmap - | Some manager -> Manager.Map.remove manager mmap) - in - List.fold_left remove_replacement manager_map replacements + List.filter_map + (fun (oph, (_ : error_classification)) -> + (* This should always return [Some _]. *) + Operation_hash.Map.find oph ops) + replacements in - match Filter.Mempool.find_manager op.protocol with - | None -> (* Not a manager operation: ignore it. *) manager_map - | Some manager -> Manager.Map.add manager op.protocol manager_map + Filter.Mempool.Conflict_map.update + conflict_map + ~new_operation:op.protocol + ~replacements let add_operation_result state (filter_config, bounding_config) op : add_result tzresult Lwt.t = @@ -333,10 +321,14 @@ module MakeAbstract in match res with | Ok (mempool, bounding_state, replacements) -> - let manager_map = - update_manager_map state.manager_map state.mempool op replacements + let conflict_map = + update_conflict_map + state.conflict_map + ~mempool_before:state.mempool + op + replacements in - let state = {state with mempool; bounding_state; manager_map} in + let state = {state with mempool; bounding_state; conflict_map} in return (state, valid_op, `Validated, replacements) | Error trace -> (* We convert any error from [check_conflict_and_bound] into an diff --git a/src/lib_shell/shell_plugin.ml b/src/lib_shell/shell_plugin.ml index 222aca77da7a..b99d423cc2c1 100644 --- a/src/lib_shell/shell_plugin.ml +++ b/src/lib_shell/shell_plugin.ml @@ -56,18 +56,25 @@ module type FILTER = sig val conflict_handler : config -> Proto.Mempool.conflict_handler - val find_manager : Proto.operation -> Signature.Public_key_hash.t option + module Conflict_map : sig + type t + + val empty : t + + val update : + t -> + new_operation:Proto.operation -> + replacements:Proto.operation list -> + t + + val fee_needed_to_replace_by_fee : + config -> candidate_op:Proto.operation -> conflict_map:t -> int64 option + end val fee_needed_to_overtake : op_to_overtake:Proto.operation -> candidate_op:Proto.operation -> int64 option - - val fee_needed_to_replace_by_fee : - config -> - op_to_replace:Proto.operation -> - candidate_op:Proto.operation -> - int64 option end end @@ -102,12 +109,18 @@ module No_filter (Proto : Registered_protocol.T) : `Replace else `Keep - let find_manager _ = None + module Conflict_map = struct + type t = unit - let fee_needed_to_overtake ~op_to_overtake:_ ~candidate_op:_ = None + let empty = () + + let update _t ~new_operation:_ ~replacements:_ = () - let fee_needed_to_replace_by_fee _config ~op_to_replace:_ ~candidate_op:_ = - None + let fee_needed_to_replace_by_fee _config ~candidate_op:_ ~conflict_map:_ = + None + end + + let fee_needed_to_overtake ~op_to_overtake:_ ~candidate_op:_ = None end end diff --git a/src/lib_shell/shell_plugin.mli b/src/lib_shell/shell_plugin.mli index aededb3e6649..bbe813cf3478 100644 --- a/src/lib_shell/shell_plugin.mli +++ b/src/lib_shell/shell_plugin.mli @@ -24,7 +24,10 @@ (* *) (*****************************************************************************) -(** Type of a protocol-specific mempool filter plugin. *) +(** Type of a protocol-specific mempool filter plugin. + + Implementations for such a plugin can be found in + [src/proto_xxx/lib_plugin/mempool.ml]. *) module type FILTER = sig module Proto : Registered_protocol.T @@ -88,9 +91,38 @@ module type FILTER = sig implementation of this function relies). *) val conflict_handler : config -> Proto.Mempool.conflict_handler - (** If the operation is a manager operation, return its source, - otherwise return [None]. *) - val find_manager : Proto.operation -> Signature.Public_key_hash.t option + (** The purpose of this module is to provide the + [fee_needed_to_replace_by_fee] function. For this function to + be correct, the caller must maintain the state of type [t] by + calling [update] on each successfully validated operation and + its induced replacements. *) + module Conflict_map : sig + (** Internal state needed by [fee_needed_to_replace_by_fee]. *) + type t + + (** Initial state. *) + val empty : t + + (** Removes all the [replacements] from the state then adds + [new_operation]. *) + val update : + t -> + new_operation:Proto.operation -> + replacements:Proto.operation list -> + t + + (** This function should be called when + [Proto.Mempool.add_operation] has returned [Unchanged]. This + means that the [candidate_op] has been rejected because there + was a conflict with an pre-existing operation and the + {!val-conflict_handler} has returned [`Keep]. This function + returns the minimal fee (in mutez) that [candidate_op] would + need so that the {!val-conflict_handler} would return + [`Replace] instead. If no such fee exists, then the function + returns [None]. *) + val fee_needed_to_replace_by_fee : + config -> candidate_op:Proto.operation -> conflict_map:t -> int64 option + end (** Compute the minimal fee (expressed in mutez) that [candidate_op] would need to have in order to be strictly greater than @@ -106,22 +138,6 @@ module type FILTER = sig op_to_overtake:Proto.operation -> candidate_op:Proto.operation -> int64 option - - (** Compute the minimal fee (expressed in mutez) that [candidate_op] - would need to have in order for the {!conflict_handler} to let it - replace [op_to_replace], when both operations are manager operations. - - Return [None] when at least one operation is not a manager operation. - - Also return [None] if both operations are manager operations but - there was an error while computing the needed fee. However, - note that this cannot happen when both manager operations have - been successfully validated by the protocol. *) - val fee_needed_to_replace_by_fee : - config -> - op_to_replace:Proto.operation -> - candidate_op:Proto.operation -> - int64 option end end diff --git a/src/lib_shell/test/test_prevalidation.ml b/src/lib_shell/test/test_prevalidation.ml index e298772dc2a0..9599709adfdb 100644 --- a/src/lib_shell/test/test_prevalidation.ml +++ b/src/lib_shell/test/test_prevalidation.ml @@ -31,15 +31,6 @@ Subject: Unit tests for {!Prevalidation.T} *) -(* Note: the logic of Prevalidation.t.manager_map is not tested here - because the mock protocol operations don't have a notion of - manager. However, it will serve to indicate the fees needed to - replace the previous operation in a manager conflict error, which - will be tested in tezt. *) -(* TODO: https://gitlab.com/tezos/tezos/-/issues/5197 - Update the comment above once the fees needed to replace are - implemented and tested. *) - let register_test ~title ~additional_tags = Test.register ~__FILE__ diff --git a/src/proto_016_PtMumbai/lib_plugin/mempool.ml b/src/proto_016_PtMumbai/lib_plugin/mempool.ml index 36117f7baec2..72b0b694290d 100644 --- a/src/proto_016_PtMumbai/lib_plugin/mempool.ml +++ b/src/proto_016_PtMumbai/lib_plugin/mempool.ml @@ -625,6 +625,53 @@ let conflict_handler config : Mempool.conflict_handler = else if Operation.compare existing_operation new_operation < 0 then `Replace else `Keep +let int64_ceil_of_q q = + let n = Q.to_int64 q in + if Q.(equal q (of_int64 n)) then n else Int64.succ n + +(* Compute the minimal fee (expressed in mutez) that [candidate_op] + would need to have in order for the {!conflict_handler} to let it + replace [op_to_replace], when both operations are manager + operations. + + As specified in {!conflict_handler}, this means that [candidate_op] + with the returned fee needs to have both its fee and its fee/gas + ratio exceed (or match) [op_to_replace]'s same metrics bumped by + the {!config}'s [replace_by_fee_factor]. + + Return [None] when at least one operation is not a manager + operation. + + Also return [None] if both operations are manager operations but + there was an error while computing the needed fee. However, note + that this cannot happen when both manager operations have been + successfully validated by the protocol. *) +let fee_needed_to_replace_by_fee config ~op_to_replace ~candidate_op = + if is_manager_operation candidate_op && is_manager_operation op_to_replace + then + (let open Result_syntax in + let* _fee, candidate_gas = compute_fee_and_gas_limit candidate_op in + let* old_fee, old_gas = compute_fee_and_gas_limit op_to_replace in + if Gas.Arith.(old_gas = zero || candidate_gas = zero) then + (* This should not happen when both operations are valid. *) + Result.return_none + else + let candidate_gas = gas_as_q candidate_gas in + let bumped_old_fee, bumped_old_ratio = + bumped_fee_and_ratio_as_q config old_fee old_gas + in + (* The new operation needs to exceed both the bumped fee and the + bumped ratio to make {!better_fees_and_ratio} return [true]. + (Having fee or ratio equal to its bumped counterpart is ok too, + hence the [ceil] in [int64_ceil_of_q].) *) + let fee_needed_for_fee = int64_ceil_of_q bumped_old_fee in + let fee_needed_for_ratio = + int64_ceil_of_q Q.(bumped_old_ratio * candidate_gas) + in + Result.return_some (max fee_needed_for_fee fee_needed_for_ratio)) + |> Option.of_result |> Option.join + else None + let find_manager {shell = _; protocol_data = Operation_data {contents; _}} = match contents with | Single (Manager_operation {source; _}) -> Some source @@ -637,6 +684,64 @@ let find_manager {shell = _; protocol_data = Operation_data {contents; _}} = | Failing_noop _ ) -> None +(* The purpose of this module is to offer a version of + [fee_needed_to_replace_by_fee] where the caller doesn't need to + provide the [op_to_replace]. Instead, it needs to call + [Conflict_map.update] every time a new operation is added to the + mempool. This setup prevents the mempool plugin from exposing the + notion of manager operations and their source. *) +module Conflict_map = struct + (* The state consists in a map of all validated manager operations, + indexed by their source. + + Note that the protocol already enforces that there is at most one + operation per source. + + The state only tracks manager operations because other kinds of + operations don't have fees that we might adjust to change the + outcome of the {!conflict_handler}, so + [fee_needed_to_replace_by_fee] will always return [None] when + they are involved anyway. *) + type t = packed_operation Signature.Public_key_hash.Map.t + + let empty = Signature.Public_key_hash.Map.empty + + (* Remove all the [replacements] from the state, then add + [new_operation]. Non-manager operations are ignored. + + It is important to remove before adding: otherwise, we would + remove the newly added operation when it has the same manager as + one of the replacements. *) + let update conflict_map ~new_operation ~replacements = + let conflict_map = + List.fold_left + (fun conflict_map op -> + match find_manager op with + | Some manager -> + Signature.Public_key_hash.Map.remove manager conflict_map + | None -> (* Non-manager operation: ignore it. *) conflict_map) + conflict_map + replacements + in + match find_manager new_operation with + | Some manager -> + Signature.Public_key_hash.Map.add manager new_operation conflict_map + | None -> (* Non-manager operation: ignore it. *) conflict_map + + let fee_needed_to_replace_by_fee config ~candidate_op ~conflict_map = + match find_manager candidate_op with + | None -> (* Non-manager operation. *) None + | Some manager -> ( + match Signature.Public_key_hash.Map.find manager conflict_map with + | None -> + (* This can only happen when the pre-existing conflicting + operation is a [Drain_delegate], which cannot be replaced by a + manager operation regardless of the latter's fee. *) + None + | Some op_to_replace -> + fee_needed_to_replace_by_fee config ~candidate_op ~op_to_replace) +end + let fee_needed_to_overtake ~op_to_overtake ~candidate_op = if is_manager_operation candidate_op && is_manager_operation op_to_overtake then @@ -661,36 +766,6 @@ let fee_needed_to_overtake ~op_to_overtake ~candidate_op = |> Option.of_result |> Option.join else None -let int64_ceil_of_q q = - let n = Q.to_int64 q in - if Q.(equal q (of_int64 n)) then n else Int64.succ n - -let fee_needed_to_replace_by_fee config ~op_to_replace ~candidate_op = - if is_manager_operation candidate_op && is_manager_operation op_to_replace - then - (let open Result_syntax in - let* _fee, candidate_gas = compute_fee_and_gas_limit candidate_op in - let* old_fee, old_gas = compute_fee_and_gas_limit op_to_replace in - if Gas.Arith.(old_gas = zero || candidate_gas = zero) then - (* This should not happen when both operations are valid. *) - Result.return_none - else - let candidate_gas = gas_as_q candidate_gas in - let bumped_old_fee, bumped_old_ratio = - bumped_fee_and_ratio_as_q config old_fee old_gas - in - (* The new operation needs to exceed both the bumped fee and the - bumped ratio to make {!better_fees_and_ratio} return [true]. - (Having fee or ratio equal to its bumped counterpart is ok too, - hence the [ceil] in [int64_ceil_of_q].) *) - let fee_needed_for_fee = int64_ceil_of_q bumped_old_fee in - let fee_needed_for_ratio = - int64_ceil_of_q Q.(bumped_old_ratio * candidate_gas) - in - Result.return_some (max fee_needed_for_fee fee_needed_for_ratio)) - |> Option.of_result |> Option.join - else None - module Internal_for_tests = struct let default_config_with_clock_drift clock_drift = {default_config with clock_drift} @@ -701,4 +776,6 @@ module Internal_for_tests = struct let get_clock_drift {clock_drift; _} = clock_drift let acceptable_op = acceptable_op + + let fee_needed_to_replace_by_fee = fee_needed_to_replace_by_fee end diff --git a/src/proto_016_PtMumbai/lib_plugin/mempool.mli b/src/proto_016_PtMumbai/lib_plugin/mempool.mli index 04c4f3118e2e..97d1cad6f8f6 100644 --- a/src/proto_016_PtMumbai/lib_plugin/mempool.mli +++ b/src/proto_016_PtMumbai/lib_plugin/mempool.mli @@ -103,10 +103,41 @@ val pre_filter : able to call {!Protocol.Alpha_context.Operation.compare}). *) val conflict_handler : config -> Protocol.Mempool.conflict_handler -(** If the operation is a manager operation, return its source, - otherwise return [None]. *) -val find_manager : - Protocol.Alpha_context.packed_operation -> Signature.Public_key_hash.t option +(** The purpose of this module is to provide the + [fee_needed_to_replace_by_fee] function. For this function to be + correct, the caller must maintain the state of type [t] by calling + [update] on each successfully validated operation and its induced + replacements. *) +module Conflict_map : sig + (** Internal state needed by [fee_needed_to_replace_by_fee]. *) + type t + + (** Initial state. *) + val empty : t + + (** Removes all the [replacements] from the state then adds + [new_operation]. *) + val update : + t -> + new_operation:Protocol.Alpha_context.packed_operation -> + replacements:Protocol.Alpha_context.packed_operation list -> + t + + (** This function should be called when + [Protocol.Mempool.add_operation] has returned [Unchanged]. This + means that the [candidate_op] has been rejected because there was + a conflict with an pre-existing operation and the + {!val-conflict_handler} has returned [`Keep]. When both + operations are manager operations, this function returns the + minimal fee (in mutez) that [candidate_op] would need in order to + win the conflict, i.e. make the {!val-conflict_handler} return + [`Replace] instead. Otherwise, it returns [None]. *) + val fee_needed_to_replace_by_fee : + config -> + candidate_op:Protocol.Alpha_context.packed_operation -> + conflict_map:t -> + int64 option +end (** Compute the minimal fee (expressed in mutez) that [candidate_op] would need to have in order to be strictly greater than [op_to_overtake] @@ -124,27 +155,6 @@ val fee_needed_to_overtake : candidate_op:Protocol.Alpha_context.packed_operation -> int64 option -(** Compute the minimal fee (expressed in mutez) that [candidate_op] - would need to have in order for the {!conflict_handler} to let it - replace [op_to_replace], when both operations are manager operations. - - As specified in {!conflict_handler}, this means that - [candidate_op] with the returned fee needs to have both its fee and - its fee/gas ratio exceed (or match) [op_to_replace]'s same metrics - bumped by the {!config}'s [replace_by_fee_factor]. - - Return [None] when at least one operation is not a manager operation. - - Also return [None] if both operations are manager operations but - there was an error while computing the needed fee. However, note - that this cannot happen when both manager operations have been - successfully validated by the protocol. *) -val fee_needed_to_replace_by_fee : - config -> - op_to_replace:Protocol.Alpha_context.packed_operation -> - candidate_op:Protocol.Alpha_context.packed_operation -> - int64 option - (** The following type, encoding, and default values are exported for [bin_sc_rollup_node/configuration.ml]. *) @@ -189,4 +199,13 @@ module Internal_for_tests : sig op_round:Round.t -> now_timestamp:Timestamp.time -> bool Environment.Error_monad.tzresult + + (** The main component of + {!Conflict_map.fee_needed_to_replace_by_fee}. See comment in the + ml file. *) + val fee_needed_to_replace_by_fee : + config -> + op_to_replace:Protocol.Alpha_context.packed_operation -> + candidate_op:Protocol.Alpha_context.packed_operation -> + int64 option end diff --git a/src/proto_016_PtMumbai/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml b/src/proto_016_PtMumbai/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml index 053889f2f6aa..7834a9cbc601 100644 --- a/src/proto_016_PtMumbai/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml +++ b/src/proto_016_PtMumbai/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml @@ -28,8 +28,8 @@ Component: Plugin.Mempool Invocation: dune exec src/proto_016_PtMumbai/lib_plugin/test/main.exe \ -- --file test_fee_needed_to_replace_by_fee.ml - Subject: Unit tests the Mempool.fee_needed_to_replace_by_fee - function of the plugin + Subject: Unit tests the fee_needed_to_replace_by_fee function + of the mempool plugin *) let register_test = @@ -53,7 +53,7 @@ let () = let test op_to_replace candidate_op = assert ( Option.is_none - (Plugin.Mempool.fee_needed_to_replace_by_fee + (Plugin.Mempool.Internal_for_tests.fee_needed_to_replace_by_fee Plugin.Mempool.default_config ~op_to_replace ~candidate_op)) @@ -85,7 +85,7 @@ let test_manager_ops config (op_to_replace, fee_r, gas_r) gas_c ; let fee_needed = WithExceptions.Option.get ~loc:__LOC__ - @@ Plugin.Mempool.fee_needed_to_replace_by_fee + @@ Plugin.Mempool.Internal_for_tests.fee_needed_to_replace_by_fee config ~op_to_replace:(snd op_to_replace) ~candidate_op:(snd candidate_op) diff --git a/src/proto_017_PtNairob/lib_plugin/mempool.ml b/src/proto_017_PtNairob/lib_plugin/mempool.ml index 36117f7baec2..72b0b694290d 100644 --- a/src/proto_017_PtNairob/lib_plugin/mempool.ml +++ b/src/proto_017_PtNairob/lib_plugin/mempool.ml @@ -625,6 +625,53 @@ let conflict_handler config : Mempool.conflict_handler = else if Operation.compare existing_operation new_operation < 0 then `Replace else `Keep +let int64_ceil_of_q q = + let n = Q.to_int64 q in + if Q.(equal q (of_int64 n)) then n else Int64.succ n + +(* Compute the minimal fee (expressed in mutez) that [candidate_op] + would need to have in order for the {!conflict_handler} to let it + replace [op_to_replace], when both operations are manager + operations. + + As specified in {!conflict_handler}, this means that [candidate_op] + with the returned fee needs to have both its fee and its fee/gas + ratio exceed (or match) [op_to_replace]'s same metrics bumped by + the {!config}'s [replace_by_fee_factor]. + + Return [None] when at least one operation is not a manager + operation. + + Also return [None] if both operations are manager operations but + there was an error while computing the needed fee. However, note + that this cannot happen when both manager operations have been + successfully validated by the protocol. *) +let fee_needed_to_replace_by_fee config ~op_to_replace ~candidate_op = + if is_manager_operation candidate_op && is_manager_operation op_to_replace + then + (let open Result_syntax in + let* _fee, candidate_gas = compute_fee_and_gas_limit candidate_op in + let* old_fee, old_gas = compute_fee_and_gas_limit op_to_replace in + if Gas.Arith.(old_gas = zero || candidate_gas = zero) then + (* This should not happen when both operations are valid. *) + Result.return_none + else + let candidate_gas = gas_as_q candidate_gas in + let bumped_old_fee, bumped_old_ratio = + bumped_fee_and_ratio_as_q config old_fee old_gas + in + (* The new operation needs to exceed both the bumped fee and the + bumped ratio to make {!better_fees_and_ratio} return [true]. + (Having fee or ratio equal to its bumped counterpart is ok too, + hence the [ceil] in [int64_ceil_of_q].) *) + let fee_needed_for_fee = int64_ceil_of_q bumped_old_fee in + let fee_needed_for_ratio = + int64_ceil_of_q Q.(bumped_old_ratio * candidate_gas) + in + Result.return_some (max fee_needed_for_fee fee_needed_for_ratio)) + |> Option.of_result |> Option.join + else None + let find_manager {shell = _; protocol_data = Operation_data {contents; _}} = match contents with | Single (Manager_operation {source; _}) -> Some source @@ -637,6 +684,64 @@ let find_manager {shell = _; protocol_data = Operation_data {contents; _}} = | Failing_noop _ ) -> None +(* The purpose of this module is to offer a version of + [fee_needed_to_replace_by_fee] where the caller doesn't need to + provide the [op_to_replace]. Instead, it needs to call + [Conflict_map.update] every time a new operation is added to the + mempool. This setup prevents the mempool plugin from exposing the + notion of manager operations and their source. *) +module Conflict_map = struct + (* The state consists in a map of all validated manager operations, + indexed by their source. + + Note that the protocol already enforces that there is at most one + operation per source. + + The state only tracks manager operations because other kinds of + operations don't have fees that we might adjust to change the + outcome of the {!conflict_handler}, so + [fee_needed_to_replace_by_fee] will always return [None] when + they are involved anyway. *) + type t = packed_operation Signature.Public_key_hash.Map.t + + let empty = Signature.Public_key_hash.Map.empty + + (* Remove all the [replacements] from the state, then add + [new_operation]. Non-manager operations are ignored. + + It is important to remove before adding: otherwise, we would + remove the newly added operation when it has the same manager as + one of the replacements. *) + let update conflict_map ~new_operation ~replacements = + let conflict_map = + List.fold_left + (fun conflict_map op -> + match find_manager op with + | Some manager -> + Signature.Public_key_hash.Map.remove manager conflict_map + | None -> (* Non-manager operation: ignore it. *) conflict_map) + conflict_map + replacements + in + match find_manager new_operation with + | Some manager -> + Signature.Public_key_hash.Map.add manager new_operation conflict_map + | None -> (* Non-manager operation: ignore it. *) conflict_map + + let fee_needed_to_replace_by_fee config ~candidate_op ~conflict_map = + match find_manager candidate_op with + | None -> (* Non-manager operation. *) None + | Some manager -> ( + match Signature.Public_key_hash.Map.find manager conflict_map with + | None -> + (* This can only happen when the pre-existing conflicting + operation is a [Drain_delegate], which cannot be replaced by a + manager operation regardless of the latter's fee. *) + None + | Some op_to_replace -> + fee_needed_to_replace_by_fee config ~candidate_op ~op_to_replace) +end + let fee_needed_to_overtake ~op_to_overtake ~candidate_op = if is_manager_operation candidate_op && is_manager_operation op_to_overtake then @@ -661,36 +766,6 @@ let fee_needed_to_overtake ~op_to_overtake ~candidate_op = |> Option.of_result |> Option.join else None -let int64_ceil_of_q q = - let n = Q.to_int64 q in - if Q.(equal q (of_int64 n)) then n else Int64.succ n - -let fee_needed_to_replace_by_fee config ~op_to_replace ~candidate_op = - if is_manager_operation candidate_op && is_manager_operation op_to_replace - then - (let open Result_syntax in - let* _fee, candidate_gas = compute_fee_and_gas_limit candidate_op in - let* old_fee, old_gas = compute_fee_and_gas_limit op_to_replace in - if Gas.Arith.(old_gas = zero || candidate_gas = zero) then - (* This should not happen when both operations are valid. *) - Result.return_none - else - let candidate_gas = gas_as_q candidate_gas in - let bumped_old_fee, bumped_old_ratio = - bumped_fee_and_ratio_as_q config old_fee old_gas - in - (* The new operation needs to exceed both the bumped fee and the - bumped ratio to make {!better_fees_and_ratio} return [true]. - (Having fee or ratio equal to its bumped counterpart is ok too, - hence the [ceil] in [int64_ceil_of_q].) *) - let fee_needed_for_fee = int64_ceil_of_q bumped_old_fee in - let fee_needed_for_ratio = - int64_ceil_of_q Q.(bumped_old_ratio * candidate_gas) - in - Result.return_some (max fee_needed_for_fee fee_needed_for_ratio)) - |> Option.of_result |> Option.join - else None - module Internal_for_tests = struct let default_config_with_clock_drift clock_drift = {default_config with clock_drift} @@ -701,4 +776,6 @@ module Internal_for_tests = struct let get_clock_drift {clock_drift; _} = clock_drift let acceptable_op = acceptable_op + + let fee_needed_to_replace_by_fee = fee_needed_to_replace_by_fee end diff --git a/src/proto_017_PtNairob/lib_plugin/mempool.mli b/src/proto_017_PtNairob/lib_plugin/mempool.mli index 04c4f3118e2e..97d1cad6f8f6 100644 --- a/src/proto_017_PtNairob/lib_plugin/mempool.mli +++ b/src/proto_017_PtNairob/lib_plugin/mempool.mli @@ -103,10 +103,41 @@ val pre_filter : able to call {!Protocol.Alpha_context.Operation.compare}). *) val conflict_handler : config -> Protocol.Mempool.conflict_handler -(** If the operation is a manager operation, return its source, - otherwise return [None]. *) -val find_manager : - Protocol.Alpha_context.packed_operation -> Signature.Public_key_hash.t option +(** The purpose of this module is to provide the + [fee_needed_to_replace_by_fee] function. For this function to be + correct, the caller must maintain the state of type [t] by calling + [update] on each successfully validated operation and its induced + replacements. *) +module Conflict_map : sig + (** Internal state needed by [fee_needed_to_replace_by_fee]. *) + type t + + (** Initial state. *) + val empty : t + + (** Removes all the [replacements] from the state then adds + [new_operation]. *) + val update : + t -> + new_operation:Protocol.Alpha_context.packed_operation -> + replacements:Protocol.Alpha_context.packed_operation list -> + t + + (** This function should be called when + [Protocol.Mempool.add_operation] has returned [Unchanged]. This + means that the [candidate_op] has been rejected because there was + a conflict with an pre-existing operation and the + {!val-conflict_handler} has returned [`Keep]. When both + operations are manager operations, this function returns the + minimal fee (in mutez) that [candidate_op] would need in order to + win the conflict, i.e. make the {!val-conflict_handler} return + [`Replace] instead. Otherwise, it returns [None]. *) + val fee_needed_to_replace_by_fee : + config -> + candidate_op:Protocol.Alpha_context.packed_operation -> + conflict_map:t -> + int64 option +end (** Compute the minimal fee (expressed in mutez) that [candidate_op] would need to have in order to be strictly greater than [op_to_overtake] @@ -124,27 +155,6 @@ val fee_needed_to_overtake : candidate_op:Protocol.Alpha_context.packed_operation -> int64 option -(** Compute the minimal fee (expressed in mutez) that [candidate_op] - would need to have in order for the {!conflict_handler} to let it - replace [op_to_replace], when both operations are manager operations. - - As specified in {!conflict_handler}, this means that - [candidate_op] with the returned fee needs to have both its fee and - its fee/gas ratio exceed (or match) [op_to_replace]'s same metrics - bumped by the {!config}'s [replace_by_fee_factor]. - - Return [None] when at least one operation is not a manager operation. - - Also return [None] if both operations are manager operations but - there was an error while computing the needed fee. However, note - that this cannot happen when both manager operations have been - successfully validated by the protocol. *) -val fee_needed_to_replace_by_fee : - config -> - op_to_replace:Protocol.Alpha_context.packed_operation -> - candidate_op:Protocol.Alpha_context.packed_operation -> - int64 option - (** The following type, encoding, and default values are exported for [bin_sc_rollup_node/configuration.ml]. *) @@ -189,4 +199,13 @@ module Internal_for_tests : sig op_round:Round.t -> now_timestamp:Timestamp.time -> bool Environment.Error_monad.tzresult + + (** The main component of + {!Conflict_map.fee_needed_to_replace_by_fee}. See comment in the + ml file. *) + val fee_needed_to_replace_by_fee : + config -> + op_to_replace:Protocol.Alpha_context.packed_operation -> + candidate_op:Protocol.Alpha_context.packed_operation -> + int64 option end diff --git a/src/proto_017_PtNairob/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml b/src/proto_017_PtNairob/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml index bf8be0987d22..f4604296f223 100644 --- a/src/proto_017_PtNairob/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml +++ b/src/proto_017_PtNairob/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml @@ -28,8 +28,8 @@ Component: Plugin.Mempool Invocation: dune exec src/proto_017_PtNairob/lib_plugin/test/main.exe \ -- --file test_fee_needed_to_replace_by_fee.ml - Subject: Unit tests the Mempool.fee_needed_to_replace_by_fee - function of the plugin + Subject: Unit tests the fee_needed_to_replace_by_fee function + of the mempool plugin *) let register_test = @@ -53,7 +53,7 @@ let () = let test op_to_replace candidate_op = assert ( Option.is_none - (Plugin.Mempool.fee_needed_to_replace_by_fee + (Plugin.Mempool.Internal_for_tests.fee_needed_to_replace_by_fee Plugin.Mempool.default_config ~op_to_replace ~candidate_op)) @@ -85,7 +85,7 @@ let test_manager_ops config (op_to_replace, fee_r, gas_r) gas_c ; let fee_needed = WithExceptions.Option.get ~loc:__LOC__ - @@ Plugin.Mempool.fee_needed_to_replace_by_fee + @@ Plugin.Mempool.Internal_for_tests.fee_needed_to_replace_by_fee config ~op_to_replace:(snd op_to_replace) ~candidate_op:(snd candidate_op) diff --git a/src/proto_alpha/lib_plugin/mempool.ml b/src/proto_alpha/lib_plugin/mempool.ml index e43a35970698..15077dd9dafc 100644 --- a/src/proto_alpha/lib_plugin/mempool.ml +++ b/src/proto_alpha/lib_plugin/mempool.ml @@ -627,6 +627,53 @@ let conflict_handler config : Mempool.conflict_handler = else if Operation.compare existing_operation new_operation < 0 then `Replace else `Keep +let int64_ceil_of_q q = + let n = Q.to_int64 q in + if Q.(equal q (of_int64 n)) then n else Int64.succ n + +(* Compute the minimal fee (expressed in mutez) that [candidate_op] + would need to have in order for the {!conflict_handler} to let it + replace [op_to_replace], when both operations are manager + operations. + + As specified in {!conflict_handler}, this means that [candidate_op] + with the returned fee needs to have both its fee and its fee/gas + ratio exceed (or match) [op_to_replace]'s same metrics bumped by + the {!config}'s [replace_by_fee_factor]. + + Return [None] when at least one operation is not a manager + operation. + + Also return [None] if both operations are manager operations but + there was an error while computing the needed fee. However, note + that this cannot happen when both manager operations have been + successfully validated by the protocol. *) +let fee_needed_to_replace_by_fee config ~op_to_replace ~candidate_op = + if is_manager_operation candidate_op && is_manager_operation op_to_replace + then + (let open Result_syntax in + let* _fee, candidate_gas = compute_fee_and_gas_limit candidate_op in + let* old_fee, old_gas = compute_fee_and_gas_limit op_to_replace in + if Gas.Arith.(old_gas = zero || candidate_gas = zero) then + (* This should not happen when both operations are valid. *) + Result.return_none + else + let candidate_gas = gas_as_q candidate_gas in + let bumped_old_fee, bumped_old_ratio = + bumped_fee_and_ratio_as_q config old_fee old_gas + in + (* The new operation needs to exceed both the bumped fee and the + bumped ratio to make {!better_fees_and_ratio} return [true]. + (Having fee or ratio equal to its bumped counterpart is ok too, + hence the [ceil] in [int64_ceil_of_q].) *) + let fee_needed_for_fee = int64_ceil_of_q bumped_old_fee in + let fee_needed_for_ratio = + int64_ceil_of_q Q.(bumped_old_ratio * candidate_gas) + in + Result.return_some (max fee_needed_for_fee fee_needed_for_ratio)) + |> Option.of_result |> Option.join + else None + let find_manager {shell = _; protocol_data = Operation_data {contents; _}} = match contents with | Single (Manager_operation {source; _}) -> Some source @@ -639,6 +686,64 @@ let find_manager {shell = _; protocol_data = Operation_data {contents; _}} = | Failing_noop _ ) -> None +(* The purpose of this module is to offer a version of + [fee_needed_to_replace_by_fee] where the caller doesn't need to + provide the [op_to_replace]. Instead, it needs to call + [Conflict_map.update] every time a new operation is added to the + mempool. This setup prevents the mempool plugin from exposing the + notion of manager operations and their source. *) +module Conflict_map = struct + (* The state consists in a map of all validated manager operations, + indexed by their source. + + Note that the protocol already enforces that there is at most one + operation per source. + + The state only tracks manager operations because other kinds of + operations don't have fees that we might adjust to change the + outcome of the {!conflict_handler}, so + [fee_needed_to_replace_by_fee] will always return [None] when + they are involved anyway. *) + type t = packed_operation Signature.Public_key_hash.Map.t + + let empty = Signature.Public_key_hash.Map.empty + + (* Remove all the [replacements] from the state, then add + [new_operation]. Non-manager operations are ignored. + + It is important to remove before adding: otherwise, we would + remove the newly added operation when it has the same manager as + one of the replacements. *) + let update conflict_map ~new_operation ~replacements = + let conflict_map = + List.fold_left + (fun conflict_map op -> + match find_manager op with + | Some manager -> + Signature.Public_key_hash.Map.remove manager conflict_map + | None -> (* Non-manager operation: ignore it. *) conflict_map) + conflict_map + replacements + in + match find_manager new_operation with + | Some manager -> + Signature.Public_key_hash.Map.add manager new_operation conflict_map + | None -> (* Non-manager operation: ignore it. *) conflict_map + + let fee_needed_to_replace_by_fee config ~candidate_op ~conflict_map = + match find_manager candidate_op with + | None -> (* Non-manager operation. *) None + | Some manager -> ( + match Signature.Public_key_hash.Map.find manager conflict_map with + | None -> + (* This can only happen when the pre-existing conflicting + operation is a [Drain_delegate], which cannot be replaced by a + manager operation regardless of the latter's fee. *) + None + | Some op_to_replace -> + fee_needed_to_replace_by_fee config ~candidate_op ~op_to_replace) +end + let fee_needed_to_overtake ~op_to_overtake ~candidate_op = if is_manager_operation candidate_op && is_manager_operation op_to_overtake then @@ -663,36 +768,6 @@ let fee_needed_to_overtake ~op_to_overtake ~candidate_op = |> Option.of_result |> Option.join else None -let int64_ceil_of_q q = - let n = Q.to_int64 q in - if Q.(equal q (of_int64 n)) then n else Int64.succ n - -let fee_needed_to_replace_by_fee config ~op_to_replace ~candidate_op = - if is_manager_operation candidate_op && is_manager_operation op_to_replace - then - (let open Result_syntax in - let* _fee, candidate_gas = compute_fee_and_gas_limit candidate_op in - let* old_fee, old_gas = compute_fee_and_gas_limit op_to_replace in - if Gas.Arith.(old_gas = zero || candidate_gas = zero) then - (* This should not happen when both operations are valid. *) - Result.return_none - else - let candidate_gas = gas_as_q candidate_gas in - let bumped_old_fee, bumped_old_ratio = - bumped_fee_and_ratio_as_q config old_fee old_gas - in - (* The new operation needs to exceed both the bumped fee and the - bumped ratio to make {!better_fees_and_ratio} return [true]. - (Having fee or ratio equal to its bumped counterpart is ok too, - hence the [ceil] in [int64_ceil_of_q].) *) - let fee_needed_for_fee = int64_ceil_of_q bumped_old_fee in - let fee_needed_for_ratio = - int64_ceil_of_q Q.(bumped_old_ratio * candidate_gas) - in - Result.return_some (max fee_needed_for_fee fee_needed_for_ratio)) - |> Option.of_result |> Option.join - else None - module Internal_for_tests = struct let default_config_with_clock_drift clock_drift = {default_config with clock_drift} @@ -703,4 +778,6 @@ module Internal_for_tests = struct let get_clock_drift {clock_drift; _} = clock_drift let acceptable_op = acceptable_op + + let fee_needed_to_replace_by_fee = fee_needed_to_replace_by_fee end diff --git a/src/proto_alpha/lib_plugin/mempool.mli b/src/proto_alpha/lib_plugin/mempool.mli index 04c4f3118e2e..97d1cad6f8f6 100644 --- a/src/proto_alpha/lib_plugin/mempool.mli +++ b/src/proto_alpha/lib_plugin/mempool.mli @@ -103,10 +103,41 @@ val pre_filter : able to call {!Protocol.Alpha_context.Operation.compare}). *) val conflict_handler : config -> Protocol.Mempool.conflict_handler -(** If the operation is a manager operation, return its source, - otherwise return [None]. *) -val find_manager : - Protocol.Alpha_context.packed_operation -> Signature.Public_key_hash.t option +(** The purpose of this module is to provide the + [fee_needed_to_replace_by_fee] function. For this function to be + correct, the caller must maintain the state of type [t] by calling + [update] on each successfully validated operation and its induced + replacements. *) +module Conflict_map : sig + (** Internal state needed by [fee_needed_to_replace_by_fee]. *) + type t + + (** Initial state. *) + val empty : t + + (** Removes all the [replacements] from the state then adds + [new_operation]. *) + val update : + t -> + new_operation:Protocol.Alpha_context.packed_operation -> + replacements:Protocol.Alpha_context.packed_operation list -> + t + + (** This function should be called when + [Protocol.Mempool.add_operation] has returned [Unchanged]. This + means that the [candidate_op] has been rejected because there was + a conflict with an pre-existing operation and the + {!val-conflict_handler} has returned [`Keep]. When both + operations are manager operations, this function returns the + minimal fee (in mutez) that [candidate_op] would need in order to + win the conflict, i.e. make the {!val-conflict_handler} return + [`Replace] instead. Otherwise, it returns [None]. *) + val fee_needed_to_replace_by_fee : + config -> + candidate_op:Protocol.Alpha_context.packed_operation -> + conflict_map:t -> + int64 option +end (** Compute the minimal fee (expressed in mutez) that [candidate_op] would need to have in order to be strictly greater than [op_to_overtake] @@ -124,27 +155,6 @@ val fee_needed_to_overtake : candidate_op:Protocol.Alpha_context.packed_operation -> int64 option -(** Compute the minimal fee (expressed in mutez) that [candidate_op] - would need to have in order for the {!conflict_handler} to let it - replace [op_to_replace], when both operations are manager operations. - - As specified in {!conflict_handler}, this means that - [candidate_op] with the returned fee needs to have both its fee and - its fee/gas ratio exceed (or match) [op_to_replace]'s same metrics - bumped by the {!config}'s [replace_by_fee_factor]. - - Return [None] when at least one operation is not a manager operation. - - Also return [None] if both operations are manager operations but - there was an error while computing the needed fee. However, note - that this cannot happen when both manager operations have been - successfully validated by the protocol. *) -val fee_needed_to_replace_by_fee : - config -> - op_to_replace:Protocol.Alpha_context.packed_operation -> - candidate_op:Protocol.Alpha_context.packed_operation -> - int64 option - (** The following type, encoding, and default values are exported for [bin_sc_rollup_node/configuration.ml]. *) @@ -189,4 +199,13 @@ module Internal_for_tests : sig op_round:Round.t -> now_timestamp:Timestamp.time -> bool Environment.Error_monad.tzresult + + (** The main component of + {!Conflict_map.fee_needed_to_replace_by_fee}. See comment in the + ml file. *) + val fee_needed_to_replace_by_fee : + config -> + op_to_replace:Protocol.Alpha_context.packed_operation -> + candidate_op:Protocol.Alpha_context.packed_operation -> + int64 option end diff --git a/src/proto_alpha/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml b/src/proto_alpha/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml index 8b58548d2da6..bca8f9d15b09 100644 --- a/src/proto_alpha/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml +++ b/src/proto_alpha/lib_plugin/test/test_fee_needed_to_replace_by_fee.ml @@ -28,8 +28,8 @@ Component: Plugin.Mempool Invocation: dune exec src/proto_alpha/lib_plugin/test/main.exe \ -- --file test_fee_needed_to_replace_by_fee.ml - Subject: Unit tests the Mempool.fee_needed_to_replace_by_fee - function of the plugin + Subject: Unit tests the fee_needed_to_replace_by_fee function + of the mempool plugin *) let register_test = @@ -53,7 +53,7 @@ let () = let test op_to_replace candidate_op = assert ( Option.is_none - (Plugin.Mempool.fee_needed_to_replace_by_fee + (Plugin.Mempool.Internal_for_tests.fee_needed_to_replace_by_fee Plugin.Mempool.default_config ~op_to_replace ~candidate_op)) @@ -85,7 +85,7 @@ let test_manager_ops config (op_to_replace, fee_r, gas_r) gas_c ; let fee_needed = WithExceptions.Option.get ~loc:__LOC__ - @@ Plugin.Mempool.fee_needed_to_replace_by_fee + @@ Plugin.Mempool.Internal_for_tests.fee_needed_to_replace_by_fee config ~op_to_replace:(snd op_to_replace) ~candidate_op:(snd candidate_op) -- GitLab From 79b437f98b5f64de3e116325e5f7b73a674371e1 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Wed, 7 Jun 2023 14:21:49 +0200 Subject: [PATCH 2/9] Shell_service: add fee_needed_in_mutez to the Operation_conflict error --- src/lib_shell/prevalidation.ml | 4 ++- src/lib_shell_services/validation_errors.ml | 36 +++++++++++++++----- src/lib_shell_services/validation_errors.mli | 5 ++- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/lib_shell/prevalidation.ml b/src/lib_shell/prevalidation.ml index d70c8510877f..ea00c1fedcf7 100644 --- a/src/lib_shell/prevalidation.ml +++ b/src/lib_shell/prevalidation.ml @@ -222,7 +222,9 @@ module MakeAbstract [Operation_replacement {old_hash = removed; new_hash = op.hash}] in return_some (removed, `Outdated trace) - | Unchanged -> error [Operation_conflict {new_hash = op.hash}] + | Unchanged -> + error + [Operation_conflict {new_hash = op.hash; needed_fee_in_mutez = None}] let bounding_add_operation bounding_state bounding_config op = Result.map_error diff --git a/src/lib_shell_services/validation_errors.ml b/src/lib_shell_services/validation_errors.ml index d1e8c8216959..f40dfc7b6fe7 100644 --- a/src/lib_shell_services/validation_errors.ml +++ b/src/lib_shell_services/validation_errors.ml @@ -29,7 +29,10 @@ type error += Parse_error type error += - | Operation_conflict of {new_hash : Operation_hash.t} + | Operation_conflict of { + new_hash : Operation_hash.t; + needed_fee_in_mutez : int64 option; + } | Operation_replacement of { old_hash : Operation_hash.t; new_hash : Operation_hash.t; @@ -75,17 +78,32 @@ let () = ~description: "The operation cannot be added because the mempool already contains a \ conflicting operation." - ~pp:(fun ppf new_hash -> + ~pp:(fun ppf (new_hash, needed_fee_in_mutez) -> Format.fprintf ppf "The operation %a cannot be added because the mempool already contains \ - a conflicting operation that should not be replaced (e.g. an \ - operation from the same manager with better fees)." + a conflicting operation. %s" Operation_hash.pp - new_hash) - (Data_encoding.obj1 (Data_encoding.req "new_hash" Operation_hash.encoding)) - (function Operation_conflict {new_hash} -> Some new_hash | _ -> None) - (fun new_hash -> Operation_conflict {new_hash}) ; + new_hash + (match needed_fee_in_mutez with + | None -> + "The pre-existing operation cannot be replaced with the new one, \ + even if fees were increased. Try again after the next block has \ + been baked." + | Some needed_fee_in_mutez -> + Format.asprintf + "To replace the latter, this particular operation would need a \ + total fee of at least %Ld mutez." + needed_fee_in_mutez)) + (Data_encoding.obj2 + (Data_encoding.req "new_hash" Operation_hash.encoding) + (Data_encoding.opt "needed_fee_in_mutez" Data_encoding.int64)) + (function + | Operation_conflict {new_hash; needed_fee_in_mutez} -> + Some (new_hash, needed_fee_in_mutez) + | _ -> None) + (fun (new_hash, needed_fee_in_mutez) -> + Operation_conflict {new_hash; needed_fee_in_mutez}) ; (* Operation replacement *) register_error_kind `Temporary @@ -137,7 +155,7 @@ let () = Data_encoding.( obj2 (req "operation_hash" Operation_hash.encoding) - (req "needed_fee_in_mutez" (option int64))) + (opt "needed_fee_in_mutez" int64)) (function | Rejected_by_full_mempool {hash; needed_fee_in_mutez} -> Some (hash, needed_fee_in_mutez) diff --git a/src/lib_shell_services/validation_errors.mli b/src/lib_shell_services/validation_errors.mli index 3c0d92a6df9d..9bbb87adab92 100644 --- a/src/lib_shell_services/validation_errors.mli +++ b/src/lib_shell_services/validation_errors.mli @@ -29,7 +29,10 @@ type error += Parse_error type error += - | Operation_conflict of {new_hash : Operation_hash.t} + | Operation_conflict of { + new_hash : Operation_hash.t; + needed_fee_in_mutez : int64 option; + } | Operation_replacement of { old_hash : Operation_hash.t; new_hash : Operation_hash.t; -- GitLab From b9282a55c7748a813b359615f6d15e61bac0825f Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Wed, 7 Jun 2023 14:58:25 +0200 Subject: [PATCH 3/9] Shell/prevalidation: indicate fee needed to win manager conflict --- src/lib_shell/prevalidation.ml | 42 ++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/lib_shell/prevalidation.ml b/src/lib_shell/prevalidation.ml index ea00c1fedcf7..60817aebec0e 100644 --- a/src/lib_shell/prevalidation.ml +++ b/src/lib_shell/prevalidation.ml @@ -141,7 +141,10 @@ module MakeAbstract (** Static information needed by [Filter.Mempool.pre_filter]. *) conflict_map : Filter.Mempool.Conflict_map.t; (** State needed by - [Filter.Mempool.Conflict_map.fee_needed_to_replace_by_fee]. *) + [Filter.Mempool.Conflict_map.fee_needed_to_replace_by_fee] in + order to provide the [needed_fee_in_mutez] field of the + [Operation_conflict] error (see the [translate_proto_add_result] + function below). *) } let create_aux ?old_state chain_store head timestamp = @@ -211,8 +214,10 @@ module MakeAbstract with [num >= 7]. *) assert false) + (* Analyse the output of [Proto.Mempool.add_operation] to extract + the potential replaced operation or return the appropriate error. *) let translate_proto_add_result (proto_add_result : Proto.Mempool.add_result) - op : replacement tzresult = + op conflict_map filter_config : replacement tzresult = let open Result in let open Validation_errors in match proto_add_result with @@ -223,8 +228,18 @@ module MakeAbstract in return_some (removed, `Outdated trace) | Unchanged -> - error - [Operation_conflict {new_hash = op.hash; needed_fee_in_mutez = None}] + (* There was an operation conflict and [op] lost to the + pre-existing operation. The error should indicate the fee + that [op] would need in order to win the conflict and replace + the old operation, if such a fee exists; otherwise the error + should contain [None]. *) + let needed_fee_in_mutez = + Filter.Mempool.Conflict_map.fee_needed_to_replace_by_fee + filter_config + ~candidate_op:op.protocol + ~conflict_map + in + error [Operation_conflict {new_hash = op.hash; needed_fee_in_mutez}] let bounding_add_operation bounding_state bounding_config op = Result.map_error @@ -253,11 +268,13 @@ module MakeAbstract [Proto.Mempool.add_operation], already contains the new operation (if it has been accepted). So the only update it may need is the removal of any operations replaced during [Bounding.add]. *) - let check_conflict_and_bound (mempool, proto_add_result) bounding_state - bounding_config op : + let check_conflict_and_bound (mempool, proto_add_result) + {bounding_state; conflict_map; _} (filter_config, bounding_config) op : (Proto.Mempool.t * Bounding.state * replacements) tzresult = let open Result_syntax in - let* proto_replacement = translate_proto_add_result proto_add_result op in + let* proto_replacement = + translate_proto_add_result proto_add_result op conflict_map filter_config + in let bounding_state = match proto_replacement with | None -> bounding_state @@ -300,10 +317,9 @@ module MakeAbstract ~new_operation:op.protocol ~replacements - let add_operation_result state (filter_config, bounding_config) op : - add_result tzresult Lwt.t = + let add_operation_result state config op : add_result tzresult Lwt.t = let open Lwt_result_syntax in - let conflict_handler = Filter.Mempool.conflict_handler filter_config in + let conflict_handler = Filter.Mempool.conflict_handler (fst config) in let* proto_output = proto_add_operation ~conflict_handler state op in (* The operation might still be rejected because of a conflict with a previously validated operation, or if the mempool is @@ -315,11 +331,7 @@ module MakeAbstract let valid_op = record_successful_signature_check op in let res = catch_e @@ fun () -> - check_conflict_and_bound - proto_output - state.bounding_state - bounding_config - valid_op + check_conflict_and_bound proto_output state config valid_op in match res with | Ok (mempool, bounding_state, replacements) -> -- GitLab From 61624d7329800084ceea508a8a4011c98aa44bf6 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Fri, 9 Jun 2023 17:32:11 +0200 Subject: [PATCH 4/9] Tezt: add helper inject_and_capture2_stderr --- tezt/lib_tezos/operation_core.ml | 21 +++++++++++++--- tezt/lib_tezos/operation_core.mli | 19 ++++++++++++++ tezt/tests/prevalidator.ml | 41 ++++++++++++++----------------- 3 files changed, 55 insertions(+), 26 deletions(-) diff --git a/tezt/lib_tezos/operation_core.ml b/tezt/lib_tezos/operation_core.ml index 125ffa24659f..caeb1e6f26cf 100644 --- a/tezt/lib_tezos/operation_core.ml +++ b/tezt/lib_tezos/operation_core.ml @@ -124,8 +124,7 @@ let hash t client : [`OpHash of string] Lwt.t = let hash = Tezos_base.Operation.hash op in return (`OpHash (Tezos_crypto.Hashed.Operation_hash.to_b58check hash)) -let inject ?(request = `Inject) ?(force = false) ?protocol ?signature ?error t - client : [`OpHash of string] Lwt.t = +let spawn_inject ?(force = false) ?protocol ?signature t client = let* signature = match signature with | None -> sign t client @@ -136,6 +135,10 @@ let inject ?(request = `Inject) ?(force = false) ?protocol ?signature ?error t if force then RPC.post_private_injection_operation else RPC.post_injection_operation in + return (RPC.Client.spawn client @@ inject_rpc (Data (`String op))) + +let inject ?(request = `Inject) ?force ?protocol ?signature ?error t client : + [`OpHash of string] Lwt.t = let waiter = let mode = Client.get_mode client in match Client.mode_to_endpoint mode with @@ -146,7 +149,7 @@ let inject ?(request = `Inject) ?(force = false) ?protocol ?signature ?error t foreign endpoint" | Some (Node node) -> Node.wait_for_request ~request node in - let runnable = RPC.Client.spawn client @@ inject_rpc (Data (`String op)) in + let* runnable = spawn_inject ?force ?protocol ?signature t client in match error with | None -> let* () = waiter in @@ -157,6 +160,18 @@ let inject ?(request = `Inject) ?(force = false) ?protocol ?signature ?error t let* () = Process.check_error ~msg process in hash t client +let inject_and_capture2_stderr ~rex ?force ?protocol ?signature t client = + let* runnable = spawn_inject ?force ?protocol ?signature t client in + let*? process = runnable in + let* stderr = Process.check_and_read_stderr ~expect_failure:true process in + match stderr =~** rex with + | None -> + Test.fail + "Injection was expected to fail with:\n%s\nbut instead failed with:\n%s" + (show_rex rex) + stderr + | Some groups -> return groups + let inject_operations ?protocol ?(request = `Inject) ?(force = false) ?error ?use_tmp_file t client : [`OpHash of string] list Lwt.t = let forge op = diff --git a/tezt/lib_tezos/operation_core.mli b/tezt/lib_tezos/operation_core.mli index 6e5505798ea4..5920661b1e1a 100644 --- a/tezt/lib_tezos/operation_core.mli +++ b/tezt/lib_tezos/operation_core.mli @@ -147,6 +147,25 @@ val inject : Client.t -> [`OpHash of string] Lwt.t +(** Same as [inject], but do not wait for the process to exit. *) +val spawn_inject : + ?force:bool -> + ?protocol:Protocol.t -> + ?signature:Tezos_crypto.Signature.t -> + t -> + Client.t -> + JSON.t Runnable.process Lwt.t + +(** Run [spawn_inject] then capture two groups on stderr with [rex]. *) +val inject_and_capture2_stderr : + rex:rex -> + ?force:bool -> + ?protocol:Protocol.t -> + ?signature:Tezos_crypto.Signature.t -> + t -> + Client.t -> + (string * string) Lwt.t + (** [inject_operations ?protocol ?request ?force ?error ?use_tmp_file ops client] is similar as [inject] for a list of operations. This function calls the RPC {!val:RPC.post_private_injection_operations} which is faster than diff --git a/tezt/tests/prevalidator.ml b/tezt/tests/prevalidator.ml index f519da2865d6..3fd4779b3f8f 100644 --- a/tezt/tests/prevalidator.ml +++ b/tezt/tests/prevalidator.ml @@ -1540,17 +1540,6 @@ module Revamped = struct let* () = check_mempool ~applied:[oph4; oph3; oph2; oph1] client1 in check_mempool ~applied:[oph4; oph3; oph2; oph1] client2 - let check_process_error_and_capture_two_groups ?__LOC__ rex process = - let* stderr = Process.check_and_read_stderr ~expect_failure:true process in - match stderr =~** rex with - | None -> - Test.fail - ?__LOC__ - "Process was expected to fail with:\n%s\nbut instead failed with:\n%s" - (show_rex rex) - stderr - | Some groups -> return groups - let test_full_mempool = Protocol.register_test ~__FILE__ @@ -1612,21 +1601,27 @@ module Revamped = struct 4 "The client should report when the mempool is full and not enough fees \ are provided." ; + let* op4 = + Operation.Manager.( + operation + [ + make + ~source:Constant.bootstrap5 + ~fee:(fee - 1) + ~gas_limit + (transfer ()); + ]) + client1 + in let* _oph, recommended_fee = - check_process_error_and_capture_two_groups - ~__LOC__ - Constant.Error_msg.rejected_by_full_mempool - (Client.spawn_transfer - ~giver:Constant.bootstrap5.alias - ~receiver:Constant.bootstrap2.alias - ~amount:(Tez.of_int 55) - ~fee:(Tez.of_mutez_int (fee - 1)) - ~gas_limit - client1) + Operation.inject_and_capture2_stderr + ~rex:Constant.Error_msg.rejected_by_full_mempool + op4 + client1 in Check.( - (recommended_fee = string_of_int (fee + 1)) - string + (int_of_string recommended_fee = fee + 1) + int ~error_msg:"The recommended fee is %L but expected %R.") ; log_step 5 "Force inject an extra operation with not enough fees." ; -- GitLab From ce307d476ce0fd19aba686712821a1fedf754319 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Mon, 12 Jun 2023 17:47:43 +0200 Subject: [PATCH 5/9] Tezt: update conflict error message --- tezt/lib_tezos/operation_core.ml | 4 ++-- tezt/lib_tezos/operation_core.mli | 16 ++++++++++++---- tezt/tests/operation_validation.ml | 2 +- tezt/tests/prevalidator.ml | 2 +- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/tezt/lib_tezos/operation_core.ml b/tezt/lib_tezos/operation_core.ml index caeb1e6f26cf..0e2eef7546cd 100644 --- a/tezt/lib_tezos/operation_core.ml +++ b/tezt/lib_tezos/operation_core.ml @@ -706,6 +706,6 @@ module Manager = struct RPC.Client.call client @@ RPC.get_chain_block_hash ?chain ~block () end -let conflict_error = +let conflict_error_with_needed_fee = rex - {|The operation [\w\d]+ cannot be added because the mempool already contains a conflicting operation that should not be replaced \(e\.g\. an operation from the same manager with better fees\)\.|} + {|The operation ([\w\d]+) cannot be added because the mempool already contains a conflicting operation\. To replace the latter, this particular operation would need a total fee of at least ([\d]+) mutez\.|} diff --git a/tezt/lib_tezos/operation_core.mli b/tezt/lib_tezos/operation_core.mli index 5920661b1e1a..aeb12e10024d 100644 --- a/tezt/lib_tezos/operation_core.mli +++ b/tezt/lib_tezos/operation_core.mli @@ -573,8 +573,16 @@ module Manager : sig val get_branch : ?chain:string -> ?offset:int -> Client.t -> string Lwt.t end -(** Regular expression recognizing the mempool error that arises when - the operation conflicts with another previously validated operation. +(** Regular expressions for specific error messages. - Intended to be provided as the [error] argument to {!val:inject}. *) -val conflict_error : rex + Can be used as e.g. + - the [error] argument of {!val:inject} + - the [rex] argument of {!val:inject_and_capture2_stderr} + - the [msg] argument of {!val:Process.check_error}. *) + +(** Matches the message produced by + [Operation_conflict {new_hash; needed_fee_in_mutez = Some fee}] + from [src/lib_shell_services/validation_errors]. + + Captures [new_hash] and [fee]. *) +val conflict_error_with_needed_fee : rex diff --git a/tezt/tests/operation_validation.ml b/tezt/tests/operation_validation.ml index 3e546fe999ad..ad2fa8d6c84a 100644 --- a/tezt/tests/operation_validation.ml +++ b/tezt/tests/operation_validation.ml @@ -63,7 +63,7 @@ let check_validate_1m_restriction_node = in let* (`OpHash _s) = Operation.Manager.inject [op1] client in - let error = Operation.conflict_error in + let error = Operation.conflict_error_with_needed_fee in Log.info "Inject a second transfer with the same manager and check that the \ injection fails with the following message:\n\ diff --git a/tezt/tests/prevalidator.ml b/tezt/tests/prevalidator.ml index 3fd4779b3f8f..5d6dd9b2c926 100644 --- a/tezt/tests/prevalidator.ml +++ b/tezt/tests/prevalidator.ml @@ -794,7 +794,7 @@ module Revamped = struct let* (`OpHash _) = Operation.Manager.( inject - ~error:Operation.conflict_error + ~error:Operation.conflict_error_with_needed_fee ~signer [make ~source:source1 ~fee @@ transfer ~dest:Constant.bootstrap4 ()] client) -- GitLab From 5d5e3c9a5c2cca4800464704e085f14f361c3903 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Tue, 13 Jun 2023 17:07:37 +0200 Subject: [PATCH 6/9] Tezt: move other error rex to Operation --- tezt/lib_tezos/constant.ml | 19 ------------------- tezt/lib_tezos/operation_core.ml | 9 +++++++++ tezt/lib_tezos/operation_core.mli | 11 +++++++++++ tezt/tests/client_commands.ml | 2 +- tezt/tests/prevalidator.ml | 2 +- 5 files changed, 22 insertions(+), 21 deletions(-) diff --git a/tezt/lib_tezos/constant.ml b/tezt/lib_tezos/constant.ml index 4a8c8e6591d3..fd1cdb7580e8 100644 --- a/tezt/lib_tezos/constant.ml +++ b/tezt/lib_tezos/constant.ml @@ -121,22 +121,3 @@ let tz4_account : Account.key = section of the reference manual. *) let wasm_echo_kernel_boot_sector = "0061736d0100000001280760037f7f7f017f60027f7f017f60057f7f7f7f7f017f60017f0060017f017f60027f7f0060000002610311736d6172745f726f6c6c75705f636f72650a726561645f696e707574000011736d6172745f726f6c6c75705f636f72650c77726974655f6f7574707574000111736d6172745f726f6c6c75705f636f72650b73746f72655f77726974650002030504030405060503010001071402036d656d02000a6b65726e656c5f72756e00060aa401042a01027f41fa002f0100210120002f010021022001200247044041e4004112410041e400410010021a0b0b0800200041c4006b0b5001057f41fe002d0000210341fc002f0100210220002d0000210420002f0100210520011004210620042003460440200041016a200141016b10011a0520052002460440200041076a200610011a0b0b0b1d01017f41dc0141840241901c100021004184022000100541840210030b0b38050041e4000b122f6b65726e656c2f656e762f7265626f6f740041f8000b0200010041fa000b0200020041fc000b0200000041fe000b0101" - -(** Regular expressions that can be provided to [Process.check_error] - to identify errors. *) -module Error_msg = struct - let gas_limit_exceeded = - rex - "Gas limit exceeded during typechecking or execution.\n\ - Try again with a higher gas limit." - - let rejected_by_full_mempool = - rex - "Operation (.*) has been rejected because the mempool is full. This \ - specific operation would need a total fee of at least (.*) mutez to be \ - considered and propagated by the mempool of this particular node right \ - now. Note that if the node receives operations with a better fee over \ - gas limit ratio in the future, the operation may be rejected even with \ - the indicated fee, or it may be successfully injected but removed at a \ - later date." -end diff --git a/tezt/lib_tezos/operation_core.ml b/tezt/lib_tezos/operation_core.ml index 0e2eef7546cd..e4d1d2699525 100644 --- a/tezt/lib_tezos/operation_core.ml +++ b/tezt/lib_tezos/operation_core.ml @@ -706,6 +706,15 @@ module Manager = struct RPC.Client.call client @@ RPC.get_chain_block_hash ?chain ~block () end +let gas_limit_exceeded = + rex + "Gas limit exceeded during typechecking or execution.\n\ + Try again with a higher gas limit." + let conflict_error_with_needed_fee = rex {|The operation ([\w\d]+) cannot be added because the mempool already contains a conflicting operation\. To replace the latter, this particular operation would need a total fee of at least ([\d]+) mutez\.|} + +let rejected_by_full_mempool_with_needed_fee = + rex + {|Operation ([\w\d]+) has been rejected because the mempool is full\. This specific operation would need a total fee of at least ([\d]+) mutez to be considered and propagated by the mempool of this particular node right now\. Note that if the node receives operations with a better fee over gas limit ratio in the future, the operation may be rejected even with the indicated fee, or it may be successfully injected but removed at a later date\.|} diff --git a/tezt/lib_tezos/operation_core.mli b/tezt/lib_tezos/operation_core.mli index aeb12e10024d..8c0d23733c5c 100644 --- a/tezt/lib_tezos/operation_core.mli +++ b/tezt/lib_tezos/operation_core.mli @@ -580,9 +580,20 @@ end - the [rex] argument of {!val:inject_and_capture2_stderr} - the [msg] argument of {!val:Process.check_error}. *) +(** Matches the client message for the [Operation_quota_exceeded] + protocol error. *) +val gas_limit_exceeded : rex + (** Matches the message produced by [Operation_conflict {new_hash; needed_fee_in_mutez = Some fee}] from [src/lib_shell_services/validation_errors]. Captures [new_hash] and [fee]. *) val conflict_error_with_needed_fee : rex + +(** Matches the message produced by + [Rejected_by_full_mempool {hash; needed_fee_in_mutez = Some fee}] + from [src/lib_shell_services/validation_errors]. + + Captures [hash] and [fee]. *) +val rejected_by_full_mempool_with_needed_fee : rex diff --git a/tezt/tests/client_commands.ml b/tezt/tests/client_commands.ml index 3af2161e0ddc..30b46f3d0b30 100644 --- a/tezt/tests/client_commands.ml +++ b/tezt/tests/client_commands.ml @@ -674,7 +674,7 @@ module Dry_run = struct ~dry_run:false client in - let msg = Constant.Error_msg.gas_limit_exceeded in + let msg = Operation.gas_limit_exceeded in let* () = Process.check_error ~msg originate_res_ko in Log.info "Originate the contract with a gas_limit of %d and check that the \ diff --git a/tezt/tests/prevalidator.ml b/tezt/tests/prevalidator.ml index 5d6dd9b2c926..319dcd41340f 100644 --- a/tezt/tests/prevalidator.ml +++ b/tezt/tests/prevalidator.ml @@ -1615,7 +1615,7 @@ module Revamped = struct in let* _oph, recommended_fee = Operation.inject_and_capture2_stderr - ~rex:Constant.Error_msg.rejected_by_full_mempool + ~rex:Operation.rejected_by_full_mempool_with_needed_fee op4 client1 in -- GitLab From 97bc383e4301b25a0b54b69902069d6468295cad Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Tue, 13 Jun 2023 10:38:42 +0200 Subject: [PATCH 7/9] Tezt: check recommended fee to replace conflicting operation --- tezt/tests/prevalidator.ml | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tezt/tests/prevalidator.ml b/tezt/tests/prevalidator.ml index 319dcd41340f..6420d1a5335e 100644 --- a/tezt/tests/prevalidator.ml +++ b/tezt/tests/prevalidator.ml @@ -754,6 +754,7 @@ module Revamped = struct let source1 = Constant.bootstrap1 in let source2 = Constant.bootstrap2 in let fee = 1_000 in + let expected_fee_needed_to_replace = 1_050 in log_step 2 "Inject transfers from [source1] and [source2] with fee [%d] and correct \ @@ -791,14 +792,23 @@ module Revamped = struct operation). Check that it fails and the mempool is unchanged. Indeed, \ the [force] argument of [inject] defaults to [false] so the faulty \ injected operation is discarded." ; - let* (`OpHash _) = + let* op3 = Operation.Manager.( - inject - ~error:Operation.conflict_error_with_needed_fee + operation ~signer - [make ~source:source1 ~fee @@ transfer ~dest:Constant.bootstrap4 ()] - client) + [make ~source:source1 ~fee (transfer ~dest:Constant.bootstrap4 ())]) + client + in + let* _oph3, fee_needed_to_replace = + Operation.inject_and_capture2_stderr + ~rex:Operation.conflict_error_with_needed_fee + op3 + client in + Check.( + (int_of_string fee_needed_to_replace = expected_fee_needed_to_replace) + int + ~error_msg:"The recommended fee is %L but expected %R.") ; let* () = check_mempool ~applied:[oph1; oph2] client in log_step -- GitLab From 2bced73b2f4ed671e7dcba5a8d84d7e368f5f246 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Wed, 14 Jun 2023 11:06:14 +0200 Subject: [PATCH 8/9] Changelog: add entry on updated error message --- CHANGES.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index e637ba1b5190..f485bf94d836 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -69,6 +69,13 @@ Node matter its fee (e.g. if it is a non-manager operation). (MRs :gl:`!6787`, :gl:`!8640`) +- Updated the message of the mempool's + ``prevalidation.operation_conflict`` error. It now provides the + minimal fee that the operation would need to replace the + pre-existing conflicting operation, when such a fee exists. (This + fee indication used to be available before V16-rc1, where it had + been removed for technical reasons.) (MR :gl:`!9016`) + - RPC ``/helpers/forge/operations`` can now take JSON formatted operations with ``attestation``, ``preattestation``, ``double_attestation_evidence`` and ``double_preattestation_evidence`` kinds. Note that the existing kinds -- GitLab From c6cf7d64d5288610a5c040c690714c317686d627 Mon Sep 17 00:00:00 2001 From: Diane Gallois-Wong Date: Tue, 20 Jun 2023 17:47:58 +0200 Subject: [PATCH 9/9] Shell/prevalidation: tweak auxiliary functions to improve readability --- src/lib_shell/prevalidation.ml | 133 +++++++++++++++++---------------- 1 file changed, 70 insertions(+), 63 deletions(-) diff --git a/src/lib_shell/prevalidation.ml b/src/lib_shell/prevalidation.ml index 60817aebec0e..30c0f9af7773 100644 --- a/src/lib_shell/prevalidation.ml +++ b/src/lib_shell/prevalidation.ml @@ -197,6 +197,7 @@ module MakeAbstract | Temporary -> `Branch_delayed trace | Outdated -> `Outdated trace + (* Wrapper around [Proto.Mempool.add_operation]. *) let proto_add_operation ~conflict_handler state op : (Proto.Mempool.t * Proto.Mempool.add_result) tzresult Lwt.t = Proto.Mempool.add_operation @@ -241,60 +242,37 @@ module MakeAbstract in error [Operation_conflict {new_hash = op.hash; needed_fee_in_mutez}] - let bounding_add_operation bounding_state bounding_config op = - Result.map_error - (fun op_to_overtake -> - let needed_fee_in_mutez = - Option.bind op_to_overtake (fun op_to_overtake -> - Filter.Mempool.fee_needed_to_overtake - ~op_to_overtake:op_to_overtake.protocol - ~candidate_op:op.protocol) - in - [ - Validation_errors.Rejected_by_full_mempool - {hash = op.hash; needed_fee_in_mutez}; - ]) - (Bounding.add_operation bounding_state bounding_config op) - - (* Analyze the output of [Proto.Mempool.add_operation] to handle a - potential operation conflict. Then use the [Bounding] module to - ensure that the mempool remains bounded. - - If successful, return the updated [mempool] and [bounding_state], - as well as any operation [replacements] caused by either the - protocol mempool or the [Bounding] module. - - Note that the [mempool] argument, as part of the output of - [Proto.Mempool.add_operation], already contains the new operation - (if it has been accepted). So the only update it may need is the - removal of any operations replaced during [Bounding.add]. *) - let check_conflict_and_bound (mempool, proto_add_result) - {bounding_state; conflict_map; _} (filter_config, bounding_config) op : - (Proto.Mempool.t * Bounding.state * replacements) tzresult = + let update_bounding_state bounding_state bounding_config op ~proto_replacement + = let open Result_syntax in - let* proto_replacement = - translate_proto_add_result proto_add_result op conflict_map filter_config - in let bounding_state = match proto_replacement with | None -> bounding_state | Some (replaced, _) -> Bounding.remove_operation bounding_state replaced in - let* bounding_state, removed_by_bounding = - bounding_add_operation bounding_state bounding_config op - in - let mempool = - List.fold_left Proto.Mempool.remove_operation mempool removed_by_bounding + let* bounding_state, removed_operation_hashes = + Result.map_error + (fun op_to_overtake -> + let needed_fee_in_mutez = + Option.bind op_to_overtake (fun op_to_overtake -> + Filter.Mempool.fee_needed_to_overtake + ~op_to_overtake:op_to_overtake.protocol + ~candidate_op:op.protocol) + in + [ + Validation_errors.Rejected_by_full_mempool + {hash = op.hash; needed_fee_in_mutez}; + ]) + (Bounding.add_operation bounding_state bounding_config op) in - let replacements = - Option.to_list proto_replacement - @ List.map - (fun removed -> - let err = [Validation_errors.Removed_from_full_mempool removed] in - (removed, classification_of_trace err)) - removed_by_bounding + let bounding_replacements = + List.map + (fun removed -> + let err = [Validation_errors.Removed_from_full_mempool removed] in + (removed, classification_of_trace err)) + removed_operation_hashes in - return (mempool, bounding_state, replacements) + return (bounding_state, bounding_replacements) let update_conflict_map conflict_map ~mempool_before op replacements = (* [mempool_before] is the protocol's mempool representation @@ -317,10 +295,13 @@ module MakeAbstract ~new_operation:op.protocol ~replacements - let add_operation_result state config op : add_result tzresult Lwt.t = + (* Implements [add_operation] but inside the [tzresult] monad. *) + let add_operation_result state (filter_config, bounding_config) op = let open Lwt_result_syntax in - let conflict_handler = Filter.Mempool.conflict_handler (fst config) in - let* proto_output = proto_add_operation ~conflict_handler state op in + let conflict_handler = Filter.Mempool.conflict_handler filter_config in + let* mempool, proto_add_result = + proto_add_operation ~conflict_handler state op + in (* The operation might still be rejected because of a conflict with a previously validated operation, or if the mempool is full and the operation does not have enough fees. Nevertheless, @@ -331,23 +312,49 @@ module MakeAbstract let valid_op = record_successful_signature_check op in let res = catch_e @@ fun () -> - check_conflict_and_bound proto_output state config valid_op + let open Result_syntax in + let* proto_replacement = + translate_proto_add_result + proto_add_result + op + state.conflict_map + filter_config + in + let* bounding_state, bounding_replacements = + update_bounding_state + state.bounding_state + bounding_config + op + ~proto_replacement + in + let mempool = + List.fold_left + (fun mempool (replaced_oph, _) -> + Proto.Mempool.remove_operation mempool replaced_oph) + mempool + bounding_replacements + in + let all_replacements = + match proto_replacement with + | None -> bounding_replacements + | Some proto_replacement -> proto_replacement :: bounding_replacements + in + let conflict_map = + update_conflict_map + state.conflict_map + ~mempool_before:state.mempool + op + all_replacements + in + let state = {state with mempool; bounding_state; conflict_map} in + return (state, valid_op, `Validated, all_replacements) in match res with - | Ok (mempool, bounding_state, replacements) -> - let conflict_map = - update_conflict_map - state.conflict_map - ~mempool_before:state.mempool - op - replacements - in - let state = {state with mempool; bounding_state; conflict_map} in - return (state, valid_op, `Validated, replacements) + | Ok add_result -> return add_result | Error trace -> - (* We convert any error from [check_conflict_and_bound] into an - [add_result] here, rather than let [add_operation] below do - the same, so that we can return the updated [valid_op]. *) + (* When [res] is an error, we convert it to an [add_result] + here (instead of letting [add_operation] do it below) so + that we can return the updated [valid_op]. *) return (state, valid_op, classification_of_trace trace, []) let add_operation state config op : add_result Lwt.t = -- GitLab