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

feat: hash_fr in assembly + removed Utils #794

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

ThomasPiellard
Copy link
Collaborator

  • removed Utils library
  • hash_fr is written in assembly

@julien-marchand there might be some conflicts with the 'calldataisation' :/

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

A remark about comment and a few remarks about optimisations, but I don't know if they help.


}
{{ end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not need to close this block and open again a few lines from here. In both cases it checks len(CommitmentConstraintIndexes) > 0

let b0 := mload(mPtr)

// [b0 || one || dst || sizeDomain]
// <-64bytes -> <- 1 byte each ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

b0 is 32 bytes.

error_sha2_256()
}

let b0 := mload(mPtr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cannot we instead store the output of the next SHA2 call to mPtr+0x20. Then b0 will be at mPtr and b1 at mPtr+0x20. Dunno if would help with gas.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

normally b0 is not needed from here, it is only used to populate the first entry of the next hash. So I leave it in mPtr and use it directly for the next hash

// the result is then 2**(8*16)*mPtr[32:] + mPtr[32:48]
res := mulmod(mload(mPtr), bb, r_mod) // <- res = 2**128 * mPtr[:32]
offset := add(mPtr, 0x10)
for {let i:=0} lt(i, 0x10) {i:=add(i,1)} // mPtr <- [xx, xx, .., | 0, 0, .. 0 || b2 ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about optimisations. Would unrolling the loop help? Otherwise there are implicit checks for overflow when doing add, no?

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 could try to unroll it. But I might get a 'stack too deep' error if the function is too big

@ThomasPiellard ThomasPiellard merged commit d361e2d into develop Jul 27, 2023
4 checks passed
@ThomasPiellard ThomasPiellard deleted the feat/clean_hash_fr branch July 27, 2023 20:10
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.

2 participants