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

introduce flexible cost model decoding #3283

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Conversation

JaredCorduan
Copy link
Contributor

@JaredCorduan JaredCorduan commented Feb 3, 2023

Description

Starting in version 9, CostModels can now be deserialized from any map of Word8 values to lists of integers. Only valid cost models are actually converted to evaluation contexts that can be used. Errors and unrecognized language versions are stored in the CostModels type so that:

  • they can accept cost models that they do not yet understand
  • upon deserializing after a software update, new cost models are available from the prior serialization.

resolves #2902

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu (which can be run with scripts/fourmolize.sh
  • Self-reviewed the diff

@JaredCorduan JaredCorduan force-pushed the jc/graceful-costmodel branch 5 times, most recently from 4534985 to 5ff2e3e Compare February 7, 2023 20:42
Copy link
Contributor

@teodanciu teodanciu left a comment

Choose a reason for hiding this comment

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

Looks good to me.

The Scripts module is becoming a big unwieldy and losing cohesion, so maybe if we need to add stuff to CostModel we can make it a separate module.
Just some other small remarks 👍

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This is really awesome!!!

It is only after going through this PR that I was able to convince myself that this is a really good approach to deal with forward compatibility of CostModels. I could not think of anything that could mess it up. Very nice!

I have a few suggestions, but nothing that would block the PR.

eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Scripts.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Scripts.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Scripts.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Scripts.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Scripts.hs Outdated Show resolved Hide resolved
eras/alonzo/impl/src/Cardano/Ledger/Alonzo/Scripts.hs Outdated Show resolved Hide resolved
eras/conway/test-suite/cddl-files/conway.cddl Outdated Show resolved Hide resolved
@JaredCorduan JaredCorduan force-pushed the jc/graceful-costmodel branch from 0152368 to de24b06 Compare February 9, 2023 03:45
Starting in version 9, 'CostModels' can now be deserialized
from any map from Word8 values to lists of integers.
Only valid cost models are actual converted to cost models.

resolves #2902
@JaredCorduan JaredCorduan enabled auto-merge (rebase) February 15, 2023 22:39
@JaredCorduan JaredCorduan merged commit da48c5b into master Feb 15, 2023
@iohk-bors iohk-bors bot deleted the jc/graceful-costmodel branch February 15, 2023 23:57
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.

Problematic cost model serialization
3 participants