Skip to content

[do not merge] V in Data - #7910

Open
zliu41 wants to merge 1 commit into
masterfrom
zliu41/v
Open

[do not merge] V in Data#7910
zliu41 wants to merge 1 commit into
masterfrom
zliu41/v

Conversation

@zliu41

@zliu41 zliu41 commented Aug 18, 2026

Copy link
Copy Markdown
Member

This PR adds the V constructor to Data. It hasn't been decided whether or not to pursue this, but if yes, then this is how it could be done.

Backwards compatibility is not trivial and not obvious. We need to think through it and review the implementation carefully, to make sure it won't cause any split between old and new node versions.

The idea is:

  • The CBOR Serialise instance for Data NO LONGER ROUNDTRIPS: encode accepts V, but decode rejects it. There's a separate decodeDataAcceptingValues function that accepts it.
    • Why not do the reverse - make decode accept V, and add a separate decodeDataLegacy that rejects V? Because it complicates ledger integration - the ledger must replace all existing decode calls with decodeDataLegacy.
    • Why not make encode reject V, and add a separate encodeDataAcceptingValues? We can do that for the sake of satisfying the roundtrip property, but it isn't otherwise needed.
  • The Flat instance for Data STILL ROUNDTRIPS, and both encoding and decoding accept V. To prevent V in Plutus V1-V3, a separate check is added in PlutusLedgerApi.Common.SerialisedScript.

@lehins The ledger integration should be fairly straightforward (though I'm not certain). It basically amounts to: before Dijkstra, use decode or decodeData. After Dijkstra, use decodeDataAcceptingValues.

cc @colll78 @kwxm @SeungheonOh

@zliu41 zliu41 added Do not merge No Changelog Required Add this to skip the Changelog Check labels Aug 18, 2026
@zliu41
zliu41 force-pushed the zliu41/v branch 4 times, most recently from 699118a to 12e6832 Compare August 19, 2026 03:12
@zliu41 zliu41 changed the title V Data [do not merge] V in Data Aug 19, 2026
@zliu41
zliu41 force-pushed the zliu41/v branch 3 times, most recently from 5dd1bd0 to 379e655 Compare August 19, 2026 13:47
@IntersectMBO IntersectMBO deleted a comment from github-actions Bot Aug 19, 2026
{-# LANGUAGE TupleSections #-}
{-# LANGUAGE ViewPatterns #-}

module PlutusCore.Value.Internal

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Most of the Value builtins are moved here, to avoid cyclic dependency between Value and Data.

@IntersectMBO IntersectMBO deleted a comment from github-actions Bot Aug 20, 2026
@SeungheonOh

SeungheonOh commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Implementation looks reasonable, I'll give a more thorough review tomorrow.

I'm not entirely sure what the purpose of this addition is. The only benefit of having V on Data is that the fact that ledger can inject Data directly and we won't have to run valueData : Data.Map -> Value(or rather, this unValueData just becomes very cheap) but most of the time this cost doesn't seem to be that high to begin with so there's not much to gain.

Also, it would be nice if you can write out the reason why Serialise instance must be changed to no round trip. I assume it's because for decoding it must run the normalisation but I'm not entirely sure and clarification would be nice.

@Unisay

Unisay commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

checkConstant only guards bare con data, so a V inside a compound constant like list data or pair data data gets through: those hit the _ -> Nothing branch, their inner Data is never inspected, and the Flat decoder accepts V at any depth. So a pre-Dijkstra script with such a constant is accepted here but rejected by an older node that doesn't know tag 1401. That's an old/new split.

Confirmed at (PlutusV3, newestPV):

1. (con data (I 1))                   -> ACCEPTED   [sanity]
2. (con data (V empty))               -> REJECTED   ("The V constructor of data is not available in language PlutusV3 ...")
3. (con (list data) [I 1])            -> ACCEPTED   [control: list data is allowed here]
4. (con (list data) [V empty])        -> ACCEPTED   [the same V that (2) rejects slips through]
5. (con (pair data data) (I 1, I 2))  -> ACCEPTED   [control: pair data data is allowed here]
6. (con (pair data data) (I 1, V ..)) -> ACCEPTED   [same slip-through via pair]

The gate needs to reject V in a constant of any type containing Data, not only in con data.

| 121 <= t && t < 128 ->
Constr (fromIntegral t - 121) <$> decodeListOf go
t
| 1280 <= t && t < 1401 ->

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
| 1280 <= t && t < 1401 ->
| 1280 <= t && t < valueTag ->

@zliu41

zliu41 commented Aug 20, 2026

Copy link
Copy Markdown
Member Author

I'm not entirely sure what the purpose of this addition is. The only benefit of having V on Data is that the fact that ledger can inject Data directly and we won't have to run valueData : Data.Map -> Value(or rather, this unValueData just becomes very cheap)

The more you do with the Value, the more the cost of unValueData is amortized. If you only perform one or two operations on the Value, then unValueData cost could very well be dominant.

@colll78

colll78 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

This looks good to me. I strongly encourage adding V to Data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Do not merge No Changelog Required Add this to skip the Changelog Check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants