Praos chain order: fix issue no-related transitivity edge case (#1086)

Closes #1075

Beware that this PR has a fairly small
$\dfrac{\text{severity}}{\text{subtlety}}$ ratio.

### Current non-transitivity of the chain order related to issue numbers

Before this PR, the `Ord PraosChainSelectView` instance is defined as
the lexicographic-ish[^lexicographic-ish] combination of the following
comparisons in descending order:

 - Chain length, preferring longer chains.
- If the issuer identity is the same, compare by the issue/opcert
number, preferring higher values, otherwise, no preference.
 - VRF tiebreaker, preferring lower values.

To see why it is not transitive, consider the following three
`SelectView`s:

|              | a | b | c |
| ------------ | - | - | - |
| Chain length | l | l | l |
| Issuer       | x | y | x |
| Issue no     | 2 | o | 1 |
| VRF          | 3 | 2 | 1 |

With the current chain order, we have

 - `a < b` and `b < c` due to the VRF tiebreaker, and
- `c < a` due to the issue number tiebreaker (as `a` and `c` have the
same issuer).

So we have have `a < b < c < a < ...`.

Note that due to `VRF a /= VRF c` and `Issuer a == Issuer c`, we must
have `Slot a /= Slot c`, even though `ChainLength a == ChainLength b`.
This is because VRFs are collision-resistant, and are a deterministic
function of the slot, the (cold) issuer identity and the epoch nonce
(which is itself determined by the slot for any given chain).

However, this case is not important for the motivating scenario of the
issue number tiebreaker, namely when an attacker got hold of the hot key
(but not the cold key) of issuer `x`, and the attacked SPO, the owner of
the cold key of `x`, creates a new hot key with an incremented issue
number, where the issue number tiebreaker is now supposed to "establish
precedence"[^precedence]. In this scenario, the attacker minted `c`, and
the attacked SPO minted `a`; this is however unrealistic as due to `Slot
a /= Slot c`, either party could have minted on top of the other block,
superseding the tiebreaker due to having a longer chain.

### Restoring transitivity

The natural fix is hence to require `Slot x == Slot y` in addition to
`Issuer x == Issuer y` as the condition on whether to compare issue
numbers when comparing `SelectView`s `x` and `y`.

In the example above, we then have

 - `a < b` and `b < c` due to the VRF tiebreaker (unchanged), and
- `a < c` also due to the VRF tiebreaker (new), as the issue number
tiebreaker is not armed.

Note that as already mentioned above, the condition `Slot x == Slot y &&
Issuer x == Issuer y` is equivalent to `VRF x == VRF y`. We could
therefore use this condition in this PR, and even remove the issuer from
`PraosChainSelectView`.

As a historical note, a very similar chain order was in place in the
past before the current non-transitivity was accidentally introduced as
a side effect in
IntersectMBO/ouroboros-network#2348.

The approach of this PR is slightly different than (but morally the same
as) the one suggested in #891; I think it is nice that the issue numbers
are still compared "before" the VRFs in this approach as that matches
the high-level intuition.

---

Based on top of #1047

[^lexicographic-ish]: Usually, one only considers the lexicographic
order constructed out of orders that are at least partial. However, the
order "compare opcert numbers when the issuers are identical, otherwise,
consider equal" on pairs of issuer identities and opcert numbers is not
a partial order as it is non-transitive. Still, the same general
principle applies.
[^precedence]: See ["Design Specification for Delegation and Incentives
in
Cardano"](https://github.com/IntersectMBO/cardano-ledger/blob/master/README.md),
Section 3.7.

Author: amesgen

ouroboros-consensus-protocol/changelog.d/20240503_194253_alexander.esgen_praos_chain_order_truly_transitive.md

@@ -0,0 +1,9 @@
+### Non-Breaking
+
+- Changed the Praos chain order such that for two blocks `A` and `B` by the same
+  issuer with the same block number, `A` is now preferred over `B` only if `A`
+  has a higher issue number *and* (new) `A` and `B` are in the same slot.
+
+  This is in line with the motiviation for the issue number tiebreaker, and
+  fixes the transitivity of the `Ord PraosChainSelectView` instance in a special
+  case.

ouroboros-consensus-protocol/src/ouroboros-consensus-protocol/Ouroboros/Consensus/Protocol/Praos/Common.hs

@@ -92,7 +92,7 @@ comparePraos ::
   -> Ordering
 comparePraos tiebreakerFlavor =
        (compare `on` csvChainLength)
-    <> when' ((==) `on` csvIssuer) (compare `on` csvIssueNo)
+    <> when' issueNoArmed (compare `on` csvIssueNo)
     <> when' vrfArmed (compare `on` Down . csvTieBreakVRF)
   where
     -- When the predicate @p@ returns 'True', use the given comparison function,
@@ -104,6 +104,12 @@ comparePraos tiebreakerFlavor =
     when' p comp a1 a2 =
         if p a1 a2 then comp a1 a2 else EQ
 
+    -- Only compare the issue numbers when the issuers and slots are identical.
+    -- Note that this case implies the VRFs also coincide.
+    issueNoArmed v1 v2 =
+           csvSlotNo v1 == csvSlotNo v2
+        && csvIssuer v1 == csvIssuer v2
+
     -- Whether to do a VRF comparison.
     vrfArmed v1 v2 = case tiebreakerFlavor of
         UnrestrictedVRFTiebreaker       -> True
@@ -120,8 +126,9 @@ comparePraos tiebreakerFlavor =
 --
 -- 1. By chain length, with longer chains always preferred.
 --
--- 2. If the tip of each chain was issued by the same agent, then we prefer
---    the chain whose tip has the highest ocert issue number.
+-- 2. If the tip of each chain was issued by the same agent and they have the
+--    same slot number, prefer the chain whose tip has the highest ocert issue
+--    number.
 --
 -- 3. By a VRF value from the chain tip, with lower values preferred. See
 --    @pTieBreakVRFValue@ for which one is used.
@@ -142,8 +149,13 @@ instance Crypto c => Ord (PraosChainSelectView c) where
 --
 -- 1. Chain length, with longer chains always preferred.
 --
--- 2. If the tip of each chain was issued by the same agent, then we prefer the
---    candidate if it has a higher ocert issue number.
+-- 2. If the tip of each chain was issued by the same agent and had the same
+--    slot number, then we prefer the candidate if it has a higher ocert issue
+--    number.
+--
+--     Note that this condition is equivalent to the VRFs being identical, as
+--     the VRF is a deterministic function of the issuer VRF key, the slot and
+--     the epoch nonce, and VRFs are collision-resistant.
 --
 -- 3. Depending on the 'VRFTiebreakerFlavor':
 --
@@ -182,7 +194,8 @@ instance Crypto c => Ord (PraosChainSelectView c) where
 --    block issuer can use their cold key to issue a new hot key with a higher
 --    opcert issue number and set up a new pool. Due to this tiebreaker rule,
 --    the blocks minted by that pool will take precedence (allowing the actual
---    block issuer to decide on eg the block contents and the predecessor), and
+--    block issuer to decide on eg the block contents and the predecessor) over
+--    blocks with the same block and slot number minted by the attacker, and
 --    they will end up on the honest chain quickly, which means that the
 --    adversary can't extend any chain containing such a block as it would
 --    violate the monotonicity requirement on opcert issue numbers.

ouroboros-consensus-protocol/test/protocol-test/Test/Consensus/Protocol/Praos/SelectView.hs

@@ -43,7 +43,7 @@ instance Crypto c => Arbitrary (PraosChainSelectView c) where
       csvChainLength <- BlockNo <$> choose (1, size)
       csvSlotNo      <- SlotNo  <$> choose (1, size)
       csvIssuer      <- elements knownIssuers
-      csvIssueNo     <- genIssueNo
+      csvIssueNo     <- choose (1, 10)
       pure PraosChainSelectView {
           csvChainLength
         , csvSlotNo
@@ -63,20 +63,6 @@ instance Crypto c => Arbitrary (PraosChainSelectView c) where
          randomSeed = mkQCGen 4 -- chosen by fair dice roll
          numIssuers = 10
 
-     -- TODO Actually randomize this once the issue number tiebreaker has been
-     -- fixed to be transitive. See the document in
-     -- https://github.com/IntersectMBO/ouroboros-consensus/pull/891 for
-     -- details.
-     --
-     -- TL;DR: In an edge case, the issue number tiebreaker prevents the
-     -- chain order from being transitive. This could be fixed relatively
-     -- easily, namely by swapping the issue number tiebreaker and the VRF
-     -- tiebreaker. However, this is technically not backwards-compatible,
-     -- impacting the current pre-Conway diffusion pipelining scheme.
-     --
-     -- See https://github.com/IntersectMBO/ouroboros-consensus/issues/1075.
-     genIssueNo = pure 1
-
      -- The header VRF is a deterministic function of the issuer VRF key, the
      -- slot and the epoch nonce. Additionally, for any particular chain, the
      -- slot determines the epoch nonce.