batcher: Wait for DA write before shutdown#14654
batcher: Wait for DA write before shutdown#14654geoknee merged 1 commit intoethereum-optimism:developfrom
Conversation
|
/ci authorize 7e0f789 |
|
This pr has been automatically marked as stale and will be closed in 5 days if no updates |
geoknee
left a comment
There was a problem hiding this comment.
This looks good to me. Is it feasible to write a test for this?
Out of scope for this PR, but curious on your thoughts about:
-
Could the
MaxConcurrentDARequestsbe effectively controlled byMaxPendingTransactions? Does it need to be a separate config var? -
I wonder if there's a neater way of arranging things, so that we don't need to thread
daGroupthrough so many functions in the call stack.
I didn't look thoroughly at this, but I would say that these are similar settings but there is no strict relationship between them. So while setting
I think there are better ways to structure this, but not without larger changes to the way the batcher works. IIRC @samlaf had a good idea how the batcher could be structured to make the flow more straightforward. |
|
@karlb good find. To be sure you haven't cherry-picked/rebased on top of 23958b1#diff-c734d1296b2fd691221b92df3edf09c7533c507a74c2316117745c75c3ad5776 right? To fix the holocene strict ordering bug in upstream batcher, I decoupled DA submission from tx submission. After receiving a DA cert, instead of sending it directly to L1, it is cached in its respective channel, such that the driver loop can pull the certs out in order and submit them to L1 with the correct nonces. I wonder if your change would still be needed in that case. Not super sure whether the tx would still be submitted in the same driver loop or not... |
@samlaf Confirmed, I didn't. |
If we don't wait for
daGroup, new txs might be added aftertxQueue.Wait(), which would cause writes to a closedreceiptsCh.I stumbled across this because it made https://github.com/celo-org/optimism/blob/celo-rebase-12/op-e2e/system/altda/failover_test.go fail.