-
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): router based forced tx inclusion #18824
Conversation
feat(protocol): router based forced tx inclusion
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
I really like the idea that forced inclusion of transactions/blocks are done out of the core protocol code. I would suggest to have this inheriting relationship: TaikoInbox -> TaikoInboxWithForcedInclusion -> MainnetInbox Then move all forced inclusion logics into TaikoInboxWithForcedInclusion and leave the router unchanged. TaikoInboxWithForcedInclusion shall always check if there is one inclusion request due, if so, it calls |
|
||
pendingForcedTxHashes--; | ||
|
||
forcedTxLists[forcedTxListHash].included = true; |
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.
forcedTxLists[forcedTxListHash].txList
is not used. I'm not sure we can simply replace _batchTxList
with forcedTxLists[forcedTxListHash].txList
, you will also need to construct a batchParams
as:
BatchParams memory params;
params.blocks = new BlockParams[](1);
params.blocks[0].numTransactions = some_constant.
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.
Basically you need to call ITaikoInbox(taikoInbox).proposeBatch
twice, one for the forced batch, the other one for the normal batch. The first batch always have 1 block with MAX_FORCED_TRANSACTIONS txs.
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.
The idea if that you would call this with its own batch, and _batchTxList
is forcedTxLists[forcedTxListHash].txList
. It is gas inefficient and will likely never be used, since we don't plan on censoring any user's transactions, but it keeps it very simple.
This function is etiher called by the preconfing gateway, with it's own batch, and is profitable because of the priorityFee
reward, or by the user themselves, and either way the input _batchTxList is indeed the forcedTxList, because forcedTxListHash
is a hash of _batchTxList
.
Does this propose a problem?
packages/protocol/contracts/layer1/preconf/impl/PreconfRouter.sol
Outdated
Show resolved
Hide resolved
|
||
function storeForcedTx(bytes calldata _txList) payable external { | ||
uint256 requiredStake = getRequiredStakeAmount(); | ||
require(msg.value >= requiredStake, InsufficientStakeAmount()); |
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.
require(msg.value >= requiredStake, InsufficientStakeAmount()); | |
require(msg.value == requiredStake, InsufficientStakeAmount()); |
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.
Otherwise, please send back the overpaid fee.
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.
will add
bytes txList; | ||
uint256 timestamp; | ||
bool included; | ||
uint256 stakeAmount; |
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.
how about priorityFee
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.
yeah that's maybe a better name
/// @inheritdoc IPreconfRouter | ||
function proposePreconfedBlocks( | ||
bytes calldata, | ||
bytes calldata _batchParams, | ||
bytes calldata _batchTxList | ||
bytes calldata _batchTxList, | ||
bool force |
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.
the preconfer can always use "false" for force
, so we are not forcing?
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 we can try to query if there is a forced txs request due, if so, force will be automatically true.
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.
force is there so that a non preconfer can call this function with force = true, and then it will check the forced block inclusion. That way, the user doesn't rely on a preconfer to include it, nor does it block preconfer from proposing.
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.
But this will ruin the preconfirmation - the preconfer may already shard some shared blocks before they are proposed on L1, now suddenly a user kicks in with a block which will reorg the chain.
Co-authored-by: Daniel Wang <[email protected]>
offchain, in the gateway itself, we can:
in the gateway itself, we can:
if that isnt done, anyone can propose the block after
inclusionWindow
passes. TaikoInbox remains untouched.Needs some more tests, and some are failing, but please look over @dantaik @AnshuJalan @davidtaikocha