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

update plutus to alonzo/rc-2 #2425

Merged
merged 3 commits into from
Aug 17, 2021
Merged

update plutus to alonzo/rc-2 #2425

merged 3 commits into from
Aug 17, 2021

Conversation

JaredCorduan
Copy link
Contributor

@JaredCorduan JaredCorduan commented Aug 17, 2021

This PR ended up being less trivial than I expected, so I will leave some notes here for posterity.

  • Plutus no longer restricts the bytestring inside Data to 64 bytes, so our tests and CDDL have been adapted accordingly.
  • The Plutus Data Constructor 102 is now serialized as #6.102([uint, [* a]]) instead of #6.102([uint, * a]).
  • The serialized size of the Plutus cost model was problematically large. We no longer transmit the keys, only the values (and assume they are sent in ascending order by key name).
  • I have moved the Alonzo trace classifier test to the nightly tests, there is a discard problem that we cannot locate and there were far too many failed CI tests to be worth it (for now).

@JaredCorduan JaredCorduan force-pushed the jc/update-plutus branch 2 times, most recently from 148a0f0 to b7e5068 Compare August 17, 2021 12:01
@JaredCorduan JaredCorduan changed the title update plutus to alonzo/rc-1 update plutus to alonzo/rc-2 Aug 17, 2021
@@ -264,7 +264,7 @@ plutus_data = ; New
/ { * plutus_data => plutus_data }
/ [ * plutus_data ]
/ big_int
Copy link
Contributor

Choose a reason for hiding this comment

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

big_int can be any big int now for the same reason.

@JaredCorduan JaredCorduan merged commit d5b184a into master Aug 17, 2021
@iohk-bors iohk-bors bot deleted the jc/update-plutus branch August 17, 2021 21:28
Left e -> fail e
Right cm -> pure cm

decodeArrayAsMap :: Ord a => Set a -> Decoder s b -> Decoder s (Map a b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so we gave you the cost model as Map Text Integer to make it very homogenous, and so you could work with it somewhat independently of the API we give you. But if we're using defaultCostModelParams during deserialization, then that's kind of gone out the window.

In which case: it seems like this would be easier if it was Map ModelKey Integer where ModelKey has a Enum/Bounded instances? It also would make for a nicer, more generic function:

decodeArrayAsMap :: (Enum k, Bounded k, Ord k) => Decoder s v -> Decoder s (Map k v)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I agree, that would probably be nicer to work with and maintain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @michaelpj ! The enumeration will have to match what we are currently doing (ascending order by key) if we want to make this change sometime before the next hardfork after alonzo (since the cost model is a part of the script integrity hash).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point!

nc6 added a commit to IntersectMBO/cardano-node that referenced this pull request Aug 19, 2021
As of IntersectMBO/cardano-ledger#2425,
the ledger serialises only the values of a CostModel, rather than the
keys. This is to allow for far more compact serialisation (the string
names were taking a lot of space!) However, it means that we reassemble
the names from the Plutus default cost model parameters, and hence
generators must match these names.

This commit alters the cost model generator to simply set values over
the Plutus `defaultCostModelParams`.
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