Merge Table #362
Merge Table #362
Conversation
| } | ||
| } | ||
|
|
||
| pub fn cond_combine_to_row_digest(&self) -> Digest { |
There was a problem hiding this comment.
What does cond_ stand for? The naming convention is a bit unclear to me
There was a problem hiding this comment.
conditional. Any other preferences ?
| @@ -285,6 +354,6 @@ mod tests { | |||
| .unwrap(), | |||
| ) | |||
| .unwrap(); | |||
| proof_pis.check_proof_public_inputs(proof.proof(), true, Some(len_dm)); | |||
| proof_pis.check_proof_public_inputs(proof.proof(), TableDimension::Compound, Some(len_dm)); | |||
There was a problem hiding this comment.
Should we add also a test for proof generation employing the API for merge circuit?
verifiable-db/src/cells_tree/api.rs
Outdated
| /// Create a circuit input for proving a leaf node whose value is considered as a multiplier | ||
| /// depending on the boolean value. | ||
| /// i.e. it means it's one of the repeated value amongst all the rows | ||
| pub fn leaf_multiplier(identifier: u64, value: U256, is_multiplier: bool) -> Self { |
There was a problem hiding this comment.
If we have leaf and leaf_multiplier methods, I would expect to call the former for cells where is_multiplier = false and the latter for cells where is_multiplier = true. In other words, wouldn't it be clearer from API perspective if this method doesn't have is_multiplier flag as input? Or you designed the API in this way because you want to always use leaf for tables not mixing multiple variable types and leaf_multiplier for mixed tables?
There was a problem hiding this comment.
my initial thought was more on keeping the previous API the same and it ended up as you said
the API in this way because you want to always use leaf for tables not mixing multiple variable types and leaf_multiplier for mixed tables
If if i set leaf to set multiplier = false directly then we would not use the individual keyword that i've been trying to instill in the rest of the codebase.
So maybe leaf_merge is better name ?
There was a problem hiding this comment.
The renaming would make sense if we want to keep the usage of leaf_merge only for merged tables. But for instance in the integration test it seems to me you are using this method for all tests cases, isn't it? So I am wondering if might be always convenient to just have a single method with the multiplier flag rather than having 2 distinct methods
There was a problem hiding this comment.
@hackaugusto what do you prefer ? :D Any opinion ? I was more on the side of letting things backward compatible and adding new endpoint, it doesn't "surprise" me more than that but dont really have an strong opinion.
verifiable-db/src/row_tree/leaf.rs
Outdated
| // final_digest = HashToInt(mul_digest) * D(ind_digest) | ||
| // NOTE This additional digest is necessary since the individual digest is supposed to be a | ||
| // full row, that is how it is extracted from MPT | ||
| let final_digest = split_digest.cond_combine_to_row_digest(b); |
There was a problem hiding this comment.
In the row circuits I think that instead of having to check whether multiplier ==NEUTRAL, it could be a bit cheaper to have an input flag that tells whether there is not a multiplier to be added. We can call this input flag something like is_combined_table, so that its semantic is clear. This will allow to remove the need to have curve_eq operation, which might require a couple of additional rows. Though I see it's not a dramatic improvement, so if you think it would make the APIs too complex we could also avoid it.
There was a problem hiding this comment.
Mhhh it doesn't make the API complex if i still rely on the check point == neutral but outside of circuit, wdyt ?
There was a problem hiding this comment.
Yeah, that would make sense. Though I suppose you should also take into account the multiplier flag for the current row? Like, if point == neutral for all the other cells but multiplier = true for the secondary index, then the scalar multiplication would still need to be performed, right?
| pub fn leaf_multiplier( | ||
| identifier: u64, | ||
| value: U256, | ||
| is_multiplier: bool, |
There was a problem hiding this comment.
Same comment as for cells tree API: I would expect this function doesn't have an is_multiplier flag as input, but will instead be called only when is_multiplier = true
Co-authored-by: nicholas-mainardi <[email protected]>
Co-authored-by: nicholas-mainardi <[email protected]>
| if payload.min() != value { | ||
| // the value of successor is different from `value`, so we don't return the | ||
| // successor node | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Confused because in the comment you say
if the successor exists in the row tree and it stores the same value of the input node (i.e.,
value); returnsNone
And yet here the value is different so i would have expected returning something. Same for the other while loop below.
There was a problem hiding this comment.
We return a node if we find the successor and it has the same value of the input node, that was the intended meaning of the comment; in this case, we found a successor but its value is different from the input node, so we don't return any node. Feel free to modify comments as you wish to clairfy
No description provided.