-
Notifications
You must be signed in to change notification settings - Fork 483
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
Require PlutusV3 scripts to evaluate to BuiltinUnit #6159
Changes from 4 commits
bf50cca
a57f342
5dad08e
5507065
7f17ccb
85e8a35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
### Changed | ||
|
||
- `evaluateScriptRestricting` and `evaluateScriptCounting` now require Plutus V3 | ||
scripts to return `BuiltinUnit`, otherwise the evaluation is considered to | ||
have failed, and a `NonUnitReturnValue` error is thrown. There is no such | ||
requirement on Plutus V1 and V2 scripts. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ module PlutusLedgerApi.Common.Eval | |
) where | ||
|
||
import PlutusCore | ||
import PlutusCore.Builtin (readKnown) | ||
import PlutusCore.Data as Plutus | ||
import PlutusCore.Default | ||
import PlutusCore.Evaluation.Machine.CostModelInterface as Plutus | ||
|
@@ -53,6 +54,7 @@ data EvaluationError = | |
| CodecError !ScriptDecodeError -- ^ A deserialisation error | ||
-- TODO: make this error more informative when we have more information about what went wrong | ||
| CostModelParameterMismatch -- ^ An error indicating that the cost model parameters didn't match what we expected | ||
| NonUnitReturnValue -- ^ The script evaluated to a value that is not the builtin unit. | ||
deriving stock (Show, Eq) | ||
makeClassyPrisms ''EvaluationError | ||
|
||
|
@@ -64,6 +66,7 @@ instance Pretty EvaluationError where | |
pretty (DeBruijnError e) = pretty e | ||
pretty (CodecError e) = pretty e | ||
pretty CostModelParameterMismatch = "Cost model parameters were not as we expected" | ||
pretty NonUnitReturnValue = "The evaluation finished but the result is not unit" | ||
|
||
-- | A simple toggle indicating whether or not we should accumulate logs during script execution. | ||
data VerboseMode = | ||
|
@@ -226,8 +229,7 @@ evaluateScriptRestricting ll pv verbose ectx budget p args = swap $ runWriter @L | |
appliedTerm <- mkTermToEvaluate ll pv p args | ||
let (res, UPLC.RestrictingSt (ExRestrictingBudget final), logs) = | ||
evaluateTerm (UPLC.restricting $ ExRestrictingBudget budget) pv verbose ectx appliedTerm | ||
tell logs | ||
liftEither $ first CekError $ void res | ||
processLogsAndErrors ll logs res | ||
pure (budget `minusExBudget` final) | ||
|
||
{-| Evaluates a script, returning the minimum budget that the script would need | ||
|
@@ -249,10 +251,31 @@ evaluateScriptCounting ll pv verbose ectx p args = swap $ runWriter @LogOutput $ | |
appliedTerm <- mkTermToEvaluate ll pv p args | ||
let (res, UPLC.CountingSt final, logs) = | ||
evaluateTerm UPLC.counting pv verbose ectx appliedTerm | ||
tell logs | ||
liftEither $ first CekError $ void res | ||
processLogsAndErrors ll logs res | ||
pure final | ||
|
||
processLogsAndErrors :: | ||
forall m. | ||
(MonadError EvaluationError m, MonadWriter LogOutput m) => | ||
PlutusLedgerLanguage -> | ||
LogOutput -> | ||
Either | ||
(UPLC.CekEvaluationException NamedDeBruijn DefaultUni DefaultFun) | ||
(UPLC.Term UPLC.NamedDeBruijn DefaultUni DefaultFun ()) -> | ||
m () | ||
processLogsAndErrors ll logs res = do | ||
tell logs | ||
case res of | ||
Left e -> throwError (CekError e) | ||
Right v -> | ||
unless (ll == PlutusV1 || ll == PlutusV2 || isBuiltinUnit v) $ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this needs a Note! With a reference to the CIP There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd also be tempted to extract this out to a top-level predicate with a descriptive name and the explanation attached to it. It's a significant thing, worth making it look more significant! |
||
throwError NonUnitReturnValue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Include the actual result here for a more informative error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can |
||
where | ||
isBuiltinUnit t = case readKnown t of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we over-constrain ourselves to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what the CIP proposed. To quote @michaelpj 's words:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. |
||
Right () -> True | ||
_ -> False | ||
{-# INLINE processLogsAndErrors #-} | ||
|
||
{- Note [Checking the Plutus Core language version] | ||
Since long ago this check has been in `mkTermToEvaluate`, which makes it a phase 2 failure. | ||
But this is really far too strict: we can check when deserializing, so it can be a phase 1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
{-# LANGUAGE BangPatterns #-} | ||
{-# LANGUAGE DataKinds #-} | ||
{-# LANGUAGE GADTs #-} | ||
{-# LANGUAGE LambdaCase #-} | ||
{-# LANGUAGE MultiParamTypeClasses #-} | ||
{-# LANGUAGE NegativeLiterals #-} | ||
{-# LANGUAGE OverloadedStrings #-} | ||
{-# LANGUAGE PatternSynonyms #-} | ||
{-# LANGUAGE TemplateHaskell #-} | ||
{-# LANGUAGE ViewPatterns #-} | ||
{-# OPTIONS_GHC -fplugin-opt PlutusTx.Plugin:target-version=1.0.0 #-} | ||
|
||
module Spec.ReturnUnit.V1 where | ||
|
||
import PlutusLedgerApi.Common.Versions | ||
import PlutusLedgerApi.Test.V1.EvaluationContext qualified as V1 | ||
import PlutusLedgerApi.V1 as V1 | ||
import PlutusPrelude | ||
import PlutusTx.Builtins qualified as PlutusTx | ||
import PlutusTx.Code | ||
import PlutusTx.IsData qualified as PlutusTx | ||
import PlutusTx.Prelude (BuiltinUnit, check) | ||
import PlutusTx.TH (compile) | ||
|
||
import Test.Tasty (TestName, TestTree, testGroup) | ||
import Test.Tasty.HUnit | ||
|
||
import Control.Monad.Writer | ||
|
||
tests :: TestTree | ||
tests = | ||
testGroup | ||
"Plutus V1 validators may evaluate to any value" | ||
[ expectSuccess "should succeed" good (I 1) | ||
, expectSuccess "returns Bool" returnsBool (I 1) | ||
, expectSuccess "too many parameters" tooManyParameters (I 1) | ||
] | ||
|
||
evalCtx :: V1.EvaluationContext | ||
evalCtx = | ||
fst . unsafeFromRight . runWriterT . V1.mkEvaluationContext $ | ||
fmap snd V1.costModelParamsForTesting | ||
|
||
expectSuccess :: | ||
forall a. | ||
TestName -> | ||
CompiledCode (BuiltinData -> a) -> | ||
-- | Script argument | ||
Data -> | ||
TestTree | ||
expectSuccess name code arg = testCase name $ case res of | ||
Left _ -> assertFailure "fails" | ||
Right _ -> pure () | ||
where | ||
sScript = serialiseCompiledCode code | ||
script = either (error . show) id $ V1.deserialiseScript conwayPV sScript | ||
effectfully marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(_, res) = V1.evaluateScriptCounting conwayPV V1.Quiet evalCtx script [arg] | ||
|
||
good :: CompiledCode (BuiltinData -> BuiltinUnit) | ||
good = | ||
$$( compile | ||
[|| | ||
\d -> | ||
let n = PlutusTx.unsafeFromBuiltinData d | ||
in check (PlutusTx.greaterThanInteger n 0) | ||
||] | ||
) | ||
|
||
returnsBool :: CompiledCode (BuiltinData -> Bool) | ||
returnsBool = | ||
$$( compile | ||
[|| | ||
\d -> | ||
let n = PlutusTx.unsafeFromBuiltinData d | ||
in PlutusTx.greaterThanInteger n 0 | ||
||] | ||
) | ||
|
||
tooManyParameters :: CompiledCode (BuiltinData -> BuiltinData -> BuiltinUnit) | ||
tooManyParameters = | ||
effectfully marked this conversation as resolved.
Show resolved
Hide resolved
|
||
$$( compile | ||
[|| | ||
\d _ -> | ||
let n = PlutusTx.unsafeFromBuiltinData d | ||
in check (PlutusTx.greaterThanInteger n 0) | ||
||] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a more descriptive name like
NonUnitReturnValueConway
, to remind ourselves that this can only happen with Conway onwardsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not about Conway but about V3. I changed the name to
InvalidReturnValue
, and made thePretty
instance more descriptive.