-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(protocol): support delayed forced inclusion of txs #18826
base: pacaya_fork
Are you sure you want to change the base?
Conversation
feat(protocol): support delayed forced inclusion of txs
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
@jeff, I had a call with our friends from Nethermind, here are some feedback/suggestions:
|
|
||
/// @title TaikoInboxWithForcedTxInclusion | ||
/// @custom:security-contact security@taiko.xyz | ||
contract TaikoInboxWithForcedTxInclusion is EssentialContract { |
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.
need a better name
Co-authored-by: Daniel Wang <99078276+dantaik@users.noreply.github.com>
uint64 _head = head; | ||
ForcedInclusion storage inclusion = queue[_head]; | ||
|
||
if (inclusion.createdAt != 0 && block.timestamp >= inclusionDelay + inclusion.createdAt) { |
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.
I'm kind of thinking that making the inclusion of these forced transactions "automatic" (as in the preconfer cannot decide easily when they are included) could be problematic for a preconfer. Because the preconfer will not always know in which L1 block its propose transaction will be included, but depending on when that happens, these forced transactions need to be inserted at the right place to be able to give preconfirmations after these are supposed to happen. It does seem to introduce a level of non-determinism that could be quite tricky for a preconfer to fully control easily.
When the preconfer can decide on his own when these transactions actually get included within a window, that seems to make the preconfers job much easier to make things 100% predictable without any tricky boundaries enforced onchai, resulting with a hard deadline on when a propose tx actually needs to be included onchain to not mess with the expected order of the preconfer.
// Call the proposeBatchWithForcedInclusion function on the ForcedInclusionInbox | ||
(, meta_) = IForcedInclusionInbox(forcedInclusionInbox).proposeBatchWithForcedInclusion( | ||
_forcedInclusionParams, _batchParams, _batchTxList | ||
); |
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.
It seems that this is still only callable by the whitelisted preconfers right? If so, would say again that forced inclusion != inclusion. Wrote about that here before: #18815 (comment)
Inclusion can only be guaranteed when block proposing is made permissionless again (at least for the forced blocks, but may as well allow any block to be proposed when that happens because why not).
The idea is inspired by @cyberhorsey's PR: https://github.com/taikoxyz/taiko-mono/pull/18824/files#diff-bea5dd46ba3d231a238b7c76adc7d09dfa3f8ebd3bf552407f9098492f3ff8f5
The difference:
Moved forced inclusion related code to two files:
TaikoInboxWithForcedTxInclusion
andIForcedInclusionStore
(impl missing).IPreconfRouter
shall now callTaikoInboxWithForcedTxInclusion
, notTaikoInbox
.Forced-inclusion data (txLists) are now in blobs, not writen to storage as
bytes