Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weight #166

Merged
merged 7 commits into from
Jul 10, 2023
Merged

Weight #166

merged 7 commits into from
Jul 10, 2023

Conversation

roconnor-blockstream
Copy link
Collaborator

This PR does a couple of related things:

  1. Makes the static analysis of memory and CPU bounds available for FFI bindings.
  2. Generates expected CPU weight bounds for the generated test vectors.
  3. Fixes the witness weight computation in Haskell, which addresses half of Incorrect weight calculations for Witness and Disconnect #159.

Primitives are still used in the specification of jets, but can no longer appear in Dags for serialization.
Indeed, attemps to serialize them prior to this commit would fail with a pattern matching error.
This removes them from the Dag data structure entirely.
Set the outputSize and inputSize from the type information.
They can be inferred from the type information.
Refactor computeEvalTCOBound into analyseBounds and allowed it to be called
outside of evalTCOExpression.
This will facilitate testing of the bounds computation.
@roconnor-blockstream roconnor-blockstream marked this pull request as draft July 6, 2023 22:04
@roconnor-blockstream roconnor-blockstream marked this pull request as ready for review July 6, 2023 22:16
@roconnor-blockstream
Copy link
Collaborator Author

@apoelstra only the C code needs to be reviewed.

I've shuffled around the commits quite a bit during development, so I do recommend giving it a pass through CI before looking at it too deeply.

@apoelstra
Copy link
Contributor

utACK 56f15be. I have a mild preference for the "fix Cost.hs" commit to come first, before any computed values are introduced, avoiding committing then reverting bad numbers. But this is fine.

Will run CI when my box is free.

@roconnor-blockstream
Copy link
Collaborator Author

Yeah regarding your "fix Cost.hs" preference. Your choices are between being able to check that the regression test works, by swapping to adjacent commits, or doing your proposal and having to swap far apart commits, regenerate the failing data and dealing with the merge conflict that would arise.

I'm okay with either approach if you have a preference. I don't see a way of getting the best of both solutions, other than maybe bizarre branches and merges.

@apoelstra
Copy link
Contributor

@roconnor-blockstream ah, yeah, good point. You do actually need to include the bad data for the test to still compile when it's swapped.

Ok, we'll keep things as is.

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 793c7b1

free(bound);
return true;
return (maxCells <= *cellsBound) ? SIMPLICITY_ERR_EXEC_MEMORY
: (maxCost <= *costBound) ? SIMPLICITY_ERR_EXEC_BUDGET
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know how I missed it, but these inequalities were strict before and now they are non-strict, which is incorrect. I'll prepare a PR to correct this.

The written specification of "the bound on memory allocation requirements exceed the allowed limits" is just too vague.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants