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

Adapt low degree proof on G2 for compatibility with perpetual power of tau #222

Merged
merged 10 commits into from
Feb 3, 2024

Conversation

bxue-l2
Copy link
Contributor

@bxue-l2 bxue-l2 commented Jan 30, 2024

Why are these changes needed?

We uses the SRS generated by perpetual power of tau

Although the perpetual power of tau is designed to have 2^28 points, in the setup, they actually generated 2^29-1 g1 points. https://github.com/dcbuild3r/ptau-deserializer/blob/main/deserialize/ptau.go#L25. Whereas there are only 2^28 G2 points. It is my miss that didn’t catch it .

Then the problem is the adversary can create a wrong degree proof. I.e the following equation
e( [poly(tau)] g1, tau^[max-claimedDegree] g2) = e( [poly(tau^(max-claimedDegree))] g1, [1] g2 )
left hand side g1 is the commitment, right hand side g1 is the degree proof.

Since adversray actually know [2max]g1 due to the artifact of the setup, the adversary can use a higher degree polynomial than claimed, and create a wrong proof by using the higher degree g1. Can you confirm?

Here is a fix, we swap g1 and g2 for the degree proof. Since adversary cannot find a higher g2, then we are safe.
Then we prove the equivalence of [poly(tau)] g1 used in universal verifier and [poly(tau)]_ g2 used in the new degree proof, by following equation
e([poly(tau)] g1, g2) = e(g1, poly(tau) g2)

Since all pairing have common g2 on the left hand side, we can batch all the pairing with 2 MSM. What do you think.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@bxue-l2 bxue-l2 changed the title init change Adapt low degree proof on G2 for compatibility with perpetual power of tau Jan 31, 2024
@bxue-l2 bxue-l2 marked this pull request as ready for review January 31, 2024 18:23
@@ -108,16 +108,19 @@ message Bundle {
message BlobHeader {
// The KZG commitment to the polynomial representing the blob.
bytes commitment = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

while we're making this breaking change, should we update the API so that it returns x and y coordinates of the commitment instead of the serialized bytes?
same as length_commitment and length_proof

Copy link
Contributor

Choose a reason for hiding this comment

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

Or JSON, it's much more widely accepted than gob

Copy link
Contributor

Choose a reason for hiding this comment

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

yup either one
@bxue-l2 if you want me to work on it let me know. I can work on a separate PR that this can rebase onto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that will be great

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -98,8 +99,8 @@ func (e *Encoder) Encode(data []byte, params core.EncodingParams) (core.BlobComm
}

func (e *Encoder) VerifyBlobLength(commitments core.BlobCommitments) error {

return e.EncoderGroup.VerifyCommit(commitments.Commitment.G1Point, commitments.LengthProof.G1Point, uint64(commitments.Length-1))
//commitments.Commitment.G1Point,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

}

// compute commit for the full poly
commit := g.Commit(poly.Coeffs)
lowDegreeCommitment := bls.LinCombG2(g.Srs.G2[:len(poly.Coeffs)], poly.Coeffs)
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the verifier know this is committing to the same polynomial as the above commitment (just use a different group)?

Copy link
Contributor

Choose a reason for hiding this comment

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

The file name convention has been using underscore to connect subparts

@@ -108,16 +108,19 @@ message Bundle {
message BlobHeader {
// The KZG commitment to the polynomial representing the blob.
bytes commitment = 1;
// The KZG commitment to the polynomial representing the blob on G2, it is uesd
Copy link
Contributor

Choose a reason for hiding this comment

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

uesd -> used.
The length_proof is used to prove the degree, not length_commitment?

@@ -108,16 +108,19 @@ message Bundle {
message BlobHeader {
// The KZG commitment to the polynomial representing the blob.
bytes commitment = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Or JSON, it's much more widely accepted than gob

@@ -14,6 +14,16 @@ type Commitment struct {
*bn254.G1Point
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should do away with all of these custom types. My suggestion:

We have

type G1Point struct {
	*bn254.G1Point
}

type G2Point struct {
	*bn254.G2Point
}

And create seralization/deserialization methods for these.

If we want to, we can also do some type aliasing, i.e.

type LengthProof = G2Point
type RevealProof = G1Point

I think it's probably okay to fully drop the Commitment and LengthCommitment tpyes/aliases and just use the G1Point and G2Point types directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this suggestion.
What about creating a new type instead of type aliasing though (type LengthProof G2Point)? I think it's safer to use stronger types

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing the stronger typing here obviously has the disadvantage that you can't use any methods like serialization/deserialization of the G2Point directly on the LengthProof object. You'd have unwrap, call the method, or call the method and then wrap.

I don't have a strong sense of the advantages of the stronger typing in this case, but I'm happy to defer to you.

Commitment *Commitment `json:"commitment"`
LengthProof *Commitment `json:"length_proof"`
Length uint `json:"length"`
Commitment *Commitment `json:"commitment"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it would be clearer/cleaner to just call the Commitment and LengthCommitment fields G1Commitment and G2Commitment?

Wdyt @bxue-l2, @jianoaix, @ian-shim?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the name of the type, yeah that sounds clearer

"github.com/stretchr/testify/require"
)

func TestBatchEquivalence(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a failing test case for difference commits?

Copy link
Collaborator

@mooselumph mooselumph 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, just a few comments on naming and tests.

@mooselumph
Copy link
Collaborator

Looks like this may need to be rebased to the current master.

Ubuntu and others added 10 commits February 3, 2024 00:08
Co-authored-by: Robert Raynor <[email protected]>
Co-authored-by: Jian Xiao <[email protected]>
Co-authored-by: steven <[email protected]>
Co-authored-by: steven <[email protected]>
Co-authored-by: siddimore <[email protected]>
Co-authored-by: Ian Shim <[email protected]>
Co-authored-by: bolatfurkan <[email protected]>
Co-authored-by: Peter Straus <[email protected]>
Co-authored-by: WELLINGTON MIRANDA BARBOSA <[email protected]>
Co-authored-by: Wellington Barbosa <[email protected]>
Co-authored-by: Jian Xiao <[email protected]>
Co-authored-by: buldazer <[email protected]>
Co-authored-by: Madhur Shrimal <[email protected]>
Co-authored-by: Daniel Mancia <[email protected]>
@bxue-l2 bxue-l2 force-pushed the adapt-low-degree-proof-to-g2 branch from 2261b80 to bd6f70d Compare February 3, 2024 00:22
@bxue-l2
Copy link
Contributor Author

bxue-l2 commented Feb 3, 2024

Looks like this may need to be rebased to the current master.

rebased

@bxue-l2
Copy link
Contributor Author

bxue-l2 commented Feb 3, 2024

Will addressed in a PR for cleanup

The file name convention has been using underscore to connect subparts from Jian
#222 (comment)

uesd -> used. Nix from Jian
#222 (comment)

Unwrap and string typing from Robert
#222 (comment)

CommitmentType from Robert
#222 (comment)

@ian-shim
Copy link
Contributor

ian-shim commented Feb 3, 2024

Also needs to address this one in the follow up PR: #222 (comment)

@bxue-l2 bxue-l2 merged commit 3aabde7 into Layr-Labs:master Feb 3, 2024
4 checks passed
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.

5 participants