Skip to content

Conversation

@nicolasLuduena
Copy link

Values of cost models are supposed to be sorted in lexicographical manner. Consequences could be seen on the blaze provider of utxorpc that got all cost models mixed up.

@Quantumplation
Copy link
Collaborator

I believe they're supposed to be, but that's convention based on the serializer in the haskell node, and in some eras has not followed that convention. This has burned me in the past.

I'm not 100% sure that's true, so we should check it carefully in all eras; if I'm right, then enforcing this in pallas itself will break projects that need to be able to sync previous eras (like archive nodes etc.)

@nicolasLuduena
Copy link
Author

Hey @Quantumplation . Thanks for taking a look. As I understand:

  • before this PR the code sorts values in the cost model arbitrarily.
  • after this PR the code sorts values in the cost model lexicographically.
    I don't understand how that can break things. Can you explain me?

@Quantumplation
Copy link
Collaborator

Before this PR, the cost model was sorted, not arbitrarily, but in the order it was constructed / deserialized.

After this, it sorts them lexographically.

In the current era, the lexographical behavior is correct.

In other eras, I believe this wasn't enforced, and it was the order the fields appeared in the Haskell record.

So, if you make this change, and someone consuming this code was taking care to construct the cost models in the correct order, you'll now be changing that behavior.

In particular this matters for the script data hash calculation, and so you'd break validating the ledger in old eras.

Again, I can't point you to a source for this, as it's just based on some frustrating debugging sessions from a year or more ago, and I could be mistaken, but I'm just raising it because you'll want to make sure this doesn't break anything. And if it does, I would recommend doing this sorting with different era specific constructors, rather than when you grab the cost model values (which might be better for performance anyway)

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

Labels

None yet

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants