Merkle proof verification #25

QuentinI · 2025-02-04T15:21:35Z

Closes #11

This PR:

Modifies batcher build process to link to espresso-sequencer-go's rust library
Moves most of espresso-related code from batcher to a separate file, espresso.go
After submitting transaction to HotShot and waiting for it to appear on query service, we now fetch and verify merkle proof and namespace proofs

This PR does not:

Key places to review:

How to test this PR:

Running devnet as usual should suffice

shenkeyao

I haven't run the devnet to test this yet, but it looks good! Mostly nit comments.

shenkeyao · 2025-02-05T01:58:04Z

op-batcher/flags/flags.go

@@ -189,9 +189,16 @@ var (
 	EspressoUrlFlag = &cli.StringFlag{
 		Name:    "espresso-url",
 		Usage:   "",
-		Value:   "", // should be larger than the builder's max-l2-tx-size to prevent endlessly throttling some txs


Just for my understanding, is this comment no longer applicable?

It wasn't actually applicable in the first place, it was a copy-paste artifact

shenkeyao · 2025-02-05T02:07:31Z

op-batcher/flags/flags.go

 		EnvVars: prefixEnvVars("ESPRESSO_URL"),
 	}
+
+	EspressoLCAddrFlag = &cli.StringFlag{


Nit: Not sure about Go naming conventions, but if it's similar to Rust, maybe it's better to use Lc rather than LC.

Another nit: Can we pick either address or url for both the Espresso URL above and the light client URL here, for naming consistency?

Nit: Not sure about Go naming conventions, but if it's similar to Rust, maybe it's better to use Lc rather than LC.

Go actually does the opposite, i.e. either "lc" or "LC" but never "Lc": https://go.dev/wiki/CodeReviewComments#initialisms

Another nit: Can we pick either address or url for both the Espresso URL above and the light client URL here, for naming consistency?

Those refer to different things though, one is an URL of an HTTP endpoint and another one is address of a contract on L1

Go actually does the opposite, i.e. either "lc" or "LC" but never "Lc": https://go.dev/wiki/CodeReviewComments#initialisms

TIL!

shenkeyao · 2025-02-05T02:25:18Z

op-batcher/batcher/espresso.go

+	}
+
+	if snapshot.Height <= height {
+		return fmt.Errorf("snapshot height is less than or equal to the requested height")


I was wondering whether this could be a warning or info rather than an error. Should we wait for a snapshot with a larger height in this case?

We explicitly requested snapshot for this height though, so this would indicate a malicious/bugged lc contract.

shenkeyao · 2025-02-05T02:28:15Z

op-batcher/batcher/espresso.go

+
+	blockMerkleTreeRoot := nextHeader.Header.GetBlockMerkleTreeRoot()
+
+	log.Info("Verifying merkle proof", "height", height)


Here we log height, but in most function calls we use snapshot.Height. Should we check whether they are the same?

Follow-up question: If they are not the same, i.e., snapshot.Height is larger (since the smaller case has been checked), what should we do?

I was thinking we should make sure each finalized height is properly handled without skipping. If so, we may need extra handling here.

shenkeyao · 2025-02-05T02:32:08Z

op-batcher/batcher/espresso.go

+	transaction := Transaction{
+		Namespace: 42,
+		TeeAttn:   []byte{1, 2, 3, 4},
+		CallData:  txdata.CallData(),
+	}.toEspresso()


Nit: It's fine as a proof of concept, but could we define this transaction somewhere at the beginning of this file and make it a const like TestTransaction? Just so we know we are going to replace it.

shenkeyao · 2025-02-05T02:37:38Z

op-batcher/batcher/espresso.go

+	timer := time.NewTimer(2 * time.Minute)
+	defer timer.Stop()
+
+	ticker := time.NewTicker(100 * time.Millisecond)
+	defer ticker.Stop()


Nit: Similar to the comment above, can we make timer and ticker consts too?

Not really, they're started at the moment of creation
The durations indeed should be declared as separate consts though, I'll address that

dailinsubjam · 2025-02-06T01:51:56Z

Devnet could run smoothly with this branch.

dailinsubjam

This is cool! I left some questions for checking my understanding.

dailinsubjam · 2025-02-06T20:24:50Z

op-batcher/batcher/espresso.go

+	// TODO: Generate a real attestation
+	teeAttestation := []byte{1, 2, 3, 4}


Here is the attestation we want to send to L1 right? Am I understanding correctly that: by this step, we know the transaction we sent to Espresso is finalized, and we'll use the espComm to form the data we sent to L1 later. ?

Wait a sec, we only want to send out attestation once to L1 iiuc, and later it will be signature on signing this TxHash(?) using the same key. I might raise up a discussion on this in zullip.

I'll go with this if that also sounds reasonable for you: Ignore the teeAttestation here and generate it right before sending a transaction to espresso (line 103 where you have TeeAttn: []byte{1, 2, 3, 4},). And I'll do the initial attestation for signer registration somewhere else.

dailinsubjam · 2025-02-06T20:30:15Z

op-batcher/batcher/espresso.go

+func (l *BatchSubmitter) submitToEspresso(txdata txData) (*EspressoCommitment, error) {
+	transaction := Transaction{
+		Namespace: 42,
+		TeeAttn:   []byte{1, 2, 3, 4},


Here is the attestation we want to send to Espresso right? This is related to one of my another comment.

dailinsubjam · 2025-02-06T20:45:08Z

op-batcher/flags/flags.go

+	EspressoLCAddrFlag = &cli.StringFlag{
+		Name:    "espresso-light-client-addr",
+		Usage:   "",
+		Value:   "0x422a3492e218383753d8006c7bfa97815b44373f",


Is this the real light client address? Does that mean when running kurtosis devnet, it's already connected to real lightclient, send real tx to hotshot and verify real merkleproof? If so, that's amazing! If not, I'm wondering how does it pass the merkle proof verification when running a virtual light client.

A separate thing: IIUC, Escape Hatch Functionality has been automatically served with using light client, is that correct?

dailinsubjam · 2025-02-07T01:50:04Z

Got this when running just locally in optimism-espresso-integration/op-batcher, I'm on x86-64 based Linux 2023, ~~might be a system mismatch,~~ will take a look tomorrow.
/usr/lib/golang/pkg/tool/linux_amd64/link: running gcc failed: exit status 1 /usr/bin/ld: cannot find -lespresso_crypto_helper-x86_64-unknown-linux-gnu: No such file or directory collect2: error: ld returned 1 exit status

Upd: The error message is fine actually. I see there's some new things need to be installed from the dockerfile change: gcc, espresso-sequencer-go etc. if we want to do it locally, would be good if @QuentinI already had instructions on that, but also fine if you don't. As these Dockerfiles should suffice.

QuentinI added 3 commits February 4, 2025 16:20

WIP

f7f4bd3

WIP

70a8482

Working

70c08f0

QuentinI changed the title ~~[WIP] Merkle proof verification~~ Merkle proof verification Feb 4, 2025

Discard changes to kurtosis-devnet/espresso.yaml

6fe0b82

QuentinI marked this pull request as ready for review February 4, 2025 20:49

shenkeyao reviewed Feb 5, 2025

View reviewed changes

dailinsubjam reviewed Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merkle proof verification #25

Merkle proof verification #25

QuentinI commented Feb 4, 2025 •

edited

Loading

shenkeyao left a comment

shenkeyao Feb 5, 2025

QuentinI Feb 5, 2025

shenkeyao Feb 5, 2025

shenkeyao Feb 5, 2025

QuentinI Feb 5, 2025

shenkeyao Feb 7, 2025

shenkeyao Feb 5, 2025

QuentinI Feb 5, 2025

shenkeyao Feb 5, 2025

shenkeyao Feb 5, 2025

shenkeyao Feb 5, 2025

shenkeyao Feb 5, 2025

QuentinI Feb 5, 2025

dailinsubjam commented Feb 6, 2025

dailinsubjam left a comment

dailinsubjam Feb 6, 2025

dailinsubjam Feb 6, 2025 •

edited

Loading

dailinsubjam Feb 7, 2025

dailinsubjam Feb 6, 2025 •

edited

Loading

dailinsubjam Feb 6, 2025

dailinsubjam Feb 6, 2025 •

edited

Loading

dailinsubjam commented Feb 7, 2025 •

edited

Loading


		blockMerkleTreeRoot := nextHeader.Header.GetBlockMerkleTreeRoot()

		log.Info("Verifying merkle proof", "height", height)

		// TODO: Generate a real attestation
		teeAttestation := []byte{1, 2, 3, 4}

Merkle proof verification #25

Are you sure you want to change the base?

Merkle proof verification #25

Conversation

QuentinI commented Feb 4, 2025 • edited Loading

This PR:

This PR does not:

Key places to review:

How to test this PR:

shenkeyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dailinsubjam commented Feb 6, 2025

dailinsubjam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dailinsubjam Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dailinsubjam Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dailinsubjam Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

dailinsubjam commented Feb 7, 2025 • edited Loading

QuentinI commented Feb 4, 2025 •

edited

Loading

dailinsubjam Feb 6, 2025 •

edited

Loading

dailinsubjam Feb 6, 2025 •

edited

Loading

dailinsubjam Feb 6, 2025 •

edited

Loading

dailinsubjam commented Feb 7, 2025 •

edited

Loading