Add CeloEspressoTimestamp for migration #200
Conversation
shenkeyao
left a comment
shenkeyao
left a comment
There was a problem hiding this comment.
LGTM overall, just one question.
op-node/rollup/derive/data_source.go
Outdated
| func isValidBatchTx(tx *types.Transaction, receipt *types.Receipt, l1Signer types.Signer, batchInboxAddr, batcherAddr common.Address, logger log.Logger, celoEspressoTimestamp *uint64) bool { | ||
| // If CeloEspresso is activated, return false if the transaction is reverted | ||
| if receipt.Status != types.ReceiptStatusSuccessful { | ||
| return false | ||
| logger.Info("tx in inbox with reverted status", "hash", tx.Hash(), "status", receipt.Status) | ||
| if celoEspressoTimestamp != nil && time.Now().Unix() >= int64(*celoEspressoTimestamp) { | ||
| logger.Info("tx is dropped since it is reverted") | ||
| return false | ||
| } |
There was a problem hiding this comment.
Is it possible that receipt has a failed status, but not because the transaction is reverted? Double-checking this so we won't mark a transaction as valid if it fails due to a different reason.
There was a problem hiding this comment.
That's a great point. OP or Celo does not have this check and we added it, so I think wrapping it in the EspressoEnabled flag adds minimal overhead and gives us extra safety. But worth tagging @QuentinI to confirm that this logic only applies to reverted transactions.
There was a problem hiding this comment.
Is it possible that receipt has a failed status, but not because the transaction is reverted?
No, a failed status necessarily means transaction was reverted
op-node/rollup/derive/data_source.go
Outdated
| func isValidBatchTx(tx *types.Transaction, receipt *types.Receipt, l1Signer types.Signer, batchInboxAddr, batcherAddr common.Address, logger log.Logger, celoEspressoTimestamp *uint64) bool { | ||
| // If CeloEspresso is activated, return false if the transaction is reverted | ||
| if receipt.Status != types.ReceiptStatusSuccessful { | ||
| return false | ||
| logger.Info("tx in inbox with reverted status", "hash", tx.Hash(), "status", receipt.Status) | ||
| if celoEspressoTimestamp != nil && time.Now().Unix() >= int64(*celoEspressoTimestamp) { | ||
| logger.Info("tx is dropped since it is reverted") | ||
| return false | ||
| } |
There was a problem hiding this comment.
Is it possible that receipt has a failed status, but not because the transaction is reverted?
No, a failed status necessarily means transaction was reverted
op-node/rollup/derive/data_source.go
Outdated
| if receipt.Status != types.ReceiptStatusSuccessful { | ||
| return false | ||
| logger.Info("tx in inbox with reverted status", "hash", tx.Hash(), "status", receipt.Status) | ||
| if celoEspressoTimestamp != nil && time.Now().Unix() >= int64(*celoEspressoTimestamp) { |
There was a problem hiding this comment.
This is not deterministic. We need to be comparing to the block timestamp, not current time.
There was a problem hiding this comment.
Unfortunately, tx.Time() isn't deterministic either, this is time this particular node has first seen the tx, not a part of consensus. I don't think we can get around pushing thorough the L1 block ref to here and checking against that.
|
After discussing with @Sneh1999, we concluded that while using an activation timestamp doesn’t hurt, it might not actually help either. Here’s the old proposed migration flow:
The key point is: op-nodes should switch to the new derivation pipeline before the new batcher is integrated. As long as this order is maintained, the check will be triggered once we switch the batcher, and the system remains safe without the activation timestamp. So, the activation timestamp doesn’t provide additional guarantees—unless the check we add can also filter out any transactions pre-Espresso. The most likely order is 2 → 4 → 1 → 3, to minimize the gap between stopping the old batcher and starting the new one. |
|
Pasting @jbearer 's comments and we might use this instead, will also document in the book: I think the canonical way to handle this type of migration is to have a flag in the chain config like |
Closes https://app.asana.com/1/1208976916964769/project/1209393353274209/task/1210651926603246?focus=true
Context: So the only change we made to normal op-node is that “reverted transactions from L1 will be skipped”, and this might lead to a chain divergence if different operators start enabling it at different time, then in such case we might need a timestamp for normal op-node.
This PR:
This PR does not:
Key places to review: