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

Remove point-at-infinity for proofs/commitments #8063

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

jtraglia
Copy link
Contributor

@jtraglia jtraglia commented Mar 11, 2024

PR Description

I noticed that KZGProof and KZGCommitment were using the wrong point-at-infinity bytes. It was 48 zero bytes and it should have been 0xc0 followed by 47 zero bytes. See the G1_POINT_AT_INFINITY constant here:

https://github.com/ethereum/consensus-specs/blob/dev/specs/deneb/polynomial-commitments.md#constants

I've made this mistake several times myself. It's the uncompressed bytes which are all zero; the compressed bytes include the "infinity" flag. This only appears to have been used in test code, so no worries.

  • Additionally, the two classes were providing this value differently.
    • I like the static variable style, so I switched the other one to that style.
  • I believe BlobSidecar#toLogString should print the abbreviated commitment.
    • I will revert this change if this was intentional.

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

Is there a way to get a test for it?

@@ -20,8 +20,8 @@
public class BlobSidecarsByRootRequestMessageSchema
extends AbstractSszListSchema<BlobIdentifier, BlobSidecarsByRootRequestMessage> {

// Size validation according to the spec (MAX_REQUEST_BLOCKS_DENEB * MAX_BLOBS_PER_BLOCK) is
// done in the RPC handler
// Size validation according to the spec (MAX_REQUEST_BLOCKS_DENEB *
Copy link
Contributor

Choose a reason for hiding this comment

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

This message means we don't throw in constructor if it exceeds (MAX_REQUEST_BLOCKS_DENEB * MAX_BLOBS_PER_BLOCK), we will have spec check later. So here we cap it by 2048 and later check that it's 6, so this comment is made for understanding why don't we cap here on 6 and where is actual validation.
So I'd revert this file change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. I misread that. Will revert that change!

Copy link
Contributor

Choose a reason for hiding this comment

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

this line where actual validation happens

@jtraglia
Copy link
Contributor Author

Is there a way to get a test for it?

Yes, I think so. I will work on that.

@zilm13
Copy link
Contributor

zilm13 commented Mar 12, 2024

@jtraglia it would be good to have it in spec tests. We could be not alone

@jtraglia
Copy link
Contributor Author

@jtraglia it would be good to have it in spec tests. We could be not alone

Good idea. I'll look into that. To back that up, mev-boost had the same issue:

@jtraglia
Copy link
Contributor Author

I chatted with a coworker and we think it would be best to remove the infinity value from the two classes. It shouldn't be necessary in clients; it should only be needed internally in the KZG library. Would it be okay for me to remove these?

@jtraglia jtraglia changed the title Use correct point-at-infinity for proofs and commitments Remove point-at-infinity for proofs/commitments Mar 12, 2024
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

 LGTM

@zilm13 zilm13 enabled auto-merge (squash) March 13, 2024 18:24
@zilm13 zilm13 merged commit 6e1f521 into Consensys:master Mar 13, 2024
14 of 15 checks passed
@zilm13
Copy link
Contributor

zilm13 commented Mar 13, 2024

Merged, thank you @jtraglia

@jtraglia jtraglia deleted the correct-point-at-infinity branch March 21, 2024 16:14
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