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

Only evaluate Plutus scripts in static checks. #2767

Merged
merged 1 commit into from May 4, 2022
Merged

Only evaluate Plutus scripts in static checks. #2767

merged 1 commit into from May 4, 2022

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented May 3, 2022

Initially, Plutus scripts were evaluated using the following:

      evalScripts @era tx sLst
        ?!## ValidationTagMismatch (getField @"isValid" tx)

However, as of #2386, additional reporting was added to Plutus script
failure, and this was changed to the following:

      case evalScripts @era tx sLst of
        Fails sss -> False ?!## ValidationTagMismatch (getField @"isValidating" tx) (pack (Prelude.unlines sss))
        Passes -> pure ()

The problem here: evalScripts is no longer gated by the ?!##
operator; it must be evaulated at least to WHNF in order to match the
Fails constructor. This means that when reapplying transactions in the
mempool (as well as when replaying blocks), we are always running all
Plutus scripts.

The current semantics for using "labeled" predicates is insufficient to
solve this, since we cannot carry out additional actions (such as
emitting events) inside the predicate. As such, we introduce additional
functionality in the STS system to allow gating sections of rules (which
do not result in a return value) with labels. For the sake of
consistency, the existing labeledPred function et al are updated to
make use of this new feature.

The entire call to evalScripts is now gated by a nonStatic label,
and hence will not be evaulated in any reapply scenario.

@nc6 nc6 changed the title Pushing for testing Only evaluate Plutus scripts in static checks. May 3, 2022
@nc6 nc6 requested review from TimSheard and lehins May 3, 2022 20:55
@nc6 nc6 marked this pull request as ready for review May 3, 2022 21:38
Copy link
Contributor

@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.

@nc6 Really good catch! 💯 I suspect this will have tremendous impact on the forger!

I also like the approach, it removes the need for most predicates to have an empty label. Couple of suggestions that should also improve that common case.

libs/small-steps/src/Control/State/Transition/Extended.hs Outdated Show resolved Hide resolved
libs/small-steps/src/Control/State/Transition/Extended.hs Outdated Show resolved Hide resolved
Initially, Plutus scripts were evaluated using the following:
```
      evalScripts @era tx sLst
        ?!## ValidationTagMismatch (getField @"isValid" tx)
```

However, as of #2386, additional reporting was added to Plutus script
failure, and this was changed to the following:
```
      case evalScripts @era tx sLst of
        Fails sss -> False ?!## ValidationTagMismatch (getField @"isValidating" tx) (pack (Prelude.unlines sss))
        Passes -> pure ()
```
The problem here: `evalScripts` is no longer gated by the `?!##`
operator; it must be evaulated at least to WHNF in order to match the
`Fails` constructor. This means that when reapplying transactions in the
mempool (as well as when replaying blocks), we are always running all
Plutus scripts.

The current semantics for using "labeled" predicates is insufficient to
solve this, since we cannot carry out additional actions (such as
emitting events) inside the predicate. As such, we introduce additional
functionality in the STS system to allow gating sections of rules (which
do not result in a return value) with labels. For the sake of
consistency, the existing `labeledPred` function et al are updated to
make use of this new feature.

The entire call to `evalScripts` is now gated by a `nonStatic` label,
and hence will not be evaulated in any `reapply` scenario.
@nc6
Copy link
Contributor Author

nc6 commented May 4, 2022

@lehins Nice suggestions, I've applied them both :-)

Copy link
Contributor

@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.

Looks great!

(?!) = labeledPred []
(?!) cond onFail =
liftF $
Predicate (if cond then Success () else Failure (() :| [])) (const onFail) ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one minor optional suggestion, which doesn't change the semantics, but might make it easier to read the code:

Suggested change
Predicate (if cond then Success () else Failure (() :| [])) (const onFail) ()
Predicate (if cond then Success () else Failure (onFail :| [])) id ()

@nc6 nc6 merged commit 4615065 into master May 4, 2022
@iohk-bors iohk-bors bot deleted the nc/labels branch May 4, 2022 12:29
Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

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

Some discussion about how the features are intended to be used, would be useful.

Comment on lines +445 to 447
when2Phase :: Rule sts ctx () -> Rule sts ctx ()
when2Phase = labeled $ lblStatic NE.:| [lbl2Phase]

Copy link
Contributor

Choose a reason for hiding this comment

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

How about some comment here, explaining exactly what is going on here, and some discussion about the correct use of this function. One might also explain what NE.:| does. It looks like a constructor function?

Copy link
Contributor

Choose a reason for hiding this comment

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

nc6 added a commit that referenced this pull request May 6, 2022
PR #2767 moved to using `labeled` sections to gate validation for more
than just predicates. However, it totally failed to take account of the
fact that `ValidateNone` should turn off all predicates, and hence
accidentally turned them all back on. *facepalm*.

This PR fixes that, and adds a set of tests to ensure that it remains
fixed.
@nc6 nc6 mentioned this pull request May 6, 2022
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.

None yet

3 participants