DO NOT MERGE feat: attempting to implement sideeffect counting#1646
Closed
dbanks12 wants to merge 10 commits into
Closed
DO NOT MERGE feat: attempting to implement sideeffect counting#1646dbanks12 wants to merge 10 commits into
dbanks12 wants to merge 10 commits into
Conversation
dbanks12
commented
Aug 29, 2023
Comment on lines
+323
to
+329
| let side_effect = SideEffectWithRange { | ||
| value: item.hash(), | ||
| start_side_effect_counter: self.inputs.side_effect_counter, | ||
| end_side_effect_counter: self.side_effect_counter, | ||
| }; | ||
| self.private_call_stack.push(side_effect); | ||
| self.side_effect_counter++; |
Contributor
Author
There was a problem hiding this comment.
Do we need to know here how many side effects the nested private call will have and increment the counter accordingly (instead of just by 1)?
attempt... Fix a lot of compilation issues, 1 pending. Remove temp param from conditional_assign C++ compiles. add side effects to TS (wip) TS compiles.
…objects instead of fields
dbanks12
force-pushed
the
db/attempt-ordering
branch
from
800fb43 to
1536f1a
Compare
Collaborator
|
Closed as stale. Please reopen as needed. |
Contributor
Author
|
I'm working on this actively, but have been stuck for a while so I haven't pushed lately. |
dbanks12
changed the title
Member
|
Closed as stale |
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
4 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.
Resolves #864
Resolves #846
Relevant to #838
Unfortunately, as I kept working on this, I realized more and more that it is far from complete. It may make sense for someone to finish this up, or it may make more sense to use it as a reference while implementing something similar but with Noir circuits.
Some notes:
SideEffecttypes which can be used to represent objects (like noteHashes) that should be paired with countersnullified_commitmentsarrayl2_to_l1_msgsto beSideEffectsbefore Lasse and Leila said that is unnecessary. So that can be reverted/ignored.new_commitmentsis always empty even though it has non-zero entries inPublicContext::finish()<SideEffect>.equals(other)or<SideEffect>.value == otherin some places<SideEffect>.isEmpty()versus<SideEffect>.value == 0Some notes:
Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.