Add cost models file to protocol parameter update command #405

carlhammann · 2023-10-26T14:15:09Z

Changelog

- description: |
    Add cost models file to the protocol parameter update commands
# 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

This PR closes #386, by adding a --new-cost-model-file to the cardano-cli * governance action create-protocol-parameters-update and the cardano-cli [legacy] governance create-update-proposal commands. The option only appears from Alonzo era onwards.

I also disambiguated the flags of these commands, because there was a collision:--governance-action-deposit is required for every governance action, and thus the name of the flag to specify a proposed change in that parameter has to have a different name. I chose let all flags for proposed changes have the format --new-*.

There is a test that the proposal is generated correctly in Conway era. ~~I couldn't find a valid PlutusV3 cost model to add to my tests, though. So, the test currently only covers PlutusV1 and PlutusV2. @CarlosLopezDeLara do you know where I could find such a thing?~~ I couldn't find a valid Plutus V3 cost model anywhere. There's no PlutusV3 cost model in the book, and the one in the one here (which @CarlosLopezDeLara pointed me to) doesn't deserialise. So, I built my own V3 cost model to use as input for the tests: I had a look here to find the right ordering of the parameters and then just set each of them to 1.

How to trust this PR

I realise this is a big diff, so I'll leave some pointers in separate comments below.

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

carlhammann · 2023-10-26T14:18:53Z

cardano-cli/src/Cardano/CLI/EraBased/Commands/Governance/Actions.hs

+      -- | New parameters to be proposed. From Alonzo onwards, the type
+      -- 'EraBasedProtocolParametersUpdate' also contains cost models. Since all
+      -- other protocol parameters are read from command line arguments, whereas
+      -- the cost models are read from a file, we separate the cost models from
+      -- the rest of the protocol parameters to ease parsing.
+      , uppNewPParams :: !(EraBasedProtocolParametersUpdate era)
+      -- | The new cost models proposed. See the comment at 'uppNewPParams' for
+      -- why this is a separate field.
+      , uppCostModelsFile :: !(Maybe (CostModelsFile era))


This is the core of the PR: I'm adding a separate field for the cost models. Notice that CostmodelsFile contains an AlonzoEraOnwards witness.

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

carlhammann · 2023-10-26T14:23:01Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs

+pCostModelsFile :: ShelleyBasedEra era -> Parser (Maybe (Cmd.CostModelsFile era))
+pCostModelsFile =
+  caseShelleyToMaryOrAlonzoEraOnwards
+    (const $ pure Nothing)
+    (\alonzoOnwards -> fmap (fmap $ Cmd.CostModelsFile alonzoOnwards . File) . optional $ pCostModels (Just "new"))


This is the era-sensitive parser for the cost models file. Compare the golden files for the help texts of Alonzo and Mary, say.

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

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs

carbolymer

I'm not so sure about prefixes to the CLI parameters - I'm leaving the final approval to the others. Otherwise, LGTM, good work!

Jimbo4350

A couple of comments

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

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs

Jimbo4350 · 2023-11-01T14:17:15Z

cardano-cli/src/Cardano/CLI/EraBased/Run/Governance/Actions.hs

@@ -302,6 +320,26 @@ runGovernanceActionCreateProtocolParametersUpdateCmd eraBasedPParams' = do
    )
    sbe

+addCostModelsToEraBasedProtocolParametersUpdate


We should write a round trip property test for this.

carlhammann · 2023-11-20T11:39:10Z

With #463 merged, we can now use the generators from cardano-api in the roundtrip property test. I think this is now ready.

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Action.hs

newhoggy · 2023-11-21T10:30:41Z

cardano-cli/test/cardano-cli-golden/files/input/governance/costmodels.json

+      "blake2b_224-cpu-arguments-slope": 1,
+      "blake2b_224-memory-arguments": 1
+  }
+}


Where did this file come from?

I made it, because I couldn't find a valid Plutus V3 cost model anywhere. There's no valid PlutusV3 cost model in the book, and the one in the one here (which @CarlosLopezDeLara pointed me to) doesn't deserialise. So I had a look here to find the right ordering of the parameters and then just set each of them to 1.

For sure, this is sub-optimal.

Nice. Ledger can support the "old" cost model style :) https://github.com/input-output-hk/cardano-ledger/pull/3367/files#diff-f481d9eb9f793235d9fc4a1965ec1e0a01bed313f90adb11dcd9aa099a4cd738R348

cardano-cli/test/cardano-cli-test/Test/Cli/AddCostModels.hs

The requested property tests are now implemented

cardano-cli/test/cardano-cli-golden/files/input/governance/costmodels.json

Jimbo4350

Nice work 👍. This one looked a little finicky 😁.

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs

cardano-cli/src/Cardano/CLI/EraBased/Run/Governance/Actions.hs

Jimbo4350 · 2023-11-22T21:03:09Z

cardano-cli/test/cardano-cli-golden/files/input/governance/costmodels.json

+      "blake2b_224-cpu-arguments-slope": 1,
+      "blake2b_224-memory-arguments": 1
+  }
+}


Nice. Ledger can support the "old" cost model style :) https://github.com/input-output-hk/cardano-ledger/pull/3367/files#diff-f481d9eb9f793235d9fc4a1965ec1e0a01bed313f90adb11dcd9aa099a4cd738R348

…pdate Use whole cost models in the roundtrip test

carlhammann requested review from Jimbo4350, newhoggy, carbolymer, kevinhammond and disassembler as code owners October 26, 2023 14:15

carlhammann commented Oct 26, 2023

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs Show resolved Hide resolved

carlhammann commented Oct 26, 2023

View reviewed changes

carbolymer reviewed Oct 27, 2023

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs Outdated Show resolved Hide resolved

carbolymer reviewed Oct 27, 2023

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs Outdated Show resolved Hide resolved

carbolymer reviewed Oct 27, 2023

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/Governance/Actions.hs Outdated Show resolved Hide resolved

carbolymer reviewed Oct 27, 2023

View reviewed changes

carlhammann force-pushed the ch/update-costmodels branch from 0f3e1cb to 4baa562 Compare October 27, 2023 14:06

carlhammann changed the title ~~Ch/update costmodels~~ Add cost models file to protocol parameter update command Nov 1, 2023

Jimbo4350 previously requested changes Nov 1, 2023

View reviewed changes

carlhammann force-pushed the ch/update-costmodels branch 2 times, most recently from 4a81c6c to 2e034e9 Compare November 2, 2023 14:47

carlhammann mentioned this pull request Nov 3, 2023

Add generators for EraBasedProtocolParametersUpdate IntersectMBO/cardano-api#352

Merged

3 tasks

carlhammann force-pushed the ch/update-costmodels branch 5 times, most recently from bf5523f to 038547c Compare November 20, 2023 11:38

carlhammann requested a review from Jimbo4350 November 20, 2023 11:39

carlhammann requested review from a team as code owners November 21, 2023 10:08

newhoggy reviewed Nov 21, 2023

View reviewed changes

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Action.hs Show resolved Hide resolved

newhoggy reviewed Nov 21, 2023

View reviewed changes

cardano-cli/test/cardano-cli-test/Test/Cli/AddCostModels.hs Outdated Show resolved Hide resolved

newhoggy self-requested a review November 21, 2023 10:35

newhoggy approved these changes Nov 21, 2023

View reviewed changes

carlhammann force-pushed the ch/update-costmodels branch from 343754f to b8cd44a Compare November 21, 2023 11:02

catch-21 reviewed Nov 22, 2023

View reviewed changes

cardano-cli/test/cardano-cli-golden/files/input/governance/costmodels.json Show resolved Hide resolved

Jimbo4350 approved these changes Nov 22, 2023

View reviewed changes

carlhammann added 5 commits November 23, 2023 16:50

Add cost models to protocol parameter update

bb2960d

Sketch round trip test for addCostModelsToEraBasedProtocolParametersU…

02628c1

…pdate Use whole cost models in the roundtrip test

Add a test for a partial cost models file

4c00091

Address some review comments from Jordan

250443a

Document and test the cost model file format

0498486

carlhammann force-pushed the ch/update-costmodels branch from 98e5fbe to 0498486 Compare November 23, 2023 15:56

carlhammann enabled auto-merge November 23, 2023 15:56

carlhammann added this pull request to the merge queue Nov 23, 2023

Merged via the queue into main with commit cead75f Nov 23, 2023
19 checks passed

carlhammann deleted the ch/update-costmodels branch November 23, 2023 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cost models file to protocol parameter update command #405

Add cost models file to protocol parameter update command #405

carlhammann commented Oct 26, 2023 •

edited

Loading

carlhammann Oct 26, 2023

carlhammann Oct 26, 2023

carbolymer left a comment

Jimbo4350 left a comment

Jimbo4350 Nov 1, 2023

carlhammann commented Nov 20, 2023

newhoggy Nov 21, 2023

carlhammann Nov 21, 2023

Jimbo4350 Nov 22, 2023 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Nov 22, 2023 •

edited

Loading

Add cost models file to protocol parameter update command #405

Add cost models file to protocol parameter update command #405

Conversation

carlhammann commented Oct 26, 2023 • edited Loading

Changelog

Context

How to trust this PR

Checklist

carlhammann Oct 26, 2023

Choose a reason for hiding this comment

carlhammann Oct 26, 2023

Choose a reason for hiding this comment

carbolymer left a comment

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

Jimbo4350 Nov 1, 2023

Choose a reason for hiding this comment

carlhammann commented Nov 20, 2023

newhoggy Nov 21, 2023

Choose a reason for hiding this comment

carlhammann Nov 21, 2023

Choose a reason for hiding this comment

Jimbo4350 Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

carlhammann commented Oct 26, 2023 •

edited

Loading

Jimbo4350 Nov 22, 2023 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Nov 22, 2023 •

edited

Loading