Refactor: SelectView = BlockNo × TiebreakerView

[amesgen]

This PR does not change any behavior; most of the diff is purely mechanical.

Previously, the SelectViews of all ConsensusProtocols contained the BlockNo as the most important criterion for comparing chains, with only tiebreaking behavior (among chains of equal length) being different across different protocols.

This PR makes this explicit: SelectView is now longer an associated type family of ConsensusProtocol, but rather an ordinary data type

data SelectView p = SelectView
  { svBlockNo :: !BlockNo
  , svTiebreakerView :: !(TiebreakerView p)
  }

where TiebreakerView is now an associated type family of ConsensusProtocol.

This PR also removes the WithBlockNo type that the HFC was already using to always attach a BlockNo to its HardForkSelectView. The code can be simpler now that SelectViews always automatically contain an explicit BlockNo.

Also see #1542 (comment)

Primary motivation

The main motivation for this is to enable weighted chain selection in Peras (tweag/cardano-peras#62), which can now be easily modeled by adding the weight of a chain to the block number component, at least morally; we probably want to eventually have more type safety here to make it hard to "forget" to account for the weight, such as defining

data WeightedSelectView p = WeightedSelectView
  { wsvWeight :: !PerasWeight
  , wsvTiebreakerView :: !(TiebreakerView p)
  }

If the only goal were to minimize the diff, then it would also be possible to add a function (BlockNo -> BlockNo) -> SelectView p -> SelectView p, but it seems more honest to "properly" extract out the block number.

[fraser-iohk]

should this be an explicitly written-out Ord instance? it would be bad if someone decided to reorder the fields in SelectView and catastrophically broke chain selection lol

[amesgen]

Yes, pushed 👍

Irrational grudge of mine: I always found it a bit depressing to write such instances by hand; motivated by #549, I wrote https://gist.github.com/amesgen/0ddcbe0e2bae458ba8b40d4799d1b6e4 (essentially a "surgery" in the sense of generic-data) that allowed you to permute the constructors of a data type in the Generic representation for DerivingVia. The same thing of course works for fields instead of constructors; with that, we could write

deriving
  via Generically (PermuteFields ["svBlockNo", "svTiebreakerView"] (SelectView p))
  instance Ord (TiebreakerView p) => Ord (SelectView p)

which is now resistant to syntactic reordering in the data declaration of data SelectView. All that to avoid writing

instance Ord (TiebreakerView p) => Ord (SelectView p) where
  compare =
    mconcat
      [ compare `on` svBlockNo
      , compare `on` svTiebreakerView
      ]

by hand 💪 💪 💪

[dnadales]

Looks good. I left a couple of comments. Let me know when the TODOs are addressed and a new review round is required 👍

[dnadales]

Probably unrelated to this PR, but I was wondering if we could elaborate on why the block number (which will become a weigth) and the notion of a tie-breaker cannot be unified into a single weight notion.

[amesgen]

I tried to capture this in the Haddocks 👍

[dnadales]

Sorry, it's still unclear to me (the answer to the question above, the haddocks comment is clear :D). It seems that ultimately, we could write a function to sort two select views which take the information of the tie-breaker into consideration. Do we want to keep the block number and tiebreaker separately because of the separation that we currently have between sorting candidates and preferring a chain?

[amesgen]

I am not exactly sure what unification you have in mind, is it something different than what we have in current main, where everybody defines SelectView as a "single" thing determining the chain order for themselves? Or sth else entirely?

we could write a function to sort two select views which take the information of the tie-breaker into consideration.

Are you expecting something different than in the Ord/ChainOrder instances for SelectView in this PR?

Do we want to keep the block number and tiebreaker separately because of the separation that we currently have between sorting candidates and preferring a chain?

It is unrelated to that distinction; the motivation is

1. (minor) to make it explicit that BlockNo must be the primary way of comparing chains; even before this PR, ConsensusProtocols didn't have the freedom to use arbitrary SelectViews, and
2. nicely supports weighted chain comparisons, see "Primary motivation" in the PR description, as well as Introduce weighted chain comparisons #1594.

[dnadales]

FTR, we discussed this offline with @amesgen. The key constraint we have is that we rely on a transitive Ord instance when selecting candidates. Therefore, we cannot unify the sorting of Views in a single function. I think that we have is indeed two comparison functions that operate on SelectViews. It's just that we're explicit about the part of the field that is used for breaking ties (which I think makes sense)

[dnadales]

Looks great.

[dnadales]

Why do we mention both in this comment? Would it make sense to clarify when we use one and when the other?

[amesgen]

Because we have instances for both of them, where ChainOrder SelectView uses ChainOrder TiebreakerView. I explicitly mentioned this also here 👍

[dnadales]

Sorry, it's still unclear to me (the answer to the question above, the haddocks comment is clear :D). It seems that ultimately, we could write a function to sort two select views which take the information of the tie-breaker into consideration. Do we want to keep the block number and tiebreaker separately because of the separation that we currently have between sorting candidates and preferring a chain?