L1 Batch Submission + L1 Event / Tx parsing#183
Conversation
There was a problem hiding this comment.
Woot woot this is lookin' awesome!!! Approving, but there are a couple comments that we should probably talk through:
- We need to at least spawn tickets to resolve the race condition between tx submission and state root submission. Left a comment on this with more detail.
- All the log handling looks super dope, but it seems like there is some logic missing for the batch submmission. Particularly I can't see anywhere where the sequencer can call the canonical tx chain's
appendL1ToL2Batch(...)andappendSafetyBatch(...). Maybe was only because you wanted to hold off until the relational DB is stood up. But basically seems like we will need to add aL1ToL2BatchEnqueuedLogHandlerandSafetyQueueBatchEnqueuedLogHandlerfor use specifically by the sequencer.
| import { Logger } from '../types' | ||
|
|
||
| export const LOG_NEWLINE_STRING = ' <\\n> ' | ||
| export const LOG_NEWLINE_STRING = process.env.LOG_NEW_LINES ? '\n' : ' <\\n> ' |
| await this.waitForTxBatchConfirms(txBatchTxHash, l2Batch.l2BatchNumber) | ||
| // Fallthrough on purpose -- this is a workflow | ||
| case L2BatchStatus.TXS_CONFIRMED: | ||
| rootBatchTxHash = await this.buildAndSendStateRootBatchTransaction( |
There was a problem hiding this comment.
I believe that currently with the rollup contracts, we are allowing anybody to submit state roots if the canonical transaction chain is longer than the state root chain. This was so that the chain could still progress if the sequencer permanently goes down. The idea was that we'd prevent a race condition where malicious parties submit faulty state roots in between the tx batch submission and the state root confirmation by having them always submitted atomically with this helper.
Very curious what you think about this--my suspicion is that right now, the helper contract might be insufficient to allow certain forms of failure recovery, but I'm not sure. In any case, seems like we either need to change something in the rollup contracts to prevent the race condition I just described, or have this l1-batch-submitter use the contract I linked.
There was a problem hiding this comment.
Talked about this with @ben-chain offline. Can update this if it's a concern, but he mentioned that @karlfloersch had been talking about potentially adding a delay for 3rd parties to append roots or some other approaches. Going to leave this as is for the time being but willing to switch if/when it makes sense.
One other thing to note is that eventually we may want to wait to submit state roots to make sure we submitted the txs at the index we expected to. After failure, the sequencer needs to catch up on the slow queue transactions and process and submit them. In that scenario, we'll probably want to submit a tx batch, wait to see our batch was added before someone appended txs from the slow queue, and then add the state roots from our batch. Otherwise we run the risk that our state root doesn't actually correspond to our tx but to a previously-submitted tx for which a state root has not yet been submitted, and therefore results in fraud.
This race condition seems like it will always exist for state root submission, though, so maybe we always add an expected state root queue index such that state roots only get appended if the cumulative number of roots queued is that number 🤷
There was a problem hiding this comment.
One other thing I didn't think about is that it's very possible that we'll want to submit roots without txs. In the slow queue case, for example, we'd have a tx we have to include, but a root may not have been appended to the State Chain for it, so we end up submitting the root but not the tx so that we can then move on to submit subsequent batches.
| `Error waiting for necessary block confirmations until final!`, | ||
| e | ||
| ) | ||
| // TODO: Should we return here? Don't want to resubmit, so I think we should update the DB |
There was a problem hiding this comment.
I'm not sure what the right path is here either, but I will note that in later iterations we probably do need to have logic for resubmitting the tx with a higher gas price, in case the gas markets are being volatile and our tx isn't making it through.
| * @param l2Batch The l2 batch from which state roots may be retrieved. | ||
| * @returns The L1 tx hash. | ||
| */ | ||
| private async buildAndSendStateRootBatchTransaction( |
There was a problem hiding this comment.
One potential upside of doing the SR and TX batch submission together as discussed above is that it would de-dupe this logic, which seems very similar to buildAndSendRollupBatchTransaction.
| * @param tx The transaction that emitted the event. | ||
| * @throws Error if there's an error with persistence. | ||
| */ | ||
| export const SafetyQueueBatchAppendedLogHandler = async ( |
There was a problem hiding this comment.
Will this handler always be called right after CalldataTxEnqueuedLogHandler? Otherwise, I'm confused why there's no tx parsing going on here.
There was a problem hiding this comment.
It may not be called right after, but yes, it will be called after. Can't append to the canonical chain something that has not yet been queued. This is part of the motivation for the SQL DB. Since we know we had to have already received the "enqueued" event, parsed the tx, and stored it in the DB, when we get the "appended" event, it's just a DB update to say "Remember that thing that was queued? Change it from queued to part of the canonical chain."
| * @param l1TxHash The L1 Transaction hash. | ||
| * @param rollupTransactions The RollupTransactions to insert. | ||
| * @returns The inserted transaction batch number. | ||
| * @param createBatch Whether or not to create a batch from the provided RollupTransactions (whether or not they're part of the canonical chain). |
There was a problem hiding this comment.
A little confused by this comment--would it be accurate to say "whether or not they're already part of the canonical chain? Might be missing context.
There was a problem hiding this comment.
This is a distinction between queued to eventually be appended to the canonical chain and actually a part of the canonical chain. For instance, Sequencer rollup txs will pass true here, whereas enqueued L1ToL2 txs and Safety Queue txs will be false until they are appended.
In the db, batches are processed in order, oldest unprocessed batch first. If it's part of the canonical chain, its place in line is set in stone. If it's a queued L1ToL2 tx or SafetyQueue tx, its place in line is not set until either the sequencer includes it or an "appended" event is received.
Co-authored-by: ben-chain <ben@pseudonym.party>
* chore: remove Initializable from CrossDomainMessenger * feat: make L1CrossDomainMessenger Initializable on its own * feat: update L2CrossDomainMessenger to retrieve other messenger address from L1Block * chore: update scripts to account for changes in CDM(s) * chore: update CDM(s) interfaces * test: update tests, add L1Block test case for setConfig * chore: update ABI snapshots and semver lock * test: add complementary test for L2CrossDomainMessenger * chore: update ChainAssertions script, move out conditional common checks * test: add natspec for tests and internal configuration functions * feat: move CrossDomainMessenger::xDomainMsgSender to a new slot * chore: pre-pr ready
Makes the server components in `kona-preimage` async to allow for remote data fetching in the `host` program.
Ref op-rs/op-reth#176 This PR adds an in-memory storage backend and tests that work with any storage backend. We can easily add more test cases to the file to test against a future SQLite backend. --------- Co-authored-by: Arun Dhyani <dhyaniarun7@gmail.com> Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
2 participants
Description
Adds L1 Batch Submitter and Log Handlers for parsing L1 logs and tx calldata necessary to submit rollup transactions and subscribe to / parse them.
Metadata
Fixes
Contributing Agreement