fix execution result incorrectly collecting logs when building tx#979
Merged
Conversation
spalladino
approved these changes
Jul 7, 2023
spalladino
left a comment
spalladino
left a comment
Contributor
There was a problem hiding this comment.
Should we add a test that reproduces this one, so we don't accidentally get bit by it again?
| std::array<uint8_t, num_bytes> calldata_hash_inputs_bytes; | ||
| // Convert all into a buffer, then copy into the array, then hash | ||
| for (size_t i = 0; i < calldata_hash_inputs.size() - 4; i++) { // -4 because logs are processed out of the loop | ||
| for (size_t i = 0; i < calldata_hash_inputs.size() - 8; i++) { // -8 because logs are processed out of the loop |
Contributor
There was a problem hiding this comment.
Why was this change needed? Change in the number of logs emitted per tx? If so, should it be related to a constant?
Contributor
There was a problem hiding this comment.
I submitted a new commit with additional comments and used the constant NUM_FIELDS_PER_SHA256 wherever it made sense to me.
Contributor
@spalladino absolutely! I think that after the hackaton we will add e2e test which captures this. Right @rahul-kothari ? |
Contributor
Author
|
@spalladino #987 - will do it next sprint |
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
3 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
When Noir calls multiple child contract functions (say child1 followed by child2), the circuit and ACVM uses a call stack so it first executes child2 followed by child1. Same happens when creating the calldata hash (especially when collecting logs and hashing them all).
Whereas in TS, when we are collecting logs to build the TX object (for sending it to sequencer), it did a
flatMapwhich collected child1 first rather than child2 i.e. it did a queue rather than a stack like thing. This resulted in a mismatch between the circuit's calldata hash AND the L2Block's getCalldataHash() which is checked before publishing block on L1.Thanks to @suyash67 and @jeanmon for sticking it out with me for debugging this. S/O also to @sirasistant who made it easier to spot this.
Checklist: