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

fix: enforce correct length of hashConstructor args #321

Merged
merged 4 commits into from
Apr 21, 2023

Conversation

dbanks12
Copy link
Collaborator

@dbanks12 dbanks12 commented Apr 21, 2023

Description

It was discovered that hashConstructor was sending any length array of args to the abis__hash_constructor cbind. That cbind always deserializes args into an array of length ARRAY_LENGTH since it expects a statically typed array. Our abis.test.ts was passing 2 args in and the C++ was serializing those 2 plus 6 garbage args into the array. So the output of hashConstructor was wrong and non-deterministic.

This PR fixes this by enforcing that the C++ cbind always gets an array of proper length.

Contents

  • hashConstructor enforces that args.length() <= ARGS_LENGTH
  • before calling the cbind, hashConstructor fills out args until full with new Fr(0)
  • added a test for max args
  • added test that expects throw on >max args
  • updated snapshots which should now be stable
  • some autoformatting changes that happened when doing a merge commit of phil's latest work

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@dbanks12 dbanks12 requested a review from suyash67 April 21, 2023 00:17
@dbanks12
Copy link
Collaborator Author

Thanks @suyash67 for pairing on this with me. @Cheethas looks like you and @LHerskind made a fix to the failing test in master while I was discovering this on my own.... Lmk what you think of my fix to hashConstructor and the new tests.

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

This explains why that snapshot had unexpectedly changed at some point. Thanks @dbanks12 for fixing it!

@iAmMichaelConnor iAmMichaelConnor merged commit 53e33f0 into master Apr 21, 2023
@iAmMichaelConnor iAmMichaelConnor deleted the db/fix_hash_constr_args branch April 21, 2023 12:46
@dbanks12
Copy link
Collaborator Author

This explains why that snapshot had unexpectedly changed at some point. Thanks @dbanks12 for fixing it!

My pleasure!

ludamad pushed a commit that referenced this pull request Jul 14, 2023
codygunton pushed a commit that referenced this pull request Jan 23, 2024
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.

TS hashConstructor is not providing fixed length 8 array for args
3 participants