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: Revamped sequencer timetable and tx processing timeout #10870

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

Conversation

spalladino
Copy link
Collaborator

This PR first reworks the sequencer timetable. Times to process txs were hardcoded to 2 seconds per tx, and allocated all the time needed to process them. The new timetable assumes some fixed costs, and then sets the time to process txs to the remaining time in the L2 slot. Note that the time to process txs is accounted twice: once for the sequencer to assemble the block, once for the validators to reexec.

This has led to something interesting: with the current fixed numbers, and assuming a 24s L2 slot, and assuming that we need to broadcast the L2 block before the second L1 slot starts (so it can propagate and reach the builder), we only have 2s for processing all txs to be included in the block. We need either really fast simulations, or lower the fixed times (see #10869 for a low hanging fruit), or increase the L2 slot time back to 36s, or have the sequencer gather the previous block from the p2p layer rather than waiting for it to be published so it can start building earlier.

With that out of the way, we add a processingDeadline to the public processor. When the deadline is hit, the processor aborts the current operation and returns whatever txs were processed so far. Note that the processing of the current tx keeps going, but it only affects a world-state fork that will be eventually dumped, and it's the block-builder who assembles the block header.

One major issue when testing this was the anvil test watcher messing up with L1 timestamps, so the system clock would be way off the slot timestamps. This is patched by having the anvil test watcher (only in the tests created via e2e setup, not the ones in the snapshot manager; yes, we still have all e2e test setup code duplicated) update a date provider, now injected into the sequencer. This still doesn't quite work, since the L1 timestamp only advances when a block is mined, and we don't have interval mining set in these tests, so we need to manually mine a block every now and then to resync the clock. But it works, at least locally.

Fixes #10838

Created #10869

@spalladino spalladino added the e2e-all CI: Enables this CI job. label Dec 19, 2024
@spalladino spalladino force-pushed the palla/sequencer-builds-until-timeout branch from 7ec28b3 to 19b0f80 Compare December 19, 2024 13:22
Copy link
Collaborator

@PhilWindle PhilWindle left a comment

Choose a reason for hiding this comment

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

This looks great. Just the comment on the timeout class which I think might needs some work in a follow up.

this.interruptPromise = new Promise<T>((_, reject) => {
this.interrupt = () => reject(error());
this.interrupt = () => reject(errorFn());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not introduced in this PR. But this class scares me a bit. If an operation is interrupted, doesn't it leave the operation conitnuing? Which if it rejects results in an unhandled promise rejection? If you agree, I think we should tackle this in a follow up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems Promise.race deliberately ignores rejections from all promises but the first one, so we're good.

See https://stackoverflow.com/a/70403594

await orchestratorFork.close();
// We wait a bit to close the forks since the processor may still be working on a dangling tx
// which was interrupted due to the processingDeadline being hit.
setTimeout(async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I think we should try and tackle this better. If a tx takes longer than 5 seconds to execute I think we will end up with an unhandled rejected promise. The fork will have been closed whilst it is still executing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just tested it in the e2e_block_building by setting the timeout to 0s. Can confirm no unhandled rejection is thrown, only the log from world-state is shown:

[12:52:25.070] VERBOSE: simulator:public_tx_simulator Simulation of enqueued public call increment_public_value completed successfully. {"eventName":"avm-simulation","appCircuitName":"increment_public_value","duration":15.344428999349475}
[12:52:25.082] VERBOSE: simulator:public-processor Processed tx 0x1172c59c9dbc3853ac7393828206f3dc103d71d1fb81137189b122af558b7406 with 1 public calls in 248.0159250004217ms {"txHash":"0x1172c59c9dbc3853ac7393828206f3dc103d71d1fb81137189b122af558b7406","txFee":2593050567495000,"revertCode":0,"gasUsed":{"totalGas":{"daGas":1536,"l2Gas":47874},"teardownGas":{"daGas":0,"l2Gas":0},"publicGas":{"daGas":512,"l2Gas":22018}},"publicDataWriteCount":1,"nullifierCount":1,"noteHashCount":0,"contractClassLogCount":0,"unencryptedLogCount":0,"privateLogCount":0,"l2ToL1MessageCount":0,"durationMs":248.0159250004217}
[12:52:25.162] WARN: simulator:public-processor Stopping tx processing due to timeout.
[12:52:25.302] INFO: sequencer Built block 5 with hash 0x019757de16fe40e3367abcc28f4777982671d5156305118f192f54bccc58b3d7 {"blockHash":"0x019757de16fe40e3367abcc28f4777982671d5156305118f192f54bccc58b3d7","globalVariables":{"chainId":31337,"version":1,"blockNumber":5,"slotNumber":8,"timestamp":1734710192,"coinbase":"0x0000000000000000000000000000000000000000","feeRecipient":"0x0000000000000000000000000000000000000000000000000000000000000000","feePerDaGas":0,"feePerL2Gas":54164067500},"txHashes":["0x0e1cca2c3a9070176edebbb5654fa54ad69d2cbc094726abadd5c0a11e2c3623","0x0e2b2235497c8de45c84038f849548bdd888948a023ac3dc745e19a8560db901","0x0e731940e061acafe7fa77d0f01ceff4724228bfbfabffa31596975549515ee0","0x0fd6ca27b20f6cd4746069ad0e72c9bd40243c0fd96cffd37fb7f19f811fa88b","0x10ba06ca09ac453402bffccba463b4b2141bfa3d018d77a91cd50dbf4068a126","0x1172c59c9dbc3853ac7393828206f3dc103d71d1fb81137189b122af558b7406","0x1361b1ce18442fea9cf964102f73c505f3cdfcd08560318b8b2e2f1d60d2b71e","0x137012dc8b3f7d025dcc085a6cb1660c097762ff55a09ad8780099044bbc005f","0x141428b0bc5ca975416b89e2d2581e6ebde1b45da8af065759930226b6e068cf","0x18ff60d20e7f642c8d50460173de284acc87577dd6e3a86400aa66e1c8b444d8","0x190af6937a9f144a07da098dcea11fcbc59c1b444b8aaa4b23de94ac00836380","0x1924f6edf924484bc91f805f241651f43c919e90de88e6ab952e6596aa29ebed","0x1a579b2b956119e152687cef324aad0a9c208abbf51b3a33ce27f0b63c706078","0x1c58b1a954ca2a4e5d254b727d0061dd5c18e3d11946a94fbc84039a71ef0c32","0x25acfab31a906232090b5289de06eff78ce64eb90aa3627ff67f9a160e4d317a","0x268470b06bf639ac10b188d167f6c31dd27c774822662c474336fe4ee4d0ac48","0x27378df0dd97d8fca31af5bd1ccbcefebe736e1396db8645c3373be57247906b","0x2a4ac03b69027b59d3cd3c1485f325d6bd22acb3a8019f3e579521baf24fcab0","0x2c4666123127ed0ef31fdf2eb50856c203dd32ffd77c7965ceda0d000c5488a1","0x2d6c332431b1d80f7a6f24983dc3891ef2c439a281f69c30aa8258a1bad5ae33","0x2f9e4eefac929a4df0756db2c5460bb8720c0d214e5dd282d883c23934fcac95","0x2fd07a55f35bd98035b14089c0115ad01a3d870d85aa476ad2122ba44dbdebb6","0x2fe589b501693d313743f80deea0076424ee4e5132c4c873acfd1e6e540e70b3"],"eventName":"l2-block-built","creator":"0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266","duration":1864.6991349998862,"publicProcessDuration":1719.8451330000535,"rollupCircuitsDuration":1858.7728369999677,"txCount":6,"blockNumber":5,"blockTimestamp":1734710192,"unencryptedLogCount":0,"unencryptedLogSize":48,"contractClassLogCount":0,"contractClassLogSize":48}

[12:52:25.307] ERROR: world-state:database Call messageId=1580 GET_SIBLING_PATH failed: Error: Fork not found

[12:52:25.308] VERBOSE: sequencer Attesting committee is empty
[12:52:25.470] VERBOSE: sequencer:publisher Sent L1 transaction 0x2cebf1908154099cc5134af126aa95cdb7c42b5797c66c1c93ce49df508c2064 {"gasLimit":12102795,"maxFeePerGas":"1.260994569","maxPriorityFeePerGas":"1.2"}
[12:52:25.477] VERBOSE: sequencer:publisher Published L2 block to L1 rollup contract {"gasPrice":1237483632,"gasUsed":440638,"blobGasUsed":131072,"blobDataGas":1,"transactionHash":"0x2cebf1908154099cc5134af126aa95cdb7c42b5797c66c1c93ce49df508c2064","calldataGas":34688,"calldataSize":3428,"sender":"0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266","txCount":6,"blockNumber":5,"blockTimestamp":1734710192,"unencryptedLogCount":0,"unencryptedLogSize":48,"contractClassLogCount":0,"contractClassLogSize":48,"eventName":"rollup-published-to-l1","slotNumber":8,"blockHash":"0x019757de16fe40e3367abcc28f4777982671d5156305118f192f54bccc58b3d7"}
[12:52:25.478] INFO: sequencer Published rollup block 5 with 6 transactions and 0 messages in 1865ms {"blockNumber":5,"blockHash":"0x019757de16fe40e3367abcc28f4777982671d5156305118f192f54bccc58b3d7","slot":8,"txCount":6,"msgCount":0,"duration":1865,"submitter":"0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266"}

@spalladino spalladino force-pushed the palla/sequencer-builds-until-timeout branch from 2115184 to dfb532b Compare December 20, 2024 15:23
Copy link
Collaborator

@PhilWindle PhilWindle left a comment

Choose a reason for hiding this comment

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

Thanks for looking into the promise reject issue

@spalladino spalladino enabled auto-merge (squash) December 20, 2024 17:20
@spalladino spalladino disabled auto-merge December 20, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Sequencers should bound execution time
2 participants