plutus icon indicating copy to clipboard operation
plutus copied to clipboard

memoryUsage of empty ByteString is 1 instead of 0

Open nau opened this issue 1 year ago • 5 comments

Summary

memoryUsage of empty ByteString is 1 instead of 0, contrary to what a comment and code say here: https://github.com/IntersectMBO/plutus/blob/c3e919785c23e06acc39ca90010dbf8718d8d950/plutus-core/plutus-core/src/PlutusCore/Evaluation/Machine/ExMemoryUsage.hs#L206

The code there is SOOOO unbearably complex that it's hard to tell why. But

echo "(program 1.0.0 [(builtin sha2_256) (con bytestring #)])" | uplc evaluate -c --trace-mode LogsWithBudgets

gives CPU budget: 906572 Memory budget: 404

and

echo "(program 1.0.0 [(builtin sha2_256) (con bytestring #aa)])" | uplc evaluate -c --trace-mode LogsWithBudgets

gives CPU budget: 906572 Memory budget: 404

Expected:

If implemented as it suppose to be (ByteString.empty => 0) than this should return CPU 876090, which is exactly 30482 units less, which is the slope value for sha2_256 CPU cost model, meaning the slope was multiplied by 1 instead of 0.

Aiken implements this "correctly", and computes memoryUsage(ByteString.empty) as 1: https://github.com/aiken-lang/aiken/blob/9e3f348c6c1cc6e9585f8668cc3b8fd2f648aad6/crates/uplc/src/machine/value.rs#L225

Is there a specification of cost calculation? Because the code there is quite hard to follow.

Steps to reproduce the behavior

echo "(program 1.0.0 [(builtin sha2_256) (con bytestring #)])" | uplc evaluate -c --trace-mode LogsWithBudgets

Actual Result

gives CPU budget: 906572 Memory budget: 404

and

echo "(program 1.0.0 [(builtin sha2_256) (con bytestring #aa)])" | uplc evaluate -c --trace-mode LogsWithBudgets

gives CPU budget: 906572 Memory budget: 404

Expected Result

If implemented as it suppose to be (ByteString.empty => 0) than this should return CPU 876090, which is exactly 30482 units less, which is the slope value for sha2_256 CPU cost model, meaning the slope was multiplied by 1 instead of 0.

Describe the approach you would take to fix this

At least change the misleading comments in the sources, I guess.

System info

MacOS, Plutus tag 1.15.1.0

nau avatar Feb 14 '24 15:02 nau

There's a comment right below the instance saying

{- The two preceding comments contradict each other.  The first one says that we
   should use `div`, but the second one says to use `quot` instead (and that is
   what is used).  The only difference here is for negative numbers: (-1) `div`
   8 == -1 and (-1) `quot` 8 == 0.  Thus the current memoryUsage for the empty
   bytestring is 1: replacing `quot` with `div` would change the memoryUsage of
   the empty bytestring from 1 to 0 and leave the memoryUsages of all other
   bytestrings unchanged.  We can probably change this safely since all that it
   would mean is that programs which use the empty bytestring as a builtin
   argument would become a little cheaper.  The difference would be very small
   though, so it's maybe not worth fixing. -}

Messy indeed. @zliu41 @kwxm we should at least polish the comments.

As for the issue itself, I think we should charge something for every object, regardless of how trivial it is, so I think the cost should stay 1 and this is only a documentation problem.

effectfully avatar Feb 15 '24 04:02 effectfully

Messy indeed. @zliu41 @kwxm we should at least polish the comments.

I think the fact that the empty bytestring has memory usage 1 was just a mistake, possibly due to confusion between div and quot. I noticed that recently when doing something else and added the comment mentioned above as a reminder to come back and fix it some other time. Perhaps now is the time.

As for the issue itself, I think we should charge something for every object, regardless of how trivial it is, so I think the cost should stay 1 and this is only a documentation problem.

Usually there's a constant in the costing function that accounts for size zero objects, so that's kind of taken care of. Having cost 1 here is an annoyance, but changing it would change the costs of scripts already on the chain (although I think it'd only make them cheaper) so unfortunately it might be safer to leave it as it is. We should document it properly though.

Is there a specification of cost calculation? Because the code there is quite hard to follow.

Not yet. There's a long-standing plan to document costing properly (probably in the Plutus Core specification), but we haven't been able to get round to that yet.

kwxm avatar Feb 15 '24 10:02 kwxm

I think the fact that the empty bytestring has memory usage 1 was just a mistake, possibly due to confusion between div and quot.

Feature, not a bug IMO.

Usually there's a constant in the costing function that accounts for size zero objects, so that's kind of taken care of.

I'm gonna argue that zero-size objects don't exist. If it exists, its size is not zero. But I'm just being pedantic, it probably doesn't matter.

effectfully avatar Feb 15 '24 20:02 effectfully

I've opened a small PR to make the comment more sensible: see https://github.com/IntersectMBO/plutus/pull/5792.

At some point we will specify all of this properly. Thanks for pointing this out @nau!

kwxm avatar Feb 21 '24 12:02 kwxm

Thank you, guys!

nau avatar Feb 21 '24 13:02 nau