create-testnet-data: allow to specify relays for SPOs (like create-staked) #632

smelc · 2024-03-05T10:08:14Z

Changelog

- description: |
    create-testnet-data: allow to specify relays for SPOs (like create-staked)
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Fixes #630

How to trust this PR

Run the following:

cabal run cardano-cli -- conway genesis create-testnet-data --out-dir create_testnet_out --stake-delegators 3 --pools 4 --utxo-keys 5 --genesis-keys 6 --testnet-magic 42 --relays pool-relays.json

pool-relays.json is the following file:

{
  "0": [
    {
      "single host name": {
        "dnsName": "node-0",
        "port": 30000
      }
    }
  ],
  "1": [
    {
      "single host name": {
        "dnsName": "node-1",
        "port": 30001
      }
    }
  ]
}

Then open create_testnet_out/genesis.json and observe that the pools have a non-empty entry in their relays field.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

smelc · 2024-03-05T16:27:36Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs

+    pRelays :: Parser FilePath
+    pRelays =
+      Opt.strOption $ mconcat
+        [ Opt.long "relays"


I chose a shorter flag name than create-staked's relay-specification-file. I feel the longer name didn't bring much value. We already chose to use different names in create-testnet-data before, so this should not be a surprise to users.

Why two different names for the same file?

Not sure what you mean @carbolymer: there is a single name in create-testnet-data and one single name in create-staked. I simply chose a shorter, I think better, name for create-testnet-data.

My question is why two different parmeter names relay-specification-file and relays? I agree with your choice, but why not just use relays everywhere, also in create-staked.

smelc · 2024-03-05T16:28:12Z

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

+  mSPOsRelays <- forM relays readRelays
+  case (relays, mSPOsRelays) of
+    (Just fp, Just stakePoolRelays) | Map.size stakePoolRelays > fromIntegral numPools ->
+      throwError $ GenesisCmdTooManyRelaysError fp (fromIntegral numPools) (Map.size stakePoolRelays)


This check wasn't in create-staked, but checked with @mgmeier that it was nice to have in create-testnet-data.

Just curious, what's wrong with having more relays than SPOs?

@Jimbo4350> it means the user is passing data that is being ignored. It's probably nice to tell the user that.

smelc · 2024-03-05T16:31:25Z

cardano-cli/src/Cardano/CLI/EraBased/Run/Genesis.hs

-           handleIOExceptT (GenesisCmdStakePoolRelayFileError fp) (LBS.readFile fp)
-         firstExceptT (GenesisCmdStakePoolRelayJsonDecodeError fp)
-           . hoistEither $ Aeson.eitherDecode relaySpecJsonBs
+  mayStakePoolRelays <- forM mStakePoolRelaySpecFile TN.readRelays


As in other parts of create-staked, it's create-staked's implementation that calls create-testnet-data's implementation when they are shared. So nothing new here.

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

Jimbo4350 · 2024-03-06T14:17:10Z

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

@@ -649,6 +657,18 @@ readAndDecodeShelleyGenesis fpath = runExceptT $ do
  firstExceptT (GenesisCmdGenesisFileDecodeError fpath . Text.pack)
    . hoistEither $ Aeson.eitherDecode' lbs

+-- @readRelays fp@ reads the relays specification from a file
+readRelays :: ()


We could have made the types here concrete.

Jimbo4350 · 2024-03-06T14:17:52Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs

+      Opt.strOption $ mconcat
+        [ Opt.long "relay-specification-file"
+        , Opt.metavar "FILE"
+        , Opt.help "JSON file specified the relays of each stake pool."


"that specifies"

Since this is in create-staked, I chose not to change anything 🙂

Jimbo4350 · 2024-03-06T14:29:14Z

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

+  case (relays, mSPOsRelays) of
+    (Just fp, Just stakePoolRelays) | Map.size stakePoolRelays > fromIntegral numPools ->
+      throwError $ GenesisCmdTooManyRelaysError fp (fromIntegral numPools) (Map.size stakePoolRelays)
+    _ -> pure ()


We can avoid the wildcard pattern (IMO this is less readable) match if you case on relays i.e

case relays of Nothing -> pure () Just relaySpecFile -> ...

@Jimbo4350> not much shorter with your version, because I need mSPOsRelays to be a Just to look at the size of the underlying Map. If I don't pattern match on it, I need to default Nothing to being of size 0, which is a bit cumbersome.

So I'll skip this one.

Jimbo4350 · 2024-03-06T14:30:24Z

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

-   poolColdVKF = File $ dir </> "cold" ++ strIndex ++ ".vkey"
-   poolVrfVKF = File $ dir </> "vrf" ++ strIndex ++ ".vkey"
-   poolRewardVKF = File $ dir </> "staking-reward" ++ strIndex ++ ".vkey"
+   lookupPoolRelay :: Map Word [L.StakePoolRelay] -> Seq.StrictSeq L.StakePoolRelay


Jimbo4350 · 2024-03-06T14:31:19Z

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

+  mSPOsRelays <- forM relays readRelays
+  case (relays, mSPOsRelays) of
+    (Just fp, Just stakePoolRelays) | Map.size stakePoolRelays > fromIntegral numPools ->
+      throwError $ GenesisCmdTooManyRelaysError fp (fromIntegral numPools) (Map.size stakePoolRelays)


Just curious, what's wrong with having more relays than SPOs?

smelc force-pushed the smelc/create-testnet-data-relays branch 3 times, most recently from 128617d to 33d3ad8 Compare March 5, 2024 16:26

smelc marked this pull request as ready for review March 5, 2024 16:26

smelc requested review from Jimbo4350, newhoggy, carbolymer, kevinhammond and disassembler as code owners March 5, 2024 16:26

github-advanced-security bot found potential problems Mar 5, 2024

View reviewed changes

smelc commented Mar 5, 2024

View reviewed changes

smelc force-pushed the smelc/create-testnet-data-relays branch from 33d3ad8 to 6ee4235 Compare March 5, 2024 16:29

smelc commented Mar 5, 2024

View reviewed changes

smelc force-pushed the smelc/create-testnet-data-relays branch 2 times, most recently from 2820153 to 38a1669 Compare March 6, 2024 09:13

carbolymer reviewed Mar 6, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs Outdated Show resolved Hide resolved

carbolymer approved these changes Mar 6, 2024

View reviewed changes

smelc added 5 commits March 6, 2024 14:08

Détail: add doc

51b2be8

create-staked: prepare relays reading for reuse

624c20f

create-testnet-data: allow relays to be specified

cb41627

Adapt golden files

69ddf03

create-testnet-data: Extend existing test to test relays

829f70e

smelc force-pushed the smelc/create-testnet-data-relays branch from b717fd6 to 829f70e Compare March 6, 2024 13:08

smelc added this pull request to the merge queue Mar 6, 2024

Merged via the queue into main with commit 156820e Mar 6, 2024
17 checks passed

smelc deleted the smelc/create-testnet-data-relays branch March 6, 2024 13:32

Jimbo4350 reviewed Mar 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create-testnet-data: allow to specify relays for SPOs (like create-staked) #632

create-testnet-data: allow to specify relays for SPOs (like create-staked) #632

smelc commented Mar 5, 2024 •

edited

smelc Mar 5, 2024

carbolymer Mar 6, 2024

smelc Mar 6, 2024

carbolymer Mar 7, 2024

smelc Mar 5, 2024

Jimbo4350 Mar 6, 2024

smelc Mar 6, 2024

smelc Mar 5, 2024

Jimbo4350 Mar 6, 2024

Jimbo4350 Mar 6, 2024

smelc Mar 6, 2024

Jimbo4350 Mar 6, 2024

smelc Mar 6, 2024

Jimbo4350 Mar 6, 2024

Jimbo4350 Mar 6, 2024

create-testnet-data: allow to specify relays for SPOs (like create-staked) #632

create-testnet-data: allow to specify relays for SPOs (like create-staked) #632

Conversation

smelc commented Mar 5, 2024 • edited

Changelog

Context

How to trust this PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelc commented Mar 5, 2024 •

edited