feat(worker): pipeline block building

[omerfirmak]

This pipelines tracing and CCC checks, aiming to reduce the effective transaction processing delay.
Currently txn processing delay is roughly tracing_time + ccc_time. With a pipelined approach, this delay becomes max(tracing_time, ccc_time). Considering that tracing_time and ccc_time is pretty similar for an average mainnet transaction, this should cut our block building time almost in half.

[0xmountaintop]

I'm worried that this PR will make "sync (future) l2geth upstream" difficult.
We may want to do a benchmark and see whether it's worth it.

[omerfirmak]

I'm worried that this PR will make "sync (future) l2geth upstream" difficult. We may want to do a benchmark and see whether it's worth it.

I believe we can just call this version of the worker.go something like scroll_worker.goand don't bother merging it with the upstream changes. If there is anything in upstream that's worth incorporating, we can do that later.

[omerfirmak]

ran some benchmarks against the recent mainnet blocks;
max speed up is 1.94x while average is 1.57x; these should get better as l2geth gets better at creating bigger blocks.

[0xmountaintop]

I'm worried that this PR will make "sync (future) l2geth upstream" difficult. We may want to do a benchmark and see whether it's worth it.

I believe we can just call this version of the worker.go something like scroll_worker.goand don't bother merging it with the upstream changes. If there is anything in upstream that's worth incorporating, we can do that later.

great, can we do it this style in this PR?

[omerfirmak]

I'm worried that this PR will make "sync (future) l2geth upstream" difficult. We may want to do a benchmark and see whether it's worth it.

I believe we can just call this version of the worker.go something like scroll_worker.goand don't bother merging it with the upstream changes. If there is anything in upstream that's worth incorporating, we can do that later.

great, can we do it this style in this PR?

Sure, will do 👍

[omerfirmak]

these should get better as l2geth gets better at creating bigger blocks

Re-ran the test against the blocks from this morning

INFO [05-07|10:53:41.568] PStats                                   lifetime=919.855626ms apply=881.202034ms apply_idle=0s apply_stall=528.638681ms ccc=737.891694ms ccc_idle=161.223213ms speedup=1.760 min=1.366 max=1.939 avg=1.726
INFO [05-07|10:53:42.544] PStats                                   lifetime=923.765538ms apply=923.486902ms apply_idle=0s apply_stall=401.590975ms ccc=646.22426ms  ccc_idle=261.266619ms speedup=1.699 min=1.366 max=1.939 avg=1.726
INFO [05-07|10:53:43.602] PStats                                   lifetime=986.193733ms apply=975.360039ms apply_idle=0s apply_stall=477.601506ms ccc=652.291066ms ccc_idle=284.070849ms speedup=1.650 min=1.366 max=1.939 avg=1.726

avg speed up is 1.726 now.

[0xmountaintop]

I'm worried that this PR will make "sync (future) l2geth upstream" difficult. We may want to do a benchmark and see whether it's worth it.

I believe we can just call this version of the worker.go something like scroll_worker.goand don't bother merging it with the upstream changes. If there is anything in upstream that's worth incorporating, we can do that later.

great, can we do it this style in this PR?

Sure, will do 👍

if we keep miner/worker.go (instead of deleting it), and only add a /* and a */ to comment out all the lines below package miner, then we will only have 2 lines diff (instead of the whold file diff).
This way it will be easier to track future changes in the upstream (or we can also look into PRs in upstream 1 by 1 though).

[omerfirmak]

I'm worried that this PR will make "sync (future) l2geth upstream" difficult. We may want to do a benchmark and see whether it's worth it.

I believe we can just call this version of the worker.go something like scroll_worker.goand don't bother merging it with the upstream changes. If there is anything in upstream that's worth incorporating, we can do that later.

great, can we do it this style in this PR?

Sure, will do 👍

if we keep miner/worker.go (instead of deleting it), and only add a /* and a */ to comment out all the lines below package miner, then we will only have 2 lines diff (instead of the whold file diff). This way it will be easier to track future changes in the upstream (or we can also look into PRs in upstream 1 by 1 though).

Done.

[omerfirmak]

btw, I have a minor suggestion PR on the style: #749

From what I can see, the most notable change in that PR is moving the local channels to Pipeline scope. I kinda like that they are local to the stage functions because it signifies that the owners of those channels are the respective stages, not the pipeline itself. It makes reasoning about how stages interact with each other easier and disallows accidentally interacting with the channels from outside of the stages.

[roylou]

I didn't read the code. Yet you add 900 lines of code (scroll_worker.go) and deletes 300 lines of testing code (scroll_worker_test.go). How can we make sure the newly added code are well tested?

[omerfirmak]

I didn't read the code. Yet you add 900 lines of code (scroll_worker.go) and deletes 300 lines of testing code (scroll_worker_test.go). How can we make sure the newly added code are well tested?

Deleted tests are for the features that we removed as well. Newly added code directly plugs in to the paths that existing tests cover, so from the callers perspective, behaviour of worker stays the same.