Fix leadership schedule for current on babbage

[newhoggy]

No description provided.

[newhoggy]

$ cabal test cardano-testnet --enable-tests --test-show-details=direct --test-options '-p Babbage.leadership-schedule'
...
Test suite cardano-testnet-tests: RUNNING...
test/Spec.hs
  Spec
    Babbage
      leadership-schedule: OK (259.17s)
          ✓ leadership-schedule passed 1 test.

All 1 tests passed (259.17s)
Test suite cardano-testnet-tests: PASS
Test suite logged to:
/Users/jky/wrk/iohk/cardano-node/dist-newstyle/build/aarch64-osx/ghc-8.10.7/cardano-testnet-1.35.0/t/cardano-testnet-tests/test/cardano-testnet-1.35.0-cardano-testnet-tests.log
1 of 1 test suites (1 of 1 test cases) passed.

[newhoggy]

Resolves #3883

Addressed concerns

[Jimbo4350]

Nice 👍 A few comments. Can you also tidy up the commit history? It's a fairly big PR and it was difficult to follow the changes.

[Jimbo4350]

This is duplicated 3 more times in cardano-testnet. We should have a single TestnetOptions that is a sum type. And if there are no testnet options, we can use a constructor of that sum type to indicate this e.g NoTestnetOptions

[newhoggy]

Currently there are different functions for setting up different testnets and they take different types because they configurations they take are different. Unifying them is desirable, but it would take a lot of work. I think this PR is the wrong place to do it.

[Jimbo4350]

Ok fair enough. Can you create an issue to track this?

[newhoggy]

#4134

[Jimbo4350]

Why?

[newhoggy]

I discovered it was an interesting way to use layout in expressions instead of parenthesis.

For example the expression:

  H.rewriteYamlFile (tempAbsPath </> "configuration.yaml") . J.rewriteObject
    $ HM.delete "GenesisFile"
    . HM.insert "Protocol" (J.toJSON @String "Cardano")
    ...
    . HM.insert "TestEnableDevelopmentNetworkProtocols" (J.toJSON True)
    . flip HM.alter "setupScribes"
      ( fmap
        ( J.rewriteArrayElements
          ( J.rewriteObject
            ( HM.insert "scFormat"
              ( case nodeLoggingFormat testnetOptions of
                  NodeLoggingFormatAsJson -> "ScJson"
                  NodeLoggingFormatAsText -> "ScText")))))

Can be written as:

  H.rewriteYamlFile (tempAbsPath </> "configuration.yaml") . J.rewriteObject
    $ HM.delete "GenesisFile"
    . HM.insert "Protocol" (J.toJSON @String "Cardano")
    ...
    . HM.insert "TestEnableDevelopmentNetworkProtocols" (J.toJSON True)
    . flip HM.alter "setupScribes" do
        fmap do
          J.rewriteArrayElements do
            J.rewriteObject do
              HM.insert "scFormat" case nodeLoggingFormat testnetOptions of
                NodeLoggingFormatAsJson -> "ScJson"
                NodeLoggingFormatAsText -> "ScText"

This works because do x y z is the same as x y z. This is true even without BlockArguments.

What BlockArguments gives us is that in the expression f $ x y z can be rewritten as f do x y z.

This gives us two nice things:

• We can use do to use layout to group expressions, which means we don't have to match parens anymore. When we write Haskell, we usually indent sub-expressions to help the reader read the code anyway. The parentheses are for the compiler because layout is not significant to it inside an expression. BlockArguments allows code to be written such that the mechanism humans use to group expressions and the compiler uses to group expressions can be one and the same.
• We normally think of $ and swappable with a pair of (). This is not always true. The ability to interchange them in this $ is unreliable because it is an operator and operators have a precedence and sometimes it is the wrong precedence.

For example if you tried to write the large expression from before using $ instead it is actually wrong and doesn't compile:

  H.rewriteYamlFile (tempAbsPath </> "configuration.yaml") . J.rewriteObject
    $ HM.delete "GenesisFile"
    . HM.insert "Protocol" (J.toJSON @String "Cardano")
    ...
    . HM.insert "TestEnableDevelopmentNetworkProtocols" (J.toJSON True)
    . flip HM.alter "setupScribes"
      $ fmap
        $ J.rewriteArrayElements
          $ J.rewriteObject
            $ HM.insert "scFormat"
              $ case nodeLoggingFormat testnetOptions of
                  NodeLoggingFormatAsJson -> "ScJson"
                  NodeLoggingFormatAsText -> "ScText"

Similarly, f $ case { x -> y } can be rewritten without the $ as f case {x -> y}.

And f $ \x -> y can be rewritten without the $ as f \x -> y.

Basically there are a number of cases where when we previously used $, it can simply be dropped.

[Jimbo4350]

You could also write it as follows:

        (fmap
          . J.rewriteArrayElements
            . J.rewriteObject
               . HM.insert "scFormat" $ case nodeLoggingFormat testnetOptions of
                                        NodeLoggingFormatAsJson -> "ScJson"
                                        NodeLoggingFormatAsText -> "ScText")

The problem I have with BlockArguments is that it makes it more difficult to discern between monadic actions and function composition. For this reason I avoid using it because I value that more than slightly more convenient code formatting.

[newhoggy]

I've taken your suggestion, as it's really only two places.

Mainly I thought that BlockArguments has some really interesting properties and that it is a better $ and wanted to shared the idea.

At the place I worked previously, we used block argument do for error handling in monadic contexts, so perhaps we can revisit this.

[Jimbo4350]

we used block argument do for error handling in monadic contexts

Do you have an example of this I can look at?

[newhoggy]

I do, but I'll figure out an appropriate time to share it. It can wait until some urgent features are done.

Review comments

[nfrisby]

I'm in a call, but I skimmed through 62082a4, which I think is the only Consensus-related commit.

It looks great, from what I can see. 👏 One minor question below, but low priority, I think. (I'm not Approving, because I only looked at so little of this PR, but I this code looks as expected. 👍 )

PS - The commit 3ffe9ee is Fix leadership schedule for current on babbage, which sounded like what I was looking for, but I think it's just maybe getting a test to build again? I'd suggest a commit title that clarifies that eg.

[nfrisby]

Nice simplifications! Thanks for discerning which parts of the Consensus API you could use here 👍

[nfrisby]

I think we (eventually?) won't have d at all for Babbage/Praos, right? Will this require an update then?

[JaredCorduan]

There is no longer an overlay schedule in Praos, so this line could be simplified to just return the set of slots in the given epoch.

(As for removing d, it is not in the Babbage era protocol parameters, but the HasField instance still exists, sadly, since d has its tendrils all throughout the reward calculation and we chose not to redo all that code.)

[newhoggy]

Since we need to keep it for TPraos anyway, I'm happy to keep this code for the moment and be guided by the type errors if/when the d parameters get deleted for Babbage onwards.

[newhoggy]

bors r+

[iohk-bors]

Build succeeded:

• buildkite/cardano-node
• ci/hydra-build:required