fix: add single and mapping struct to integration test for generic extraction (Part 4) #397

silathdiir · 2024-10-28T09:14:16Z

No description provided.

nikkolasg

Overall very nice work ! My comments are mostly related to the

external API, how should it be, and I will probably make more comments down the line after this first review
The genericity of the description of what are we extracting. We can not customize for each case that we support now we have to think about all the use case we want to support later as well and all the combination possible.

nikkolasg · 2024-11-06T13:02:37Z

mp2-v1/src/api.rs

@@ -338,7 +339,7 @@ fn value_metadata<const MAX_COLUMNS: usize, const MAX_FIELD_PER_EVM: usize>(
 }

 /// Compute the table information for the value columns.
-fn compute_table_info(
+pub fn compute_table_info(


Since we are doing big changes, let's take a shot at having a better unified API.
For example, the address chain_id fields could be directly put inside extra no ? Since we don't prove the computation of these identifiers, we dont need to have special ordering or whatever.
Then we could add if we want another API endpoint that call this one with explicit chain_id address genesis_block for example since those are the information we put. But we can do that on DQ side it's fine.

I see, in commit c21fe13 I added the similar functions (*_raw) for this and other identifiers computation which could only pass the extra argument, the original functions (have contract_address and chain_id arguments) call these functions. If we set extra = (contract_address || chain_id || extra), the result of two similar functions should be same.

nikkolasg · 2024-11-06T14:41:27Z

mp2-v1/src/api.rs

@@ -209,7 +210,7 @@ pub enum SlotInputs {
    MappingWithLength(Vec<SlotInput>, u8),
 }

-#[derive(Debug)]
+#[derive(Clone, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)]
 pub struct SlotInput {
    /// Slot information of the variable
    pub(crate) slot: u8,


This is redundant information, can we extract it and put it separately ?
i.e. a table column's set is represented by (slot, Vec<SlotInput>)

I see, but I make this SlotInput Struct corresponding to ColumnInfo, and seems it makes the conversion complicated. Please correct me if I was wrong, thanks.

nikkolasg · 2024-11-06T14:44:12Z

mp2-v1/src/api.rs

@@ -209,7 +210,7 @@ pub enum SlotInputs {
    MappingWithLength(Vec<SlotInput>, u8),
 }

-#[derive(Debug)]
+#[derive(Clone, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)]


SlotInput::new() <-- now that we now there is no bitpacking level, then we can get rid of the bit_offset argument and just set it to 0 for the moment, until we refactor the circuit to be simpler.

Remove bit_offset in the SlotInput and the new function in commit a2c4cfc.

nikkolasg · 2024-11-06T15:00:34Z

mp2-v1/tests/common/cases/table_source.rs

+    MappingValues((MappingValuesExtractionArgs, Option<LengthExtractionArgs>)),
+    /// Test arguments for single struct extraction
+    SingleStruct(SingleStructExtractionArgs),
+    /// Test arguments for mapping struct extraction
+    MappingStruct((MappingStructExtractionArgs, Option<LengthExtractionArgs>)),


We need to bind MappingValues and MappingStruct together, otherwise it will lead to an cambrian explosion of combination down the line.
From what I see the only difference is that the latter has the additional information about the metadata, but we could make it as well for the single value, there is no reason not to, or am i missing stg ?

Same for the SingleValues and SIngleStruct ?

I tried to fix in commit b33cd73:

Combine both single test cases into one, its table includes 4 single value slots and 1 Struct slot now.

Combine the mapping arguments to MappingExtractionArgs<V> (V could be Address or LargeStruct).

Update the merge case for the above single columns (4 single values and 1 Struct) and MappingExtractArgs<LargeStruct>.

nikkolasg · 2024-11-06T15:02:37Z

mp2-v1/tests/common/cases/table_source.rs

+    /// Test arguments for single struct extraction
+    SingleStruct(SingleStructExtractionArgs),
+    /// Test arguments for mapping struct extraction
+    MappingStruct((MappingStructExtractionArgs, Option<LengthExtractionArgs>)),
    Merge(MergeSource),


The MergeSourceshould be adapted to be able to use the combined enum above, not just SingleValue and MappingValue (well it will be after you merge the values andstruct together as i suggest before)

I fixed this merge case to a more complicated case, it includes the single columns of 4 single values and 1 Struct, and the mapping columns of Struct now. Please correct me if I was wrong.

nikkolasg · 2024-11-06T15:17:12Z

mp2-v1/src/values_extraction/mod.rs

+    outer_key_id: Option<u64>,
+    inner_key_id: Option<u64>,


It was a bit hard for me to realize what should the API look like before this PR, but this definitely doesn't sound good. We need an API that is generic enough to cover

myVar uint256 myStruct MyStruct myMapping mapping(uint256 => Address) myStructMapping mapping(uint256 => MyStruct) myArraySingle []uint256 myArrayStruct []MyStruct myMappingArray mapping(uint256 => []MyStruct) myDoubleMapping mapping(uint256 => mapping(Address => MyStruct))

The last two are the most complicated ones but you see the point.
We should not have to ahve these two outer and inner keys if we don't need them here. It looks to me the StorageSlot structure because it's recursive could already support giving all the details for that. Is this correct or is it missing some things ?
It's ok we don't support the array and mapping of arrays use case now but we should eventually, so thinking about this in the most generic way should be the goal here.

I see, the StorageSlotInfo is a collection which fields should be used during the values extraction proving process in the integration test storage trie. I think it may be different in DQ (let me think more details in PR #404).

Maybe I should replace this outer_key_id and inner_key_id to a vector of mapping key IDs. What do you think?

Single slot has no mapping key ID (it only has value IDs).

Mapping slot has one mapping key ID.

Mapping of mappings slot has both the outer and inner mapping key IDs.

I remove outer_key_id and inner_key_id from StorageSlotInfo in commit 50a58d9.

…' into generic-extraction-integration-test

nicholas-mainardi

Leave some comments but after a while I skipped table_source and indexing because I agree with @nikkolasg that the main change is to avoid having specific data structures for each variant: ideally, we should have a data structure for simple variables (where each could be a scalar or a struct), and one for mappings (where the mapping value can be either a simple variable or a struct). So I think it makes sense to review deeply these files only after these changes

mp2-v1/src/values_extraction/gadgets/column_gadget.rs

mp2-v1/tests/common/cases/table_source.rs

mp2-v1/tests/common/storage_trie.rs

mp2-v1/tests/common/values_extraction.rs

mp2-v1/src/api.rs

mp2-v1/src/values_extraction/gadgets/column_info.rs

…' into generic-extraction-integration-test

…case.

nikkolasg

Looks good ! Left a few comments tho that I think could make a cleaner approach as suggestion, but let's discuss, it's also LGTM as current code !

nikkolasg · 2024-11-11T20:41:31Z

mp2-v1/tests/common/cases/contract.rs

+}
+
+/// Common functions for a specific type to interact with the test contract
+pub trait SimpleContractValue {


Can we get rid of the Simple here ? simple starts to get overloaded.
Maybe ContractManipulator or ContractController or stg ?

Rename to ContractController in commit 911aadb.

nikkolasg · 2024-11-11T20:43:35Z

mp2-v1/tests/common/cases/contract.rs

+    async fn update_contract_single_values(&self, ctx: &TestContext, contract: &Contract);
+
+    /// Update the mapping values to the test contract.
+    async fn update_contract_mapping_values(&self, ctx: &TestContext, contract: &Contract);


Why two methods different with the same inputs ? From what I see, only one or the other is implemented ?
So we could make the trait simpler no ?
stg like

trait ContractController { async fn current_values(...) -> Self; async fn update_contract(&self, ....); }

wdyt ?

Fix the trait functions to current_values and update_contract in commit 911aadb.

nikkolasg · 2024-11-11T21:09:52Z

mp2-v1/tests/common/cases/table_source.rs

+        metadata(&table_info)
+    }
+
+    fn storage_slots(&self, metadata: &[MetadataGadget]) -> Vec<StorageSlot> {


I know it's not related to this PR but it's frankly confusing to see an Array of metadata gadget.

Gadget name refers usually to circuit, here we're not dealing with any circuits

The reason there's an array is just to separate the columns that corresponds to a certain EVM word. But a regular HashMap works as well. We could have a single struct Metadata { words: HashMap<EvmWord, MetadataSubset> for example.

and that would allow us to avoid doing weird shenanigans with re-ordering the columns according to the EVM words being used (here) etc.

I rename the previous MetadataGadget to ColumnsMetadata, but leave build and assign functions to MetadataGadget (empty struct now) in commit 051bf96.

Metadata { words: HashMap<EvmWord, MetadataSubset>

Seems the ColumnsMetadata (or MetadataSubset) needs to include the all information to extract the EVM value, since one ColumnsMetadata belongs to one leaf node of values extraction.

I also don't find this super intuitive for the end user. Maybe we could change mp2-v1 APIs (i.e., like these methods) to take as input, in place of ColumnsMetadata, the description of the table (i.e., Vec<ColumnInfo>) and the set of ids of columns to be extracted, so that MetadataGadget (which is a circuit specific data structure, and so should not be directly exposed if possible) can always be constructed inside mp2-v1? Alternatively, in place of the set of ids of columns to be extracted, we might only provide the evm_word as input and then the columns to be extracted are the ones with ColumnInfo.evm_word == evm_word?
This would also allow to avoid having both MetadataGagdet and the "alias" struct ColumnsMetadata being publicly exposed, which looks a bit weird to me. Wdyt?

I see, I fix to pass the evm_word and table_info to these APIs, and extract the column info of same slot and evm word to build the metadata in internal. Please help review this commit 50a58d9, thanks.

nikkolasg · 2024-11-11T21:12:19Z

mp2-v1/tests/common/cases/table_source.rs

+                                    panic!("Wrong slot number");
+                                }
+                                current_values
+                                    .update_contract_single_values(ctx, contract)


since the updates are deterministic then couldn't we put that inside the trait implementation directly ? Instead of computing the updates outside of the trait (which requires us to be very specific about which struct / value etc it is) and then just ask the trait to update. Wdyt ? Maybe i'm missing stg obvious here, not sure..

Since the update value is random (as +1 or -2), and we don't exactly know if slot (or field of a Struct) is the secondary index (when it's a secondary index update), I combine the both 4 single slots and 1 Struct into this SingleExtractionArgs and set a random slot_input as the secondary index (it could be one single slot or a field of Struct), so seems only SingleExtractionArgs knows which is the secondary index (not the Value which implements the trait).

…assign` functions to `MetadataGadget`.

…s computation.

nicholas-mainardi

Looks much better, thanks for the significant refactoring. Still some smaller changes, but it's getting in a good shape ;)

mp2-v1/tests/common/cases/contract.rs

mp2-v1/tests/common/cases/table_source.rs

nicholas-mainardi · 2024-11-12T21:07:06Z

mp2-v1/tests/common/cases/table_source.rs

        );
-        old_table_values.compute_update(&new_table_values[0])
+        let input = ExtractionProofInput::Single(ExtractionTableProof {
+            dimension: TableDimension::Compound,


Is TableDimension still necessary or can we remove it?

Yes, I delete the TableDimension in commit 29fd77b, re-run the integration test and it could work for me.

mp2-v1/tests/common/cases/indexing.rs

nicholas-mainardi · 2024-11-13T08:50:08Z

mp2-v1/tests/common/cases/indexing.rs

+    };
+    debug!("MAPPING ZK COLUMNS -> {:?}", columns);
+    let index_genesis_block = ctx.block_number().await;
+    let row_unique_id = TableRowUniqueID::Mapping(columns.secondary.identifier());


The row unique id, in case of mappings, should be always computed from the mapping key, independently from whether the mapping key is the secondary index or not.

Yes, sorry, I fix to the key ID in commit 2d1aa8a.

mp2-v1/tests/common/storage_trie.rs

nicholas-mainardi · 2024-11-13T09:25:41Z

mp2-v1/tests/common/cases/table_source.rs

+        metadata(&table_info)
+    }
+
+    fn storage_slots(&self, metadata: &[MetadataGadget]) -> Vec<StorageSlot> {


I also don't find this super intuitive for the end user. Maybe we could change mp2-v1 APIs (i.e., like these methods) to take as input, in place of ColumnsMetadata, the description of the table (i.e., Vec<ColumnInfo>) and the set of ids of columns to be extracted, so that MetadataGadget (which is a circuit specific data structure, and so should not be directly exposed if possible) can always be constructed inside mp2-v1? Alternatively, in place of the set of ids of columns to be extracted, we might only provide the evm_word as input and then the columns to be extracted are the ones with ColumnInfo.evm_word == evm_word?
This would also allow to avoid having both MetadataGagdet and the "alias" struct ColumnsMetadata being publicly exposed, which looks a bit weird to me. Wdyt?

mp2-v1/src/api.rs

…ct case.

nicholas-mainardi

LGTM now! APIs for column gadget looks much better now, thanks for all the hard work on this PR

…' into generic-extraction-integration-test

…on-integration-test

silathdiir requested review from nikkolasg and nicholas-mainardi October 28, 2024 09:14

silathdiir marked this pull request as draft October 28, 2024 09:14

silathdiir force-pushed the generic-extraction-integration-test branch 5 times, most recently from f453df0 to a38724c Compare October 31, 2024 09:35

silathdiir changed the base branch from generic-extraction-tree-creation to generic-extraction-row-id-update October 31, 2024 09:36

silathdiir force-pushed the generic-extraction-integration-test branch 3 times, most recently from da7559c to 1f979a4 Compare November 1, 2024 13:53

silathdiir mentioned this pull request Nov 1, 2024

feat: update DB creation circuits for generic extraction (Part 2) #393

Open

silathdiir force-pushed the generic-extraction-integration-test branch 3 times, most recently from 7c95a5b to 869a1ab Compare November 3, 2024 12:19

silathdiir changed the title ~~[WIP] fix: add single and mapping struct to integration test for generic extraction (Part 3)~~ fix: add single and mapping struct to integration test for generic extraction (Part 3) Nov 3, 2024

silathdiir marked this pull request as ready for review November 3, 2024 12:38

Update integration test for generic extraction.

93c80de

silathdiir force-pushed the generic-extraction-integration-test branch from 869a1ab to 93c80de Compare November 4, 2024 01:45

nikkolasg reviewed Nov 6, 2024

View reviewed changes

silathdiir added 2 commits November 7, 2024 15:29

Remove bit_offset in API.

a2c4cfc

Merge remote-tracking branch 'origin/generic-extraction-row-id-update…

d7607cf

…' into generic-extraction-integration-test

nicholas-mainardi requested changes Nov 7, 2024

View reviewed changes

nicholas-mainardi reviewed Nov 7, 2024

View reviewed changes

mp2-v1/src/values_extraction/gadgets/column_info.rs Outdated Show resolved Hide resolved

silathdiir added 2 commits November 7, 2024 19:00

Merge remote-tracking branch 'origin/generic-extraction-row-id-update…

51768ea

…' into generic-extraction-integration-test

Combine the single and mapping test cases, and update the merge test …

b33cd73

…case.

nikkolasg approved these changes Nov 11, 2024

View reviewed changes

silathdiir added 2 commits November 12, 2024 11:57

Rename to ContractController and update the trait function names.

911aadb

Add TODO to the deprecated bit_offset.

6b8d4bc

silathdiir added 5 commits November 12, 2024 14:24

Fix the wrong log.

e2b717c

Add back the MPT key and ptr check.

302b17e

Fix last_byte_offset to not restrict the maximum length.

6155a66

Rename MetadataGadget to ColumnsMetadata, and leave build and `…

051bf96

…assign` functions to `MetadataGadget`.

Add more common _raw functions for the values extraction identifier…

c21fe13

…s computation.

silathdiir requested a review from nicholas-mainardi November 12, 2024 10:39

nicholas-mainardi reviewed Nov 13, 2024

View reviewed changes

silathdiir added 7 commits November 13, 2024 22:11

Remove TableDimension.

29fd77b

Fix the row unique ID always get from the key ID column.

2d1aa8a

Fix to the value column as the secondary index column in mapping stru…

38d6d30

…ct case.

Merge the match arms for mapping update.

be3db69

Set to simple rest for the row key.

973325d

Fix the slot checking logic in the storage trie.

c423611

Refactor the columns metadata and APIs.

50a58d9

silathdiir requested a review from nicholas-mainardi November 14, 2024 11:04

Fix test.

f53f0ee

nicholas-mainardi approved these changes Nov 15, 2024

View reviewed changes

silathdiir added 2 commits November 15, 2024 23:12

Fix test.

45deacf

Merge remote-tracking branch 'origin/generic-extraction-row-id-update…

8e1fb40

…' into generic-extraction-integration-test

silathdiir changed the title ~~fix: add single and mapping struct to integration test for generic extraction (Part 3)~~ fix: add single and mapping struct to integration test for generic extraction (Part 4) Dec 13, 2024

silathdiir and others added 2 commits December 13, 2024 23:08

Merge remote-tracking branch 'origin/generic-extraction-row-id-update…

a22e6d7

…' into generic-extraction-integration-test

Merge branch 'generic-extraction-row-id-update' into generic-extracti…

47a5784

…on-integration-test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add single and mapping struct to integration test for generic extraction (Part 4) #397

fix: add single and mapping struct to integration test for generic extraction (Part 4) #397

silathdiir commented Oct 28, 2024 •

edited

Loading

nikkolasg left a comment

nikkolasg Nov 6, 2024

silathdiir Nov 12, 2024 •

edited

Loading

nikkolasg Nov 6, 2024

silathdiir Nov 12, 2024

nikkolasg Nov 6, 2024

silathdiir Nov 7, 2024

nikkolasg Nov 6, 2024

silathdiir Nov 11, 2024

nikkolasg Nov 6, 2024

silathdiir Nov 11, 2024

nikkolasg Nov 6, 2024

silathdiir Nov 12, 2024

silathdiir Nov 14, 2024 •

edited

Loading

nicholas-mainardi left a comment

nikkolasg left a comment

nikkolasg Nov 11, 2024

silathdiir Nov 12, 2024

nikkolasg Nov 11, 2024

silathdiir Nov 12, 2024

nikkolasg Nov 11, 2024

silathdiir Nov 12, 2024

nicholas-mainardi Nov 13, 2024

silathdiir Nov 14, 2024

nikkolasg Nov 11, 2024

silathdiir Nov 12, 2024 •

edited

Loading

nicholas-mainardi left a comment

nicholas-mainardi Nov 12, 2024

silathdiir Nov 13, 2024

nicholas-mainardi Nov 13, 2024

silathdiir Nov 14, 2024

nicholas-mainardi Nov 13, 2024

nicholas-mainardi left a comment

fix: add single and mapping struct to integration test for generic extraction (Part 4) #397

Are you sure you want to change the base?

fix: add single and mapping struct to integration test for generic extraction (Part 4) #397

Conversation

silathdiir commented Oct 28, 2024 • edited Loading

nikkolasg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silathdiir Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silathdiir Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

nicholas-mainardi left a comment

Choose a reason for hiding this comment

nikkolasg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silathdiir Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

nicholas-mainardi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholas-mainardi left a comment

Choose a reason for hiding this comment

silathdiir commented Oct 28, 2024 •

edited

Loading

silathdiir Nov 12, 2024 •

edited

Loading

silathdiir Nov 14, 2024 •

edited

Loading

silathdiir Nov 12, 2024 •

edited

Loading