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

swap g2 A0, A1 for X and Y #1184

Conversation

bxue-l2
Copy link
Contributor

@bxue-l2 bxue-l2 commented Jan 29, 2025

This PR addresses the inconsistency issue when serializing the blobHeader at the controller side and at the client side.

When the controller computes a merkle tree for a batch, it serialize all Blobheader into leaves in the tree, and generate merkle proof for each blob. The merkle proof is given to the client, who computes the leaf locally and query the smart contract for the merkle verification.

Because two serialization are different, the merkle proof won't verify.

Note, the G2 serialization only affect G2 inside the blobCommitment. Not the one inside attestation.ApkG2 . The serialization in attestation.ApkG2 is A1, A0.

The BlobCommitments is not used for computation at all. So it is purely just standardizing the serialization format for G2. As it will be weird that we do both serialization in the codebase.

In the current integration test, it called VerifyDACertV2FromSignedBatch.
But in the client, we used an upgraded version called VerifyDACertV2.

@bxue-l2 bxue-l2 marked this pull request as ready for review January 29, 2025 21:02
Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM overall

core/v2/serialization.go Outdated Show resolved Hide resolved
core/v2/serialization_test.go Outdated Show resolved Hide resolved
Comment on lines 116 to 115
// afa39b4c45197f0254f7e8e2127c797c74578357e9f077eab7a8aa62e1402bec has not verified in solidity
assert.Equal(t, "afa39b4c45197f0254f7e8e2127c797c74578357e9f077eab7a8aa62e1402bec", hex.EncodeToString(hash[:]))
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean you computed the hash onchain by calling the smart contract by hand and then posted the code here? Shouldn't we just have an integration test instead that hashes offchain and onchain and makes sure they are equal?

Copy link
Contributor

Choose a reason for hiding this comment

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

These are done in chisel with a script that constructs the same struct. @bxue-l2 can you update this when you've done the validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@samlaf samlaf Jan 30, 2025

Choose a reason for hiding this comment

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

Still unclear as to how this is computed How could I reproduce using chisel? Bowen did you really use chisel? Do you have to like load the contract and then call the function? Still feel like an automated test would be way better... do we have one?

core/v2/serialization.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

lgtm, let's update core/v2/serialization_test.go when you've done the validation in solidity

core/v2/serialization_test.go Outdated Show resolved Hide resolved
Comment on lines 116 to 115
// afa39b4c45197f0254f7e8e2127c797c74578357e9f077eab7a8aa62e1402bec has not verified in solidity
assert.Equal(t, "afa39b4c45197f0254f7e8e2127c797c74578357e9f077eab7a8aa62e1402bec", hex.EncodeToString(hash[:]))
Copy link
Contributor

Choose a reason for hiding this comment

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

These are done in chisel with a script that constructs the same struct. @bxue-l2 can you update this when you've done the validation?

@bxue-l2 bxue-l2 force-pushed the swap-g2-serialization-when-generating-the-merkle-proof branch from 75da7d7 to 4a04c81 Compare January 29, 2025 23:04
Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

kudos to the pr description and code documentation, making the context clear

Copy link
Contributor

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM, although I reopened the chisel convo because I feel like we should have an automated test if we don't have one already.

@bxue-l2 bxue-l2 merged commit 2a9b5a7 into Layr-Labs:master Jan 30, 2025
8 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