feat: add dummy circuit to final extraction for indexing Data with no provable extraction by silathdiir · Pull Request #415 · Lagrange-Labs/mapreduce-plonky2

silathdiir · 2024-11-29T09:56:04Z

The related document: Indexing Data with No Provable Extraction

Summary

Add a dummy circuit to final extraction, it exports the input digests as public values directly (without proving).
Add compute_table_row_digest and merge_table_row_digests for table row digest computation.
Add run_no_provable_test_cases to wrap the original test cases to run the final extraction and check with DB result directly (without values extraction).

…(key)))`.

…cuit.

…Gadget`.

…dget`.

…ble.

… contract `Simple.sol`.

…ng-with-no-probvable-ext

…n table

…ng-with-no-probvable-ext

nikkolasg · 2025-02-06T12:06:19Z

mp2-v1/src/final_extraction/api.rs

    }
+    /// Instantiate inputs for the dummy circuit dealing with no provable extraction case
+    pub fn new_no_provable_input<PrimaryIndex: PartialEq + Eq + Default + Clone + Debug>(
+        block_number: U256,


Can we rename this "primary index" ?

And shouldn't this be of type PrimaryIndex ? Or why we have a primary index generic in that case ?

Renamed block_number -> primary_index, block_hash -> root_of_trust, prev_block_hash -> prev_root_of_trust. Regarding the PrimaryIndex generic: I mostly introduced it to satisfy the trait bound for the CellsCollection provided as input to represent the rows of the table. It required some additional bounds but I have changed the type of primary_index to PrimaryIndex as well, given that we have the generic type I agree it makes sense to use it also for the primary_index input parameter

nikkolasg · 2025-02-07T08:11:44Z

mp2-v1/src/final_extraction/api.rs

+    pub fn new_no_provable_input<PrimaryIndex: PartialEq + Eq + Default + Clone + Debug>(
+        block_number: U256,
+        block_hash: OffChainRootOfTrust,
+        prev_block_hash: HashOutput,


Do we have to provide one ? If it's not equal to the previous insertion, will it fail ? Couldn't we simply put a dummy value inside the API ?

Yes, if we don't provide here the block_hash used for previous primary index, a constraint in IVC proof will fail. I think it would be simpler and cleaner to provide the previous root of trust here rather than having to handle that constraint differently in the IVC proof, no?

As agreed in DM, in commit 2f71b25 I changed the interface of this method to take as input the previous IVC proof rather than the previous block hash, so that we can fetch the previous block hash from the proof internally

nikkolasg · 2025-02-07T08:13:42Z

mp2-v1/src/values_extraction/mod.rs


+/// Compute the identifier for a column of a table containing off-chain data
+pub fn identifier_offchain_column(table_name: &str, column_name: &str) -> u64 {
+    let inputs: Vec<F> = vec![table_name, column_name]


should we put a DST just because ? like OFFCHAIN || table_name || col_name ?

Done in commit 4a513aa

nikkolasg · 2025-02-07T08:30:06Z

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

+            ColumnMetadata::NoPrimaryKey(REMAINING_COLUMN_NAMES[0].to_string()),
+            ColumnMetadata::NoPrimaryKey(REMAINING_COLUMN_NAMES[1].to_string()),
+        ];
+        let mut off_chain_data_args = OffChainDataArgs::new(


Could we name it OffchainTable ?

Renamed to OffChainTableArgs in commit 4a513aa to keep naming consistent with the other table types

nikkolasg · 2025-02-07T08:32:24Z

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

+                // there is no previous primary index, so we generate previous hash at random
+                HashOutput::from(array::from_fn(|_| rng.gen()))


Even if there is, could we avoid relying on this ?

AFAIU this is asking the same as this comment?

This has been removed after the API changes in commit 2f71b25

nikkolasg · 2025-02-07T08:33:43Z

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

+            prev_hash,
+            primary_index: self.last_update_epoch,
+            rows,
+            primary_key_columns: self.primary_key_column_ids(),


This is confusing there should only be ONE primary key column no ?

Not necessarily, a primary key may be composed of multiple columns: think of mapping of mappings tables, where the primary key is given by the keys of both the outer and the inner mappings. To clarify, primary key != primary index:: the primary key is the set of columns that uniquely identifies a row, while the primary index is just an index we build on a column (but it isn't necessarily unique for each row). I added a comment to ColumnMetadata of OffchainTableArgs data structure to clarify the meaning of primary key columns.

Ahhh gotcha. ok. @delehef something we discussed this morning.

Zyouell

Looks pretty good to me! I had one or two comments but they are really more of a personal taste thing than actual needs so I've still mentioned them but will leave it up to you to decide whther they are good ideas or not!

Zyouell · 2025-02-12T16:08:38Z

mp2-v1/src/values_extraction/mod.rs

+}
+
+/// Compute the row unique data using the set of column values provided as input
+pub fn row_unique_data<'a, I: Iterator<Item = &'a [u8]>>(columns: I) -> HashOutput {


I believe if you changed this to row_unique_data<I: IntoIterator<Item = &[u8]>> and then changed it to let packed_columns = columns.into-iter(). .... it would make it so that you can pass anything to this function as long as it iterates a slices (and also gets rid of the lifetime) which would mean we wouldn't have to make everything into vec![stuff].into_iter() when we call this function above.

Done in commit 0cf3693, though it didn't get rid of the lifetime, but that's ok I guess

Zyouell · 2025-02-12T16:16:09Z

mp2-v1/tests/common/cases/query/aggregated_queries.rs

    let initial_epoch = table.row.initial_epoch().await;
    // choose query bounds outside of the range [initial_epoch, last_epoch]
-    let min_block = 0;
+    let min_block = max(0, initial_epoch - 2) as usize;


This seems pointless to me as a usize can't be smaller than zero

initial_epoch is a signed integer, so subtracting 2 could yield a negative value, that's why I added the max method. Does it make sense?

Oh yeah of course, you are converting to usize after the call to max my bad

Zyouell · 2025-02-12T16:23:02Z

mp2-v1/tests/common/cases/query/mod.rs

        | TableSource::MappingStruct(_, _)
        | TableSource::MappingOfSingleValueMappings(_)
        | TableSource::MappingOfStructMappings(_) => query_mapping(ctx, &table, &t).await?,
+        TableSource::OffChain(_) => {


As an alternative solution to tables having to keep track of where they came from (because in my opinion once a table is a "table" it shouldn't care about where it came from), could we make query_table_with_no_consecutive_blocks() return and Option<QueryCookin> and then ust handle the None case in the query_mapping function?

Done in commit 0cf3693, nice suggestion, thanks

…secutive_blocks_ryhope

…ng-with-no-probvable-ext

…bvable-ext

silathdiir and others added 30 commits October 7, 2024 05:50

Fix to pack before digest computation as `D(key_id || pack(left_pad32…

9c9519a

…(key)))`.

Fix to only add values digest if EVM word is zero in leaf mapping cir…

6640d85

…cuit.

Fix to use one field for the prefix KEY.

87809b8

Fix wrong byte order in unpack_u32_to_u8_target.

94c9de8

Add mapping of mappings leaf circuit.

691b69f

Update storage key.

4f27919

Update values extraction API.

08642f3

Add tests for column and metadata gadgets.

c7371b0

Add tests for storage key.

6b8c7af

Add test for leaf single circuit.

cdc353b

Add test to leaf mapping circuit.

7127536

Add test for leaf mapping of mappings circuit.

e54449a

Merge branch 'main' into feat/merge_table

0cd9487

Delete aggregation type (is_simple_aggregation) in branch circuit.

1da45a1

Universal query gadgets + refactor universal query circuit

61dc87a

fmt

40ecb96

Add universal_query_gadget file

f772131

Replace 31 with MAPPING_LEAF_VALUE_LEN - 1.

b23465d

Replace wrong curve_eq with connect_curve_points in `TestMetadata…

