Conversation
jfecher
reviewed
Jul 24, 2023
2 tasks
AztecBot
pushed a commit
that referenced
this pull request
Mar 21, 2024
…es#5160) Will close #2019 This PR converts SHA hashing inside noir circuits from outputting 2 128-bit fields to outputting 1 248-bit field. To fit inside the field, we truncate one byte. --- ### Noir Changes The constant `NUM_FIELDS_PER_SHA256` is now 1, so any hardcoded test values and function returns have been changed to use an array of one. I've kept it as an array rather than a single `Fr` to minimise changes across the repo and ensure if we want to revert `NUM_FIELDS_PER_SHA256` in future, it won't be so painful. However, we can also just use a single `Fr` if that's preferable. `TX_EFFECTS_HASH_LOG_FIELDS` Methods: - `field_from_bytes_32_trunc`: Converts a 32 byte array to a 31 byte field element (useful for comparisons with new `sha256_to_field`), tests in `types/src/utils/field.nr`. - `sha256_to_field`: Uses the same method as the previous version to convert the sha result (BE) bytes array to field, but leaves out the final byte. - `accumulate_sha256`: Used almost exclusively for enc/unenc logs hashing - takes in 2 31 byte field elements, assumed to be outputs of a previous sha hash, pads to 32 bytes and hashes them with `sha256_to_field` as a 64 byte array. Note that as before, other circuits that use sha (like tx effects hash and messages hash) do not use this method and instead create a flat byte array, then call `sha256_to_field`. --- ### L1 Contract Changes To match the Noir method, the `sha256ToField` function now truncates a byte and prepends a blank byte. Not prepending the blank byte means changing many struct fields from `bytes32` to `bytes31`. This (IIRC) is the same gas cost and creates more awkward encoding, so I kept the length with a blank byte. This also changes the slither file, as I removed some of the old encoding which flagged with new encoding... which also flags. ~Only the 'leaves' used in computing the `txsHash` in `TxsDecoder` and logs hashes have been changed to 31 bytes to match the Noir SHA accumulation (since we are repeating hashes of hashes).~ ~The TS code (see below) does pack the Header struct with 31 bytes per SHA, so we must shift the decoding in HeaderLib` by 3 bytes.~ As of 21.3, there have been a lot of changes in master to the way the txs effect hash (formerly calldata hash/txs hash) is calculated. Plus, now we actually recalculate the in/outHash (i.e. the root of the sha tree of messages) in the contract, so I have reverted to using 32 bytes everywhere with a prepended blank byte. --- ### TS Changes All `.hash()` methods which are also computed in the circuit have been changed to match the Noir code. In most places this just means truncating a byte with `.subarray(0, 31)` on the buffer. ~The `ContentCommitment` serialise/deserialise methods have been modified, as keeping `NUM_BYTES_PER_SHA256 = 32` caused a lot of issues in the background. Changing it to 31 to match Noir does mean slightly different encoding, but many fewer changes across the repo (and hopefully less confusion).~ As of 21.3, due to changes in master, it's now cleaner to keep `NUM_BYTES_PER_SHA256 = 32` and be sure to truncate and pad all SHA hashes which touch the Noir circuits. Since I've kept the hash output as an array of one in Noir, there are many tuples of one in ts (for the above reasoning) - this can be changed if preferable. Methods: - `toTruncField`: Mirrors Noir's `field_from_bytes_32_trunc` to convert to a field element - used in place of old method `to2Fields` (tested in `free_funcs.test.ts`). - `fromTruncField`: Converts the above back to a 31 byte buffer (tested as above). ---
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Related to #1971
not sure if it resolves this issue
Summary*
only
proveandexecutecommand canprintthings it seemsthis pr will enable them to show
printby defaultas for
test. when tests passes, the--show-outputarg canprintthingswhen tests are failing, with the current setup,
--show-outputcan'tprintthings even when it is enabledif i understand correctly, sometimes error are emitted during compilation and never reach execution. seems better to improve compilation errors in tests. at the moment compilation errors in tests are not displayed
Additional Context
PR Checklist*
cargo fmton default settings.