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 reading of Plutus V2 cost models with different lengths in AlonzoGenesis in different eras #564

Merged

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Jun 24, 2024

Changelog

- description: |
    Fix reading of Plutus V2 cost models with different lengths in AlonzoGenesis in different eras
# 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

Part of the fix for:

Plutus V2 cost model for Babbage and for Conway have a different number of parameters. Ledger's JSON decoder for a map format of the cost model is unaware about this fact and just tries to validate for all parameters and load all 185 parameters no matter the era. This in order makes a problem for CBOR decoder, which is era-aware and will fail for a cost model with 185 parameters in Babbage.

This PR adds a function for how we decoding the cost model from JSON, which makes an adjustment in loaded parameters count depending on an era.

@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch 13 times, most recently from 0369191 to 0f7383e Compare July 1, 2024 21:49
@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from 0f7383e to f7d91b4 Compare July 2, 2024 13:00
@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from 3868745 to 8b5577b Compare July 4, 2024 14:31
@palas
Copy link
Contributor

palas commented Jul 5, 2024

FYI: Because we now enforce the use of cabal-gild to format cabal files and, in order to reduce the amount of hassle, I have cherry-picked the appropriate commit into your PR. This will make the cabal-gild CI check pass for the PR. The commit should be discarded automatically as soon as you either rebase or merge the PR (since it is already in the main branch). Feel free to discard it if you want, but that will make the new required action to fail until you rebase.

@palas palas force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from 3b81f26 to b298536 Compare July 6, 2024 02:41
@palas
Copy link
Contributor

palas commented Jul 6, 2024

FYI: I have rebased your branch because we have done changes to the formatting. I have made a copy of the unrebased branch here: backup/mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel

@palas palas force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from b298536 to 1636dbf Compare July 6, 2024 03:33
{-# LANGUAGE GeneralisedNewtypeDeriving #-}
{-# LANGUAGE LambdaCase #-}

Check warning

Code scanning / HLint

Unused LANGUAGE pragma Warning

cardano-api/internal/Cardano/Api/Genesis.hs:5:1-27: Warning: Unused LANGUAGE pragma
  
Found:
  {-# LANGUAGE LambdaCase #-}
  
Perhaps you should remove it.
{-# LANGUAGE GeneralisedNewtypeDeriving #-}
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TupleSections #-}

Check warning

Code scanning / HLint

Unused LANGUAGE pragma Warning

cardano-api/internal/Cardano/Api/Genesis.hs:7:1-30: Warning: Unused LANGUAGE pragma
  
Found:
  {-# LANGUAGE TupleSections #-}
  
Perhaps you should remove it.
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TupleSections #-}
{-# LANGUAGE TypeApplications #-}

Check warning

Code scanning / HLint

Unused LANGUAGE pragma Warning

cardano-api/internal/Cardano/Api/Genesis.hs:8:1-33: Warning: Unused LANGUAGE pragma
  
Found:
  {-# LANGUAGE TypeApplications #-}
  
Perhaps you should remove it.
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TupleSections #-}
{-# LANGUAGE TypeApplications #-}
{-# LANGUAGE TypeOperators #-}

Check warning

Code scanning / HLint

Unused LANGUAGE pragma Warning

cardano-api/internal/Cardano/Api/Genesis.hs:9:1-30: Warning: Unused LANGUAGE pragma
  
Found:
  {-# LANGUAGE TypeOperators #-}
  
Perhaps you should remove it.
  
Note: may require {-# LANGUAGE ExplicitNamespaces #-} adding to the top of the file
@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch 5 times, most recently from a0311c6 to 6faa4c4 Compare July 8, 2024 13:59
@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch 2 times, most recently from 2116359 to 5a49ca6 Compare July 8, 2024 15:49
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.

I think there is a significantly simpler way to approach this.

Parameterize cardano-api's alonzoGenesisDefaults on the era. If conway era is specified, add in the additional cost model parameters. If not don't add them. This should result in minimal changes. This will generate the appropriate cost model when using create-testnet-data and we should't have any issues so long as we generate an array instead of a json object. We should add a cli command that renders the array cost model as JSON. In future I want to switch to the array to avoid this issue.

@Jimbo4350 Jimbo4350 self-requested a review July 10, 2024 09:20
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.

LGTM however this code will disappear when we hardfork to Conway. In future we should avoid this situation completely by using an array in the genesis file and having a mechanism to pretty print the cost models (makes debugging easier).

When supplying an array, ledger calls mkCostModel which always retains the original values supplied. Plutus's mkEvaluationContext will then automatically adjust the cost model (either filling in missing values or truncating them).

Our pretty printing function can potentially do some validation and suggest to the user if they are missing parameters from their particular cost model. However I want to avoid us manually doing the validation as much as possible, to the point of requesting ledger/plutus to expose additional functions if needed.

Can you confirm with @mgmeier and @mkoura that this fixes their issue?

@carbolymer
Copy link
Contributor Author

Parameterize cardano-api's alonzoGenesisDefaults on the era. If conway era is specified, add in the additional cost model parameters. If not don't add them. This should result in minimal changes.

The problem occurs when we provide alonzo genesis specification from a file. We're not using alonzoGenesisDefaults there, so it won't solve this.

When supplying an array, ledger calls mkCostModel which always retains the original values supplied. Plutus's mkEvaluationContext will then automatically adjust the cost model (either filling in missing values or truncating them).

That's correct. The whole purpose of this fix was to support flexible map format.

Our pretty printing function can potentially do some validation and suggest to the user if they are missing parameters from their particular cost model.

I don't understand which pretty printing function are you referring to.

@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Jul 10, 2024

The problem occurs when we provide alonzo genesis specification from a file. We're not using alonzoGenesisDefaults there, so it won't solve this.

Hmm. Micheal said "When I create genesis, and then manually remove 10 list entries from the V2 cost model, the pparam query in Babbage works" so I was under the assumption they were not supplying a cost model file. @mgmeier?

I don't understand which pretty printing function are you referring to.

We would have to implement it.

The whole purpose of this fix was to support flexible map format.

Yes but this requires us to maintain code that we should not be maintaining. Benchmarking wants to be able to view the cost model as a json map. We can give them that whilst avoiding the additional gymnastics around decoding the cost model from a json map. In the future its possible for this situation to arise again and we can (and should) avoid it. We would unfortunately have to enforce cost models being supplied as arrays.

@carbolymer
Copy link
Contributor Author

Yes but this requires us to maintain code that we should not be maintaining

I completely agree. I'll gladly remove this code when we won't need to support this edge case anymore.

Benchmarking wants to be able to view the cost model as a json map.

I understood that the use case is to be able to define and inspect cost model in JSON and not just only view it. I guess we could provide separate translation layer for them doing pretty much what's done in this decoding function. It still means that we have to fix ledger bugs in cardano-api either way.

@palas
Copy link
Contributor

palas commented Jul 11, 2024

FYI: I have rebased your branch because we have done changes to the formatting. I have made a copy of the unrebased branch here: https://github.com/IntersectMBO/cardano-api/tree/backup2/mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel

@palas palas force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from e2124ac to ca59f40 Compare July 11, 2024 16:38
@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from ca59f40 to e2124ac Compare July 12, 2024 13:22
@palas palas closed this Jul 12, 2024
@palas palas force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from e2124ac to 6dd96c6 Compare July 12, 2024 16:57
@palas palas reopened this Jul 12, 2024
@palas palas force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch 4 times, most recently from 1e9c1a8 to 1774476 Compare July 12, 2024 20:06
@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from 1774476 to 2468b1f Compare July 15, 2024 14:32
@carbolymer carbolymer force-pushed the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch from 2468b1f to e289988 Compare July 16, 2024 11:11
@carbolymer carbolymer added this pull request to the merge queue Jul 18, 2024
Merged via the queue into main with commit 25b3433 Jul 18, 2024
25 checks passed
@carbolymer carbolymer deleted the mgalazyn/fix/allow-reading-plutus-v2-175-param-costmodel branch July 18, 2024 17:50
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.

3 participants