Skip to content

Padding in set target#200

Merged
ed255 merged 3 commits intomainfrom
feat/padding_in_set_target
Apr 21, 2025
Merged

Padding in set target#200
ed255 merged 3 commits intomainfrom
feat/padding_in_set_target

Conversation

@ed255
Copy link
Collaborator

@ed255 ed255 commented Apr 17, 2025

Resolve #183 with a twist

The issue was about processing the witness outside of the set_target method, and that processing includes padding. Part of this change was already started in #196

My original arguments for this pattern were:

  • Make the code around the circuit as simple and transparent as possible so that we focus on the constraint logic
  • Allow writing tests that use set_targets and directly set all the witness values

But once I started applying this pattern in the MainCircuit I noticed some issues. Here are my counter arguments:

  • The code looks more clean by doing the padding inside set_targets because it allows reusing existing types as arguments to set_targets (and those types are not yet padded). It also allows having the padding logic in the circuit code, which makes sense because the circuit logic defines what is considered padding.
  • We're not doing any test that requires direct manipulation of witness values that would not be supported by set_targets methods that do some processing
    • If we had such requirement we could resolve it with: a test_set_targets method that does what the test needs. Or setting the targets in the test by making the FooTarget fields pub(crate)

So for this reason I'm reverting the merkle tree padding outside of set_target from #196

In this PR i also remove the enabled from MerkleClaimAndProof following the discussion #196 (comment) because it makes things more clear and less error prone, and the enabled: bool is not actually used in the random access, it's the BoolTarget counterpart, which still exists inside of MerkleClaimAndProofTarget.

@ed255 ed255 requested review from arnaucube and ax0 April 17, 2025 16:21
@ed255 ed255 force-pushed the feat/padding_in_set_target branch 4 times, most recently from ce995fd to 6b81e97 Compare April 18, 2025 11:13
@ed255 ed255 force-pushed the feat/padding_in_set_target branch from 6b81e97 to 00709fa Compare April 18, 2025 11:18
Copy link
Collaborator

@arnaucube arnaucube left a comment

Choose a reason for hiding this comment

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

LGTM, happy with moving the padding & witness preparation logic to the set_targets methods 😺

Co-authored-by: Ahmad Afuni <root@ahmadafuni.com>
@ed255 ed255 merged commit 26a6b2d into main Apr 21, 2025
5 checks passed
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.

Refactor: preprocess the witness outside of the circuit set_targets methods

3 participants