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

feat(prover | p2p): extend epoch quote validation #10863

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Maddiaa0
Copy link
Member

@Maddiaa0 Maddiaa0 commented Dec 18, 2024

Overview

  • Replaces the request to get EIP712 prover coordination signatures to compute them locally, rather than requesting it from the rollup each time
  • Alters the l1 contract such that the rollup version is the same as the EIP712 version
  • Changes VERSION to ROLLUP_VERSION cus searching VERSION brings up 2000 results, this will be much nicer to maintain

fixes: #8911

Base automatically changed from md/other-type-p2p-validators to master December 18, 2024 22:02
@Maddiaa0 Maddiaa0 marked this pull request as ready for review December 18, 2024 22:02
@Maddiaa0 Maddiaa0 force-pushed the md/fix-epoch-quote-local-calculation branch from 8143000 to 80dcf74 Compare December 18, 2024 22:14
@Maddiaa0 Maddiaa0 requested a review from just-mitch December 18, 2024 22:17
@Maddiaa0 Maddiaa0 added network-all Run this CI job. e2e-p2p CI: Enables this CI job. labels Dec 18, 2024
@Maddiaa0 Maddiaa0 force-pushed the md/fix-epoch-quote-local-calculation branch from 80dcf74 to 65fc64d Compare January 18, 2025 01:43
@Maddiaa0 Maddiaa0 requested a review from LHerskind January 19, 2025 20:56
@spalladino
Copy link
Collaborator

Changes VERSION to ROLLUP_VERSION cus searching VERSION brings up 2000 results, this will be much nicer to maintain

Smart guy

Copy link
Collaborator

@spalladino spalladino left a comment

Choose a reason for hiding this comment

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

Code looks good (except for a uint256 vs uint32, but I believe it does not affect the end result). I'm a bit worried about the L1 contract and ts code falling out of sync though: we have three new places (EpochProofQuoteHasher, EpochProofQuotePayload.typeHash, and EpochProofQuotePayload.getPayloadToSign.abi) where the struct info is repeated.

WDYT about adding a small unit test that grabs the struct definition from the rollup abi and compares it against what we have defined in all those places, just to fail early if we make a change? We could also create these values dynamically out of the abi, or even abuse the tsc compiler to check them, but a unit test is probably the easiest one.

We can extract it from a the claimEpochProofRight function in the abi:

  {
    "type": "function",
    "name": "claimEpochProofRight",
    "inputs": [
      {
        "name": "_quote",
        "type": "tuple",
        "internalType": "struct SignedEpochProofQuote",
        "components": [
          {
            "name": "quote",
            "type": "tuple",
            "internalType": "struct EpochProofQuote",
            "components": [
              {
                "name": "epochToProve",
                "type": "uint256",
                "internalType": "Epoch"
              },
              {
                "name": "validUntilSlot",
                "type": "uint256",
                "internalType": "Slot"
              },
              {
                "name": "bondAmount",
                "type": "uint256",
                "internalType": "uint256"
              },
              {
                "name": "prover",
                "type": "address",
                "internalType": "address"
              },
              {
                "name": "basisPointFee",
                "type": "uint32",
                "internalType": "uint32"
              }
            ]
          },
          {
            "name": "signature",
            "type": "tuple",
            "internalType": "struct Signature",
            "components": [
              {
                "name": "isEmpty",
                "type": "bool",
                "internalType": "bool"
              },
              {
                "name": "v",
                "type": "uint8",
                "internalType": "uint8"
              },
              {
                "name": "r",
                "type": "bytes32",
                "internalType": "bytes32"
              },
              {
                "name": "s",
                "type": "bytes32",
                "internalType": "bytes32"
              }
            ]
          }
        ]
      }
    ],
    "outputs": [],
    "stateMutability": "nonpayable"
  },

Another sanity check would be checking that the resulting typehash we compute in ts matches the EPOCH_PROOF_QUOTE_TYPEHASH on L1 during startup. Maybe it's too paranoid, but it'd save us from issues caused by version mismatches.

@@ -72,6 +82,19 @@ export class EpochProofQuotePayload {
);
}

getPayloadToSign(): Buffer {
const abi = parseAbiParameters('bytes32, uint256, uint256, uint256, address, uint256');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the last one a uint32?

@Maddiaa0
Copy link
Member Author

We could also create these values dynamically out of the abi

Opted for this one, was not too much effort!

@Maddiaa0 Maddiaa0 force-pushed the md/fix-epoch-quote-local-calculation branch from f344316 to 6b5aa6f Compare January 20, 2025 15:38
@Maddiaa0 Maddiaa0 enabled auto-merge (squash) January 20, 2025 15:40
@Maddiaa0 Maddiaa0 changed the title feat: extend epoch quote validation feat(prover | p2p): extend epoch quote validation Jan 20, 2025
@Maddiaa0 Maddiaa0 added trigger-workflow Used by ci script to manually trigger CI3. and removed trigger-workflow Used by ci script to manually trigger CI3. labels Jan 20, 2025
@Maddiaa0 Maddiaa0 marked this pull request as draft January 20, 2025 15:44
auto-merge was automatically disabled January 20, 2025 15:44

Pull request was converted to draft

@Maddiaa0 Maddiaa0 marked this pull request as ready for review January 20, 2025 15:44
@spalladino
Copy link
Collaborator

Just reviewed the commit, looks great.

One question, out of curiosity: did you notice any slowdown in vscode autocomplete in epoch_proof_quote_hasher.ts after you added viem's getAbiItem from the typed rollup abi?

@Maddiaa0
Copy link
Member Author

Just reviewed the commit, looks great.

One question, out of curiosity: did you notice any slowdown in vscode autocomplete in epoch_proof_quote_hasher.ts after you added viem's getAbiItem from the typed rollup abi?

I will play around now, i hopped straight onto another branch after pushing this, did you experience it on your machine, if so i can revert it

@spalladino
Copy link
Collaborator

I didn't try it. Just asking because I had experienced slowdowns around some files that deal with L1, and I suspect that viem convoluted types may be the issue.

Copy link
Contributor

@LHerskind LHerskind left a comment

Choose a reason for hiding this comment

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

Tiny edge cases that I would expect could cause some pain.

*
* @param types - The types of the EpochProofQuote struct
*/
function constructTypeHash(types: EpochProofQuoteTypeHash): `0x${string}` {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we encounter something like a struct or list I would kinda expect to fail miserably. Could we specify it to explode if it ecounter tuple or tuple[] as the type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-p2p CI: Enables this CI job. network-all Run this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: able to compute epoch proof quote digest in typescript
3 participants