Enable SoPs and all builtins in PlutusV1/V2 at PV11. #7223
Conversation
kwxm
added
Plutus Core
Builtins
Costing
|
| where no new builtins are added. See Note [New builtins/language versions and | ||
| protocol versions] | ||
| -} | ||
| builtinsIntroducedIn :: PlutusLedgerLanguage -> Map.Map MajorProtocolVersion (Set.Set DefaultFun) |
There was a problem hiding this comment.
This (and other functions in this module) are used to perform validity checks during on-chain deserialisation, so it's important that they're efficient. I think that things like batch2 ++ batch3 ++ batch4 ++ batch5 ++ batch6 will be computed at compile time, but Set.fromList and Map.fromList won't, so this isn't as efficient as it could be. It seems that this problem can be solved using Template Haskell, and I plan to look into this in a later PR: see this issue.
Another possibility would be to cache the results: see this comment.
There was a problem hiding this comment.
I had to rework these functions because the previous versions relied on assumptions that are no longer valid (specifically, the predicate in takeWhileAntitone must satisfy a certain property, and enabling new builtins in PlutusV1/V2 meant that this no longer held in the previous version of the function.
There was a problem hiding this comment.
Does this cause any performance regression? Previously builtinsIntroducedIn was a top-level Map that can be cached, but it is now a function.
There was a problem hiding this comment.
I ran the validation-decode benchmarks a few times and the results suggest that there's a slowdown of about 1.5-3% in deserialisation times. An earlier comment mentions this and some ways to deal with (like evaluate as much as possible at compile time) it and I plan to come back and look at it again in the near future.
Superficially this looks as if it could be more efficient than the previous version because that had to look at all (LL,PV) combinations and the new version only has to look at the PV values once we've callled the function to get everything for the relevant LL version. It does appear that it's not more efficient and I want to fix that. Also, the previous version no longer works because it assumes that if something's available in a praticular LL and PV then it's avaliable for all earlier LLs and PVs, and that no longer holds (we're enabling things in PV11 that aren't available in PV10, for example).
There was a problem hiding this comment.
Like you mentioned on Slack, this check can be significantly simplified from PV11. So perhaps you'd want to implement a separate check just for PV11?
There was a problem hiding this comment.
I think that things like batch2 ++ batch3 ++ batch4 ++ batch5 ++ batch6 will be computed at compile time
You don't know that, unless you've checked it. These are not small definitions and they're not marked as INLINE, GHC can easily avoid inlining them, in which case fusion doesn't kick in, in which case they don't get computed statically.
Previously builtinsIntroducedIn was a top-level Map that can be cached, but it is now a function.
All we need to make it efficient again is just to put
Map.fromList
[ (alonzoPV, Set.fromList batch1)
, (pv11PV, Set.fromList (batch2 ++ batch3 ++ batch4 ++ batch5 ++ batch6))
]etc into their own top-level definitions marked as OPAQUE.
There was a problem hiding this comment.
The tests here are still not ideal (see this issue), so we should come back and take another look at them.
There was a problem hiding this comment.
These tests needed substantial reworking because the previous versions relied on assumptions that are no longer true. What's really being tested in many tests is that functions like builtinsAvailableIn behave properly. These functions are used on the chain and it's important that they do what they're supposed to do, so I've tried to make the tests as comprehensive as possible.
There was a problem hiding this comment.
The changes to builtinsAvailableIn and related functions will make the builtins pass the deserialisation checks, making the builtins usable in principle on the chain at PV11, except that the cost model parameters also need to be updated in order to make the builtins (and SoPs) affordable . The changes to these files enable the update to work properly.
|
/benchmark validation-full |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'validation-full' on 'aeaafb922d' (base) and '3893622224' (PR) Results table
|
| parameters and enable them all at PV11 and with a suitable parameter update. | ||
| However, if we do do this there's a theoretical risk of turning a phase 2 | ||
| failure into a phase 1 failure: would that be problematic? | ||
| -} |
There was a problem hiding this comment.
The way the three sets of cost model parameters are set up now means that the PlutusV1 parameters are a permutation of the PlutusV2 parameters (but the cost models they represent wll be exactly the same). This is unavoidable because the parameters for serialiseData were already present in the middle of the V2 parameters but had to be added to the end of the previous version of the V1 parameters (where serialiseData didn't exist). The parameters for the two integer/bytestring conversion functions are also in different places, but the V2 ones are in the correct place for the as-yet-unenacted parameter update that will enable them. It may just be easier to leave things as they are and not try to match the parameters up. See the discussion here.
|
/benchmark validation-decode |
|
Click here to check the status of your benchmark. |
|
^ Not sure if that's benchmarking what I want; I'll check later. |
|
Comparing benchmark results of 'validation-decode' on 'aeaafb922d' (base) and '3893622224' (PR) Results table
|
|
/benchmark validation-decode |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'validation-decode' on 'aeaafb922d' (base) and '3893622224' (PR) Results table
|
|
/benchmark validation-decode |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'validation-decode' on 'aeaafb922d' (base) and '3893622224' (PR) Results table
|
|
/benchmark validation-decode |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'validation-decode' on 'aeaafb922d' (base) and '3893622224' (PR) Results table
|
| @@ -0,0 +1,7 @@ | |||
| <!-- | |||
| -- decide what PVs the test should include. UPDATE THIS when we're expecting to | ||
| -- release new builtins in a forthcoming PV. | ||
| newestPV :: MajorProtocolVersion | ||
| newestPV = anonPV |
There was a problem hiding this comment.
If new builtins are assigned to futurePV first, then changed to the appropriate PV when ready, then I don't see why we need this.
There was a problem hiding this comment.
If new builtins are assigned to
futurePVfirst, then changed to the appropriate PV when ready, then I don't see why we need this.
The preceding comment attempts to explain that. Some of the tests (here for instance) iterate over all of the deployed (or just-about-to-be-deployed) PVs and use newestPV to say where to stop. We can't use futurePV for that because it's maxBound. I added newestPV so that we wouldn't have to go through the tests and replace the actual most recent PV everywhere when we update it.
| where no new builtins are added. See Note [New builtins/language versions and | ||
| protocol versions] | ||
| -} | ||
| builtinsIntroducedIn :: PlutusLedgerLanguage -> Map.Map MajorProtocolVersion (Set.Set DefaultFun) |
There was a problem hiding this comment.
Does this cause any performance regression? Previously builtinsIntroducedIn was a top-level Map that can be cached, but it is now a function.
| -} | ||
| batch1 :: [DefaultFun] | ||
| batch1 = | ||
| [ AddInteger, SubtractInteger, MultiplyInteger, DivideInteger, QuotientInteger |
There was a problem hiding this comment.
It's tedious to check the correctness of these batches manually. How about keeping the old version of builtinsIntroducedIn, and adding some property tests to verify that the old and new versions always agree (up to PV11)?
There was a problem hiding this comment.
I'm not sure what you mean. Do you want to keep both the old and the new versions? The batches were implcitly present in the previous version and here I've pulled them out to the top level to make things a bit more explicit and easier to relate to the specification; also, they're used in the tests so it helps to be able to export them. Of course, you should only ever add anything to the most recent batch (or add a new batch); maybe I should add a prominent comment to point that out.
There was a problem hiding this comment.
I mean adding a test verifying that the new builtinsAvailableIn ll pv and the old builtinsAvailableIn ll pv always return the exact same set. This is especially important for pv=10. Or is this already covered in existing tests?
There was a problem hiding this comment.
So we'd have to keep the old builtinsAvailableIn function around specifically for the tests, even though it's not used? It gives incorrect results for PV11, so we'd have to modify it anyway: that's why I changed the implementation!
There was a problem hiding this comment.
This test should test that deserialisation succeeds or fails as expected for every single builtin in every LL/PV combination, and it's much more thorough than the previous version. It depends on batch1, batch2, etc being correct, but it's hard to see how to test the correctness of bultinsAvailableIn without using those since we don't have any independent descrioption of which builtins are available.
The batches in the new version start here and they're essentially just lifted from the previous version of builtinsAvailableIn, with batch4 having to be split into two to handle integerToByteString and byteStringToInteger in PlutusV2 at Plomin.
There was a problem hiding this comment.
So we'd have to keep the old builtinsAvailableIn function around specifically for the tests, even though it's not used?
Yeah, why not, we can move it to a test package. But if there's already a test that verifies that the new builtinsAvailableIn gives correct results for all ll/pv (especially PV10 and PV11), then we are good - that coverage is all we need.
There was a problem hiding this comment.
OK. The old version works correctly up to PV10, so I can move it into the test code somewhere and add a test that the new implementation matches up to PV10 (but not PV11, where it breaks).
| | Ripemd_160'cpu'arguments'intercept | ||
| | Ripemd_160'cpu'arguments'slope | ||
| | Ripemd_160'memory'arguments | ||
| -- Not yet deployed |
There was a problem hiding this comment.
Aren't these also to be deployed in PV11? Why are they "Not yet deployed"?
There was a problem hiding this comment.
I've changed that to "To be deployed in PV11", but I can take it out if you want. I may have copied it from the PlutusV3 parameters, where it marks the boundaries between the parameters for the things that are on the chain now and those that aren't.
A lot of the code in plutus-ledger-api is a bit weird because in a sort of in-between state where it's about stuff that's waiting to happen.
| pv11PV :: MajorProtocolVersion | ||
| pv11PV = MajorProtocolVersion 11 | ||
|
|
||
| -- | The set of protocol versions that are "known", i.e. that have been released |
There was a problem hiding this comment.
nit: I'm not quite sure what it means for a PV to be "released". It's probably more accurate to say: these are the PVs in which some Plutus primitives are enabled.
| -- associate something with the wrong protocol version. | ||
| -- We're sometimes in an intermediate state where we've added new builtins but | ||
| -- not yet released them (but intend to). This is used by some of the tests to | ||
| -- decide what PVs the test should include. UPDATE THIS when we're expecting to |
There was a problem hiding this comment.
Rather than saying "UPDATE THIS" in a comment here - which is extremely easy to overlook - it would be nice to have a checklist somewhere, that documents everything we need to do when we are ready to release new builtins.
There was a problem hiding this comment.
Ah, I have an issue to do that: thanks for reminding me. I'll put the issue in the current sprint so I can do it while the process is still fresh in my mind. I had in mind to write some kind of textual document, but maybe an issue template would be a better way to do it.
| at the moment, it's tempting to insert the 4a parameter before the 4b | ||
| parameters and enable them all at PV11 and with a suitable parameter update. | ||
| However, if we do do this there's a theoretical risk of turning a phase 2 | ||
| failure into a phase 1 failure: would that be problematic? |
There was a problem hiding this comment.
Good point - different node versions disagreeing on whether a failure is phase 1 or phase 2 can indeed be very problematic.
| machineParametersFor ledgerLang majorPV = | ||
| MachineParameters | ||
| (if majorPV < futurePV | ||
| (if majorPV < pv11PV |
| import PlutusLedgerApi.V1 qualified as V1 | ||
| import PlutusLedgerApi.V2 qualified as V2 | ||
| import PlutusLedgerApi.V3 qualified as V3 | ||
| -- import PlutusCore.Evaluation.Machine.ExBudgetingDefaults (defaultCostModelParamsForTesting) |
| where no new builtins are added. See Note [New builtins/language versions and | ||
| protocol versions] | ||
| -} | ||
| builtinsIntroducedIn :: PlutusLedgerLanguage -> Map.Map MajorProtocolVersion (Set.Set DefaultFun) |
There was a problem hiding this comment.
I think that things like batch2 ++ batch3 ++ batch4 ++ batch5 ++ batch6 will be computed at compile time
You don't know that, unless you've checked it. These are not small definitions and they're not marked as INLINE, GHC can easily avoid inlining them, in which case fusion doesn't kick in, in which case they don't get computed statically.
Previously builtinsIntroducedIn was a top-level Map that can be cached, but it is now a function.
All we need to make it efficient again is just to put
Map.fromList
[ (alonzoPV, Set.fromList batch1)
, (pv11PV, Set.fromList (batch2 ++ batch3 ++ batch4 ++ batch5 ++ batch6))
]etc into their own top-level definitions marked as OPAQUE.
3 participants
This fixes https://github.com/IntersectMBO/plutus-private/issues/1655 and https://github.com/IntersectMBO/plutus-private/issues/1656.
This will enable all builtins in PlutusV1 and PlutusV2 at PV11, and also sums of products. A protocol parameter update will also be required to update the cost model parameters before the new features become usable on the chain: the required parameters can be obtained using
dump-cost-model-parameters(see PR #7171).