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

Add sha256 function #117

Merged
merged 14 commits into from
Oct 17, 2023
Merged

Add sha256 function #117

merged 14 commits into from
Oct 17, 2023

Conversation

Acaccia
Copy link
Collaborator

@Acaccia Acaccia commented Oct 11, 2023

This PR adds sha256 function that works on buffers and (u)int.

This is entirely written in Webassembly. I even used some SIMD whenever I could. I'm proud.

This is part of #69. sha256 is needed in order to write hash160.

@Acaccia Acaccia requested a review from obycode October 11, 2023 20:06
@Acaccia Acaccia self-assigned this Oct 11, 2023
Copy link
Collaborator

@obycode obycode left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks @Acaccia. Just one comment and also a request to add property tests as well.

clar2wasm/src/wasm_generator.rs Show resolved Hide resolved
@Acaccia
Copy link
Collaborator Author

Acaccia commented Oct 16, 2023

@obycode any possibility to ask someone else to write the property tests here? I'm very focused on the implementation of hash160 right now.

There are a few complications to consider in this case:

  • We should test it against the sha256 that is used in stacks-blockchain/clarity.
  • We have to create a new Strategy for proptest to generate buffs.
  • When I designed the property tests, I used a "huge hack" where I put the Store in a Refcell to hide its mutability to the compiler. This allowed us to create the pair (instance, store) and to load the tested function only once before launching the property test. In this case, we actually have to write to the memory inside the Store, so I don't know how the proptest crate will react to this.

Also, I would say that in this case, the testing of multiple input is not that important. A tiny drift in sha256 causes massive implications, and I believe that the current tests are enough in this case.

obycode
obycode previously approved these changes Oct 16, 2023
Copy link
Collaborator

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Following up from the meeting, I'm okay with getting this merged and saving the additional testing for later, with @csgui taking a look after his current work.

Copy link
Collaborator

@obycode obycode left a comment

Choose a reason for hiding this comment

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

lgtmn!

@Acaccia Acaccia added this pull request to the merge queue Oct 17, 2023
Merged via the queue into main with commit 1f1bf9b Oct 17, 2023
4 checks passed
@Acaccia Acaccia deleted the feat/sha256 branch October 17, 2023 15:48
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.

None yet

2 participants