Skip to content

feat: implement provably extract LIMIT/OFFSET circuits (Part1)#288

Open
Insun35 wants to merge 10 commits into
mainfrom
feat/provably-extract-limit-offset
Open

feat: implement provably extract LIMIT/OFFSET circuits (Part1)#288
Insun35 wants to merge 10 commits into
mainfrom
feat/provably-extract-limit-offset

Conversation

@Insun35

@Insun35 Insun35 commented Aug 5, 2024

Copy link
Copy Markdown
Contributor

Address issue #181

  • Public inputs
  • Record
  • Intermediate(Not implemented in this PR)
  • No results(Not implemented in this PR)

Intermediate and No-results circuits will be implemented on follow up PR because the size of this PR may be too large.

Refer to the results extraction circuits spec.

@Insun35 Insun35 force-pushed the feat/provably-extract-limit-offset branch 3 times, most recently from f34c61f to cc9b20a Compare August 8, 2024 14:58
@Insun35 Insun35 changed the title feat: add public inputs for results extraction circuits feat: implement provably LIMIT/OFFSET circuit Aug 8, 2024
@Insun35 Insun35 changed the title feat: implement provably LIMIT/OFFSET circuit feat: implement provably extract LIMIT/OFFSET circuit Aug 8, 2024
@Insun35 Insun35 changed the title feat: implement provably extract LIMIT/OFFSET circuit feat: implement provably extract LIMIT/OFFSET circuits Aug 8, 2024
@Insun35 Insun35 force-pushed the feat/provably-extract-limit-offset branch 2 times, most recently from 9384db3 to 4377c28 Compare August 11, 2024 12:57
@Insun35 Insun35 marked this pull request as ready for review August 12, 2024 04:43
@Insun35 Insun35 force-pushed the feat/provably-extract-limit-offset branch from 4377c28 to 27f8fb3 Compare August 12, 2024 05:31
@Insun35 Insun35 force-pushed the feat/provably-extract-limit-offset branch from 27f8fb3 to 1ab5337 Compare August 12, 2024 12:04
@Insun35 Insun35 changed the title feat: implement provably extract LIMIT/OFFSET circuits feat: implement provably extract LIMIT/OFFSET circuits (Part1) Aug 12, 2024
Comment thread verifiable-db/src/results_tree/extraction/public_inputs.rs Outdated
Comment thread verifiable-db/src/results_tree/extraction/record.rs Outdated
Comment thread verifiable-db/src/results_tree/extraction/record.rs Outdated
Comment thread verifiable-db/src/results_tree/extraction/record.rs Outdated
Comment thread verifiable-db/src/results_tree/extraction/record.rs
Comment thread verifiable-db/src/results_tree/extraction/record.rs Outdated
- replace accumulator input from final_tree_hash to tree_hash
- add test for accumulator check
Add new method to properly initialize the circuit ensuring that
indexed_items[1] == U256::ZERO if it is a dummy value.

Also modify test code to use new method when constructing the circuit.

@silathdiir silathdiir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nicholas-mainardi nicholas-mainardi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a minor comment

.chain(empty_hash_fields)
.chain(indexed_items[1].to_fields())
.chain(indexed_items[1].to_fields())
.chain(second_indexed_item.unwrap_or(U256::ZERO).to_fields())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: we could replace this with test_circuit.second_indexed_item so that we don't have to do this unwrap_or everywhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 716e652.

@delehef delehef force-pushed the main branch 8 times, most recently from d8cf76a to a9e2b59 Compare January 25, 2025 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants