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.

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

Feature: WRITE_KZG_COMMITMENT hint #176

Conversation

odesenfans
Copy link
Contributor

@odesenfans odesenfans commented Apr 17, 2024

This implementation uses a default (panicking) implementation of the KZG
commitment computation as we do not know how these must be computed.

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

@odesenfans odesenfans force-pushed the od/write-zkg-commitment branch 2 times, most recently from 1d93fe6 to 2d73870 Compare April 17, 2024 12:16
@odesenfans odesenfans changed the title Od/write zkg commitment Feature: WRITE_ZKG_COMMITMENT Apr 17, 2024
@odesenfans odesenfans changed the title Feature: WRITE_ZKG_COMMITMENT Feature: WRITE_ZKG_COMMITMENT hint Apr 17, 2024
Copy link
Collaborator

@maciejka maciejka left a comment

Choose a reason for hiding this comment

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

In what sense we do not know what needs to be computed? Python implementation is not clear?
In case it is not already known: https://community.starknet.io/t/data-availability-with-eip4844/113065

@@ -29,7 +30,10 @@ type StorageByAddress = HashMap<Felt252, OsSingleStarknetStorage<DictStorage, Pe

/// Maintains the info for executing txns in the OS
#[derive(Debug)]
pub struct ExecutionHelper {
pub struct ExecutionHelper<KZG>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do we gain by adding type parameter here? KZG calculation algorithm is not expected to change any time soon, neither we need multiple versions of ExecutionHelper with different KZG computers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, which is why a generic makes sense here. It's specified as a callback for some reason in the Python code, hinting that several implementations may be used. On the other hand, it's Rust, so it's better if we can get this indirection to be compile-time. This led me to add this as a generic. I'd be happy with having only one implementation as well but I could not find the implementation of the KZG computation anywhere.

Copy link
Collaborator

@maciejka maciejka Apr 22, 2024

Choose a reason for hiding this comment

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

Over-engineering imho. Even if there are multiple implementations of kzg there is no need to have multiple ExecutionHelpers using different implementations at the same time. I would consult Ariel, maybe I am missing something.

Copy link
Collaborator

@maciejka maciejka Apr 22, 2024

Choose a reason for hiding this comment

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

Python uses(in the nonpublic version): https://pydigger.com/pypi/ckzg.
It seems there is a pure rust version: https://github.com/grandinetech/rust-kzg.
Another thing to consult with Ariel.

This implementation uses a default (panicking) implementation of the KZG
commitment computation as we do not know how these must be computed.
@odesenfans odesenfans force-pushed the od/write-zkg-commitment branch from 2d73870 to f3d61cd Compare June 4, 2024 13:10
@odesenfans odesenfans changed the title Feature: WRITE_ZKG_COMMITMENT hint Feature: WRITE_KZG_COMMITMENT hint Jul 25, 2024
@odesenfans
Copy link
Contributor Author

Superseded by #304.

@odesenfans odesenfans closed this Aug 22, 2024
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.

2 participants