Add nitro support#39
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a Nitro batch verifier and an end-to-end testing environment for Nitro-Espresso integration. Key additions include a Docker Compose setup, Nitro configuration files, and a NitroEspressoBatchVerifier that ensures Nitro full node blocks align with data from the Espresso streamer. Feedback focuses on improving the mockLightClient implementation to handle errors and context correctly, correcting a hostname in the Nitro configuration, optimizing block verification by using headers instead of full blocks, and adding validation for the verification interval to prevent potential panics.
| "wait-for-max-delay": false, | ||
| "l1-block-bound": "ignore", | ||
| "parent-chain-wallet": { | ||
| "private-key": "b6b15c8cb491557369f3c7d2c287b053eb229daa9c22138887752191c9520659" |
There was a problem hiding this comment.
why did we push private key here?
There was a problem hiding this comment.
https://docs.arbitrum.io/run-arbitrum-node/run-nitro-dev-node#development-account-used-by-default
this is the address it uses. i will move it to the env file and pass it in as parameter though
| @@ -0,0 +1,157 @@ | |||
| services: | |||
| l1-geth: | |||
| image: ghcr.io/espressosystems/timeboost/geth-l1:rollup-creator-round2 | |||
There was a problem hiding this comment.
why are we using timeboost image?
There was a problem hiding this comment.
this was a prebuilt l1 image we used for timeboost. i will move away from this.
| ESPRESSO_SEQUENCER_L1_PROVIDER: http://l1-geth:${L1_HTTP_PORT} | ||
| ESPRESSO_DEPLOYER_ACCOUNT_INDEX: 0 | ||
| ESPRESSO_DEV_NODE_EPOCH_HEIGHT: "4294967295" | ||
| ESPRESSO_SEQUENCER_ETH_MNEMONIC: "giant issue aisle success illegal bike spike question tent bar rely arctic volcano long crawl hungry vocal artwork sniff fantasy very lucky have athlete" |
| @@ -0,0 +1,161 @@ | |||
| package feedclient | |||
There was a problem hiding this comment.
have you copy pasted this file from Nitro?
There was a problem hiding this comment.
no i looked at the nitro code and tried to make it simpler, all we need to do is stream in the messages from the feed. I can look to make this cleaner and make sure i didnt miss anything
| espressoStore "proxy/store" | ||
|
|
||
| espressoClient "github.com/EspressoSystems/espresso-network/sdks/go/client" | ||
| nitroStreamer "github.com/EspressoSystems/espresso-streamers/nitro" |
There was a problem hiding this comment.
should we first do a pr to espresso streamers repo?
| } | ||
| if !updated { | ||
| v.logger.Warn("espresso state not updated — message position not greater than current", | ||
| "msg_pos", espressoMsg.Pos) |
There was a problem hiding this comment.
we can add a return here, but i also need to advance the streamer.
| return header.Number.Uint64(), nil | ||
| } | ||
|
|
||
| func (v *NitroEspressoBatchVerifier) syncEspressoStateWithNitroFinality(nitroFinalizedBlock uint64) error { |
There was a problem hiding this comment.
I think this function can be common between the two verifiers
| // lives in the L1 delayed inbox and is not included in the messages sent to hotshot. | ||
| // In that case only compare the header and delayed message counter. | ||
| if len(espresso.Message.L2msg) == 0 { | ||
| espressoHeader, feedHeader := espresso.Message.Header, feed.Message.Header |
There was a problem hiding this comment.
I dont think you need to compare exact values, just convert to bytes using rlp that nitro uses and then compare the bytes or else you will have to change the code if they change any field or anything
There was a problem hiding this comment.
this is only for delayed messages as the feed will have l2 msg data, and what comes from espresso does not. maybe its enough to just check the delayed messages read?
| espressoHeader := espresso.Message.Header | ||
| feedHeader := feed.Message.Header | ||
| return fmt.Errorf( | ||
| "espresso message does not match Nitro feed message\n"+ |
There was a problem hiding this comment.
This error is pretty ugly
There was a problem hiding this comment.
yea i was actually debugging something with an issue and was printing out too much stuff. im going to make this a lot simpler now
| v.logger.Info("Nitro verifier stopped") | ||
| } | ||
|
|
||
| func ValidateNitroVerifierConfig(config *NitroEspressoBatchVerifierConfig) error { |
There was a problem hiding this comment.
can you put this in config file?
There was a problem hiding this comment.
i dont believe this is needed, when i first started i wrote my own parser but since moved the logic inside validate(). Good catch going to delete.
Sneh1999
left a comment
There was a problem hiding this comment.
Can you add tests it seems like most files are missing tests?
Closes #<ISSUE_NUMBER>
This PR:
Add initial support for the nitro verifier. Also a docker compose setup that spins up a sequencer, batchposter, and fullnode. To test this out you can do:
Then run the verifier:
This PR does not:
Add any e2e tests to reduce the size of the PR.
Key places to review: