-
Notifications
You must be signed in to change notification settings - Fork 0
Caff Node only allow finalized L1 Origin #79
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
Conversation
…ystems/optimism-espresso-integration into sishan/L1_finalization
| Usage: "Enable use of only finalized L1 blocks as L1 origin. Overwrites the value of 'sequencer.l1-confs'.", | ||
| EnvVars: prefixEnvVars("SEQUENCER_USE_FINALIZED"), | ||
| Value: false, | ||
| Value: false, // Sishan TODO: So Celo set it to false by default? Do we want to change to true if deriving from caff node? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to turn it on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe due to Celo is actively working on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to turn it on?
Yeah, I think so!
Maybe due to Celo is actively working on it.
Looks like this PR has been merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I guess the reason they haven't turned it on is because later they'll send some other fix relevant to this feature.
| Usage: "Enable use of only finalized L1 blocks as L1 origin. Overwrites the value of 'sequencer.l1-confs'.", | ||
| EnvVars: prefixEnvVars("SEQUENCER_USE_FINALIZED"), | ||
| Value: false, | ||
| Value: false, // Sishan TODO: So Celo set it to false by default? Do we want to change to true if deriving from caff node? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to turn it on?
Yeah, I think so!
Maybe due to Celo is actively working on it.
Looks like this PR has been merged.
philippecamacho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
shenkeyao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Pasting the note from [Zulip](#eng-integrations-optimism > Review requests @ 💬):
Sishan and I just discussed this PR and concluded that we shouldn't update messagesWithHeights when the L1 status fetch fails. In this way we won't drop the current block, which can then be proceeded when we retry.
A potential issue of doing so is that if the L1 fetch always fails, we'll get stuck. We think this is fine because if we fail to fetch the L1 status for this block, we probably will fail to fetch the status for the next block too. There might be a better way to handle it, which we could discuss later.
Caff Node only allow finalized L1 Origin
Caff Node only allow finalized L1 Origin
Caff Node only allow finalized L1 Origin
Closes https://app.asana.com/0/1209392461754458/1209743779164691/f
This PR:
This PR does not:
Key places to review: