fix: replace MPT metadata with counter for row_id computation
#399
Conversation
silathdiir
force-pushed
the
generic-extraction-row-id-update
branch
from
a24cfde
to
0836ee3
Compare
verifiable-db/src/cells_tree/mod.rs
Outdated
|
|
||
| b.register_curve_public_input(values_digest.individual); | ||
| b.register_curve_public_input(values_digest.multiplier); | ||
| b.register_curve_public_input(metadata_digest.individual); | ||
| b.register_curve_public_input(metadata_digest.multiplier); | ||
|
|
||
| (cell, child_values_digest, child_metadata_digest) |
There was a problem hiding this comment.
child_metadata_digest could be removed from the test now, no?
There was a problem hiding this comment.
Remove this child_metadata_digest in commit aa84a07.
verifiable-db/src/row_tree/row.rs
Outdated
|
|
||
| // individual_counter = p.individual_counter + is_individual | ||
| let individual_cnt = cells_pi.individual_counter() | ||
| + if self.cell.is_individual() { |
There was a problem hiding this comment.
There should be a method F::from_bool if I remember correctly
There was a problem hiding this comment.
Fix to use F::from_bool in commit 3a686f9.
verifiable-db/src/row_tree/row.rs
Outdated
|
|
||
| // multiplier_counter = p.multiplier_counter + not is_individual | ||
| let multiplier_cnt = cells_pi.multiplier_counter() | ||
| + if self.cell.is_multiplier() { |
There was a problem hiding this comment.
Same as above: method F::from_bool could be used here
There was a problem hiding this comment.
Fix to use F::from_bool in commit 3a686f9.
verifiable-db/src/row_tree/api.rs
Outdated
| @@ -184,33 +184,19 @@ impl CircuitInput { | |||
| pub fn leaf( | |||
| identifier: u64, | |||
| value: U256, | |||
| mpt_metadata: HashOut<F>, | |||
| row_unique_data: HashOut<F>, | |||
There was a problem hiding this comment.
row_unique_data should be typed as HashOutput to avoid exposing plonky2 types in external APIs
There was a problem hiding this comment.
Fix to HashOutput type in commit d5c9e24.
There was a problem hiding this comment.
looks good to me but with a few question - i think @nicholas-mainardi should be able to answer.
| ); | ||
| // Check individual counter | ||
| let multiplier_cnt = F::from_bool(is_multiplier); | ||
| assert_eq!(pi.individual_counter(), F::ONE - multiplier_cnt); |
There was a problem hiding this comment.
I don't understand this.
We officially only support single table so the multipler columns can be 1 or more, that means this subtraction can underflow.
Then in my mind the total number of columns = individual_counter + multiplier_counter so if we want the individual_columns = total - multiplier_columns , not one...
There was a problem hiding this comment.
This is testing a leaf node of a cells tree, so the counter is either 0 or 1, depending on whether the current cell is accumulated as individual or multiplier
| b.connect(digest.multiplier_cnt, min_child.multiplier_counter_target()); | ||
| b.connect(digest.multiplier_cnt, max_child.multiplier_counter_target()); |
There was a problem hiding this comment.
Shouldn't we also check that the individual counter are the same between the children and this node ?
There was a problem hiding this comment.
The individual counter is not exposed as public input in rows tree nodes, since it's already included in the value digest, to compute row_id_individual. This should already be enforced to be the same in MPT circuits, where we ensure that row_id_individual for each item being extracted is computed using a counter provably bound to the number of columns specified in the metadata digest; therefore, by checking in MPT circuits that the metadata digest is always the same across all the extracted items, we should implicitly check that all counters employed to compute row_id_individual, and so the value digest, are the same for all extracted items.
…' into generic-extraction-row-id-update
…' into generic-extraction-row-id-update
No description provided.