From cf34fbc42ad5e44caf20ba2c8a5e1f5f13a2f52b Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Thu, 11 Aug 2022 16:51:02 +0200
Subject: [PATCH 1/7] Proto/validate_operation: add remove_manager_operation

---
 .../lib_protocol/validate_operation.ml        | 52 +++++++++++++++++++
 .../lib_protocol/validate_operation.mli       | 16 ++++++
 2 files changed, 68 insertions(+)

diff --git a/src/proto_alpha/lib_protocol/validate_operation.ml b/src/proto_alpha/lib_protocol/validate_operation.ml
index bd2dd89c7f26..da87c0b27e09 100644
--- a/src/proto_alpha/lib_protocol/validate_operation.ml
+++ b/src/proto_alpha/lib_protocol/validate_operation.ml
@@ -1889,6 +1889,54 @@ module Manager = struct
       maybe_update_remaining_block_gas vi vs batch_state
     in
     return {vs with manager_state = {managers_seen; remaining_block_gas}}
+
+  let rec sum_batch_gas_limit :
+      type kind.
+      Gas.Arith.integral ->
+      kind Kind.manager contents_list ->
+      Gas.Arith.integral =
+   fun acc contents_list ->
+    match contents_list with
+    | Single (Manager_operation {gas_limit; _}) -> Gas.Arith.add gas_limit acc
+    | Cons (Manager_operation {gas_limit; _}, tail) ->
+        sum_batch_gas_limit (Gas.Arith.add gas_limit acc) tail
+
+  let remove_manager_operation (type manager_kind) vi vs
+      (operation : manager_kind Kind.manager operation) =
+    let source =
+      match operation.protocol_data.contents with
+      | Single (Manager_operation {source; _})
+      | Cons (Manager_operation {source; _}, _) ->
+          source
+    in
+    match
+      Signature.Public_key_hash.Map.find_opt
+        source
+        vs.manager_state.managers_seen
+    with
+    | None -> (* Nothing to do *) vs
+    | Some _oph ->
+        let managers_seen =
+          Signature.Public_key_hash.Map.remove
+            source
+            vs.manager_state.managers_seen
+        in
+        let remaining_block_gas =
+          match vi.mode with
+          | Block ->
+              let gas_limit =
+                sum_batch_gas_limit
+                  Gas.Arith.zero
+                  operation.protocol_data.contents
+              in
+              Gas.Arith.(
+                sub vs.manager_state.remaining_block_gas (fp gas_limit))
+          | Mempool ->
+              (* The remaining block gas is never updated in [Mempool]
+                 mode anyway (see {!maybe_update_remaining_block_gas}). *)
+              vs.manager_state.remaining_block_gas
+        in
+        {vs with manager_state = {managers_seen; remaining_block_gas}}
 end
 
 let init_info_and_state ctxt mode chain_id all_expected_consensus_features =
@@ -2006,6 +2054,10 @@ let validate_operation (vi : validate_operation_info)
   in
   return (vs, Operation_validated_stamp)
 
+(* This function will be replaced with a generic remove_operation in
+   the future. *)
+let remove_manager_operation = Manager.remove_manager_operation
+
 module TMP_for_plugin = struct
   type 'a should_check_signature =
     | Check_signature of 'a operation
diff --git a/src/proto_alpha/lib_protocol/validate_operation.mli b/src/proto_alpha/lib_protocol/validate_operation.mli
index d4c61fb60aa9..b0c3c9bc125f 100644
--- a/src/proto_alpha/lib_protocol/validate_operation.mli
+++ b/src/proto_alpha/lib_protocol/validate_operation.mli
@@ -169,6 +169,22 @@ val validate_operation :
   'kind operation ->
   (validate_operation_state * stamp) tzresult Lwt.t
 
+(** Remove a manager operation from the {!validate_operation_state}.
+
+    This function is intended for a mempool: when an operation A has
+    already been validated, but another operation B conflicts with A
+    (e.g. they have the same manager) and is more desirable than A
+    (e.g. better fees/gas ratio), then the mempool may remove A in
+    order to validate B instead.
+
+    This function will be replaced with a generic function
+    [remove_operation] in the future. *)
+val remove_manager_operation :
+  validate_operation_info ->
+  validate_operation_state ->
+  'a Kind.manager operation ->
+  validate_operation_state
+
 (** Functions for the plugin.
 
     These functions are temporary.
-- 
GitLab


From 06c4dd27c296b774ec31acd607531d3fd6b9e984 Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Tue, 2 Aug 2022 18:46:54 +0200
Subject: [PATCH 2/7] Plugin & shell: alpha plugin now relies on protocol
 validation for 1M

The plugin of proto_alpha no longer checks the one-operation-per-manager-
per-block restriction (neither in the pre_filter nor in precheck),
but instead catches the manager restriction protocol error. When this
happens, the plugin is still responsible for deciding which of the two
operations from the same manager should be prioritized. When appropriate,
the plugin will remove the old operation from the protocol validation
state before attempting again to validate the new one.
---
 src/lib_shell/prevalidation.ml               |   4 +
 src/lib_shell/prevalidation.mli              |   4 +
 src/lib_shell/prevalidator.ml                |  18 +-
 src/lib_shell/prevalidator_filters.ml        |   1 +
 src/lib_shell/prevalidator_filters.mli       |   6 +-
 src/proto_012_Psithaca/lib_plugin/plugin.ml  |   3 +-
 src/proto_013_PtJakart/lib_plugin/plugin.ml  |   3 +-
 src/proto_014_PtKathma/lib_plugin/mempool.ml |   6 +-
 src/proto_alpha/lib_plugin/mempool.ml        | 706 +++++++++++--------
 9 files changed, 442 insertions(+), 309 deletions(-)

diff --git a/src/lib_shell/prevalidation.ml b/src/lib_shell/prevalidation.ml
index fe5362ad9ffa..9a06dfdcf51d 100644
--- a/src/lib_shell/prevalidation.ml
+++ b/src/lib_shell/prevalidation.ml
@@ -96,6 +96,8 @@ module type T = sig
 
   val validation_state : t -> validation_state
 
+  val set_validation_state : t -> validation_state -> t
+
   val pp_result : Format.formatter -> result -> unit
 
   module Internal_for_tests : sig
@@ -261,6 +263,8 @@ module MakeAbstract
 
   let validation_state {state; _} = state
 
+  let set_validation_state t state = {t with state}
+
   let pp_result ppf =
     let open Format in
     function
diff --git a/src/lib_shell/prevalidation.mli b/src/lib_shell/prevalidation.mli
index 9c4b93d436e9..3fc0a837337e 100644
--- a/src/lib_shell/prevalidation.mli
+++ b/src/lib_shell/prevalidation.mli
@@ -116,6 +116,10 @@ module type T = sig
       to the type {!validation_state} of the protocol. *)
   val validation_state : t -> validation_state
 
+  (** Updates the subset of [t] corresponding to the type
+      {!validation_state} of the protocol. *)
+  val set_validation_state : t -> validation_state -> t
+
   val pp_result : Format.formatter -> result -> unit
 
   module Internal_for_tests : sig
diff --git a/src/lib_shell/prevalidator.ml b/src/lib_shell/prevalidator.ml
index 6c06fa364453..e5e9da3dcc41 100644
--- a/src/lib_shell/prevalidator.ml
+++ b/src/lib_shell/prevalidator.ml
@@ -453,10 +453,10 @@ module Make_s
         shell.parameters.tools.chain_tools.clear_or_cancel old_hash ;
         []
 
-  let precheck ~disable_precheck ~filter_config ~filter_state ~validation_state
-      (op : protocol_operation operation) =
+  let precheck ~disable_precheck ~filter_config ~filter_state
+      ~validation_state:prevalidation_t (op : protocol_operation operation) =
     let open Lwt_syntax in
-    let validation_state = Prevalidation_t.validation_state validation_state in
+    let validation_state = Prevalidation_t.validation_state prevalidation_t in
     if disable_precheck then Lwt.return `Undecided
     else
       let+ v =
@@ -469,11 +469,16 @@ module Make_s
           op.protocol
       in
       match v with
-      | `Passed_precheck (filter_state, replacement) ->
+      | `Passed_precheck (filter_state, validation_state, replacement) ->
           (* The [precheck] optimization triggers: no need to call the
               protocol [apply_operation]. *)
+          let prevalidation_t =
+            Prevalidation_t.set_validation_state
+              prevalidation_t
+              validation_state
+          in
           let new_op = Prevalidation_t.increment_successful_precheck op in
-          `Passed_precheck (filter_state, new_op, replacement)
+          `Passed_precheck (filter_state, prevalidation_t, new_op, replacement)
       | (`Branch_delayed _ | `Branch_refused _ | `Refused _ | `Outdated _) as
         errs ->
           (* Note that we don't need to distinguish some failure cases
@@ -522,7 +527,8 @@ module Make_s
       | `Fail errs ->
           (* Precheck rejected the operation *)
           Lwt.return_error errs
-      | `Passed_precheck (filter_state, new_op, replacement) ->
+      | `Passed_precheck (filter_state, validation_state, new_op, replacement)
+        ->
           (* Precheck succeeded *)
           let to_handle =
             match replacement with
diff --git a/src/lib_shell/prevalidator_filters.ml b/src/lib_shell/prevalidator_filters.ml
index 574d4ba9a3d3..32763bc77a93 100644
--- a/src/lib_shell/prevalidator_filters.ml
+++ b/src/lib_shell/prevalidator_filters.ml
@@ -61,6 +61,7 @@ module type FILTER = sig
       nb_successful_prechecks:int ->
       [ `Passed_precheck of
         state
+        * Proto.validation_state
         * [ `No_replace
           | `Replace of
             Operation_hash.t * Prevalidator_classification.error_classification
diff --git a/src/lib_shell/prevalidator_filters.mli b/src/lib_shell/prevalidator_filters.mli
index 921f0ae8512f..70ccaa0be6a3 100644
--- a/src/lib_shell/prevalidator_filters.mli
+++ b/src/lib_shell/prevalidator_filters.mli
@@ -76,8 +76,9 @@ module type FILTER = sig
         replaced operation. If the function returns [`Undecided] it means that
         [apply_operation] should be called.
 
-        This function takes a [state] as parameter and returns it updated if the
-        operation has been [prechecked]. It also takes an under-approximation
+        This function takes a filter [state] and a [Proto.validation_state]
+        as parameters, and returns them updated if the operation has been
+        successfully [prechecked]. It also takes an under-approximation
         [nb_successful_prechecks] of the number of times the given operation
         has been successfully prechecked. *)
     val precheck :
@@ -89,6 +90,7 @@ module type FILTER = sig
       nb_successful_prechecks:int ->
       [ `Passed_precheck of
         state
+        * Proto.validation_state
         * [ `No_replace
           | `Replace of
             Operation_hash.t * Prevalidator_classification.error_classification
diff --git a/src/proto_012_Psithaca/lib_plugin/plugin.ml b/src/proto_012_Psithaca/lib_plugin/plugin.ml
index a884958d24c5..e201c42e2a9c 100644
--- a/src/proto_012_Psithaca/lib_plugin/plugin.ml
+++ b/src/proto_012_Psithaca/lib_plugin/plugin.ml
@@ -1171,6 +1171,7 @@ module Mempool = struct
       nb_successful_prechecks:int ->
       [ `Passed_precheck of
         state
+        * validation_state
         * [`No_replace | `Replace of Operation_hash.t * error_classification]
       | error_classification
       | `Undecided ]
@@ -1209,7 +1210,7 @@ module Mempool = struct
               let filter_state =
                 add_manager_restriction filter_state oph info source replacement
               in
-              `Passed_precheck (filter_state, replacement)
+              `Passed_precheck (filter_state, validation_state, replacement)
           | (`Refused _ | `Branch_delayed _ | `Branch_refused _ | `Outdated _)
             as errs ->
               errs)
diff --git a/src/proto_013_PtJakart/lib_plugin/plugin.ml b/src/proto_013_PtJakart/lib_plugin/plugin.ml
index 38f737f8b7a0..42d5667e43b6 100644
--- a/src/proto_013_PtJakart/lib_plugin/plugin.ml
+++ b/src/proto_013_PtJakart/lib_plugin/plugin.ml
@@ -1153,6 +1153,7 @@ module Mempool = struct
       nb_successful_prechecks:int ->
       [ `Passed_precheck of
         state
+        * validation_state
         * [`No_replace | `Replace of Operation_hash.t * error_classification]
       | error_classification
       | `Undecided ]
@@ -1192,7 +1193,7 @@ module Mempool = struct
               let filter_state =
                 add_manager_restriction filter_state oph info source replacement
               in
-              `Passed_precheck (filter_state, replacement)
+              `Passed_precheck (filter_state, validation_state, replacement)
           | (`Refused _ | `Branch_delayed _ | `Branch_refused _ | `Outdated _)
             as errs ->
               errs)
diff --git a/src/proto_014_PtKathma/lib_plugin/mempool.ml b/src/proto_014_PtKathma/lib_plugin/mempool.ml
index 500144cf377c..3fa2844818fd 100644
--- a/src/proto_014_PtKathma/lib_plugin/mempool.ml
+++ b/src/proto_014_PtKathma/lib_plugin/mempool.ml
@@ -1086,7 +1086,9 @@ let precheck :
     Main.operation ->
     nb_successful_prechecks:int ->
     [ `Passed_precheck of
-      state * [`No_replace | `Replace of Operation_hash.t * error_classification]
+      state
+      * validation_state
+      * [`No_replace | `Replace of Operation_hash.t * error_classification]
     | error_classification
     | `Undecided ]
     Lwt.t =
@@ -1125,7 +1127,7 @@ let precheck :
             let filter_state =
               add_manager_restriction filter_state oph info source replacement
             in
-            `Passed_precheck (filter_state, replacement)
+            `Passed_precheck (filter_state, validation_state, replacement)
         | (`Refused _ | `Branch_delayed _ | `Branch_refused _ | `Outdated _) as
           errs ->
             errs)
diff --git a/src/proto_alpha/lib_plugin/mempool.ml b/src/proto_alpha/lib_plugin/mempool.ml
index 500144cf377c..f1d7b75dd5b9 100644
--- a/src/proto_alpha/lib_plugin/mempool.ml
+++ b/src/proto_alpha/lib_plugin/mempool.ml
@@ -168,71 +168,74 @@ let config_encoding : config Data_encoding.t =
           int31
           default_config.max_prechecked_manager_operations))
 
-(* For each Prechecked manager operation (batched or not), we associate the
-   following information to its source:
-   - the operation's hash, needed in case the operation is replaced
-     afterwards,
-   - the total fee and gas_limit, needed to compare operations of the same
-     manager to decide which one has more fees w.r.t. announced gas limit
-     (modulo replace_by_fee_factor)
-*)
+(** An Alpha_context manager operation, packed so that the type is not
+    parametrized by ['kind]. *)
+type manager_op = Manager_op : 'kind Kind.manager operation -> manager_op
+
+(** Information stored for each prechecked manager operation.
+
+    Note that this record does not include the operation hash because
+    it is instead used as key in the map that stores this information
+    in the [state] below. *)
 type manager_op_info = {
-  operation_hash : Operation_hash.t;
-  gas_limit : Gas.Arith.fp;
+  manager_op : manager_op;
+      (** Used when we want to remove the operation with
+          {!Validate_operation.remove_manager_operation}. *)
   fee : Tez.t;
+  gas_limit : Fixed_point_repr.integral_tag Gas.Arith.t;
+      (** Both [fee] and [gas_limit] are used to determine whether a new
+          operation from the same manager should replace this one. *)
   weight : Q.t;
+      (** Used to update [ops_prechecked] and [min_prechecked_op_weight]
+          in [state] when appropriate. *)
 }
 
 type manager_op_weight = {operation_hash : Operation_hash.t; weight : Q.t}
 
-let op_weight_of_info (info : manager_op_info) : manager_op_weight =
-  {operation_hash = info.operation_hash; weight = info.weight}
+(** Build a {!manager_op_weight} from operation hash and {!manager_op_info}. *)
+let mk_op_weight oph (info : manager_op_info) =
+  {operation_hash = oph; weight = info.weight}
+
+let compare_manager_op_weight op1 op2 =
+  let c = Q.compare op1.weight op2.weight in
+  if c <> 0 then c
+  else Operation_hash.compare op1.operation_hash op2.operation_hash
 
 module ManagerOpWeightSet = Set.Make (struct
   type t = manager_op_weight
 
   (* Sort by weight *)
-  let compare op1 op2 =
-    let c = Q.compare op1.weight op2.weight in
-    if c <> 0 then c
-    else Operation_hash.compare op1.operation_hash op2.operation_hash
+  let compare = compare_manager_op_weight
 end)
 
 type state = {
   grandparent_level_start : Timestamp.t option;
   round_zero_duration : Period.t option;
-  op_prechecked_managers : manager_op_info Signature.Public_key_hash.Map.t;
-      (** All managers that are the source of manager operations
-            prechecked in the mempool. Each manager in the map is associated to
-            a record of type [manager_op_info] (See for record details above).
-            Each manager in the map should be accessible
-            with an operation hash in [operation_hash_to_manager]. *)
-  operation_hash_to_manager : Signature.Public_key_hash.t Operation_hash.Map.t;
-      (** Map of operation hash to manager used to remove a manager from
-            [op_prechecked_managers] with an operation hash. Each manager in the
-            map should also be in [op_prechecked_managers]. *)
-  prechecked_operations_count : int;
+  prechecked_manager_op_count : int;
       (** Number of prechecked manager operations.
-            Invariants:
-            - [Operation_hash.Map.cardinal operation_hash_to_manager =
-               prechecked_operations_count]
-            - [prechecked_operations_count <= max_prechecked_manager_operations] *)
-  ops_prechecked : ManagerOpWeightSet.t;
+          Invariants:
+          - [prechecked_manager_op_count
+               = Operation_hash.Map.cardinal prechecked_manager_ops
+               = ManagerOpWeightSet.cardinal prechecked_op_weights]
+          - [prechecked_manager_op_count <= max_prechecked_manager_operations] *)
+  prechecked_manager_ops : manager_op_info Operation_hash.Map.t;
+      (** All prechecked manager operations. See {!manager_op_info}. *)
+  prechecked_op_weights : ManagerOpWeightSet.t;
+      (** The {!manager_op_weight} of all prechecked manager operations. *)
   min_prechecked_op_weight : manager_op_weight option;
       (** The prechecked operation in [op_prechecked_managers], if any, with
             the minimal weight.
             Invariant:
-            - [min_prechecked_op_weight = min { x | x \in ops_prechecked }] *)
+            - [min_prechecked_op_weight = min { x | x \in prechecked_op_weights }] *)
 }
 
 let empty : state =
   {
     grandparent_level_start = None;
     round_zero_duration = None;
-    op_prechecked_managers = Signature.Public_key_hash.Map.empty;
-    operation_hash_to_manager = Operation_hash.Map.empty;
-    prechecked_operations_count = 0;
-    ops_prechecked = ManagerOpWeightSet.empty;
+    prechecked_manager_op_count = 0;
+    prechecked_manager_ops = Operation_hash.Map.empty;
+    prechecked_op_weights = ManagerOpWeightSet.empty;
     min_prechecked_op_weight = None;
   }
 
@@ -284,63 +287,6 @@ let on_flush config filter_state ?(validation_state : validation_state option)
   ignore filter_state ;
   init config ?validation_state ~predecessor ()
 
-let remove ~(filter_state : state) oph =
-  let removed_oph_source = ref None in
-  let operation_hash_to_manager =
-    Operation_hash.Map.update
-      oph
-      (function
-        | None -> None
-        | Some source ->
-            removed_oph_source := Some source ;
-            None)
-      filter_state.operation_hash_to_manager
-  in
-  match !removed_oph_source with
-  | None ->
-      (* Not present anywhere in the filter state, because of invariants.
-         See {!state} *)
-      filter_state
-  | Some source ->
-      let prechecked_operations_count =
-        filter_state.prechecked_operations_count - 1
-      in
-      let removed_op = ref None in
-      let op_prechecked_managers =
-        Signature.Public_key_hash.Map.update
-          source
-          (function
-            | None -> None
-            | Some op ->
-                removed_op := Some op ;
-                None)
-          filter_state.op_prechecked_managers
-      in
-      let ops_prechecked =
-        match !removed_op with
-        | None -> filter_state.ops_prechecked
-        | Some op ->
-            ManagerOpWeightSet.remove
-              (op_weight_of_info op)
-              filter_state.ops_prechecked
-      in
-      let min_prechecked_op_weight =
-        match filter_state.min_prechecked_op_weight with
-        | None -> None
-        | Some op ->
-            if Operation_hash.equal op.operation_hash oph then
-              ManagerOpWeightSet.min_elt ops_prechecked
-            else Some op
-      in
-      {
-        filter_state with
-        op_prechecked_managers;
-        operation_hash_to_manager;
-        ops_prechecked;
-        prechecked_operations_count;
-        min_prechecked_op_weight;
-      }
-
 let get_manager_operation_gas_and_fee contents =
   let open Operation in
   let l = to_list (Contents_list contents) in
@@ -489,38 +435,6 @@ let better_fees_and_ratio =
     Q.compare new_ratio (bump config old_ratio) >= 0
     && Q.compare new_fee (bump config old_fee) >= 0
 
-let check_manager_restriction config filter_state source ~fee ~gas_limit =
-  match
-    Signature.Public_key_hash.Map.find
-      source
-      filter_state.op_prechecked_managers
-  with
-  | None -> `Fresh
-  | Some
-      {
-        operation_hash = old_hash;
-        gas_limit = old_gas;
-        fee = old_fee;
-        weight = _;
-      } ->
-      (* Manager already seen: one manager per block limitation triggered.
-         Can replace old operation if new operation's fees are better *)
-      if
-        better_fees_and_ratio
-          config
-          (Gas.Arith.floor old_gas)
-          old_fee
-          gas_limit
-          fee
-      then `Replace old_hash
-      else
-        `Fail
-          (`Branch_delayed
-            [
-              Environment.wrap_tzerror
-                (Manager_restriction {oph = old_hash; fee = old_fee});
-            ])
-
 let size_of_operation op =
   (WithExceptions.Option.get ~loc:__LOC__
   @@ Data_encoding.Binary.fixed_length
@@ -579,7 +493,7 @@ let check_minimal_weight ?validation_state config filter_state ~fee ~gas_limit
           op
       in
       if
-        filter_state.prechecked_operations_count
+        filter_state.prechecked_manager_op_count
         < config.max_prechecked_manager_operations
       then
         (* The precheck mempool is not full yet *)
@@ -612,7 +526,6 @@ let pre_filter_manager :
     config ->
     state ->
     validation_state_before:validation_state option ->
-    public_key_hash ->
     Operation.packed_protocol_data ->
     t Kind.manager contents_list ->
     [ `Passed_prefilter of Q.t list
@@ -620,7 +533,7 @@ let pre_filter_manager :
     | `Branch_delayed of tztrace
     | `Refused of tztrace
     | `Outdated of tztrace ] =
- fun config filter_state ~validation_state_before source packed_op op ->
+ fun config filter_state ~validation_state_before packed_op op ->
   let size = size_of_operation packed_op in
   let check_gas_and_fee fee gas_limit =
     let fees_in_nanotez =
@@ -652,25 +565,20 @@ let pre_filter_manager :
   match get_manager_operation_gas_and_fee op with
   | Error err -> `Refused (Environment.wrap_tztrace err)
   | Ok (fee, gas_limit) -> (
-      match
-        check_manager_restriction config filter_state source ~fee ~gas_limit
-      with
-      | `Fail errs -> errs
-      | `Fresh | `Replace _ -> (
-          match check_gas_and_fee fee gas_limit with
-          | `Refused _ as err -> err
-          | `Fees_ok -> (
-              match
-                check_minimal_weight
-                  ?validation_state:validation_state_before
-                  config
-                  filter_state
-                  ~fee
-                  ~gas_limit
-                  packed_op
-              with
-              | `Fail errs -> errs
-              | `Weight_ok (_, weight) -> `Passed_prefilter weight)))
+      match check_gas_and_fee fee gas_limit with
+      | `Refused _ as err -> err
+      | `Fees_ok -> (
+          match
+            check_minimal_weight
+              ?validation_state:validation_state_before
+              config
+              filter_state
+              ~fee
+              ~gas_limit
+              packed_op
+          with
+          | `Fail errs -> errs
+          | `Weight_ok (_, weight) -> `Passed_prefilter weight))
 
 type Environment.Error_monad.error += Outdated_endorsement
 
@@ -906,7 +814,7 @@ let pre_filter_far_future_consensus_ops config
 let pre_filter config ~(filter_state : state) ?validation_state_before
     ({shell = _; protocol_data = Operation_data {contents; _} as op} :
       Main.operation) =
-  let prefilter_manager_op source manager_op =
+  let prefilter_manager_op manager_op =
     Lwt.return
     @@
     match
@@ -914,7 +822,6 @@ let pre_filter config ~(filter_state : state) ?validation_state_before
         config
         filter_state
         ~validation_state_before
-        source
         op
         manager_op
     with
@@ -949,70 +856,133 @@ let pre_filter config ~(filter_state : state) ?validation_state_before
   | Single (Vdf_revelation _)
   | Single (Ballot _) ->
       Lwt.return @@ `Passed_prefilter other_prio
-  | Single (Manager_operation {source; _}) as op ->
-      prefilter_manager_op source op
-  | Cons (Manager_operation {source; _}, _) as op ->
-      prefilter_manager_op source op
-
-let precheck_manager :
-    type t.
-    config ->
-    state ->
-    validation_state ->
-    Operation_hash.t ->
-    Tezos_base.Operation.shell_header ->
-    t Kind.manager protocol_data ->
-    nb_successful_prechecks:int ->
-    fee:Tez.t ->
-    gas_limit:Gas.Arith.fp ->
-    public_key_hash ->
-    [> `Prechecked_manager of
-       [`No_replace | `Replace of Operation_hash.t * error_classification]
-    | error_classification ]
-    Lwt.t =
- fun config
-     filter_state
-     validation_state
-     oph
-     shell
-     ({contents; _} as protocol_data : t Kind.manager protocol_data)
-     ~nb_successful_prechecks
-     ~fee
-     ~gas_limit
-     source ->
-  let precheck_manager_and_check_signature ~on_success =
-    let should_check_signature =
-      if Compare.Int.(nb_successful_prechecks > 0) then
-        (* Signature successfully checked at least once. *)
-        Validate_operation.TMP_for_plugin.Skip_signature_check
-      else
-        (* Signature probably never checked. *)
-        Validate_operation.TMP_for_plugin.Check_signature {shell; protocol_data}
-    in
-    Main.precheck_manager validation_state contents should_check_signature
-    >|= function
-    | Ok (_ : Validate_operation.stamp) -> on_success
-    | Error err -> (
-        let err = Environment.wrap_tztrace err in
+  | Single (Manager_operation _) as op -> prefilter_manager_op op
+  | Cons (Manager_operation _, _) as op -> prefilter_manager_op op
+
+(** Call the protocol's {!Validate_operation.validate_operation} and
+    return either:
+
+    - the updated {!validation_state} when the validation is
+      successful, or
+
+    - the protocol error trace converted to an [error trace], together
+      with the corresponding {!error_classification}.
+
+    The signature check is skipped when the operation has previously
+    been validated successfully, ie. [nb_successful_prechecks > 0]. *)
+let proto_validate_operation validation_state oph ~nb_successful_prechecks
+    (operation : 'kind operation) :
+    (validation_state, error trace * error_classification) result Lwt.t =
+  let open Lwt_result_syntax in
+  let*! res =
+    Validate_operation.validate_operation
+      validation_state.validate_operation_info
+      validation_state.validate_operation_state
+      ~should_check_signature:(nb_successful_prechecks <= 0)
+      oph
+      operation
+  in
+  match res with
+  | Ok (validate_operation_state, (_ : Validate_operation.stamp)) ->
+      return {validation_state with validate_operation_state}
+  | Error tztrace ->
+      let err = Environment.wrap_tztrace tztrace in
+      let error_classification =
         match classify_trace err with
         | Branch -> `Branch_refused err
         | Permanent -> `Refused err
         | Temporary -> `Branch_delayed err
-        | Outdated -> `Outdated err)
-  in
-  let gas_limit = Gas.Arith.floor gas_limit in
-  match
-    check_manager_restriction config filter_state source ~fee ~gas_limit
-  with
-  | `Fail err -> Lwt.return err
-  | `Replace old_oph ->
-      let err =
-        Environment.wrap_tzerror
-        @@ Manager_operation_replaced {old_hash = old_oph; new_hash = oph}
+        | Outdated -> `Outdated err
       in
-      precheck_manager_and_check_signature
-        ~on_success:(`Prechecked_manager (`Replace (old_oph, `Outdated [err])))
-  | `Fresh -> (
+      fail (err, error_classification)
+
+(** Call the protocol's {!Validate_operation.validate_operation} on a
+    manager operation and return:
+
+    - [`Success] containing the updated [validation_state] when the
+      validation is successful;
+
+    - [`Conflict] containing the hash of the conflicting operation,
+      and the {!error_classification} corresponding to the protocol error
+      trace, when the validation fails because of the
+      one-manager-operation-per-manager-per-block restriction;
+
+    - an error containing the relevant {!error_classification} when
+      the validation fails with any other protocol error.
+
+    The signature check is skipped when the operation has previously
+    been validated successfully, ie. [nb_successful_prechecks > 0]. *)
+let proto_validate_manager_operation validation_state oph
+    ~nb_successful_prechecks
+    (operation : 'a Kind.manager Alpha_context.operation) :
+    ( [> `Success of validation_state
+      | `Conflict of Operation_hash.t * error_classification ],
+      error_classification )
+    result
+    Lwt.t =
+  let open Lwt_result_syntax in
+  let*! res =
+    proto_validate_operation
+      validation_state
+      oph
+      ~nb_successful_prechecks
+      operation
+  in
+  match res with
+  | Ok validation_state -> return (`Success validation_state)
+  | Error (err, error_classification) -> (
+      match err with
+      | Environment.Ecoproto_error
+          (Validate_errors.Manager.Manager_restriction
+            (_manager, conflicting_oph))
+        :: _ ->
+          return (`Conflict (conflicting_oph, error_classification))
+      | _ -> fail error_classification)
+
+(** Remove a manager operation from the protocol's [validation_state]. *)
+let remove_from_validation_state validation_state (Manager_op op) =
+  let validate_operation_state =
+    Validate_operation.remove_manager_operation
+      validation_state.validate_operation_info
+      validation_state.validate_operation_state
+      op
+  in
+  {validation_state with validate_operation_state}
+
+(** Call the protocol validation on a manager operation and handle
+    potential conflicts: if either the 1M restriction is triggered or
+    the mempool exceeds the maximal number of prechecked operations,
+    then this function is responsible for either discarding the new
+    operation, or removing an old operation to free up space for the
+    new operation.
+
+    Return the updated protocol [validation_state] and, when
+    applicable, the replaced operation accompanied by its new
+    classification.
+
+    Note that this function does not handle the update of the
+    [filter_state]. *)
+let validate_manager_operation_and_handle_conflicts config filter_state
+    validation_state oph ~nb_successful_prechecks fee gas_limit
+    (operation : 'manager_kind Kind.manager operation) :
+    ( validation_state
+      * [`No_replace | `Replace of Operation_hash.t * error_classification],
+      error_classification )
+    result
+    Lwt.t =
+  let open Lwt_result_syntax in
+  let* proto_validation_outcome =
+    proto_validate_manager_operation
+      validation_state
+      oph
+      ~nb_successful_prechecks
+      operation
+  in
+  match proto_validation_outcome with
+  | `Success validation_state -> (
+      (* The operation has been successfully validated and there is no
+         1M conflict. We now need to ensure that the mempool does not
+         exceed its maximal number of prechecked manager operations. *)
       match
         check_minimal_weight
           ~validation_state
@@ -1020,63 +990,230 @@ let precheck_manager :
           filter_state
           ~fee
           ~gas_limit
-          (Operation_data protocol_data)
+          (Operation_data operation.protocol_data)
       with
-      | `Fail err -> Lwt.return err
-      | `Weight_ok (replacement, _weight) ->
-          let on_success =
-            match replacement with
-            | `No_replace -> `Prechecked_manager `No_replace
-            | `Replace oph ->
-                (* The operation with the lowest fees ratio, is reclassified as
-                   branch_delayed. *)
-                (* TODO: https://gitlab.com/tezos/tezos/-/issues/2347 The
-                   branch_delayed ring is bounded to 1000, so we may loose
-                   operations. We can probably do better. *)
-                `Prechecked_manager
-                  (`Replace
-                    ( oph,
-                      `Branch_delayed
-                        [
-                          Environment.wrap_tzerror
-                            Removed_fees_too_low_for_mempool;
-                        ] ))
+      | `Weight_ok (`No_replace, _weight) ->
+          (* The mempool is not full: no need to replace any operation. *)
+          return (validation_state, `No_replace)
+      | `Weight_ok (`Replace min_weight_oph, _weight) ->
+          (* The mempool is full yet the new operation has enough weight
+             to be included: the old operation with the lowest weight is
+             reclassified as [Branch_delayed]. *)
+          (* TODO: https://gitlab.com/tezos/tezos/-/issues/2347 The
+             branch_delayed ring is bounded to 1000, so we may loose
+             operations. We can probably do better. *)
+          let validation_state =
+            match
+              Operation_hash.Map.find
+                min_weight_oph
+                filter_state.prechecked_manager_ops
+            with
+            | None -> assert false
+            | Some {manager_op; _} ->
+                remove_from_validation_state validation_state manager_op
+          in
+          let replace_err =
+            Environment.wrap_tzerror Removed_fees_too_low_for_mempool
           in
-          precheck_manager_and_check_signature ~on_success)
+          let replacement =
+            `Replace (min_weight_oph, `Branch_delayed [replace_err])
+          in
+          return (validation_state, replacement)
+      | `Fail err ->
+          (* The mempool is full and the weight of the new operation is
+             too low: raise the error returned by {!check_minimal_weight}. *)
+          fail err)
+  | `Conflict (old_oph, _proto_error) ->
+      (* The protocol [validation_state] already contains an operation
+         from the same manager. We look at the fees and gas limits of
+         both operations to decide whether to replace the old one. *)
+      let old_info =
+        match
+          Operation_hash.Map.find old_oph filter_state.prechecked_manager_ops
+        with
+        | None -> assert false
+        | Some info -> info
+      in
+      if
+        better_fees_and_ratio
+          config
+          old_info.gas_limit
+          old_info.fee
+          gas_limit
+          fee
+      then
+        (* The new operation is better and replaces the old one from
+           the same manager. Note that there is no need to check the
+           number of prechecked operations in the mempool
+           here. Indeed, the removal of the old operation frees up a
+           spot in the mempool anyway. *)
+        let validation_state =
+          remove_from_validation_state validation_state old_info.manager_op
+        in
+        let* proto_validation_outcome2 =
+          proto_validate_manager_operation
+            validation_state
+            oph
+            ~nb_successful_prechecks
+            operation
+        in
+        match proto_validation_outcome2 with
+        | `Success validation_state ->
+            let replace_err =
+              Environment.wrap_tzerror
+                (Manager_operation_replaced {old_hash = old_oph; new_hash = oph})
+            in
+            let replacement = `Replace (old_oph, `Outdated [replace_err]) in
+            return (validation_state, replacement)
+        | `Conflict (_oph, conflict_proto_error) ->
+            (* This should not happen: a manager operation should not
+               conflict with multiple operations. *)
+            fail conflict_proto_error
+      else
+        (* The new operation is not interesting enough so it is rejected. *)
+        let err = Manager_restriction {oph = old_oph; fee = old_info.fee} in
+        fail (`Branch_delayed [Environment.wrap_tzerror err])
+
+(** Remove a manager operation hash from the filter state.
+    Do nothing if the operation was not in the state. *)
+let remove ~filter_state oph =
+  match Operation_hash.Map.find oph filter_state.prechecked_manager_ops with
+  | None ->
+      (* Not present in the filter_state: nothing to do. *)
+      filter_state
+  | Some info ->
+      let prechecked_manager_ops =
+        Operation_hash.Map.remove oph filter_state.prechecked_manager_ops
+      in
+      let prechecked_manager_op_count =
+        filter_state.prechecked_manager_op_count - 1
+      in
+      let prechecked_op_weights =
+        ManagerOpWeightSet.remove
+          (mk_op_weight oph info)
+          filter_state.prechecked_op_weights
+      in
+      let min_prechecked_op_weight =
+        match filter_state.min_prechecked_op_weight with
+        | None -> None
+        | Some min_op_weight ->
+            if Operation_hash.equal min_op_weight.operation_hash oph then
+              ManagerOpWeightSet.min_elt prechecked_op_weights
+            else Some min_op_weight
+      in
+      {
+        filter_state with
+        prechecked_manager_op_count;
+        prechecked_manager_ops;
+        prechecked_op_weights;
+        min_prechecked_op_weight;
+      }
 
-let add_manager_restriction filter_state oph info source replacement =
+(** Add a manager operation hash and information to the filter state.
+    Do nothing if the operation is already present in the state. *)
+let add_manager_op filter_state oph info replacement =
   let filter_state =
     match replacement with
     | `No_replace -> filter_state
-    | `Replace (oph, _class) -> remove ~filter_state oph
+    | `Replace (oph, _classification) -> remove ~filter_state oph
   in
-  let prechecked_operations_count =
-    if Operation_hash.Map.mem oph filter_state.operation_hash_to_manager then
-      filter_state.prechecked_operations_count
-    else filter_state.prechecked_operations_count + 1
+  if Operation_hash.Map.mem oph filter_state.prechecked_manager_ops then
+    (* Already present in the filter_state: nothing to do. *)
+    filter_state
+  else
+    let prechecked_manager_op_count =
+      filter_state.prechecked_manager_op_count + 1
+    in
+    let prechecked_manager_ops =
+      Operation_hash.Map.add oph info filter_state.prechecked_manager_ops
+    in
+    let op_weight = mk_op_weight oph info in
+    let prechecked_op_weights =
+      ManagerOpWeightSet.add op_weight filter_state.prechecked_op_weights
+    in
+    let min_prechecked_op_weight =
+      match filter_state.min_prechecked_op_weight with
+      | Some old_min when compare_manager_op_weight old_min op_weight <= 0 ->
+          Some old_min
+      | _ -> Some op_weight
+    in
+    {
+      filter_state with
+      prechecked_manager_op_count;
+      prechecked_manager_ops;
+      prechecked_op_weights;
+      min_prechecked_op_weight;
+    }
+
+(** Call {!validate_manager_operation_and_handle_conflicts} then
+    update the [filter_state] by adding the newly validated operation,
+    and removing the replaced one one when applicable.
+
+    Return either the updated [filter_state], updated
+    [validation_state], and operation replacement, or an error
+    containing the appropriate classification. *)
+let precheck_manager_result config filter_state validation_state oph
+    ~nb_successful_prechecks (operation : 'manager_kind Kind.manager operation)
+    :
+    ( state
+      * validation_state
+      * [`No_replace | `Replace of Operation_hash.t * error_classification],
+      error_classification )
+    result
+    Lwt.t =
+  let open Lwt_result_syntax in
+  let*? fee, gas_limit =
+    Result.map_error
+      (fun err -> `Refused (Environment.wrap_tztrace err))
+      (get_manager_operation_gas_and_fee operation.protocol_data.contents)
   in
-  let op_weight = op_weight_of_info info in
-  let min_prechecked_op_weight =
-    match filter_state.min_prechecked_op_weight with
-    | Some mini when Q.(mini.weight < info.weight) -> Some mini
-    | Some _ | None -> Some op_weight
+  let* validation_state, replacement =
+    validate_manager_operation_and_handle_conflicts
+      config
+      filter_state
+      validation_state
+      oph
+      ~nb_successful_prechecks
+      fee
+      gas_limit
+      operation
   in
-  {
-    filter_state with
-    op_prechecked_managers =
-      (* Manager not seen yet, record it for next ops *)
-      Signature.Public_key_hash.Map.add
-        source
-        info
-        filter_state.op_prechecked_managers;
-    operation_hash_to_manager =
-      Operation_hash.Map.add oph source filter_state.operation_hash_to_manager
-      (* Record which manager is used for the operation hash. *);
-    ops_prechecked =
-      ManagerOpWeightSet.add op_weight filter_state.ops_prechecked;
-    prechecked_operations_count;
-    min_prechecked_op_weight;
-  }
+  let weight =
+    weight_manager_operation
+      ~validation_state
+      ~fee
+      ~gas:gas_limit
+      (Operation_data operation.protocol_data)
+  in
+  let info = {manager_op = Manager_op operation; gas_limit; fee; weight} in
+  let filter_state = add_manager_op filter_state oph info replacement in
+  return (filter_state, validation_state, replacement)
+
+(** Call {!precheck_manager_result} then convert its error monad
+    result into the appropriate return type for [precheck]. *)
+let precheck_manager config filter_state validation_state oph
+    ~nb_successful_prechecks operation :
+    [> `Passed_precheck of
+       state
+       * validation_state
+       * [`No_replace | `Replace of Operation_hash.t * error_classification]
+    | error_classification ]
+    Lwt.t =
+  precheck_manager_result
+    config
+    filter_state
+    validation_state
+    oph
+    ~nb_successful_prechecks
+    operation
+  >>= function
+  | Ok (filter_state, validation_state, replacement) ->
+      Lwt.return
+        (`Passed_precheck (filter_state, validation_state, replacement))
+  | Error
+      ((`Refused _ | `Branch_delayed _ | `Branch_refused _ | `Outdated _) as
+      err) ->
+      Lwt.return err
 
 let precheck :
     config ->
@@ -1086,9 +1223,11 @@ let precheck :
     Main.operation ->
     nb_successful_prechecks:int ->
     [ `Passed_precheck of
-      state * [`No_replace | `Replace of Operation_hash.t * error_classification]
-    | error_classification
-    | `Undecided ]
+      state
+      * validation_state
+      * [`No_replace | `Replace of Operation_hash.t * error_classification]
+    | `Undecided
+    | error_classification ]
     Lwt.t =
  fun config
      ~filter_state
@@ -1096,45 +1235,18 @@ let precheck :
      oph
      {shell = shell_header; protocol_data = Operation_data protocol_data}
      ~nb_successful_prechecks ->
-  let precheck_manager protocol_data source op =
-    match get_manager_operation_gas_and_fee op with
-    | Error err -> Lwt.return (`Refused (Environment.wrap_tztrace err))
-    | Ok (fee, gas_limit) -> (
-        let weight =
-          weight_manager_operation
-            ~validation_state
-            ~fee
-            ~gas:gas_limit
-            (Operation_data protocol_data)
-        in
-        let gas_limit = Gas.Arith.fp gas_limit in
-        let info = {operation_hash = oph; gas_limit; fee; weight} in
-        precheck_manager
-          config
-          filter_state
-          validation_state
-          oph
-          shell_header
-          protocol_data
-          source
-          ~nb_successful_prechecks
-          ~fee
-          ~gas_limit
-        >|= function
-        | `Prechecked_manager replacement ->
-            let filter_state =
-              add_manager_restriction filter_state oph info source replacement
-            in
-            `Passed_precheck (filter_state, replacement)
-        | (`Refused _ | `Branch_delayed _ | `Branch_refused _ | `Outdated _) as
-          errs ->
-            errs)
+  let call_precheck_manager (protocol_data : _ Kind.manager protocol_data) =
+    precheck_manager
+      config
+      filter_state
+      validation_state
+      oph
+      ~nb_successful_prechecks
+      {shell = shell_header; protocol_data}
   in
   match protocol_data.contents with
-  | Single (Manager_operation {source; _}) as op ->
-      precheck_manager protocol_data source op
-  | Cons (Manager_operation {source; _}, _) as op ->
-      precheck_manager protocol_data source op
+  | Single (Manager_operation _) -> call_precheck_manager protocol_data
+  | Cons (Manager_operation _, _) -> call_precheck_manager protocol_data
   | Single _ -> Lwt.return `Undecided
 
 open Apply_results
-- 
GitLab


From 36a3493e37557d21dcafaad10a3e7e38621154b1 Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Fri, 12 Aug 2022 18:29:30 +0200
Subject: [PATCH 3/7] Plugin: precheck all operations using validate_operation

---
 src/proto_alpha/lib_plugin/mempool.ml | 40 ++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/src/proto_alpha/lib_plugin/mempool.ml b/src/proto_alpha/lib_plugin/mempool.ml
index f1d7b75dd5b9..c555a7e47baa 100644
--- a/src/proto_alpha/lib_plugin/mempool.ml
+++ b/src/proto_alpha/lib_plugin/mempool.ml
@@ -1215,6 +1215,38 @@ let precheck_manager config filter_state validation_state oph
       err) ->
       Lwt.return err
 
+(** Call the protocol's {!Validate_operation.validate_operation}. If
+    successful, return the updated [validation_state], the unchanged
+    [filter_state], and no operation replacement. Otherwise, return the
+    classification associated with the protocol error. Note that when
+    there is a conflict with a previously validated operation, the new
+    operation is always discarded. As it does not allow for any fee
+    market, this function is designed for non-manager operations. *)
+let precheck_non_manager filter_state validation_state oph
+    ~nb_successful_prechecks operation =
+  proto_validate_operation
+    validation_state
+    oph
+    ~nb_successful_prechecks
+    operation
+  >>= function
+  | Ok validation_state ->
+      Lwt.return
+        (`Passed_precheck (filter_state, validation_state, `No_replace))
+  | Error
+      ( _err,
+        ((`Refused _ | `Branch_delayed _ | `Branch_refused _ | `Outdated _) as
+        error_classification) ) ->
+      Lwt.return error_classification
+
+(* Now that [precheck] uses {!Validate_operation.validate_operation}
+   for every kind of operation, it must never return
+   [`Undecided]. Indeed, this would cause the prevalidator to call
+   {!Apply.apply_operation}, which relies on updates to the alpha
+   context to detect incompatible operations, whereas
+   [validate_operation] only updates the
+   {!Validate_operation.validate_operation_state}. Therefore, it would
+   be possible for the mempool to accept conflicting operations. *)
 let precheck :
     config ->
     filter_state:state ->
@@ -1247,7 +1279,13 @@ let precheck :
   match protocol_data.contents with
   | Single (Manager_operation _) -> call_precheck_manager protocol_data
   | Cons (Manager_operation _, _) -> call_precheck_manager protocol_data
-  | Single _ -> Lwt.return `Undecided
+  | Single _ ->
+      precheck_non_manager
+        filter_state
+        validation_state
+        oph
+        ~nb_successful_prechecks
+        {shell = shell_header; protocol_data}
 
 open Apply_results
 
-- 
GitLab


From 37d029eb0f33ad20623ad9a4ffdc98ef021a1463 Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Fri, 12 Aug 2022 18:34:21 +0200
Subject: [PATCH 4/7] Proto: remove obsolete TMP_for_plugin and
 precheck_manager

Fixes #3245
---
 src/proto_alpha/lib_protocol/main.ml          |  8 ----
 src/proto_alpha/lib_protocol/main.mli         |  8 ----
 .../lib_protocol/validate_operation.ml        | 21 ---------
 .../lib_protocol/validate_operation.mli       | 45 -------------------
 4 files changed, 82 deletions(-)

diff --git a/src/proto_alpha/lib_protocol/main.ml b/src/proto_alpha/lib_protocol/main.ml
index c18b87c7e9e6..d282f9701fd6 100644
--- a/src/proto_alpha/lib_protocol/main.ml
+++ b/src/proto_alpha/lib_protocol/main.ml
@@ -758,12 +758,4 @@ let value_of_key ~chain_id:_ ~predecessor_context:ctxt ~predecessor_timestamp
   Alpha_context.prepare ctxt ~level ~predecessor_timestamp ~timestamp
   >>=? fun (ctxt, _, _) -> return (Apply.value_of_key ctxt)
 
-let precheck_manager {validate_operation_info; validate_operation_state; _}
-    contents_list should_check_signature =
-  Validate_operation.TMP_for_plugin.precheck_manager
-    validate_operation_info
-    validate_operation_state
-    contents_list
-    should_check_signature
-
 (* Vanity nonce: TBD *)
diff --git a/src/proto_alpha/lib_protocol/main.mli b/src/proto_alpha/lib_protocol/main.mli
index bcaedc6771a2..007a5ef25803 100644
--- a/src/proto_alpha/lib_protocol/main.mli
+++ b/src/proto_alpha/lib_protocol/main.mli
@@ -118,14 +118,6 @@ type operation = Alpha_context.packed_operation = {
   protocol_data : operation_data;
 }
 
-(** See {!Validate_operation.precheck_manager}. *)
-val precheck_manager :
-  validation_state ->
-  'a Alpha_context.Kind.manager Alpha_context.contents_list ->
-  'a Alpha_context.Kind.manager
-  Validate_operation.TMP_for_plugin.should_check_signature ->
-  Validate_operation.stamp tzresult Lwt.t
-
 include
   Updater.PROTOCOL
     with type block_header_data = Alpha_context.Block_header.protocol_data
diff --git a/src/proto_alpha/lib_protocol/validate_operation.ml b/src/proto_alpha/lib_protocol/validate_operation.ml
index da87c0b27e09..94d3b45d03da 100644
--- a/src/proto_alpha/lib_protocol/validate_operation.ml
+++ b/src/proto_alpha/lib_protocol/validate_operation.ml
@@ -2057,24 +2057,3 @@ let validate_operation (vi : validate_operation_info)
 (* This function will be replaced with a generic remove_operation in
    the future. *)
 let remove_manager_operation = Manager.remove_manager_operation
-
-module TMP_for_plugin = struct
-  type 'a should_check_signature =
-    | Check_signature of 'a operation
-    | Skip_signature_check
-
-  let precheck_manager vi vs contents_list should_check_signature =
-    let open Lwt_result_syntax in
-    let open Manager in
-    let* batch_state, source_pk =
-      check_sanity_and_find_public_key vi vs contents_list
-    in
-    let* _batch_state = validate_contents_list vi batch_state contents_list in
-    let*? () =
-      match should_check_signature with
-      | Check_signature operation ->
-          Operation.check_signature source_pk vi.chain_id operation
-      | Skip_signature_check -> ok ()
-    in
-    return Operation_validated_stamp
-end
diff --git a/src/proto_alpha/lib_protocol/validate_operation.mli b/src/proto_alpha/lib_protocol/validate_operation.mli
index b0c3c9bc125f..b67b913eb5b0 100644
--- a/src/proto_alpha/lib_protocol/validate_operation.mli
+++ b/src/proto_alpha/lib_protocol/validate_operation.mli
@@ -184,48 +184,3 @@ val remove_manager_operation :
   validate_operation_state ->
   'a Kind.manager operation ->
   validate_operation_state
-
-(** Functions for the plugin.
-
-    These functions are temporary.
-
-    TODO: https://gitlab.com/tezos/tezos/-/issues/3245
-    Update the plugin to call directly {!validate_operation} then
-    remove these functions. *)
-module TMP_for_plugin : sig
-  (** Indicate whether the signature should be checked in
-      {!precheck_manager}; if so, provide the raw operation.
-
-      We could have used an [option], but this makes calls to
-      {!precheck_manager} more readable. *)
-  type 'a should_check_signature =
-    | Check_signature of 'a operation
-    | Skip_signature_check
-
-  (** Similar to {!validate_operation}, but do not check the
-      one-operation-per-manager-per-block restriction (1M).
-
-      Indeed, 1M is already handled by the plugin. This function is
-      purposefully close to the former
-      [Apply.precheck_manager_contents_list], so that few changes are
-      needed in the plugin.
-
-      The signature is only checked if the [should_check_signature]
-      argument is [Check_signature _].
-
-      The {!validate_operation_state} does not need to be updated
-      because:
-
-      + 1M is not handled here anyway.
-
-      + In mempool mode, the block gas limit is not tracked.
-
-      This function is called by {!Main.precheck_manager}, which is
-      called in [lib_plugin/mempool.ml]. *)
-  val precheck_manager :
-    validate_operation_info ->
-    validate_operation_state ->
-    'a Kind.manager contents_list ->
-    'a Kind.manager should_check_signature ->
-    stamp tzresult Lwt.t
-end
-- 
GitLab


From 995112f4d17b76526561b8665b4c790a2ff0c8ac Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Tue, 30 Aug 2022 19:07:05 +0200
Subject: [PATCH 5/7] Plugin/test: update helpers and flush test

---
 src/proto_alpha/lib_plugin/mempool.ml         |   2 +-
 src/proto_alpha/lib_plugin/test/generators.ml | 110 +++++++++---------
 .../lib_plugin/test/test_plugin.ml            |   6 +-
 src/proto_alpha/lib_plugin/test/test_utils.ml | 107 +++++++----------
 4 files changed, 101 insertions(+), 124 deletions(-)

diff --git a/src/proto_alpha/lib_plugin/mempool.ml b/src/proto_alpha/lib_plugin/mempool.ml
index c555a7e47baa..18c6543f8b0e 100644
--- a/src/proto_alpha/lib_plugin/mempool.ml
+++ b/src/proto_alpha/lib_plugin/mempool.ml
@@ -284,7 +284,7 @@ let other_prio = `Medium
 
 let on_flush config filter_state ?(validation_state : validation_state option)
     ~predecessor () =
-  ignore filter_state ;
+  ignore (filter_state : state) ;
   init config ?validation_state ~predecessor ()
 
 let get_manager_operation_gas_and_fee contents =
diff --git a/src/proto_alpha/lib_plugin/test/generators.ml b/src/proto_alpha/lib_plugin/test/generators.ml
index 38d6e4e13509..ead5883bf06d 100644
--- a/src/proto_alpha/lib_plugin/test/generators.ml
+++ b/src/proto_alpha/lib_plugin/test/generators.ml
@@ -40,75 +40,79 @@ let operation_hash_gen : Operation_hash.t QCheck2.Gen.t =
   let+ s = QCheck2.Gen.string_size (return 32) in
   Operation_hash.of_string_exn s
 
-let dummy_manager_op_info oph =
-  {
-    Plugin.Mempool.operation_hash = oph;
-    gas_limit = Alpha_context.Gas.Arith.zero;
-    fee = Alpha_context.Tez.zero;
-    weight = Q.zero;
-  }
+let dummy_manager_op_info =
+  let fee = Alpha_context.Tez.zero in
+  let gas_limit = Alpha_context.Gas.Arith.zero in
+  let manager_op =
+    let open Alpha_context in
+    let source = Signature.Public_key_hash.zero in
+    let counter = Z.zero in
+    let storage_limit = Z.zero in
+    let operation = Set_deposits_limit None in
+    let contents =
+      Manager_operation
+        {source; fee; counter; operation; gas_limit; storage_limit}
+    in
+    let contents = Single contents in
+    let protocol_data = {contents; signature = Some Signature.zero} in
+    let branch = Block_hash.zero in
+    Mempool.Manager_op {shell = {branch}; protocol_data}
+  in
+  Mempool.{manager_op; fee; gas_limit; weight = Q.zero}
 
-let dummy_manager_op_info_with_key_gen :
-    (Plugin.Mempool.manager_op_info * Signature.public_key_hash) QCheck2.Gen.t =
+let oph_and_info_gen =
   let open QCheck2.Gen in
-  let+ oph, (pkh, _, _) = pair operation_hash_gen public_key_hash_gen in
-  (dummy_manager_op_info oph, pkh)
+  let+ oph = operation_hash_gen in
+  (oph, dummy_manager_op_info)
 
 let filter_state_gen : Plugin.Mempool.state QCheck2.Gen.t =
   let open QCheck2.Gen in
   let open Plugin.Mempool in
-  let+ inputs = small_list (pair operation_hash_gen public_key_hash_gen) in
+  let+ ops = small_list oph_and_info_gen in
   List.fold_left
-    (fun state (oph, (pkh, _, _)) ->
-      match Operation_hash.Map.find oph state.operation_hash_to_manager with
-      | Some _ -> state
-      | None ->
-          let info = dummy_manager_op_info oph in
-          let prechecked_operations_count =
-            if Operation_hash.Map.mem oph state.operation_hash_to_manager then
-              state.prechecked_operations_count
-            else state.prechecked_operations_count + 1
-          in
-          let op_weight = op_weight_of_info info in
-          let min_prechecked_op_weight =
-            match state.min_prechecked_op_weight with
-            | Some mini when Q.(mini.weight < info.weight) -> Some mini
-            | Some _ | None -> Some op_weight
-          in
-          {
-            state with
-            op_prechecked_managers =
-              Signature.Public_key_hash.Map.add
-                pkh
-                info
-                state.op_prechecked_managers;
-            operation_hash_to_manager =
-              Operation_hash.Map.add oph pkh state.operation_hash_to_manager;
-            ops_prechecked =
-              ManagerOpWeightSet.add op_weight state.ops_prechecked;
-            prechecked_operations_count;
-            min_prechecked_op_weight;
-          })
+    (fun state (oph, info) ->
+      if Operation_hash.Map.mem oph state.prechecked_manager_ops then state
+      else
+        let prechecked_manager_op_count =
+          state.prechecked_manager_op_count + 1
+        in
+        let op_weight = mk_op_weight oph info in
+        let min_prechecked_op_weight =
+          match state.min_prechecked_op_weight with
+          | Some old_min
+            when Mempool.compare_manager_op_weight old_min op_weight <= 0 ->
+              Some old_min
+          | Some _ | None -> Some op_weight
+        in
+        {
+          state with
+          prechecked_manager_op_count;
+          prechecked_manager_ops =
+            Operation_hash.Map.add oph info state.prechecked_manager_ops;
+          prechecked_op_weights =
+            ManagerOpWeightSet.add op_weight state.prechecked_op_weights;
+          min_prechecked_op_weight;
+        })
     Plugin.Mempool.empty
-    inputs
+    ops
 
+(** Generate a pair of operation hash and manager_op_info, that has
+    even odds of belonging to the given filter_state or being fresh. *)
 let with_filter_state_operation_gen :
     Plugin.Mempool.state ->
-    (Plugin.Mempool.manager_op_info * Signature.public_key_hash) QCheck2.Gen.t =
+    (Operation_hash.t * Plugin.Mempool.manager_op_info) QCheck2.Gen.t =
  fun state ->
   let open QCheck2.Gen in
   let* use_fresh = bool in
-  let to_ops map =
-    Operation_hash.Map.bindings map
-    |> List.map (fun (oph, pkh) -> (dummy_manager_op_info oph, pkh))
-  in
-  if use_fresh || Operation_hash.Map.is_empty state.operation_hash_to_manager
-  then dummy_manager_op_info_with_key_gen
-  else oneofl (to_ops state.operation_hash_to_manager)
+  if use_fresh || Operation_hash.Map.is_empty state.prechecked_manager_ops then
+    oph_and_info_gen
+  else oneofl (Operation_hash.Map.bindings state.prechecked_manager_ops)
 
+(** Generate both a filter_state, and a pair of operation hash and
+    manager_op_info. The pair has even odds of belonging to the
+    filter_state or being fresh. *)
 let filter_state_with_operation_gen :
-    (Plugin.Mempool.state
-    * (Plugin.Mempool.manager_op_info * Signature.public_key_hash))
+    (Plugin.Mempool.state * (Operation_hash.t * Plugin.Mempool.manager_op_info))
     QCheck2.Gen.t =
   let open QCheck2.Gen in
   filter_state_gen >>= fun state ->
diff --git a/src/proto_alpha/lib_plugin/test/test_plugin.ml b/src/proto_alpha/lib_plugin/test/test_plugin.ml
index 907ef219418b..9a2251f2ecfa 100644
--- a/src/proto_alpha/lib_plugin/test/test_plugin.ml
+++ b/src/proto_alpha/lib_plugin/test/test_plugin.ml
@@ -47,11 +47,9 @@ let test_flush () =
          QCheck2.Gen.bool)
   in
   List.iter_es
-    (fun ( (filter_state, ((op_info : manager_op_info), pkh)),
+    (fun ( (filter_state, (oph, (op_info : manager_op_info))),
            with_validation_state ) ->
-      let filter_state =
-        add_manager_restriction filter_state op_info.operation_hash op_info pkh
-      in
+      let filter_state = add_manager_op filter_state oph op_info `No_replace in
       Context.init1 () >>= function
       | Error e ->
           failwith "Error at context initialisation: %a" pp_print_trace e
diff --git a/src/proto_alpha/lib_plugin/test/test_utils.ml b/src/proto_alpha/lib_plugin/test/test_utils.ml
index cf25d367381e..57402233a49e 100644
--- a/src/proto_alpha/lib_plugin/test/test_utils.ml
+++ b/src/proto_alpha/lib_plugin/test/test_utils.ml
@@ -36,48 +36,34 @@ let config drift_opt =
     replace_by_fee_factor = Q.make (Z.of_int 105) (Z.of_int 100);
   }
 
-let pp_prechecked_managers fmt set =
+let pp_prechecked_manager_ops fmt set =
   Format.fprintf
     fmt
     "[%a]"
-    (Format.pp_print_list (fun ppf (pkh, (op_info : manager_op_info)) ->
+    (Format.pp_print_list (fun ppf (oph, (op_info : manager_op_info)) ->
          Format.fprintf
            ppf
-           "(%a -> (hash:%a,gas:%a,fee:%a))"
-           Signature.Public_key_hash.pp
-           pkh
+           "(%a -> {fee: %a; gas: %a; weight: %a})"
            Operation_hash.pp
-           op_info.operation_hash
+           oph
+           Alpha_context.Tez.pp
+           op_info.fee
            Alpha_context.Gas.Arith.pp
            op_info.gas_limit
-           Alpha_context.Tez.pp
-           op_info.fee))
-    (Signature.Public_key_hash.Map.bindings set)
-
-let pp_operation_hash_manager fmt map =
-  Format.fprintf
-    fmt
-    "[%a]"
-    (Format.pp_print_list (fun ppf (hash, pkh) ->
-         Format.fprintf
-           ppf
-           "(%a -> %a)"
-           Operation_hash.pp
-           hash
-           Signature.Public_key_hash.pp
-           pkh))
-    (Operation_hash.Map.bindings map)
+           Q.pp_print
+           op_info.weight))
+    (Operation_hash.Map.bindings set)
 
 let pp_manager_op_weight fmt weight =
   Format.fprintf
     fmt
-    "(oph: %a;weight: %a)"
+    "{oph: %a; weight: %a}"
     Operation_hash.pp
     weight.operation_hash
     Q.pp_print
     weight.weight
 
-let pp_ops_prechecked fmt set =
+let pp_prechecked_op_weights fmt set =
   Format.fprintf
     fmt
     "[%a]"
@@ -85,57 +71,46 @@ let pp_ops_prechecked fmt set =
          pp_manager_op_weight ppf manager_op_weight))
     (ManagerOpWeightSet.elements set)
 
-let pp_min_prechecked_op_weight fmt weight =
-  Option.fold
-    ~none:(Format.fprintf fmt "None")
-    ~some:(fun w -> Format.fprintf fmt "Some %a" pp_manager_op_weight w)
-    weight
+let pp_op_weight_opt fmt = function
+  | None -> Format.fprintf fmt "None"
+  | Some op_weight ->
+      Format.fprintf fmt "Some %a" pp_manager_op_weight op_weight
 
 let pp_state fmt state =
   Format.fprintf
     fmt
     "@[<v 0>state:@,\
      {@,\
-     prechecked_managers:%a;@,\
-     operation_hash_to_managers:%a;@,\
-     prechecked_operations_count:%d@,\
-     ops_prechecked:%a;@,\
-     min_prechecked_op_weight:%a@,\
+     prechecked_manager_op_count: %d@,\
+     prechecked_manager_ops: %a;@,\
+     prechecked_op_weights: %a;@,\
+     min_prechecked_op_weight: %a@,\
      }@]"
-    pp_prechecked_managers
-    state.op_prechecked_managers
-    pp_operation_hash_manager
-    state.operation_hash_to_manager
-    state.prechecked_operations_count
-    pp_ops_prechecked
-    state.ops_prechecked
-    pp_min_prechecked_op_weight
+    state.prechecked_manager_op_count
+    pp_prechecked_manager_ops
+    state.prechecked_manager_ops
+    pp_prechecked_op_weights
+    state.prechecked_op_weights
+    pp_op_weight_opt
     state.min_prechecked_op_weight
 
-let eq_prechecked_managers =
-  Signature.Public_key_hash.Map.equal
+let eq_prechecked_manager_ops =
+  Operation_hash.Map.equal
     (fun
-      ({operation_hash = oph1; gas_limit = _; fee = _; weight = _} :
-        manager_op_info)
-      ({operation_hash = oph2; gas_limit = _; fee = _; weight = _} :
-        manager_op_info)
-    -> Operation_hash.equal oph1 oph2)
+      {manager_op = _; fee = fee1; gas_limit = gas1; weight = w1}
+      {manager_op = _; fee = fee2; gas_limit = gas2; weight = w2}
+    -> Tez.equal fee1 fee2 && Gas.Arith.equal gas1 gas2 && Q.equal w1 w2)
+
+let eq_op_weight_opt =
+  Option.equal (fun op_weight1 op_weight2 ->
+      Operation_hash.equal op_weight1.operation_hash op_weight2.operation_hash
+      && Q.equal op_weight1.weight op_weight2.weight)
 
 (* This function needs to be updated if the filter state is extended *)
 let eq_state s1 s2 =
-  let eq_min_prechecked_op_weight =
-    match (s1.min_prechecked_op_weight, s2.min_prechecked_op_weight) with
-    | None, None -> true
-    | Some _, None | None, Some _ -> false
-    | Some w1, Some w2 ->
-        Operation_hash.equal w1.operation_hash w2.operation_hash
-        && Q.equal w1.weight w2.weight
-  in
-  eq_prechecked_managers s1.op_prechecked_managers s2.op_prechecked_managers
-  && Operation_hash.Map.equal
-       (fun k1 k2 -> Signature.Public_key_hash.equal k1 k2)
-       s1.operation_hash_to_manager
-       s2.operation_hash_to_manager
-  && s1.prechecked_operations_count = s2.prechecked_operations_count
-  && ManagerOpWeightSet.equal s1.ops_prechecked s2.ops_prechecked
-  && eq_min_prechecked_op_weight
+  s1.prechecked_manager_op_count = s2.prechecked_manager_op_count
+  && eq_prechecked_manager_ops
+       s1.prechecked_manager_ops
+       s2.prechecked_manager_ops
+  && ManagerOpWeightSet.equal s1.prechecked_op_weights s2.prechecked_op_weights
+  && eq_op_weight_opt s1.min_prechecked_op_weight s2.min_prechecked_op_weight
-- 
GitLab


From 322c73366a9198d5bd6be08e91b729c775761821 Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Wed, 31 Aug 2022 17:57:02 +0200
Subject: [PATCH 6/7] Plugin/test: update filter state tests

Notably, remove plugin tests on 1M, since the plugin is no longer
responsible for enforcing 1M. Similar tests on 1M already exist
in tezt, and will be extended in the next commit to cover all
the cases of the removed tests.
---
 src/proto_alpha/lib_plugin/test/generators.ml |  15 +
 .../lib_plugin/test/test_filter_state.ml      | 445 +++++-------------
 2 files changed, 143 insertions(+), 317 deletions(-)

diff --git a/src/proto_alpha/lib_plugin/test/generators.ml b/src/proto_alpha/lib_plugin/test/generators.ml
index ead5883bf06d..e1309201f541 100644
--- a/src/proto_alpha/lib_plugin/test/generators.ml
+++ b/src/proto_alpha/lib_plugin/test/generators.ml
@@ -117,3 +117,18 @@ let filter_state_with_operation_gen :
   let open QCheck2.Gen in
   filter_state_gen >>= fun state ->
   pair (return state) (with_filter_state_operation_gen state)
+
+(** Generate a filter_state, and two pairs of operation hash and
+    manager_op_info. The pairs have indepedent, even odds of belonging
+    to the filter_state or being fresh. *)
+let filter_state_with_two_operations_gen :
+    (Plugin.Mempool.state
+    * (Operation_hash.t * Plugin.Mempool.manager_op_info)
+    * (Operation_hash.t * Plugin.Mempool.manager_op_info))
+    QCheck2.Gen.t =
+  let open QCheck2.Gen in
+  let* filter_state = filter_state_gen in
+  triple
+    (return filter_state)
+    (with_filter_state_operation_gen filter_state)
+    (with_filter_state_operation_gen filter_state)
diff --git a/src/proto_alpha/lib_plugin/test/test_filter_state.ml b/src/proto_alpha/lib_plugin/test/test_filter_state.ml
index 24c8b7a86720..032e46ae902f 100644
--- a/src/proto_alpha/lib_plugin/test/test_filter_state.ml
+++ b/src/proto_alpha/lib_plugin/test/test_filter_state.ml
@@ -27,7 +27,7 @@
     -------
     Component:    Shell (Plugin)
     Invocation:   dune exec src/proto_alpha/lib_plugin/test/test_filter_state.exe
-    Subject:      Unit tests the filter tests function of the plugin
+    Subject:      Unit tests the filter state functions of the plugin
 *)
 
 open Lib_test.Qcheck2_helpers
@@ -36,346 +36,157 @@ open Test_utils
 
 let count = 1000
 
-(** Test that [check_manager_restriction] returns [Fresh] when we use an unknown
-    [pkh] in the filter state. *)
-let test_check_manager_restriction_fresh =
-  let open QCheck2 in
-  Test.make
-    ~count
-    ~name:
-      "[check_manager_restriction empty (remove pkh filter) pkh] returns \
-       [Fresh]"
-    Generators.filter_state_with_operation_gen
-    (fun (filter_state, (_oph, pkh)) ->
-      let config = config None in
-      let filter_state =
-        {
-          filter_state with
-          op_prechecked_managers =
-            Signature.Public_key_hash.Map.remove
-              pkh
-              filter_state.op_prechecked_managers;
-        }
-      in
-      match
-        check_manager_restriction
-          config
-          filter_state
-          pkh
-          ~fee:Alpha_context.Tez.zero
-          ~gas_limit:Alpha_context.Gas.Arith.zero
-      with
-      | `Fresh -> true
-      | `Replace _ | `Fail (`Branch_delayed _) ->
-          QCheck2.Test.fail_reportf
-            "@[<v 2>Check manager restriction failed!@,\
-             %a should not be in the set of precheck managers:@,\
-             %a@]"
-            Signature.Public_key_hash.pp
-            pkh
-            pp_state
-            filter_state)
-
-(** Test that [check_manager_restriction] returns [Fail (`Branch_delayed _)]
-    when we use a known [pkh] that is associated with an operation having higher
-    fees. *)
-let test_check_manager_restriction_fail =
-  let open QCheck2 in
-  Test.make
-    ~count
-    ~name:
-      "[check_manager_restriction {pkh} pkh fee gas_limit] returns [`Fail \
-       `Branch_delayed _] if [fee] < [old_op_fee]"
-    Generators.filter_state_with_operation_gen
-    (fun (filter_state, (op_info, pkh)) ->
-      let config = config None in
-      let filter_state =
-        {
-          filter_state with
-          op_prechecked_managers =
-            Signature.Public_key_hash.Map.add
-              pkh
-              {op_info with fee = Alpha_context.Tez.one}
-              (* We force higher fee than below: [one > zero]. *)
-              filter_state.op_prechecked_managers;
-        }
-      in
-      match
-        check_manager_restriction
-          config
-          filter_state
-          pkh
-          ~fee:Alpha_context.Tez.zero
-          ~gas_limit:Alpha_context.Gas.Arith.zero
-      with
-      | `Fail (`Branch_delayed _) -> true
-      | `Fresh ->
-          QCheck2.Test.fail_reportf
-            "@[<v 2>Check manager restriction failed!@,\
-             %a should be in the set of precheck managers:@,\
-             %a@]"
-            Signature.Public_key_hash.pp
-            pkh
-            pp_state
-            filter_state
-      | `Replace old_oph ->
-          QCheck2.Test.fail_reportf
-            "@[<v 2>Check manager restriction failed!@,\
-             %a is in the set of precheck managers:@,\
-             %a@,\
-             but should not replace the old operation:%a@]"
-            Signature.Public_key_hash.pp
-            pkh
-            pp_state
-            filter_state
-            Operation_hash.pp
-            old_oph)
-
-(** Test that [check_manager_restriction] returns [Replace] when we use a known
-    [pkh] already associated with an operation that has a lower fees. *)
-let test_check_manager_restriction_replace =
-  let open QCheck2 in
-  Test.make
-    ~count
-    ~name:
-      "[check_manager_restriction {pkh} pkh fee gas_limit] returns [`Replace \
-       _] if [fee] >= [old_op_fee]"
-    Generators.filter_state_with_operation_gen
-    (fun (filter_state, (op_info, pkh)) ->
-      let config = config None in
-      let fee = Alpha_context.Tez.zero in
-      let filter_state =
-        {
-          filter_state with
-          op_prechecked_managers =
-            Signature.Public_key_hash.Map.add
-              pkh
-              op_info
-              filter_state.op_prechecked_managers;
-        }
-      in
-      match
-        check_manager_restriction
-          config
-          filter_state
-          pkh
-          ~fee
-          ~gas_limit:Alpha_context.Gas.Arith.zero
-      with
-      | `Replace _ -> true
-      | `Fresh ->
-          QCheck2.Test.fail_reportf
-            "@[<v 2>Check manager restriction failed!@,\
-             %a should be in the set of precheck managers:@,\
-             %a@]"
-            Signature.Public_key_hash.pp
-            pkh
-            pp_state
-            filter_state
-      | `Fail (`Branch_delayed _) ->
-          QCheck2.Test.fail_reportf
-            "@[<v 2>Check manager restriction failed!@,\
-             %a is in the set of prechecked managers:@,\
-             %a but the old version should have been replaced because the new \
-             fees(%a) are higher@]"
-            Signature.Public_key_hash.pp
-            pkh
-            pp_state
-            filter_state
-            Alpha_context.Tez.pp
-            fee)
-
-(** Test that [add_manager_restriction] adds the [pkh] in the filter state. *)
-let test_add_manager_restriction_set =
-  let open QCheck2 in
-  Test.make
-    ~count
-    ~name:"[mem pkh (add_manager_restriction _ _ _ pkh).op_prechecked_managers]"
-    (Gen.triple
-       Generators.filter_state_with_operation_gen
-       Generators.operation_hash_gen
-       Gen.bool)
-    (fun ((filter_state, (op_info, pkh)), oph_to_replace, replace) ->
-      let replace =
-        if replace then `Replace (oph_to_replace, ()) else `No_replace
-      in
-      let filter_state =
-        add_manager_restriction
-          filter_state
-          op_info.operation_hash
-          op_info
-          pkh
-          replace
-      in
-      qcheck_cond
-        ~pp:(fun fmt set ->
-          Format.fprintf
-            fmt
-            "%a was not found in prechecked_managers : %a"
-            Signature.Public_key_hash.pp
-            pkh
-            pp_prechecked_managers
-            set)
-        ~cond:(Signature.Public_key_hash.Map.mem pkh)
-        filter_state.op_prechecked_managers
-        ())
-
-(** Test that [add_manager_restriction (`Replace oph)] remove the [oph] from the
-    filter state. *)
-let test_add_manager_restriction_replace =
-  let open QCheck2 in
-  Test.make
-    ~count
-    ~name:
-      "[mem oph (add_manager_restriction _ _ _ (`Replace \
-       oph).op_prechecked_managers]"
-    Generators.filter_state_with_operation_gen
-    (fun (filter_state, (op_info, pkh)) ->
-      match
-        Operation_hash.Map.choose filter_state.operation_hash_to_manager
-      with
-      | None -> true
-      | Some (oph, _) when Operation_hash.equal oph op_info.operation_hash ->
-          true
-      | Some (oph_to_replace, _) ->
-          let filter_state =
-            add_manager_restriction
-              filter_state
-              op_info.operation_hash
-              op_info
-              pkh
-              (`Replace (oph_to_replace, ()))
-          in
-          qcheck_cond
-            ~pp:(fun fmt map ->
-              Format.fprintf
-                fmt
-                "%a was not found in prechecked_managers : %a"
-                Operation_hash.pp
-                oph_to_replace
-                pp_operation_hash_manager
-                map)
-            ~cond:(fun map -> not (Operation_hash.Map.mem oph_to_replace map))
-            filter_state.operation_hash_to_manager
-            ())
+let check_filter_state_invariants filter_state =
+  qcheck_cond
+    ~pp:(fun fmt filter_state ->
+      Format.fprintf
+        fmt
+        "The following filter_state breaks invariants:@.%a"
+        pp_state
+        filter_state)
+    ~cond:(fun filter_state ->
+      filter_state.prechecked_manager_op_count
+      = Operation_hash.Map.cardinal filter_state.prechecked_manager_ops
+      && filter_state.prechecked_manager_op_count
+         = ManagerOpWeightSet.cardinal filter_state.prechecked_op_weights
+      && ManagerOpWeightSet.for_all
+           (fun {operation_hash; weight} ->
+             match
+               Operation_hash.Map.find
+                 operation_hash
+                 filter_state.prechecked_manager_ops
+             with
+             | None -> false
+             | Some info -> Q.equal weight info.weight)
+           filter_state.prechecked_op_weights
+      && eq_op_weight_opt
+           (ManagerOpWeightSet.min_elt filter_state.prechecked_op_weights)
+           filter_state.min_prechecked_op_weight)
+    filter_state
+    ()
 
-(** Test that [check_manager_restriction pkh] returns [Fail | Replace] on a
-    filter state returned by [add_manager_restriction pkh]. *)
-let test_add_manager_restriction_check =
+(** Test that [add_manager_op] adds the given operation to the filter
+    state, and removes the replaced operation if applicable. *)
+let test_add_manager_op =
   let open QCheck2 in
   Test.make
     ~count
-    ~name:
-      "[check_manager_restriction filter_state pkh] returns [`Fail \
-       (`Branch_delayed)_ | `Replace _]"
-    (Gen.triple
-       Generators.filter_state_with_operation_gen
-       Generators.operation_hash_gen
-       Gen.bool)
-    (fun ((filter_state, (op_info, pkh)), oph_to_replace, replace) ->
-      let config = config None in
-      let replace =
-        if replace then `Replace (oph_to_replace, ()) else `No_replace
+    ~name:"Add a manager operation"
+    (Gen.pair Generators.filter_state_with_two_operations_gen Gen.bool)
+    (fun ((filter_state, (oph, op_info), (oph_to_replace, _)), should_replace)
+    ->
+      (* Both [oph] and [oph_to_replace] have even odds of being
+         already present in [filter_state] or fresh. *)
+      let replacement =
+        if should_replace then (
+          assume (not (Operation_hash.equal oph_to_replace oph)) ;
+          `Replace (oph_to_replace, ()))
+        else `No_replace
       in
-      let filter_state =
-        add_manager_restriction
-          filter_state
-          op_info.operation_hash
-          op_info
-          pkh
-          replace
-      in
-      match
-        check_manager_restriction
-          config
-          filter_state
-          pkh
-          ~fee:Alpha_context.Tez.zero
-          ~gas_limit:Alpha_context.Gas.Arith.zero
-      with
-      | `Fresh ->
-          QCheck2.Test.fail_reportf
-            "@[<v 2>Check manager restriction failed!@,\
-             %a should be in the set of precheck managers:@,\
-             %a@]"
-            Signature.Public_key_hash.pp
-            pkh
-            pp_state
-            filter_state
-      | `Fail (`Branch_delayed _) | `Replace _ -> true)
+      let filter_state = add_manager_op filter_state oph op_info replacement in
+      check_filter_state_invariants filter_state
+      && qcheck_cond
+           ~pp:(fun fmt set ->
+             Format.fprintf
+               fmt
+               "%a was not found in prechecked_manager_ops: %a"
+               Operation_hash.pp
+               oph
+               pp_prechecked_manager_ops
+               set)
+           ~cond:(Operation_hash.Map.mem oph)
+           filter_state.prechecked_manager_ops
+           ()
+      &&
+      if should_replace then
+        qcheck_cond
+          ~pp:(fun fmt () ->
+            Format.fprintf
+              fmt
+              "%a should have been removed from prechecked_manager_ops."
+              Operation_hash.pp
+              oph_to_replace)
+          ~cond:(fun () ->
+            not
+              (Operation_hash.Map.mem
+                 oph_to_replace
+                 filter_state.prechecked_manager_ops))
+          ()
+          ()
+      else true)
 
-(** Test that [remove pkh] removes the key from the filter state returned
-    by [add_manager_restriction pkh]. *)
-let test_remove =
+(** Test that [remove] removes the operation from the filter state if
+    it was present, and that adding then removing is the same as doing
+    nothing but removing the replaced operation if there is one. *)
+let test_remove_present =
   let open QCheck2 in
   Test.make
     ~count
-    ~name:"[removing an existing operation hash works]"
+    ~name:"Remove an existing operation hash"
     (Gen.triple
        Generators.filter_state_with_operation_gen
-       Generators.operation_hash_gen
+       Generators.oph_and_info_gen
        Gen.bool)
-    (fun ((filter_state, (op_info, pkh)), oph_to_replace, replace) ->
-      let replace =
-        if replace then `Replace (oph_to_replace, ()) else `No_replace
+    (fun ((initial_state, (oph_to_replace, _)), (oph, op_info), should_replace)
+    ->
+      (* Add a fresh operation [oph] to the state. *)
+      assume
+        (not (Operation_hash.Map.mem oph initial_state.prechecked_manager_ops)) ;
+      let replacement =
+        if should_replace then `Replace (oph_to_replace, ()) else `No_replace
       in
-      let filter_state =
-        add_manager_restriction
-          filter_state
-          op_info.operation_hash
-          op_info
-          pkh
-          replace
+      let filter_state = add_manager_op initial_state oph op_info replacement in
+      (* Remove [oph] from the state, in which it was present. *)
+      let filter_state = remove ~filter_state oph in
+      let (_ : bool) =
+        (* Check that the state invariants are preserved and that
+           [oph] has been removed. *)
+        check_filter_state_invariants filter_state
+        && qcheck_cond
+             ~pp:(fun fmt () ->
+               Format.fprintf
+                 fmt
+                 "%a should have been removed from prechecked_manager_ops."
+                 Operation_hash.pp
+                 oph)
+             ~cond:(fun () ->
+               not
+                 (Operation_hash.Map.mem
+                    oph
+                    filter_state.prechecked_manager_ops))
+             ()
+             ()
       in
-      let actual_state = remove ~filter_state op_info.operation_hash in
-      let expected_prechecked_managers =
-        Signature.Public_key_hash.Map.remove
-          pkh
-          filter_state.op_prechecked_managers
+      (* Check that adding a fresh operation then removing it is the
+         same as doing nothing except removing any replaced operation. *)
+      let initial_state_without_replaced_op =
+        if should_replace then remove ~filter_state:initial_state oph_to_replace
+        else initial_state
       in
       qcheck_eq
-        ~pp:pp_prechecked_managers
-        ~eq:eq_prechecked_managers
-        expected_prechecked_managers
-        actual_state.op_prechecked_managers)
+        ~pp:pp_state
+        ~eq:eq_state
+        initial_state_without_replaced_op
+        filter_state)
 
-(** Test that [remove pkh] leaves the filter state intact if [pkh] is unknown. *)
+(** Test that [remove] leaves the filter state intact if the operation
+    hash is unknown. *)
 let test_remove_unknown =
   let open QCheck2 in
   Test.make
     ~count
-    ~name:"[removing an unknown operation hash leaves the filter state intact"
-    Generators.filter_state_with_operation_gen
-    (fun (filter_state, (op_info, _pkh)) ->
-      let filter_state =
-        {
-          filter_state with
-          operation_hash_to_manager =
-            Operation_hash.Map.remove
-              op_info.operation_hash
-              filter_state.operation_hash_to_manager;
-        }
-      in
-      let actual_state = remove ~filter_state op_info.operation_hash in
-      qcheck_eq ~pp:pp_state ~eq:eq_state filter_state actual_state)
+    ~name:"Remove an unknown operation hash"
+    (Gen.pair Generators.filter_state_gen Generators.operation_hash_gen)
+    (fun (initial_state, oph) ->
+      assume
+        (not (Operation_hash.Map.mem oph initial_state.prechecked_manager_ops)) ;
+      let filter_state = remove ~filter_state:initial_state oph in
+      qcheck_eq ~pp:pp_state ~eq:eq_state initial_state filter_state)
 
 let () =
   Alcotest.run
     "Filter_state"
     [
-      ( "check_manager_restriction",
-        qcheck_wrap
-          [
-            test_check_manager_restriction_fresh;
-            test_check_manager_restriction_replace;
-            test_check_manager_restriction_fail;
-          ] );
-      ( "add_manager_restriction",
-        qcheck_wrap
-          [
-            test_add_manager_restriction_set;
-            test_add_manager_restriction_check;
-            test_add_manager_restriction_replace;
-          ] );
-      ("remove", qcheck_wrap [test_remove; test_remove_unknown]);
+      ("add_manager_op", qcheck_wrap [test_add_manager_op]);
+      ("remove", qcheck_wrap [test_remove_present; test_remove_unknown]);
     ]
-- 
GitLab


From 247c3bc885f133cb1e7437295ce0edf922c4567c Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Thu, 1 Sep 2022 15:15:45 +0200
Subject: [PATCH 7/7] Tezt/prevalidator: extend 1M test

to cover cases that used to be tested in the plugin tests, which
have been removed by the previous commit.
---
 tezt/tests/prevalidator.ml | 107 +++++++++++++++++++++++++++++--------
 1 file changed, 84 insertions(+), 23 deletions(-)

diff --git a/tezt/tests/prevalidator.ml b/tezt/tests/prevalidator.ml
index 0a5b99358367..f433584fa541 100644
--- a/tezt/tests/prevalidator.ml
+++ b/tezt/tests/prevalidator.ml
@@ -820,14 +820,27 @@ module Revamped = struct
              oph3)) ;
     unit
 
-  (** This test checks that if we inject an operation from the same source,
-      and with the same counter, injection will fail if ~force is false
-      because the operation will not be applied/prechecked. *)
-  let one_operation_per_manager_per_block_inject_not_applied =
+  (** Test the one-operation-per-manager-per-block restriction (1M)
+      during the injection of operations in an isolated node.
+
+      Check that:
+
+      - operations from distinct managers are [applied] without issue;
+
+      - a second operation from the same manager with the same fee
+        cannot be injected at all with [~force:false];
+
+      - another operation from the same manager with the same fee,
+        injected with [~force:true], gets classified as [branch_delayed];
+
+      - another operation from the same manager with twice the fee
+        gets [applied] and causes the old operation to be reclassified as
+        [outdated]. *)
+  let one_operation_per_manager_per_block_inject_isolated_node =
     Protocol.register_test
       ~__FILE__
-      ~title:"Manager_restriction_inject_1M"
-      ~tags:["mempool"; "manager_restriction"; "inject"]
+      ~title:"Manager_restriction_inject_isolated_node"
+      ~tags:["mempool"; "manager_restriction"; "inject"; "isolated_node"]
     @@ fun protocol ->
     log_step 1 "Initialize a node and a client." ;
     let* _node, client =
@@ -838,41 +851,89 @@ module Revamped = struct
         ()
     in
 
-    log_step 2 "Inject a transfer with a correct counter." ;
+    let source1 = Constant.bootstrap1 in
+    let source2 = Constant.bootstrap2 in
+    let fee = 1_000 in
+    log_step
+      2
+      "Inject transfers from [source1] and [source2] with fee [%d] and correct \
+       counters. Check that both are applied (i.e. the manager restriction \
+       does not prevent similar operations from distinct managers)."
+      fee ;
     let* (`OpHash oph1) =
-      Operation.Manager.(inject [make @@ transfer ()] client)
+      Operation.Manager.(
+        inject
+          [make ~source:source1 ~fee @@ transfer ~dest:Constant.bootstrap3 ()]
+          client)
+    in
+    let* (`OpHash oph2) =
+      Operation.Manager.(
+        inject
+          [make ~source:source2 ~fee @@ transfer ~dest:Constant.bootstrap3 ()]
+          client)
     in
+    let* () = check_mempool ~applied:[oph1; oph2] client in
+
     log_step
       3
-      "Attempt to inject a transfer with a correct counter but different \
-       destination (~force: %b)."
-      false ;
-
+      "Inject another transfer from [source1] with the same fee and a correct \
+       counter (but a different destination so that it is not the same \
+       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 error =
       rex
         ~opts:[`Dotall]
         "Fatal error:\n  Command failed: Error while applying operation.*:"
     in
-    let* _ =
-      RPC.Client.call client @@ RPC.get_chain_mempool_pending_operations ()
-    in
     (* By putting the wrong signature, we also ensure that the
        signature is checked only after the 1M restriction check. *)
-    let* _oph2 =
+    let* (`OpHash _) =
       Operation.Manager.(
         inject
           ~error
-          ~signer:Constant.bootstrap2
-          [make @@ transfer ~dest:Constant.bootstrap3 ()]
+          ~signer:Constant.bootstrap3
+          [make ~source:source1 ~fee @@ transfer ~dest:Constant.bootstrap4 ()]
           client)
     in
+    let* () = check_mempool ~applied:[oph1; oph2] client in
+
+    log_step
+      4
+      "Inject yet another transfer from [source1] with the same fee and a \
+       correct counter, but this time with [~force:true]. Check that the new \
+       operation is included in the mempool as [branch_delayed]." ;
+    let* (`OpHash oph1bis) =
+      Operation.Manager.(
+        inject
+          ~force:true
+          ~signer:Constant.bootstrap3
+          [make ~source:source1 ~fee @@ transfer ~dest:Constant.bootstrap5 ()]
+          client)
+    in
+    let* () =
+      check_mempool ~applied:[oph1; oph2] ~branch_delayed:[oph1bis] client
+    in
 
     log_step
       5
-      "Check that the mempool contains %s as applied and no op as \
-       branch_delayed."
-      oph1 ;
-    check_mempool ~applied:[oph1] client
+      "Inject a new transfer from [source2] with a much higher fee than the \
+       first one from the same source. Check that the new operation is \
+       [applied] while the old one has become [outdated]." ;
+    let* (`OpHash oph2bis) =
+      Operation.Manager.(
+        inject
+          [
+            make ~source:source2 ~fee:(2 * fee)
+            @@ transfer ~dest:Constant.bootstrap4 ();
+          ]
+          client)
+    in
+    check_mempool
+      ~applied:[oph1; oph2bis]
+      ~branch_delayed:[oph1bis]
+      ~outdated:[oph2]
+      client
 
   (** This test checks that an operation with a wrong signature that is
       initially branch_delayed (1 M) will be correctly
@@ -4111,7 +4172,7 @@ let register ~protocols =
   Revamped.one_operation_per_manager_per_block_flush protocols ;
   Revamped.one_operation_per_manager_per_block_ban protocols ;
   Revamped.one_operation_per_manager_per_block_flush_on_ban protocols ;
-  Revamped.one_operation_per_manager_per_block_inject_not_applied protocols ;
+  Revamped.one_operation_per_manager_per_block_inject_isolated_node protocols ;
   Revamped.max_refused_operations_branch_delayed protocols ;
   Revamped.max_refused_operations_branch_refused protocols ;
   Revamped.max_refused_operations_refused protocols ;
-- 
GitLab