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: sha256() for Slices and rework its description #1936

Merged
merged 10 commits into from
Feb 24, 2025
Merged

Conversation

novusnota
Copy link
Member

@novusnota novusnota commented Feb 20, 2025

Also made the passed Slices use the same function instead of the string_hash() from FunC's stdlib, otherwise they'd still have all the same problems we've fixed before for Strings in #1626. Added positive and negative tests, showing that previous implementation for Slices was incorrect on Slices larger than 127 bytes.

Issue

Closes #1879.
Closes #1937.

Checklist

  • I have updated CHANGELOG.md
  • I have run the linter, formatter and spellchecker
  • I did not do unrelated and/or undiscussed refactorings

Also made the passed Slices use the same function, otherwise they'd
still have all the same problems we've fixed before for Strings — the
benchmarks didn't show those issues.
@novusnota novusnota added this to the Doc: 2025-02 milestone Feb 20, 2025
@novusnota novusnota requested a review from a team as a code owner February 20, 2025 19:45
@anton-trunov anton-trunov self-assigned this Feb 20, 2025
@anton-trunov anton-trunov requested a review from i582 February 20, 2025 19:55
@anton-trunov
Copy link
Member

Looks like this should have a different linked issue. This is a fix of a fix. Can you please add an example that fails into a separate issue?

@novusnota
Copy link
Member Author

Looks like this should have a different linked issue. This is a fix of a fix. Can you please add an example that fails into a separate issue?

Opened and linked: #1937

@anton-trunov
Copy link
Member

Is it still possible to recover the previous behavior? I.e. if the user want to hash just the slice data, without any traversals?

@novusnota novusnota changed the title docs: mark sha256() as gas-expensive and rework its description docs/fix: mark sha256() as gas-expensive, fix it for Slices and rework its description Feb 20, 2025
@novusnota
Copy link
Member Author

novusnota commented Feb 20, 2025

Is it still possible to recover the previous behavior? I.e. if the user want to hash just the slice data, without any traversals?

If the Slice would not contain any references the results are the same as before.

If the user still wants to hash just the data even if the slice contains some refs, in the "Before Tact 1.6" note I've said that TVM instructions that perform SHA-256 hashing all operate on data only. With that they can simply introduce their own helper asm function if that's needed.

Or do we want to add a new function to the stdlib? .hashData() or somesuch?

@anton-trunov anton-trunov modified the milestones: Doc: 2025-02, v1.6.0 Feb 21, 2025
@anton-trunov anton-trunov changed the title docs/fix: mark sha256() as gas-expensive, fix it for Slices and rework its description fix: sha256() for Slices and rework its description Feb 21, 2025
@anton-trunov
Copy link
Member

Or do we want to add a new function to the stdlib? .hashData() or somesuch?

I think we should. .hashData sounds good

@novusnota
Copy link
Member Author

novusnota commented Feb 21, 2025

Or do we want to add a new function to the stdlib? .hashData() or somesuch?

I think we should. .hashData sounds good

Sure thing, I've opened a separate issue for this — #1950. Will be resolved in a separate PR soon, let's finish this one first

@anton-trunov
Copy link
Member

merge conflicts

@novusnota
Copy link
Member Author

merge conflicts

Resolved, please take a look

Copy link
Contributor

@i582 i582 left a comment

Choose a reason for hiding this comment

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

Looks good!

@anton-trunov anton-trunov merged commit 76d0295 into main Feb 24, 2025
25 checks passed
@anton-trunov anton-trunov deleted the closes-1879 branch February 24, 2025 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants