Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix leadership schedule for current on babbage #4106

Merged
merged 18 commits into from
Jul 8, 2022

Conversation

newhoggy
Copy link
Contributor

No description provided.

@newhoggy newhoggy force-pushed the newhoggy/fix-leadership-schedule-for-babbage branch from 645287f to 8dcf6f5 Compare June 28, 2022 11:37
@newhoggy newhoggy changed the title Newhoggy/fix leadership schedule for babbage Fix leadership schedule for current on babbage Jun 28, 2022
@newhoggy newhoggy marked this pull request as ready for review June 28, 2022 11:37
@newhoggy newhoggy force-pushed the newhoggy/fix-leadership-schedule-for-babbage branch 3 times, most recently from cfe0475 to 823fddb Compare June 30, 2022 01:56
@newhoggy
Copy link
Contributor Author

$ 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
Copy link
Contributor Author

Resolves #3883

cardano-testnet/cardano-testnet.cabal Outdated Show resolved Hide resolved
@newhoggy newhoggy requested a review from Jimbo4350 June 30, 2022 20:47
@newhoggy newhoggy force-pushed the newhoggy/fix-leadership-schedule-for-babbage branch 5 times, most recently from 736666c to 3e4f8ca Compare June 30, 2022 22:38
@newhoggy newhoggy dismissed Jimbo4350’s stale review June 30, 2022 22:45

Addressed concerns

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cardano-testnet/src/Testnet/Babbage.hs Outdated Show resolved Hide resolved
{- HLINT ignore "Use let" -}


data TestnetOptions = TestnetOptions deriving (Eq, Show)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cardano-testnet/src/Test/Runtime.hs Show resolved Hide resolved
cardano-testnet/src/Test/Process.hs Show resolved Hide resolved
cardano-testnet/test/Spec/Cli/Alonzo/LeadershipSchedule.hs Outdated Show resolved Hide resolved
cardano-testnet/cardano-testnet.cabal Show resolved Hide resolved
cardano-testnet/src/Testnet/Babbage.hs Outdated Show resolved Hide resolved
@@ -0,0 +1,202 @@
{-# LANGUAGE BlockArguments #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

@newhoggy newhoggy Jul 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Jimbo4350 Jimbo4350 Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we used block argument do for error handling in monadic contexts

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cardano-api/src/Cardano/Api/LedgerState.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/fix-leadership-schedule-for-babbage branch 2 times, most recently from 7acc550 to bcec4d4 Compare July 2, 2022 07:27
@newhoggy newhoggy requested a review from Jimbo4350 July 3, 2022 04:59
@newhoggy newhoggy dismissed Jimbo4350’s stale review July 3, 2022 05:00

Review comments

@newhoggy newhoggy force-pushed the newhoggy/fix-leadership-schedule-for-babbage branch 2 times, most recently from 9fdd4fb to 85068a6 Compare July 3, 2022 06:26
@newhoggy newhoggy force-pushed the newhoggy/fix-leadership-schedule-for-babbage branch from b1e0a82 to 9265bcd Compare July 6, 2022 23:34
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Crypto.certifiedOutput
$ Crypto.evalCertified () (TPraos.mkSeed TPraos.seedL slot epochNonce) vrfSkey'
-> Either LeadershipError (Set SlotNo)
isLeadingSlotsTPraos slotRangeOfInterest poolid snapshotPoolDistr eNonce vrfSkey activeSlotCoeff' = do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(Right . ShelleyAPI.individualPoolStake)
(Map.lookup poolHash setSnapshotPoolDistr)
let slotRangeOfInterest = Set.filter
(not . Ledger.isOverlaySlot firstSlotOfEpoch (getField @"_d" (toLedgerPParams sbe pParams)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 newhoggy force-pushed the newhoggy/fix-leadership-schedule-for-babbage branch from 9265bcd to 207cb9f Compare July 8, 2022 00:17
@newhoggy
Copy link
Contributor Author

newhoggy commented Jul 8, 2022

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 8, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants