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

Interface changes to free the blobs #123

Merged
merged 3 commits into from
Feb 20, 2023

Conversation

asn-d6
Copy link
Contributor

@asn-d6 asn-d6 commented Feb 8, 2023

This PR implements the specs changes from this PR:

@asn-d6

This comment was marked as outdated.

@asn-d6 asn-d6 marked this pull request as draft February 8, 2023 23:54
@jtraglia

This comment was marked as outdated.

@jtraglia

This comment was marked as outdated.

@asn-d6

This comment was marked as outdated.

@xrchz

This comment was marked as outdated.

@potuz

This comment was marked as outdated.

@jtraglia jtraglia changed the title Implement amortized verification for when we free the blobs Interface changes to free the blobs Feb 16, 2023
@jtraglia
Copy link
Member

The bindings have been updated to support the new interface. Additionally, I updated the benchmarks for Go/Rust/Java to reflect these changes. Didn't really do much with the tests; updated what I could but removed/commented out lots of them. I would like to focus on tests in a future PR, as this one is already quite big. If it compiles it should be fine, right?

@jtraglia jtraglia marked this pull request as ready for review February 16, 2023 19:50
Copy link
Contributor Author

@asn-d6 asn-d6 left a comment

Choose a reason for hiding this comment

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

Bindings changes look reasonable but it's hard to say without tests or clients using them.

bindings/csharp/ckzg.c Outdated Show resolved Hide resolved
bindings/node.js/kzg.cxx Outdated Show resolved Hide resolved
bindings/node.js/kzg.cxx Outdated Show resolved Hide resolved
src/c_kzg_4844.c Outdated
/* Forward function definition */
static void compute_powers(fr_t *out, fr_t *x, uint64_t n);
/* Input size to the Fiat-Shamir challenge computation */
#define CHALLENGE_INPUT_SIZE 32 + BYTES_PER_BLOB + 48
Copy link
Contributor

Choose a reason for hiding this comment

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

Would static const int be better here? or even enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I think it's standard (and reasonable) practice to have public constants as #defines. We do the same thing for BYTES_PER_COMMITMENT etc.

I don't see a clear benefit to making it static const int, and I'm not sure how we would use an enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some thoughts on the benefits can be found here https://stackoverflow.com/questions/1674032/static-const-vs-define-vs-enum. In this case I was thinking of it primarily because we're doing arithmetic (+) unlike BYTES_PER_COMMITMENT which is a bare integer literal. (BYTES_PER_BLOB however would face the same argument.) We count on the compiler to constant fold this arithmetic when using macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinion here. I will defer to @jtraglia .

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah to be clear I don't really have a strong opinion either.

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion either, but I'm leaning towards a static const. In my opinion it's safer. If you decide to stick with a macro, you should wrap it in parentheses in-case it's every used in a multiplication/division operation. This was a problem with BYTES_PER_BLOB at one point in time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK! Fixed in 5353434 .

@xrchz
Copy link
Contributor

xrchz commented Feb 17, 2023

I'm happy with the changes to src. Haven't looked at the others yet.

@asn-d6
Copy link
Contributor Author

asn-d6 commented Feb 20, 2023

lgtm!

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM 💯

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.

4 participants