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.

Sign up for GitHub

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

Jump to bottom

create-protocol-parameters-update: validate cost models' size #1012

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

create-protocol-parameters-update: validate cost models' size #1012

wants to merge 4 commits into from

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Jan 9, 2025

Changelog

- description: |
    create-protocol-parameters-update: validate cost models' size
# 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
  # - refactoring    # QoL changes
  # - 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 #928

How to trust this PR

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

@smelc smelc added the next-update Needs a dependency to be updated to be merged label Jan 9, 2025
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 9, 2025
View reviewed changes
cardano-cli/src/Cardano/CLI/Read.hs
Comment on lines +1117 to +1121
Just (model :: L.CostModel) -> do
let actual = Map.size (L.costModelToMap model)
expected = languageToParameterCount lang
unless (expected == actual) $ throwE $ CostModelsErrorWrongSize expected actual
return ()

Check warning

Code scanning / HLint

Redundant return Warning

cardano-cli/src/Cardano/CLI/Read.hs:(1117,38)-(1121,17): Warning: Redundant return
  
Found:
  do let actual = Map.size (L.costModelToMap model)
         expected = languageToParameterCount lang
     unless (expected == actual)
       $ throwE $ CostModelsErrorWrongSize expected actual
     return ()
  
Perhaps:
  do let actual = Map.size (L.costModelToMap model)
         expected = languageToParameterCount lang
     unless (expected == actual)
       $ throwE $ CostModelsErrorWrongSize expected actual
smelc
smelc commented Jan 9, 2025
View reviewed changes
cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Action.hs Outdated
hprop_golden_conway_governance_action_create_protocol_parameters_bad_costmodel_size =
propertyOnce . H.moduleWorkspace "tmp" $ \tempDir -> do
-- This file has 184 entries, whereas PV1 requires 166
-- TODO @smelc this test does not pass: the command succeeds, whereas it should fail!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that for PV1 the ledger deserialization fixes the problem by filling in missing parameters. Investigating. (this could also be an issue if the ledger is automatically trimming extra parameters, I need to check that too).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's something along those lines. The defaulting logic may also change depending whether the cost model is an array or a map. 🫠

ledger is automatically trimming extra parameters

I think it is not, we're only doing trimming in alonzo genesis reading, because the node didn't want to start in some cases.

carbolymer
carbolymer reviewed Jan 10, 2025
View reviewed changes
cardano-cli/src/Cardano/CLI/Read.hs
L.PlutusV2 ->
let nbParamNames = length $ allValues @PlutusV2.ParamName in -- 185
caseShelleyToBabbageOrConwayEraOnwards
(const $ nbParamNames - 10) -- Ten parameters were added to V2 in Conway, need to remove them here
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure about hardcoding that number 10. We should account for the fact that the number of params can change in the future, however it's unlikely. That's why there's jumping over many hoops in Cardano.Api.Genesis and Test.Cardano.Api.Genesis, so we will get a clear signal when things unexpectedly change.

cardano-cli/src/Cardano/CLI/Read.hs
return costModels
where
checkCostModelSize :: L.CostModels -> L.Language -> ExceptT CostModelsError IO ()
checkCostModelSize models lang =
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 those checks should be in cardano-api so we can reuse them in decoding genesis as well, because genesis can contain PV2 and PV3 models. This is used only in starting testnets afaik.

cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/Action.hs Outdated
hprop_golden_conway_governance_action_create_protocol_parameters_bad_costmodel_size =
propertyOnce . H.moduleWorkspace "tmp" $ \tempDir -> do
-- This file has 184 entries, whereas PV1 requires 166
-- TODO @smelc this test does not pass: the command succeeds, whereas it should fail!
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's something along those lines. The defaulting logic may also change depending whether the cost model is an array or a map. 🫠

ledger is automatically trimming extra parameters

I think it is not, we're only doing trimming in alonzo genesis reading, because the node didn't want to start in some cases.

cardano-cli/src/Cardano/CLI/Read.hs
languageToParameterCount = \case
L.PlutusV1 -> length $ allValues @PlutusV1.ParamName -- 166
L.PlutusV2 ->
let nbParamNames = length $ allValues @PlutusV2.ParamName in -- 185
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let nbParamNames = length $ allValues @PlutusV2.ParamName in -- 185
let nParamNames = length $ allValues @PlutusV2.ParamName in -- 185

😄

@smelc smelc force-pushed the smelc/validate-cost-models-size branch from ad333c5 to 20fee47 Compare January 10, 2025 15:36
@smelc smelc force-pushed the smelc/validate-cost-models-size branch from 20fee47 to a2ce41c Compare January 10, 2025 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer left review comments

@Jimbo4350 Jimbo4350 Awaiting requested review from Jimbo4350 Jimbo4350 will be requested when the pull request is marked ready for review Jimbo4350 is a code owner

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy will be requested when the pull request is marked ready for review newhoggy is a code owner

@palas palas Awaiting requested review from palas palas will be requested when the pull request is marked ready for review palas is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond will be requested when the pull request is marked ready for review kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler will be requested when the pull request is marked ready for review disassembler is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
next-update Needs a dependency to be updated to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] - Input cost models validation
2 participants
@smelc @carbolymer