Conversation
b399491 to
016b17f
Compare
89ef737 to
89789c4
Compare
| // 2) Retrieve the G1 generator (1,2) from the bn254 package | ||
| var g1Aff bn254.G1Affine | ||
| g1Aff.X.SetOne() | ||
| g1Aff.Y.SetUint64(2) |
There was a problem hiding this comment.
Why is this hard-coded instead of using library function? See https://github.com/consensys/gnark-crypto/blob/v0.16.0/ecc/bn254/bn254.go#L151
There was a problem hiding this comment.
It’s hardcoded to use the same generators as on the Solidity side. Otherwise, I couldn’t verify the proof. (Or I need to pass them as an input too)
There was a problem hiding this comment.
OK, makes sense. But then I think the comment "Retrieve the G1 generator (1,2) from the bn254 package" is misleading.
| * (2) sharedSecret = bidPub^sk | ||
| * using the non-interactive challenge c = H(...). | ||
| * | ||
| * The proof is {c, z}, plus we have inputs {providerPub, bidPub, sharedSecret}. |
There was a problem hiding this comment.
Would be good to recall here what zkProof[0], zkProof[1], etc., are.
aloknerurkar
left a comment
There was a problem hiding this comment.
Apart from the minor comments LGTM!
| if (msg.sender == winner) { | ||
| require( | ||
| _verifyZKProof(zkProof), | ||
| ProviderZKProofInvalid(msg.sender) |
There was a problem hiding this comment.
We should also add commitment information here to be safe. The txn failures get the error and print it in the logs. So it would be helpful to know in logs as well the exact commitment that failed.
| pk *bn254.G1Affine | ||
| err error | ||
| ) | ||
| if bid.NikePublicKey == nil { |
There was a problem hiding this comment.
Why is this check required? We will always generate a new one no?
mrekucci
left a comment
There was a problem hiding this comment.
Apart from the comments, LGTM!
e3d6ce1 to
e1dda17
Compare
| * @param decayEndTimeStamp The end time of the decay | ||
| * @param bidSignature The signature of the bid | ||
| * @param sharedSecretKey The shared secret key | ||
| * @param zkProof The zk proof |
There was a problem hiding this comment.
| * @param zkProof The zk proof | |
| * @param zkProof The zk proof is an array of c and z, representing the challenge and z = k - c*sk |
There was a problem hiding this comment.
technically I guess that isn't the case, since you're wrapping a bunch of data into that array.
Describe your changes
Overview of the Changes in PR #586 - Zero-Knowledge Shared Key
Below is a high-level summary of what's going on in this pull request, focusing on the major changes and how they fit together. It’s grouped by core themes:
Doc for the reference: https://www.notion.so/primev/Preventing-Unopenable-Commitments-1606865efd6f8097927ed9c162ea7631
1. Zero-Knowledge (ZK) Context and Proof Flows
New
BN128.solUtilitycontracts/contracts/utils/BN128.sol, which provides wrappers for two bn128 precompiles:ecAdd(bn128 addition at0x06)ecMul(bn128 multiplication at0x07)BN128AddFailedorBN128MulFailed.ZK Proof in
PreconfManagerPreconfManager.solnow:zkContextHash(like"mev-commit opening <chainID>")._zkProofinopenCommitment(...)._verifyZKProof(zkProof)._verifyZKProof(zkProof):a = g^z * (providerPub)^ca' = B^z * C^czkContextHash→ a challenge_zkProof’s challengeABI Changes
PreconfManager.abi& generated Go bindings changed:sharedSecretKeyin function parameters/events replaced byuint256[] zkProof.BN128AddFailed,BN128MulFailed,ProviderZKProofInvalid.2. Struct / Signature Layout Changes
Commitment Data
sharedSecretKeyis removed; each side now supplies an 8-elementzkProofarray ([providerX, providerY, bidderX, bidderY, sharedX, sharedY, c, z]).Bid Hash
EIP-712 struct for a bid now includes bidder’s BN254 public key:
Go code (e.g. encryptor/encryptor.go) updated to include (bidderPKx, bidderPKy) in ABI-encoded hashing.
Pre-Confirmation Hash
Similarly updated from:
to
3. Go Libraries and BN254 Key Handling
p2p/pkg/crypto/ecdh.goandzkproof.goGenerateKeyPairBN254()→(sk, pk)DeriveSharedKey(skA, pkB)→ shared G1 pointC.GenerateOptimizedProof(sk, A, B, C, context)→(c, z)VerifyOptimizedProof(proof, A, B, C, context)→boolBN254PublicKeyToBytes(...) / BN254PublicKeyFromBytes(...)(96 bytes uncompressed).BN254PrivateKeyToBytes(...) / BN254PrivateKeyFromBytes(...)(32 bytes forfr.Element).keysstore.Store:SetBN254PrivateKey(...),GetBN254PrivateKey(...)SetBN254PublicKey(...),GetBN254PublicKey(...)4. Effects on the P2P Side
p2p/pkg/p2p/libp2p/internal/handshake/now exchanges BN254 public keys instead of ECDH P256.Keysstruct inp2p/pkg/p2p/p2p.gochanged:NIKEPublicKey→*bn254.G1Affine(was*ecdh.PublicKey).5. Contract and Storage Adjustments
PreconfManager:openCommitment(...)→ final param_zkProof.msg.sender == winner, calls_verifyZKProof(_zkProof).ProviderZKProofInvalidif verification fails.sharedSecretKeyin events.6. Typical Flow
(PKb)in EIP-712 bid signature.(PKb), computes shared secretC = PKb^skProvider, and generates a ZK proof(c,z)for that ephemeral secret.openCommitment(…, _zkProof)._verifyZKProof(...).7. Summary
sharedSecretKey bytesapproach with BN254 ECDH + zero-knowledge proof to ensure the ephemeral key is valid, without exposing it.Checklist before requesting a review