IntersectMBO / plutus

The Plutus language implementation and tools
Apache License 2.0
1.57k stars 479 forks source link

Traces simplified away #5460

Closed ch1bo closed 6 months ago

ch1bo commented 1 year ago

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

michaelpj commented 1 year ago

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

ch1bo commented 1 year ago

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

michaelpj commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 7 months ago

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

zliu41 commented 7 months ago

I think it's still on my todo list!

zliu41 commented 6 months ago

@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 https://github.com/IntersectMBO/plutus/issues/5853.

ch1bo commented 4 months ago

@zliu41 I tried using the conservative optimization only, but our benchmarks degraded. We saw significantly higher memory budget usage: https://github.com/input-output-hk/hydra/pull/1450