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

cardano-testnet: separate in types the options encoded in genesis files, from other options #5976

Merged
merged 10 commits into from
Sep 13, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Sep 5, 2024

Description

This PR implements a decision made on Slack: we wanted to change that parameters of cardanoTestnet are containing some values both in the CardanoTestnetOptions type and in the genesis files, causing confusion.

So this PR changes cardanoTestnet input types so that this duplication is removed: CardanoTestnetOptions now has less fields.

Note that Haskell callers to cardanoTestnet should now obtain the default value for CardanoTestnetDefault by calling def (from the Default class), instead of using cardanoDefaultTestnetOptions (which was removed).

How to review this PR

The commits are iterative, because I was testing out the design while writing this PR, and so every commit compiles and makes sense on its own. But it's nevertheless better to just look at the global diff in one shot.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Self-reviewed the diff

9223372036854775807,
9223372036854775807,
9223372036854775807
1292075,
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 don't know sure what's causing these changes. What I did in this PR should have no runtime effect, so I'm wondering.

@carbolymer> would you have a clue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't read your code thoroughly but you're using data-default-class, maybe it pulled a sane plutus v2 cost model default from ledger instead of 2^63-1?

Copy link
Contributor Author

@smelc smelc Sep 6, 2024

Choose a reason for hiding this comment

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

OK found from where the data comes. It's those costs that are showing up now: https://github.com/IntersectMBO/cardano-api/blob/4b3e3305e8bcb5b7eef33cdd1baf1b93d330a7b2/cardano-api/internal/Cardano/Api/Genesis.hs#L726

Still don't know what causes this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK found it: https://github.com/IntersectMBO/cardano-node/pull/5976/files#r1749955020

The change is correct: before, we were not honoring the user-supplied era in cardanoTestnetDefault.

@smelc smelc force-pushed the smelc/separate-cardano-testnet-options-stages branch 2 times, most recently from 8b7dacf to 28be576 Compare September 9, 2024 09:38
cardanoTestnetDefault opts conf = do
AnyShelleyBasedEra sbe <- pure $ cardanoNodeEra cardanoDefaultTestnetOptions
cardanoTestnetDefault testnetOptions shelleyOptions conf = do
AnyShelleyBasedEra sbe <- pure cardanoNodeEra
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This explains the change in the protocol parameters golden file: before we were not honoring the era passed within the CardanoTestnetOptions parameters. We were wrongly using cardanoDefaultTestnetOptions.

Now we take the era from the input parameter.

@smelc smelc marked this pull request as ready for review September 9, 2024 09:54
@smelc smelc requested a review from a team as a code owner September 9, 2024 09:54
@smelc smelc force-pushed the smelc/separate-cardano-testnet-options-stages branch from 28be576 to 2cf3bfb Compare September 11, 2024 09:08
Copy link
Contributor

@erikd erikd left a comment

Choose a reason for hiding this comment

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

This needs a rebase but otherwise LGTM.

@smelc smelc force-pushed the smelc/separate-cardano-testnet-options-stages branch from 2cf3bfb to a25c6c1 Compare September 13, 2024 07:53
@smelc smelc added this pull request to the merge queue Sep 13, 2024
Merged via the queue into master with commit e6a9c24 Sep 13, 2024
24 of 25 checks passed
@smelc smelc deleted the smelc/separate-cardano-testnet-options-stages branch September 13, 2024 09:28
-- See 'cardanoTestnet' for additional documentation.
cardanoTestnetDefault
:: ()
=> HasCallStack
=> CardanoTestnetOptions
-> ShelleyTestnetOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

We now have two testnet options types here. This is not ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the naming is bad. Would ShelleyTestnetOptions better be named GenesisOptions maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

GenesisOptions is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR doing the renaming: #5987

-- in the parser, and then get split into 'CardanoTestnetOptions' and
-- 'ShelleyTestnetOptions'
data CardanoTestnetCliOptions = CardanoTestnetCliOptions
{ cliTestnetOptions :: CardanoTestnetOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between these two types? We should not have two types that describe the testnet options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jimbo4350> the difference is explained in the haddocks of the definitions of those types:

-- | Options which, contrary to 'ShelleyTestnetOptions' are not implemented
-- by tuning the genesis files.
data CardanoTestnetOptions = CardanoTestnetOptions

and

-- | Options that are implemented by writing fields in the Shelley genesis file.
data ShelleyTestnetOptions = ShelleyTestnetOptions

}

-- | Options that are implemented by writing fields in the Shelley genesis file.
data ShelleyTestnetOptions = ShelleyTestnetOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

This split seems arbitrary. Why are we introducing another configuration type?

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 think the split is clear: ShelleyTestnetOptions contains values that are encoded in genesis files whereas CardanoTestnetOptions contains the rest of the values.

This is visible e.g. in the new prototype of cardanoTestnet that contains CardanoTestnetOptions and the genesis files, as opposed to cardanoTestnetDefault. cardanoTestnetDefault has both ShelleyTestnetOptions and CardanoTestnetOptions, because the genesis files are not yet around at this stage.

Copy link
Contributor

@Jimbo4350 Jimbo4350 Sep 16, 2024

Choose a reason for hiding this comment

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

I think the source of confusion for me came from cardanoMaxSupply existing in both types. The naming is also confusing. Cardano vs Shelley Tesnetoptions. The difference should be immediately obvious in the names (you suggested GenesisOptions which is better).

We have:

data CardanoTestnetOptions = CardanoTestnetOptions
  { -- | List of node options. Each option will result in a single node being
    -- created.
    cardanoNodes :: [TestnetNodeOptions]
  , cardanoNodeEra :: AnyShelleyBasedEra -- ^ The era to start at
  , cardanoMaxSupply :: Word64 -- ^ The amount of Lovelace you are starting your testnet with (forwarded to shelley genesis)
                               -- TODO move me to ShelleyTestnetOptions when https://github.com/IntersectMBO/cardano-cli/pull/874 makes it to cardano-node
  , cardanoEnableP2P :: Bool
  , cardanoNodeLoggingFormat :: NodeLoggingFormat
  , cardanoNumDReps :: Int -- ^ The number of DReps to generate at creation
  , cardanoEnableNewEpochStateLogging :: Bool -- ^ if epoch state logging is enabled
  } deriving (Eq, Show)

I think we should aim for:

data CardanoTestnetOptions = CardanoTestnetOptions
  { cardanoNodes :: [TestnetNodeOptions]
  }

There are other random configuration values here e.g cardanoEnableP2P and cardanoNumDReps which do not belong here and then we have to reconcile configuration overrides which just leads to more confusion. We are trying to do too much wrt the configuration and I think leaning more on cardano-cli to generate the configuration is the right direction. We will eliminate a lot of potential bug sources doing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

These overrides can live in create-testnet-data but we also have the issue of relisting all of the potential configuration values in the create-testnet-data which obviously isn't sensible. Maybe we can move the existing overrides to create-testnet-data.

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