Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(kernel): dont chop commitments when matched to transient reads #1079

Merged
merged 7 commits into from
Jul 18, 2023

Conversation

dbanks12
Copy link
Collaborator

@dbanks12 dbanks12 commented Jul 14, 2023

Description

Closes #1073

  • Don't chop commitment when it matches a read in ordering circuit (chopping is reserved for when nullifier is matched to commitment)
  • Only forward transient reads to next kernel (since non-transient are currently membership-checked by kernel)
  • Ordering circuit does not forward any reads to its output (ultimately we can actually remove it from final public inputs struct)
  • Update tests accordingly
  • Some additional (or modifications to) error messages
  • Misc cleanup

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@dbanks12 dbanks12 marked this pull request as draft July 17, 2023 01:30
@dbanks12 dbanks12 changed the base branch from master to db/circuits-tests-cleanup July 17, 2023 18:17
Base automatically changed from db/circuits-tests-cleanup to master July 17, 2023 18:37
@dbanks12 dbanks12 requested a review from jeanmon July 17, 2023 20:27
@dbanks12 dbanks12 marked this pull request as ready for review July 17, 2023 20:27
Copy link
Contributor

@jeanmon jeanmon left a comment

Choose a reason for hiding this comment

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

@dbanks12 Excellent work overall with great improvements of old code along the way.

@@ -315,19 +357,16 @@ void common_contract_logic(DummyBuilder& builder,
}
}

void common_initialise_end_values(PreviousKernelData<NT> const& previous_kernel,
KernelCircuitPublicInputs<NT>& public_inputs)
void common_inner_ordering_initialise_end_values(PreviousKernelData<NT> const& previous_kernel,
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, common prefix is meant for re-usability and does not imply that all circuits use the pre-fixed method. Adding the "inner_ordering" is a bit too verbose in my opinion but I can live with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm I wanted to make it more clear which circuits use them, but i think you're right that it's overkill.

@jeanmon jeanmon force-pushed the db/1073-dont-chop-reads branch from 8835072 to 3b3eccb Compare July 18, 2023 08:55
@jeanmon jeanmon self-requested a review July 18, 2023 09:00
@jeanmon jeanmon merged commit fdf41ed into master Jul 18, 2023
@jeanmon jeanmon deleted the db/1073-dont-chop-reads branch July 18, 2023 09:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ordering circuit should not chop commitments matched to read-requests
2 participants