Escape hatch #323
Escape hatch #323
Conversation
arbnode/transaction_streamer.go
Outdated
| live, err := s.lightClientReader.IsHotShotLive(s.config().EspressoSwitchDelayThreshold) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // If hotshot is down, escape hatch is activated, the only thing is to check if hotshot is live again | ||
| if s.escapheHatchOn { | ||
| if live { | ||
| log.Info("HotShot is up, disabling the escape hatch") | ||
| s.escapheHatchOn = false | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // If hotshot is up, escape hatch is disabled | ||
| // - check if escape hatch should be activated | ||
| // - check if the submitted transaction should be skipped from espresso verification | ||
| if !live { | ||
| log.Warn("enabling the escape hatch, hotshot is down") | ||
| s.escapheHatchOn = true | ||
| } |
There was a problem hiding this comment.
I'm not sure about this approach of using the "real time" hotshot liveness as one of the escape hatch conditions. As per previous discussions of the escape hatch, don't we only want to be opening it based on l1 height. This introduces cases where the if hotshot is not live for the current block, messages won't be included in hotshot even if they should be based on the liveness at their respective l1 height alone.
I think as the "liveness" reported by the contract is a proxy for liveness and can change based on how often state is pushed to this contract, it might be incorrect behavior to add these checks. This is especially true with the batch poster reading escapeHatchOn from the transaction streamer before submitting messages to the transaction streamer to be finalized and confirmed.
Say we have a message with l1 height x where hotshot was live based on the data stored in the light client, but based on the current l1 height y, the light client reports that hotshot is not live. When this message hits this method, the transaction streamer will report that the escape hatch is on, and we will skip including this message. I believe that the correct behavior should be to include the message.
There was a problem hiding this comment.
escapeHatchOn is only used to check if the batch poster should submit transactions to hotshot. If hotshot is currently down, then bypass it. If hotshot is currently up, then turn off the escape hatch. Don't we need a real time liveness to see if we should enable/disable the escape hatch?
There was a problem hiding this comment.
I think using the L1 height in the message header is incorrect. HotShot blocks also have an L1 height. During the time when transaction approaching HotShot, the L1 height can be different.
There was a problem hiding this comment.
One thing to note here:
Say we have a message with l1 height x where hotshot was live based on the data stored in the light client
This doesnt apply anymore because earlier we had changes in the sequencer code, so we decided if we want to create an espresso message or not based on whether hotshot was live. But now, all we care about is that hotshot is able to include the l2Messages before it gets posted to L1 in real time. So I think Jeremy's current code behaviour seems right but let me know if you think otherwise!
There was a problem hiding this comment.
@ImJeremyHe
Each batch poster configuration should allow the chain to choose between two behaviours: either waiting for Hotshot to go live before posting the batch, or, if Hotshot is not live, proceeding without checking batch consistency with Hotshot.
We wanted to make this a config because then decision will shift to the chain. I am not sure if you were there but we discussed this in a standup. Let me know if this makes sense! Happy to answer any questions on this, if any
| @@ -1664,8 +1793,18 @@ func (s *TransactionStreamer) espressoSwitch(ctx context.Context, ignored struct | |||
| log.Error("ArbOS Config is nil") | |||
| return retryRate | |||
| } | |||
| if config.ArbitrumChainParams.EnableEspresso { | |||
| return s.submitEspressoTransactions(ctx, ignored) | |||
| if config.ArbitrumChainParams.EnableEspresso && s.config().SovereignSequencerEnabled { | |||
There was a problem hiding this comment.
Should we delete SovereignSequencerEnabled here?
There was a problem hiding this comment.
For finality nodes, they shouldn't check the escape hatch, so I use this condition.
There was a problem hiding this comment.
Can you expand further on this? Our code will only run inside the batch poster, so we wanted to check the escape hatch condition.
Which finality nodes are we referring to here?
There was a problem hiding this comment.
These code are in the tx streamer. So finality node in this test will be able to run this function. I do think there should be a better way. I will try to refactor a bit
There was a problem hiding this comment.
Sorry that test is outdated, we will probably have to make changes to espresso finality node as well. So dont worry about the finality node right now. We will change it to not use SovereignSequencerEnabled .
We can actually delete this line:
builder.nodeConfig.TransactionStreamer.SovereignSequencerEnabled = false
Finality node doesnt need the transaction streamer as its only building blocks from hotshot transactions that have already been finalized. We can refactor this later though!
There was a problem hiding this comment.
Add a comment on it. We can do more clean up after most of the stuff having being done
- If escape hatch is on, batch poster will bypass espresso validation - If escape hatch is off, batch poster will wait for espresso being live
ImJeremyHe
requested review from
alysiahuggins,
sveitser and
jbearer
as code owners
Closes #300
The test for escape hatch is not ready.
This PR:
This PR does not:
test the escape hatch totally. Now we don't have a good way to see if any hotshot transactions occur when hotshot is down and escape hatch is on. Or we can think of any other way to test the escape hatch fully. However, this PR has already included so many stuff. Getting it merged first can reduce conflicts
Key places to review:
batch_poster.go
transaction_streamer.go