plutus

plutus copied to clipboard

Summary

Moving from V1 where txOutDatumHash is a Maybe DatumHash which according to the IsData instance this effectively becomes OutputDatumHash DatumHash | NoOutputDatum on-chain

In V2, this is changed to OutputDatum instead that serializes like NoOutputDatum | OutputDatumHash DatumHash | OutputDatum Datum

The crux of the problem being that the order of the two fields was changed around. Is this an intended change, and if so are the possible bugs born out of this cascading through the ecosystem worth it?

Steps to reproduce the behavior

Follow the links, changes are self-evident

Actual Result

Expected Result

The V1 encoding should have been extended by adding a new constructor and preserving their order from V1

Describe the approach you would take to fix this

Switch the order of NoOutputDatum and OutputDatumHash constructors around in the V2 implementation

System info

Any

Hmm, I don't think we thought about preserving the representation between versions. You do need to handle all the cases, regardless - code which assumes it will keep on working unchanged will be wrong. But I'm not sure that's a good enough reason to change things around. We could try and preserve this in future, but it's too late for any more changes to go into Vasil.

This is potentially also a performance consideration and possibly worth reordering in V3, as matching on a datum hash is more common than on no datum.

Agreed however that it is unfortunately too late to introduce to Vasil,

The ordering of the constructors doesn't necessarily affect the efficiency of the script. Checking the index is always O(log index).

@michaelpj

But I'm not sure that's a good enough reason to change things around. We could try and preserve this in future, but it's too late for any more changes to go into Vasil.

I think the conclusion here is that we shouldn't break things unless there's an explicit reason for that. Accidental breakage is pretty stupid. Therefore we need to think of a way to ensure things (or at least some subset of them) do not get broken unintentionally.

I'm removing the bug label, since at this point there's nothing we can do apart from trying to prevent similar issues in future.

Therefore we need to think of a way to ensure things (or at least some subset of them) do not get broken unintentionally.

@michaelpj do you happen to know if we have an initiative of this sort? I do understand that tracking the order of constructors of a data type seems very non-trivial, but maybe we could have an entry in the style guide at least? Or something, I don't know.

Nothing so far. We'd want to say something like "given the 'same' ScriptContext in V1 and V2, the Data representation is the same", over a wide variety of ScriptContexts. That would require some generators and also the ability to translate script contexts between ledger languages etc.

@michaelpj

Nothing so far. We'd want to say something like "given the 'same' ScriptContext in V1 and V2, the Data representation is the same", over a wide variety of ScriptContexts. That would require some generators and also the ability to translate script contexts between ledger languages etc.

Are there any plans on doing that? This ticket doesn't seem to be moving forward in any way.

It appears that this ticket isn't considered high-priority, so I'm removing the status: needs action from the team label and adding the Low priority one.