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

Traces simplified away #5460

Closed
ch1bo opened this issue Aug 7, 2023 · 10 comments
Closed

Traces simplified away #5460

ch1bo opened this issue Aug 7, 2023 · 10 comments
Assignees

Comments

@ch1bo
Copy link

ch1bo commented Aug 7, 2023

Summary

When updating https://github.com/input-output-hk/hydra from using plutus 1.1.1.0 to 1.7.0.0, we saw our so-called mutation tests fail. These tests do run transaction validation on mutated txs, expecting the validators to fail with a specific error.

At this commit, for example, the mutation tests for the abort transaction fail:

cabal test hydra-node --test-options "-m Abort"

When disabling INLINABLE, as seen in this commit, the problem goes away!

When using dump-pir one can see that the error code we expect (H30) is included in the initial pir, but not in the simplified pir.

Consequently, using no-simplifier-inline in the plutus-tx plugin solves this problem (see this commit), but might have other adverse effects (increased cost? UPDATE: seems not the case)

Steps to reproduce the behavior

Try to build & run the specific tests using the commits above.

Actual Result

The validator fails, but the trace does not include our error code.

Expected Result

The traces remain in the code throughout simplification.

Describe the approach you would take to fix this

No response

System info

GHC 9.2.8 (problem appeared only on upgrading plutus, GHC upgrade from 8.10.7 itself did not change anything)
Plutus 1.7.0.0

@ch1bo ch1bo added the bug label Aug 7, 2023
@github-actions github-actions bot added the status: needs triage GH issues that requires triage label Aug 7, 2023
@michaelpj
Copy link
Contributor

Can you try running with -fplugin-opt PlutusTx.Plugin:conservative-optimisation?

@ch1bo
Copy link
Author

ch1bo commented Aug 18, 2023

@michaelpj That works as well. Would you recommend conservative-optimisation over no-simplifier-inline?

@michaelpj
Copy link
Contributor

Yes, conservative-optimisation says "don't do anything that will change the effects of the program (most notably logs)", which is actually what you want. no-simplifier-inline just turns off inlining, which won't defend you against other non-conservative passes in general.

You probably only want to use it for your tests, though.

Also a UX question: this flag basically controls whether we optimize more (good for on-chain usage) at the cost of messing with traces (bad for debugging and testing). We have some disagreement about whether it should be on by default with a way to turn it off (as it is today), or off by default with a way to turn it on. What would you prefer?

@ch1bo
Copy link
Author

ch1bo commented Aug 21, 2023

I would expect traces to stay in the program unless I configure the compiler to remove them. Optimizations should not remove them as not seeing the traces messes with my understanding of which code path was executed on-chain. My intuition of an optimization is that the code behaves the same when observed at the system boundary (i.e. only inner workings change and size or cpu/memory consumption is optimized)

@effectfully
Copy link
Contributor

I would expect traces to stay in the program unless I configure the compiler to remove them.

Me too. We've run into this issue internally as well and we're going to have a discussion about it.

@effectfully effectfully added status: needs action from the team and removed status: needs triage GH issues that requires triage labels Nov 2, 2023
@effectfully effectfully self-assigned this Nov 2, 2023
@effectfully
Copy link
Contributor

We've discussed this issue internally and we agree that this behavior is pretty much a bug. We're going to fix it soon.

@effectfully
Copy link
Contributor

@zliu41 do you happen to know what the status of this issue is?

@zliu41
Copy link
Member

zliu41 commented Mar 13, 2024

I think it's still on my todo list!

@zliu41
Copy link
Member

zliu41 commented Mar 25, 2024

@ch1bo You don't need no-simplifier-inline. What you need is conservative-optimisation (I verified that it works). When conservative optimisation is off (which is the default), the optimizer is allowed to optimize more aggressively, which includes removing some trace messages. conservative-optimisation is much better than no-simplifier-inline, since not running the inliner often causes too many optimization opportunities to be missed.

I think we should be able to let users fine tune the different behaviors controlled by conservative-optimisation, and to that end I created #5853.

@zliu41 zliu41 closed this as completed Mar 25, 2024
ch1bo added a commit to cardano-scaling/hydra that referenced this issue May 23, 2024
According to IntersectMBO/plutus#5460 this
should be enough to ensure traces are not simplified away, but still
allow for some simplifications and potential smaller / faster code.
ch1bo added a commit to cardano-scaling/hydra that referenced this issue May 23, 2024
According to IntersectMBO/plutus#5460 this
should be enough to ensure traces are not simplified away, but still
allow for some simplifications and potential smaller / faster code.
ch1bo added a commit to cardano-scaling/hydra that referenced this issue May 23, 2024
According to IntersectMBO/plutus#5460 this
should be enough to ensure traces are not simplified away, but still
allow for some simplifications and potential smaller / faster code.
ch1bo added a commit to cardano-scaling/hydra that referenced this issue May 23, 2024
According to IntersectMBO/plutus#5460 this
should be enough to ensure traces are not simplified away, but still
allow for some simplifications and potential smaller / faster code.
@ch1bo
Copy link
Author

ch1bo commented May 29, 2024

@zliu41 I tried using the conservative optimization only, but our benchmarks degraded. We saw significantly higher memory budget usage: cardano-scaling/hydra#1450

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

No branches or pull requests

4 participants