IntersectMBO / cardano-api

Cardano API
Apache License 2.0
26 stars 23 forks source link

Fix reading of Plutus V2 cost models with different lengths in AlonzoGenesis in different eras #564

Closed carbolymer closed 3 months ago

carbolymer commented 4 months ago

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.

palas commented 4 months ago

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 commented 4 months ago

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

carbolymer commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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