-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimize oct and quad insertion #1125
Conversation
8f91e33
to
cd379d7
Compare
@@ -69,6 +86,172 @@ pub fn insert<E: Engine, CS: ConstraintSystem<E>>( | |||
Ok(result) | |||
} | |||
|
|||
pub fn insert_4<E: Engine, CS: ConstraintSystem<E>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when do we use quads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quads are needed any time we need to build a tree which cannot be constructed either with:
- only oct trees
- only oct and binary trees
Put differently, we need them whenever log2 N mod 3 = 2, where N is the number of 32-byte leaves. Although not currently supported, this would include either 16 or 128GiB sectors. Since we have only currently generated parameters for 32 and 64GiB sectors, quad hashing is dormant. But it's important that we preserve the freedom to support any power of two sector size — so the audit needs to cover quad hashes.
let (b0, b1) = (&bits[0], &bits[1]); | ||
let (a, b, c, d) = (&element, &elements[0], &elements[1], &elements[2]); | ||
|
||
/// Define constrain macro to allow legible definition of positional constraints. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is a nice use of macros, however (imho) this adds code without expressing new ideas or clarifying preexisting ones. The "assign and constrain" operator looks (imo) out of place with respect to bellman's view of how gadgets should be defined and values allocated. Can the condition bit ever not be a reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after a rebase
fd9a152
625e0d8
to
eab7af5
Compare
eab7af5
to
7591998
Compare
WARNING: this PR reduces constraints and is therefore a breaking circuit change. Only merge once last v26 release has been cut. cc: @cryptonemo
This PR updates
insertion.rs
to use hand-optimized versions of insertion when the size of the output array is either 4 or 8. These are the sizes used for our merkle trees and for which optimization will help. By taking advantage of redundancy in the output for each position, we can avoid the full generality otherwise required. This allows us to avoid the quadratic cost ofsize * (size - 1)
constraints.In order to aid in legibility and for consistency with internal spec audit (the motivation for this change), I have introduced a macro,
witness!
which allows declarative assignment of constraints to allocated numbers based on the value of an allocated boolean value.Previous behavior is preserved and verified in tests — so bit input for insertion is still little endian. Bit and position indices are 0-indexed, and witness names rely on these conventions systematically.
For reference, here are the constraint definitions used to compute the oct insertion result: