Merkle Path with Predecessor/Successor + Universal Query Gadget #388
Conversation
feat: allow for either aggregation-only or tabular-only queries
| } | ||
| } | ||
|
|
||
| /// Build an instance of `Self` without range-check the `UInt256Target`s |
There was a problem hiding this comment.
Can you write in which conditions this is safe to use ? (i'm reading the PR top to bottom so maybe you explain it below)
There was a problem hiding this comment.
It's up to the caller to determine whether it's safe to use this method or not, depending on how the values are employed. For instance, if the node info is employed to compute the hash of the node, it should be safe to allocate without range checks since the hash will be different in case the prover provides out-of-range limbs.
| /// Input wires related to the data of the end node whose membership is proven with `MerklePathWithNeighborsGadget` | ||
| pub struct EndNodeInputs { | ||
| node_min: UInt256Target, | ||
| node_max: UInt256Target, | ||
| left_child_exists: BoolTarget, | ||
| left_child_info: NodeInfoTarget, | ||
| right_child_exists: BoolTarget, | ||
| right_child_info: NodeInfoTarget, | ||
| } |
There was a problem hiding this comment.
Naming: This comment is saying it's being used to verify the merkle path with neirghbors yet this struct contains the children, not the neighbors (predecessor / successor). It's confusing, since right after, naming seems more consistent:
/// Target containing data about a neighbor of a node (neighbor can be
/// either the predecessor or the successor of a node)
pub struct NeighborInfoTarget
There was a problem hiding this comment.
Could you specify what is the data and why is it required / why is there ?
There was a problem hiding this comment.
Comments added in commit 600a6e9.
| pub struct MerklePathWithNeighborsTargetInputs<const MAX_DEPTH: usize> | ||
| where | ||
| [(); MAX_DEPTH - 1]:, | ||
| { | ||
| pub(crate) path_inputs: MerklePathTargetInputs<MAX_DEPTH>, | ||
| pub(crate) end_node_inputs: EndNodeInputs, | ||
| } |
There was a problem hiding this comment.
Naming: Here the name claims with Neighbors, and yet there is no neighbors information (predecessor/successor) inside. Could you clarify what is the input and why it's needed there ? And why do you put neighbors if it doesn't contain neighbors information ? or maybe im just missing it, where is it ?
There was a problem hiding this comment.
Like right after
pub struct MerklePathWithNeighborsTarget<const MAX_DEPTH: usize>
and it uses
pub(crate) predecessor_info: NeighborInfoTarget,
which makes sense there then.
There was a problem hiding this comment.
Because MerklePathWithNeighborsTargetInputs is the data structure that contains only the input wires (i.e., the ones that need to be assigned). The information about the neighbors is an output of the gadget, that's why they appear only in the latter data structure. Might be clearer if I rename the data structure to MerklePathWithNeighborsInputTargets maybe?
There was a problem hiding this comment.
lol ok maybe just MPathInputs or stg ? It's ok to not put the full name in relation to what it does, personally i find MerklePathWithNeighborsTargetInputs more confusing.
There was a problem hiding this comment.
I added the WithNeighbors sufffix to distinguish this from the simpler gadget MerklePathTargetInputs which only checks membership in the merkle path, without computing data about the predecessor/successor nodes
| pub struct MerklePathWithNeighborsGadget<const MAX_DEPTH: usize> | ||
| where | ||
| [(); MAX_DEPTH - 1]:, | ||
| { | ||
| path_gadget: MerklePathGadget<MAX_DEPTH>, | ||
| end_node_min: U256, | ||
| end_node_max: U256, | ||
| end_node_children: [Option<NodeInfo>; 2], | ||
| } |
There was a problem hiding this comment.
Naming: same here, neighbor mentionned but no neighbors structs used, only NodeInfo... Could you clarify what are the fields and what's the purpose of the struct, wrt to the "neighbor" especially ?
There was a problem hiding this comment.
Same as above, there are no neighbors since this a gadget is expected to contain only the input values. I will add comments to clarify the purpose of the additional fields. They re basically needed because in case the predecessor/successor is not in the path, then we can get its value from the min and max of the children of the node.
There was a problem hiding this comment.
Comments added in commit 600a6e9.
| fn build_common<const MAX_DEPTH: usize>( | ||
| b: &mut CircuitBuilder<F, D>, | ||
| end_node: HashOutTarget, | ||
| index_id: Target, | ||
| with_neighbors: bool, | ||
| end_node_info: Option<EndNodeInputs>, | ||
| ) -> MerklePathWires<MAX_DEPTH> |
There was a problem hiding this comment.
The API is a bit convoluted imo with_neighbors must always be correct wrt end_node_info AND when looking at the actual function logic, most lines are only regarding the successor/predecessor function.
Also, maybe I'm missing something, but it looks like all the logic only need the node_hash computed from the "normal / without neighbor" logic, is that correct ?
Wdyt about we maybe extract out the only small portion about verifying the hashes, return all hashes and do the successor/predecessor check in its own function ?
That would separate the concern cleanly imo and also able to avoid the convoluted arguments, since now we would not mix at all the two logic ("verifying mpath" + "extracting successor").
There was a problem hiding this comment.
I initially thought also about what you suggested but I was unsure whether the build_path method would have been well-defined or not. But probably it is since you suggested this structure too :) Done in commit 600a6e9.
| let rest = node_min[i] | ||
| .to_targets() | ||
| .into_iter() | ||
| .chain(node_max[i].to_targets()) | ||
| .chain(once(index_id)) | ||
| .chain(node_value[i].to_targets()) | ||
| .chain(embedded_tree_hash[i].to_targets()) | ||
| .collect_vec(); |
There was a problem hiding this comment.
Could we use the node input target struct which already defines the sequence of elements to hash ?
There was a problem hiding this comment.
I am not sure I fully understand what you mean: you are suggesting to add a method to NodeInfoTarget that will compute rest basically? If so, I am not sure how to name this method, computing rest inside of the whole hash is an optimization but it's not a "well-defined" operation. Wdyt?
…th-with-successor-gadget
This PR introduces a circuit to expose the results of simple `SELECT` queries without aggregation functions, avoiding the need to build a results tree.
First PR for batching query circuits. It provides:
universal_query_hash_gadgetanduniversal_query_value_gadget, which will then be employed distinctly in the new batching circuits