46eeeb4

…Gadget`.

Replace wrong curve_eq with connect_curve_points in `TestColumnGa…

65fe5cf

…dget`.

Fix a bug in ColumnGadget.

5b8ad23

Move common fields to MetadataGadget.

23ecf24

Fix unpack_u32_to_u8_targets.

cd5fa5c

Fix to only set maximum as u8::MAX + 8 for first_bits_5 lookup ta…

3407665

…ble.

Fix to generic paramters for impl CircuitLogicWires.

bc84083

Add API tests.

380be6b

Add simple struct, mapping struct and mapping of mappings to the test…

02290cd

… contract `Simple.sol`.

Update bindings.

ef7d07b

Enable single value extraction in integration test.

43791de

Enable mapping value extraction in integration test.

08ebeea

nicholas-mainardi added 13 commits February 3, 2025 12:23

fmt

ea59525

Improve comments to bracketer secondary index

737079d

Fix after rebase

1298394

fmt

73540da

Merge branch 'feat/support_non_consecutive_blocks_ryhope' into indexi…

09a641f

…ng-with-no-probvable-ext

Change indexing integration test to support non-consecutive blocks

cc8326c

Indexing integration test supporting non-consecutive blocks

3116e7f

Query in integration test compatible with non-conseuctive blocks

38a6167

Query in integration test compatible with non-conseuctive blocks

2f13f53

Merge branch 'feat/support_non_consecutive_blocks_ryhope' into indexi…

add5bd9

…ng-with-no-probvable-ext

Add test with empty range between non-consecutive blocks for off-chai…

c1afa79

…n table

Fix typo in bracketer_primary_index query

54a23a2

Merge branch 'feat/support_non_consecutive_blocks_ryhope' into indexi…

2e26327

…ng-with-no-probvable-ext

nicholas-mainardi changed the base branch from generic-extraction-integration-test-mapping-of-mappings to feat/support_non_consecutive_blocks_ryhope February 6, 2025 11:50

nicholas-mainardi requested a review from Zyouell February 6, 2025 11:58

nikkolasg reviewed Feb 7, 2025

View reviewed changes

nicholas-mainardi added 2 commits February 7, 2025 10:27

Address comments

4a513aa

Change no provable circuit API to get previous proof as input

2f71b25

Zyouell approved these changes Feb 12, 2025

View reviewed changes

Address comments

0cf3693

mergify bot mentioned this pull request Mar 5, 2025

[do no merge] visualization pr #460

Closed

nicholas-mainardi and others added 5 commits March 5, 2025 22:31

Merge branch 'feat/new_extraction_features' into feat/support_non_con…

981a1d3

…secutive_blocks_ryhope

Merge branch 'feat/support_non_consecutive_blocks_ryhope' into indexi…

d9ef2b2

…ng-with-no-probvable-ext

chore: add missing feature flag

5940e9d

chore: add serde to circuit inputs

5e403b7

Increase MAX_BLOCK_SIZE

6ff215e

Base automatically changed from feat/support_non_consecutive_blocks_ryhope to feat/new_extraction_features March 5, 2025 22:38

Merge branch 'feat/new_extraction_features' into indexing-with-no-pro…

fbcf575

…bvable-ext

nicholas-mainardi merged commit 9d292a7 into feat/new_extraction_features Mar 6, 2025
5 checks passed

nicholas-mainardi deleted the indexing-with-no-probvable-ext branch March 6, 2025 09:18

		// there is no previous primary index, so we generate previous hash at random
		HashOutput::from(array::from_fn(\|_\| rng.gen()))

Conversation

silathdiir commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zyouell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

silathdiir commented Nov 29, 2024 •

edited

Loading