Skip to content
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

Optimisations in the sequencer inbox #68

Closed
wants to merge 41 commits into from
Closed

Conversation

yahgwai
Copy link
Contributor

@yahgwai yahgwai commented Oct 4, 2023

Nitro changes required:

  • SequencerBatchDelivered now emitted from the bridge, also the event args have changed to include the sequencer inbox address.
  • The sequencer inbox is now immutable, so we will now deploy a new one when changes are required in the sequencer inbox
  • The variant of addSequencerL2BatchFromOrigin without the prevMessageCount, newMessageCount and parameters have been removed, however it will continue to exist on the old sequencer inbox which wont be updated to have it removed. If this function abi was used for decoding historical logs we should instead do this with standard abi decoding.

@cla-bot cla-bot bot added the s label Oct 4, 2023
gzeoneth
gzeoneth previously approved these changes Oct 5, 2023
src/bridge/SequencerInbox.sol Outdated Show resolved Hide resolved
src/bridge/SequencerInbox.sol Show resolved Hide resolved
@gzeoneth gzeoneth self-requested a review October 8, 2023 17:07
Comment on lines +87 to +90
delayBlocks = maxTimeVariation_.delayBlocks;
futureBlocks = maxTimeVariation_.futureBlocks;
delaySeconds = maxTimeVariation_.delaySeconds;
futureSeconds = maxTimeVariation_.futureSeconds;
Copy link
Member

@gzeoneth gzeoneth Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth noting the uint64 cast below is not safe, this is an existing issue and will only be a problem with misconfiguration; tho we might also consider to have some sanity check here to make sure each value is within some bound

or we can just perform some check there, we already check for underflow but not overflow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to have maxtimevariation as uint64s right? I've updated to that now

@shotaronowhere
Copy link
Contributor

shotaronowhere commented Nov 28, 2023

The modifier refundsGas(IGasRefunder gasRefunder) in the GasRefundEnabled abstract contract is unchanged, however it still calculates the gas refund assuming the calling contract is a proxy (copying calldata to memory). Since the sequencerInbox is no longer a proxy, the refundsGas modifier miscalculates. Underpayment is a problem, but overpayment isn't a big issue (this case), however it's an unnecessary calculation (wasted gas).

I would either make the GasRefundEnabled contract delegate call aware, or avoid abstracting the gas refund logic as general purpose contract all together.

I presume a refactor of the gasRefunder is planned given the eip 4844 support #96 , just highlighting the need.

src/bridge/SequencerInbox.sol Outdated Show resolved Hide resolved
ICommon.BatchDataLocation dataLocation
);

function postUpgradeInit() external onlyDelegated onlyProxyOwner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we include setSequencerInbox in here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, I thinkn that makes sense, will update.

yahgwai and others added 4 commits January 9, 2024 16:10
fix: backward compatible maxTimeVariation
Co-authored-by: Daniel Goldman <dzgoldman@wesleyan.edu>
@yahgwai
Copy link
Contributor Author

yahgwai commented Jan 16, 2024

The modifier refundsGas(IGasRefunder gasRefunder) in the GasRefundEnabled abstract contract is unchanged, however it still calculates the gas refund assuming the calling contract is a proxy (copying calldata to memory). Since the sequencerInbox is no longer a proxy, the refundsGas modifier miscalculates. Underpayment is a problem, but overpayment isn't a big issue (this case), however it's an unnecessary calculation (wasted gas).

I would either make the GasRefundEnabled contract delegate call aware, or avoid abstracting the gas refund logic as general purpose contract all together.

I presume a refactor of the gasRefunder is planned given the eip 4844 support #96 , just highlighting the need.

Great catch, updated the gasrefundenabled contract to be delegate call aware. 4844 gas refund changes will happen in a separate pr.

@gzeoneth
Copy link
Member

#110

@gzeoneth gzeoneth closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants