Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: split_commitment function in KZG data availability implementation #391

Merged
merged 13 commits into from
Oct 8, 2024
Merged

fix: split_commitment function in KZG data availability implementation #391

merged 13 commits into from
Oct 8, 2024

Conversation

ocdbytes
Copy link
Contributor

@ocdbytes ocdbytes commented Oct 1, 2024 •
edited by whichqua
Loading

Problem:
The bug was that the commitment split returned by this function was wrong. I encountered this when I was integrating the snos with madara orchestrator and performing an e2e test. On the final step of the block processing the contract was giving an error saying that "precompile evaluation failed". After debugging we found out that this is the issue the commitment returned by the snos was wrong.

Issue Number: N/A

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Breaking changes?

  • yes
  • no

@whichqua
Copy link
Collaborator

whichqua commented Oct 1, 2024

I checked out the PR. Here are my questions:

  • What exactly was the bug with the split commitment function, and how did your encounter it?
  • Could you link the edge cases were covered by the new test for split commitment to its cairo-lang alternative? (A simple comment should do)
  • Did you notice or expect any performance changes with this fix?

All in all thanks for this PR!

@ocdbytes
Copy link
Contributor Author

ocdbytes commented Oct 1, 2024

  • The bug was that the commitment split returned by this function was wrong. I encountered this when I was integrating the snos with madara orchestrator and performing an e2e test. On the final step of the block processing the contract was giving an error saying that "precompile evaluation failed". After debugging we found out that this is the issue the commitment returned by the snos was wrong.

  • I am not knowledgable enough to get the edge cases for this function as I have very little knowledge of snos but I checked the logic with the e2e testing and even with the contract logic which is mentioned in the comment above the test. Now the split returned by this function is correct and we are able to properly verify the proof on Starknet Core Contract. Please do tell me what edge cases can be here It will also be helpful for me in future and will be an addition to my knowledge.

  • I didn't notice any (noticable) performance changes with this fix as it is a simple split function.

If you need any more clarification or there is any confusion we can also discuss this on call.

Thanks

@whichqua
Copy link
Collaborator

whichqua commented Oct 2, 2024

Thanks for your speedy response.

If you need any more clarification or there is any confusion we can also discuss this on call.

I think you have clarified enough. But I am open to a call, generally just to discuss and understand how you are using snos.

@ocdbytes
Copy link
Contributor Author

ocdbytes commented Oct 2, 2024

Thanks for your speedy response.

If you need any more clarification or there is any confusion we can also discuss this on call.

I think you have clarified enough. But I am open to a call, generally just to discuss and understand how you are using snos.

Sure here is my calendly link you can schedule a call whenever you are free:

https://calendly.com/d/cpt8-mjq-vjf/30min

whichqua
whichqua requested changes Oct 3, 2024
View reviewed changes
crates/starknet-os/src/hints/kzg.rs Outdated Show resolved Hide resolved
crates/starknet-os/src/hints/kzg.rs Outdated Show resolved Hide resolved
@whichqua whichqua changed the title Fix : Kzg commitment in snos output fix: split_commitment function in KZG data availability implementation Oct 4, 2024
@HermanObst
Copy link
Collaborator

@ocdbytes thanks for this LGTM

notlesh
notlesh approved these changes Oct 4, 2024
View reviewed changes
Copy link
Collaborator

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

Looks good. I'd prefer to use constants as was done previously, but I won't make a big deal out of that. (CC @HermanObst)

@HermanObst
Copy link
Collaborator

@notlesh agree.
@ocdbytes please add that and we will merge it :)

whichqua
whichqua approved these changes Oct 8, 2024
View reviewed changes
Copy link
Collaborator

@whichqua whichqua left a comment

Choose a reason for hiding this comment

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

LGTM

@HermanObst HermanObst merged commit 3bd95bf into keep-starknet-strange:main Oct 8, 2024
5 of 6 checks passed
notlesh pushed a commit that referenced this pull request Oct 11, 2024
@ocdbytes @HermanObst @notlesh
fix: split_commitment function in KZG data availability implementat…
3a10373 
…ion (#391)

* debug : prints added for blob

* debug : prints added for blob

* debug : prints added for blob

* debug : prints added for blob

* fix: fixed split commitment function

* fix: fixed split commitment function

* fix: fixed split commitment function

* feat : removed logs

* fix : lint and clippy

* test : added a test for split commitment function

* feat : requested changes done

* chore : added constants for hardcoded values

---------

Co-authored-by: Herman Obst Demaestri <70286869+HermanObst@users.noreply.github.com>
HermanObst added a commit that referenced this pull request Oct 18, 2024
@whichqua @ftheirs
@shamsasari @HermanObst @ocdbytes @notlesh
fix: Identifier retdata is not relocatable (#393)
662d170 
* wip: adds integration tests for invoke with delpoy_contract syscall

* test: add tests for contract with no calldata

* Fix inconsistent hash (#390)

* point to snos_requirements branch

* fix inconsistent hash

* add blocks to CI

* Using reference PIE to validate our PIEs are correct (#389)

The reference PIE was created with `full_output` enabled, so `output.rs` was updated to support that. The code was refactored to better mirror the Cairo serialization code.

Co-authored-by: Herman Obst Demaestri <hodemaestri@gmail.com>

* fix: `split_commitment` function in KZG data availability implementation (#391)

* debug : prints added for blob

* debug : prints added for blob

* debug : prints added for blob

* debug : prints added for blob

* fix: fixed split commitment function

* fix: fixed split commitment function

* fix: fixed split commitment function

* feat : removed logs

* fix : lint and clippy

* test : added a test for split commitment function

* feat : requested changes done

* chore : added constants for hardcoded values

---------

Co-authored-by: Herman Obst Demaestri <70286869+HermanObst@users.noreply.github.com>

* QOL++

* Bugfix: work around retdata being Int(0)

* Add test case for retdata issue

* Update hashes for modified contract code

* First pass at retdata hack

* fmt

* clippy

* Fix compile errors in tests

* fmt

* Wrap os_input in Arc

* s/Arc/Rc/

* fmt

* clippy

* Avoid hideous clone

* Don't unwrap

* Leave link to relevant code

* Refine comment about no-oping

* Add link to retdata cast()

* clippy

* Pass compiled OS to run_os

---------

Co-authored-by: ftheirs <fntheirs@gmail.com>
Co-authored-by: Shams Asari <shams@moonsonglabs.com>
Co-authored-by: Herman Obst Demaestri <hodemaestri@gmail.com>
Co-authored-by: Arun Jangra <arunjangra1001@gmail.com>
Co-authored-by: Herman Obst Demaestri <70286869+HermanObst@users.noreply.github.com>
Co-authored-by: Stephen Shelton <steve@brewcraft.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whichqua whichqua whichqua approved these changes

@HermanObst HermanObst HermanObst approved these changes

@notlesh notlesh notlesh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ocdbytes @whichqua @HermanObst @notlesh