From 3df5a6585d35b81aae9b66f2b43a38db45a13f80 Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Thu, 10 Apr 2025 17:48:24 +0200
Subject: [PATCH 1/2] proto/operation_repr: refactor consensus operation
 comparison

---
 .../lib_protocol/operation_repr.ml            | 339 ++++++++----------
 1 file changed, 143 insertions(+), 196 deletions(-)

diff --git a/src/proto_alpha/lib_protocol/operation_repr.ml b/src/proto_alpha/lib_protocol/operation_repr.ml
index 0fb269978f8c..ee4b6d4dc926 100644
--- a/src/proto_alpha/lib_protocol/operation_repr.ml
+++ b/src/proto_alpha/lib_protocol/operation_repr.ml
@@ -2259,94 +2259,88 @@ let compare_inner_pass : type a b. a pass -> b pass -> int =
    failed to convert in a {!int}, the value of [round] is (-1). *)
 type round_infos = {level : int32; round : int}
 
-(** [preattestation_infos] is the pair of a {!round_infos} and a [slot]
-   convert into an {!int}. *)
-type preattestation_infos = {round : round_infos; slot : int}
+type preattestation_specific = {slot : int}
 
-(** [aggregate_infos] is the pair of a {!round_infos} and a [committee] that is
-    a list of slots converted into a {!int list}. *)
-type aggregate_infos = {round : round_infos; committee : int list}
+type attestation_specific = {slot : int; number_of_dal_attested_slots : int}
 
-(** [attestation_infos] is the tuple consisting of a {!round_infos} value, a
-    [slot], and the number of DAL slots in the DAL attestation. *)
-type attestation_infos = {
+type aggregate_specific = {committee : int list}
+
+type consensus_kind_specific =
+  | Consensus_weight_preattestation of preattestation_specific
+  | Consensus_weight_attestation of attestation_specific
+  | Consensus_weight_aggregation of aggregate_specific
+
+type consensus_weight = {
   round_infos : round_infos;
-  slot : int;
-  number_of_dal_attested_slots : int;
+  kind_specific : consensus_kind_specific;
 }
 
 (** [double_baking_infos] is the pair of a {!round_infos} and a
     {!block_header} hash. *)
 type double_baking_infos = {round : round_infos; bh_hash : Block_hash.t}
 
-(** Compute a {!round_infos} from a {consensus_content} of a valid
-   operation. Hence, the [round] must convert in {!int}.
+(** {!type-round_infos} builder from protocol types.
 
-    Precondition: [c] comes from a valid operation. The [round] from a
-   valid operation should succeed to convert in {!int}. Hence, for the
-   unreachable path where the convertion failed, we put (-1) as
-   [round] value. *)
-let round_infos_from_consensus_content (c : consensus_content) =
-  let level = Raw_level_repr.to_int32 c.level in
-  match Round_repr.to_int c.round with
+    Precondition: [level] and [round] come from the consensus content
+    of a valid operation. This means that [round] should successfully
+    convert to {!int}. Hence, for the unreachable path where the
+    convertion failed, we put (-1) as [round] value. *)
+let round_infos ~level ~round =
+  let level = Raw_level_repr.to_int32 level in
+  match Round_repr.to_int round with
   | Ok round -> {level; round}
   | Error _ -> {level; round = -1}
 
-(** Compute a {!attestation_infos} from a {!consensus_content}. It is
-   used to compute the weight of {!Attestation} and {!Preattestation}.
-
-    Precondition: [c] comes from a valid operation. The {!Attestation}
-   or {!Preattestation} is valid, so its [round] must succeed to
-   convert into an {!int}. Hence, for the unreachable path where the
-   convertion fails, we put (-1) as [round] value (see
-   {!round_infos_from_consensus_content}). *)
-let attestation_infos_from_consensus_content (c : consensus_content) =
-  let slot = Slot_repr.to_int c.slot in
-  let round = round_infos_from_consensus_content c in
-  {round; slot}
-
-(** Compute a {!attestation_infos} value from a {!consensus_content} value
-    and an optional {!dal_content} value. It is used to compute the weight of
-    an {!Attestation}.
-
-    An {!Attestation} with no DAL content or with a DAL content that has 0
-    attested DAL slots have the same weight (everything else being
-    equal). That's ok, because they are semantically equal, therefore it
-    does not matter which one is picked.
-
-    Precondition: [c] and [d] come from a valid operation.  *)
-let attestation_infos_from_content (c : consensus_content)
-    (d : dal_content option) =
-  let slot = Slot_repr.to_int c.slot in
-  let round_infos = round_infos_from_consensus_content c in
+(** Builds {!type-round_infos} from a {!consensus_content}.
+
+    Precondition: the {!consensus_content} comes from a valid
+    operation; see {!val-round_infos}. *)
+let round_infos_from_consensus_content
+    {level; round; block_payload_hash = _; slot = _} =
+  round_infos ~level ~round
+
+let preattestation_weight (Preattestation consensus_content) =
+  let slot = Slot_repr.to_int consensus_content.slot in
   {
-    round_infos;
-    slot;
-    number_of_dal_attested_slots =
-      Option.fold
-        ~none:0
-        ~some:(fun d ->
-          Dal_attestation_repr.number_of_attested_slots d.attestation)
-        d;
+    round_infos = round_infos_from_consensus_content consensus_content;
+    kind_specific = Consensus_weight_preattestation {slot};
   }
 
-(** Compute an {!aggregate_info} value from {!consensus_aggregate_content}
-    and {!Slot_repr.t list} values. It is used to compute the weight of an
-    {!Aggregate_attestation}.
-
-    Precondition: [aggregate_consensus_content] and [committee] come from a
-    valid operation.  *)
-let aggregate_infos_from_content (proposal : consensus_aggregate_content)
-    (committee : Slot_repr.t list) =
-  let level = Raw_level_repr.to_int32 proposal.level in
-  let round =
-    match Round_repr.to_int proposal.round with
-    | Ok round -> round
-    | Error _ -> -1
+let attestation_weight (Attestation {consensus_content; dal_content}) =
+  let slot = Slot_repr.to_int consensus_content.slot in
+  (* An {!Attestation} with no DAL content or with a DAL content that
+     has 0 attested DAL slots have the same weight (everything else
+     being equal). That's ok, because they are semantically equal,
+     therefore it does not matter which one is picked. *)
+  let number_of_dal_attested_slots =
+    match dal_content with
+    | Some {attestation} ->
+        Dal_attestation_repr.number_of_attested_slots attestation
+    | None -> 0
   in
-  let round_infos = {level; round} in
+  {
+    round_infos = round_infos_from_consensus_content consensus_content;
+    kind_specific =
+      Consensus_weight_attestation {slot; number_of_dal_attested_slots};
+  }
+
+let preattestations_aggregate_weight
+    (Preattestations_aggregate
+      {consensus_content = {level; round; block_payload_hash = _}; committee}) =
+  let committee = List.map Slot_repr.to_int committee in
+  {
+    round_infos = round_infos ~level ~round;
+    kind_specific = Consensus_weight_aggregation {committee};
+  }
+
+let attestations_aggregate_weight
+    (Attestations_aggregate
+      {consensus_content = {level; round; block_payload_hash = _}; committee}) =
   let committee = List.map Slot_repr.to_int committee in
-  {round = round_infos; committee}
+  {
+    round_infos = round_infos ~level ~round;
+    kind_specific = Consensus_weight_aggregation {committee};
+  }
 
 (** Compute a {!double_baking_infos} and a {!Block_header_repr.hash}
    from a {!Block_header_repr.t}. It is used to compute the weight of
@@ -2375,13 +2369,12 @@ type dal_entrapment_info = {level : int32; number_of_attested_slots : int}
 
 (** The weight of an operation.
 
-   Given an operation, its [weight] carries on static information that
-   is used to compare it to an operation of the same pass.
-    Operation weight are defined by validation pass.
+    It contains static information used to compare it to other
+    operations of the same validation pass. (When two operations have
+    different validation passes, they are simply ordered according to
+    {!compare_inner_pass}.)
 
-    The [weight] of an {!Attestation} or {!Preattestation} depends on its
-    {!attestation_infos}. For {!Attestation}s it also depends on the number of
-    attested DAL slots.
+    For consensus operations, see {!compare_consensus_weight}.
 
    The [weight] of a voting operation depends on the pair of its
    [period] and [source].
@@ -2412,11 +2405,7 @@ type dal_entrapment_info = {level : int32; number_of_attested_slots : int}
     The [weight] of {!Manager_operation} depends on its [fee] and
    [gas_limit] ratio expressed in {!Q.t}. *)
 type _ weight =
-  | Weight_attestation : attestation_infos -> consensus_pass_type weight
-  | Weight_preattestation : preattestation_infos -> consensus_pass_type weight
-  | Weight_attestations_aggregate :
-      aggregate_infos
-      -> consensus_pass_type weight
+  | Weight_consensus : consensus_weight -> consensus_pass_type weight
   | Weight_proposals :
       int32 * Signature.Public_key_hash.t
       -> voting_pass_type weight
@@ -2502,32 +2491,23 @@ let weight_manager :
 
 (** Computing the {!operation_weight} of an operation. [weight_of
    (Failing_noop _)] is unreachable, for completness we define a
-   Weight_noop which carrries no information. *)
+   Weight_noop which carrries no information.
+
+    Precondition: [op] is a valid operation (required by
+    {!val-round_infos}). *)
 let weight_of : packed_operation -> operation_weight =
  fun op ->
   let (Operation_data protocol_data) = op.protocol_data in
   match protocol_data.contents with
   | Single (Failing_noop _) -> W (Noop, Weight_noop)
-  | Single (Preattestation consensus_content) ->
-      W
-        ( Consensus,
-          Weight_preattestation
-            (attestation_infos_from_consensus_content consensus_content) )
-  | Single (Attestation {consensus_content; dal_content}) ->
-      W
-        ( Consensus,
-          Weight_attestation
-            (attestation_infos_from_content consensus_content dal_content) )
-  | Single (Preattestations_aggregate {consensus_content; committee}) ->
-      let aggregate_infos =
-        aggregate_infos_from_content consensus_content committee
-      in
-      W (Consensus, Weight_attestations_aggregate aggregate_infos)
-  | Single (Attestations_aggregate {consensus_content; committee}) ->
-      let aggregate_infos =
-        aggregate_infos_from_content consensus_content committee
-      in
-      W (Consensus, Weight_attestations_aggregate aggregate_infos)
+  | Single (Preattestation _ as contents) ->
+      W (Consensus, Weight_consensus (preattestation_weight contents))
+  | Single (Attestation _ as contents) ->
+      W (Consensus, Weight_consensus (attestation_weight contents))
+  | Single (Preattestations_aggregate _ as contents) ->
+      W (Consensus, Weight_consensus (preattestations_aggregate_weight contents))
+  | Single (Attestations_aggregate _ as contents) ->
+      W (Consensus, Weight_consensus (attestations_aggregate_weight contents))
   | Single (Proposals {period; source; _}) ->
       W (Voting, Weight_proposals (period, source))
   | Single (Ballot {period; source; _}) ->
@@ -2593,8 +2573,6 @@ let compare_pair_in_lexico_order ~cmp_fst ~cmp_snd (a1, b1) (a2, b2) =
 (** Compare in reverse order. *)
 let compare_reverse (cmp : 'a -> 'a -> int) a b = cmp b a
 
-(** {4 Comparison of {!consensus_infos}} *)
-
 (** Two {!round_infos} pairs [(level, round)] compare in
    lexicographic order: the one with the greater [level] being the
    greater [round_infos]. When levels are the same, the one with the
@@ -2619,105 +2597,58 @@ let compare_round_infos (infos1 : round_infos) (infos2 : round_infos) =
     (infos1.level, infos1.round)
     (infos2.level, infos2.round)
 
-(** Two {!Preattestation}s are compared by their {!preattestation_infos}.
-   When their {!round_infos} are equal, they are compared according to
-   their [slot]: the smaller the better. *)
-let compare_preattestation_infos (infos1 : preattestation_infos)
-    (infos2 : preattestation_infos) =
-  compare_pair_in_lexico_order
-    ~cmp_fst:compare_round_infos
-    ~cmp_snd:(compare_reverse Compare.Int.compare)
-    (infos1.round, infos1.slot)
-    (infos2.round, infos2.slot)
+(** {4 Comparison of valid operations of the same validation pass} *)
 
-(** Two {!double_baking_infos} are compared as their {!round_infos}.
-   When their {!round_infos} are equal, they are compared as the
-   hashes of their first denounced block header. *)
-let compare_baking_infos infos1 infos2 =
-  compare_pair_in_lexico_order
-    ~cmp_fst:compare_round_infos
-    ~cmp_snd:Block_hash.compare
-    (infos1.round, infos1.bh_hash)
-    (infos2.round, infos2.bh_hash)
+(** {5 Comparison of valid consensus operations} *)
 
-(** Two {!Attestation}s are compared by their {!attestation_infos}. When their
-    {!round_infos} are equal, they are compared according to their [slot]: the
-    smaller the better. When the slots are also equal they are compared
-    according to the number of attested DAL slots: the more the better. *)
-let compare_attestation_infos
-    {round_infos = infos1; slot = slot1; number_of_dal_attested_slots = n1}
-    {round_infos = infos2; slot = slot2; number_of_dal_attested_slots = n2} =
-  compare_pair_in_lexico_order
-    ~cmp_fst:
-      (compare_pair_in_lexico_order
-         ~cmp_fst:compare_round_infos
-         ~cmp_snd:(compare_reverse Compare.Int.compare))
-    ~cmp_snd:Compare.Int.compare
-    ((infos1, slot1), n1)
-    ((infos2, slot2), n2)
+(** Consensus operations of any kind (even mixed kinds) are compared
+    by {!round_infos}, because a greater {!round_infos} means a more
+    advanced state of the chain.
 
-let compare_dal_entrapment_infos infos1 infos2 =
-  compare_pair_in_lexico_order
-    ~cmp_fst:Compare.Int32.compare
-    ~cmp_snd:Compare.Int.compare
-    (infos1.level, infos1.number_of_attested_slots)
-    (infos2.level, infos2.number_of_attested_slots)
+    When {!round_infos} are equal, we look at the operation kind:
+    {!Attestations_aggregate} = {!Preattestations_aggregate} >
+    {!Attestation} > {!Preattestation}.
 
-(** Two {!Attestations_aggregate} are compared by their {!aggregate_infos}. When their
-    {!round_infos} are equal, they are compared according to their [committee]
-    by lexicographic order. *)
-let compare_attestations_aggregate_infos
-    {round = infos1; committee = committee1}
-    {round = infos2; committee = committee2} =
-  compare_pair_in_lexico_order
-    ~cmp_fst:compare_round_infos
-    ~cmp_snd:(List.compare Compare.Int.compare)
-    (infos1, committee1)
-    (infos2, committee2)
+    When the operations have the same kind or equally weighted kinds:
 
-(** {4 Comparison of valid operations of the same validation pass} *)
+    - {!Preattestation}s are compared by [slot]: the smaller the better.
 
-(** {5 Comparison of valid consensus operations} *)
+    - {!Attestation}s are compared by [slot]: the smaller the better;
+    when slots are equal, more attested DAL slots is better.
 
-(** Comparing consensus operations by their [weight] uses the comparison on
-    {!attestation_infos} for {!Attestation}s and {!Preattestation}s. In case of
-    equality of their {!round_infos}, either they are of the same kind and their
-    [slot]s have to be compared in the reverse order, otherwise the
-    {!Attestation}s are better. In case of {!Attestation}s, the number of
-    attested DAL slots is taken into account when all else is equal.
-
-    {!Dal_attestation} is smaller than the other kinds of
-   consensus operations. Two valid {!Dal_attestation} are
-   compared by {!compare_dal_attestation}. *)
-let compare_consensus_weight w1 w2 =
-  match (w1, w2) with
-  | Weight_attestation infos1, Weight_attestation infos2 ->
-      compare_attestation_infos infos1 infos2
-  | Weight_preattestation infos1, Weight_preattestation infos2 ->
-      compare_preattestation_infos infos1 infos2
-  | ( Weight_attestation {round_infos = round_infos1; _},
-      Weight_preattestation {round = round_infos2; _} ) ->
-      let cmp = compare_round_infos round_infos1 round_infos2 in
-      if Compare.Int.(cmp <> 0) then cmp else 1
-  | ( Weight_preattestation {round = round_infos1; _},
-      Weight_attestation {round_infos = round_infos2; _} ) ->
-      let cmp = compare_round_infos round_infos1 round_infos2 in
-      if Compare.Int.(cmp <> 0) then cmp else -1
-  | Weight_attestations_aggregate infos1, Weight_attestations_aggregate infos2
-    ->
-      compare_attestations_aggregate_infos infos1 infos2
-  | ( Weight_attestations_aggregate {round = round_infos1; _},
-      Weight_preattestation {round = round_infos2; _} )
-  | ( Weight_attestations_aggregate {round = round_infos1; _},
-      Weight_attestation {round_infos = round_infos2; _} ) ->
-      let cmp = compare_round_infos round_infos1 round_infos2 in
-      if Compare.Int.(cmp <> 0) then cmp else 1
-  | ( Weight_preattestation {round = round_infos1; _},
-      Weight_attestations_aggregate {round = round_infos2; _} )
-  | ( Weight_attestation {round_infos = round_infos1; _},
-      Weight_attestations_aggregate {round = round_infos2; _} ) ->
-      let cmp = compare_round_infos round_infos1 round_infos2 in
-      if Compare.Int.(cmp <> 0) then cmp else -1
+    - {!Attestations_aggregate}s or {!Preattestations_aggregate}s are
+    compared by [committee] in lexicographic order.
+
+    Note that two operations may be considered equal according to this
+    comparison in spite of containing different data (e.g. different
+    {!field-block_payload_hash}). This means that none of them is
+    notably better than the other, and {!compare} will order them
+    according to their hash.
+*)
+let compare_consensus_weight (Weight_consensus w1) (Weight_consensus w2) =
+  Compare.or_else (compare_round_infos w1.round_infos w2.round_infos)
+  @@ fun () ->
+  match (w1.kind_specific, w2.kind_specific) with
+  | ( Consensus_weight_aggregation _,
+      (Consensus_weight_attestation _ | Consensus_weight_preattestation _) ) ->
+      1
+  | ( (Consensus_weight_attestation _ | Consensus_weight_preattestation _),
+      Consensus_weight_aggregation _ ) ->
+      -1
+  | Consensus_weight_attestation _, Consensus_weight_preattestation _ -> 1
+  | Consensus_weight_preattestation _, Consensus_weight_attestation _ -> -1
+  | ( Consensus_weight_preattestation {slot = slot1},
+      Consensus_weight_preattestation {slot = slot2} ) ->
+      compare_reverse Compare.Int.compare slot1 slot2
+  | ( Consensus_weight_attestation
+        {slot = slot1; number_of_dal_attested_slots = n1},
+      Consensus_weight_attestation
+        {slot = slot2; number_of_dal_attested_slots = n2} ) ->
+      Compare.or_else (compare_reverse Compare.Int.compare slot1 slot2)
+      @@ fun () -> Compare.Int.compare n1 n2
+  | ( Consensus_weight_aggregation {committee = committee1},
+      Consensus_weight_aggregation {committee = committee2} ) ->
+      List.compare Compare.Int.compare committee1 committee2
 
 (** {5 Comparison of valid voting operations} *)
 
@@ -2742,6 +2673,22 @@ let compare_vote_weight w1 w2 =
 
 (** {5 Comparison of valid anonymous operations} *)
 
+let compare_dal_entrapment_infos infos1 infos2 =
+  compare_pair_in_lexico_order
+    ~cmp_fst:Compare.Int32.compare
+    ~cmp_snd:Compare.Int.compare
+    (infos1.level, infos1.number_of_attested_slots)
+    (infos2.level, infos2.number_of_attested_slots)
+
+(** Two {!double_baking_infos} are compared by {!round_infos}, then by
+    the hash of the first denounced block header. *)
+let compare_baking_infos infos1 infos2 =
+  compare_pair_in_lexico_order
+    ~cmp_fst:compare_round_infos
+    ~cmp_snd:Block_hash.compare
+    (infos1.round, infos1.bh_hash)
+    (infos2.round, infos2.bh_hash)
+
 (** Comparing two {!Double_attestation_evidence}, or two
    {!Double_preattestation_evidence}, or comparing them to each other
    is comparing their {!round_infos}, see {!compare_round_infos} for
-- 
GitLab


From 85857562c51454665e68e90718720d361811efa0 Mon Sep 17 00:00:00 2001
From: Diane Gallois-Wong <diane.gallois-wong@nomadic-labs.com>
Date: Fri, 11 Apr 2025 17:00:43 +0200
Subject: [PATCH 2/2] proto/operation_repr: compare aggregation operations by
 committee size

---
 .../lib_protocol/operation_repr.ml            | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/proto_alpha/lib_protocol/operation_repr.ml b/src/proto_alpha/lib_protocol/operation_repr.ml
index ee4b6d4dc926..7b1c256c0ecc 100644
--- a/src/proto_alpha/lib_protocol/operation_repr.ml
+++ b/src/proto_alpha/lib_protocol/operation_repr.ml
@@ -2263,7 +2263,7 @@ type preattestation_specific = {slot : int}
 
 type attestation_specific = {slot : int; number_of_dal_attested_slots : int}
 
-type aggregate_specific = {committee : int list}
+type aggregate_specific = {committee_size : int}
 
 type consensus_kind_specific =
   | Consensus_weight_preattestation of preattestation_specific
@@ -2327,19 +2327,19 @@ let attestation_weight (Attestation {consensus_content; dal_content}) =
 let preattestations_aggregate_weight
     (Preattestations_aggregate
       {consensus_content = {level; round; block_payload_hash = _}; committee}) =
-  let committee = List.map Slot_repr.to_int committee in
+  let committee_size = List.length committee in
   {
     round_infos = round_infos ~level ~round;
-    kind_specific = Consensus_weight_aggregation {committee};
+    kind_specific = Consensus_weight_aggregation {committee_size};
   }
 
 let attestations_aggregate_weight
     (Attestations_aggregate
       {consensus_content = {level; round; block_payload_hash = _}; committee}) =
-  let committee = List.map Slot_repr.to_int committee in
+  let committee_size = List.length committee in
   {
     round_infos = round_infos ~level ~round;
-    kind_specific = Consensus_weight_aggregation {committee};
+    kind_specific = Consensus_weight_aggregation {committee_size};
   }
 
 (** Compute a {!double_baking_infos} and a {!Block_header_repr.hash}
@@ -2616,8 +2616,8 @@ let compare_round_infos (infos1 : round_infos) (infos2 : round_infos) =
     - {!Attestation}s are compared by [slot]: the smaller the better;
     when slots are equal, more attested DAL slots is better.
 
-    - {!Attestations_aggregate}s or {!Preattestations_aggregate}s are
-    compared by [committee] in lexicographic order.
+    - {!Attestations_aggregate}s or {!Preattestations_aggregate}s by
+    committee size: a larger committee is better.
 
     Note that two operations may be considered equal according to this
     comparison in spite of containing different data (e.g. different
@@ -2646,9 +2646,9 @@ let compare_consensus_weight (Weight_consensus w1) (Weight_consensus w2) =
         {slot = slot2; number_of_dal_attested_slots = n2} ) ->
       Compare.or_else (compare_reverse Compare.Int.compare slot1 slot2)
       @@ fun () -> Compare.Int.compare n1 n2
-  | ( Consensus_weight_aggregation {committee = committee1},
-      Consensus_weight_aggregation {committee = committee2} ) ->
-      List.compare Compare.Int.compare committee1 committee2
+  | ( Consensus_weight_aggregation {committee_size = n1},
+      Consensus_weight_aggregation {committee_size = n2} ) ->
+      Compare.Int.compare n1 n2
 
 (** {5 Comparison of valid voting operations} *)
 
-- 
GitLab