Skip to content

contracts: optimize L1Block.setL1BlockValues#2596

Merged
mergify[bot] merged 2 commits intodevelopfrom
optimization/l1block
Jun 8, 2022
Merged

contracts: optimize L1Block.setL1BlockValues#2596
mergify[bot] merged 2 commits intodevelopfrom
optimization/l1block

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented May 22, 2022

Description

The amount of gas required to update the
L1 block values can be reduced by ~5000 gas by tightly
packing the uint64s into a single storage slot.
This is important because there will be a single
transaction at the beginning of each block that will
be updating these values. ~100 gas is saved by using
yul instead of straight solidity. I don't feel like the
yul is particularly difficult to read in this context,
and saving 100 gas per block will add up to a lot over
the history of the chain. This logic is covered by foundry
fuzzing.

@changeset-bot
Copy link

changeset-bot bot commented May 22, 2022

⚠️ No Changeset found

Latest commit: f53fd07

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mergify mergify bot requested review from Inphi and mslipper May 22, 2022 04:31
@mergify
Copy link
Contributor

mergify bot commented May 22, 2022

This PR changes implementation code, but doesn't include a changeset. Did you forget to add one?

@mslipper mslipper requested review from smartcontracts and removed request for mslipper May 23, 2022 21:03
@mergify mergify bot requested a review from mslipper May 23, 2022 21:04
@mslipper
Copy link
Collaborator

Recusing myself from review because I don't have enough Solidity knowledge to be helpful.

@mergify
Copy link
Contributor

mergify bot commented Jun 2, 2022

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Jun 2, 2022
@tynes tynes force-pushed the optimization/l1block branch from f358735 to 98e1bb6 Compare June 2, 2022 21:09
@mergify mergify bot removed the conflict label Jun 2, 2022
@tynes
Copy link
Contributor Author

tynes commented Jun 2, 2022

I have a preference for this implementation because it is much more clear that the underlying storage slots are being packed

Copy link
Contributor

@Inphi Inphi left a comment

Choose a reason for hiding this comment

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

kinda annoying that solc cannot perform such a basic optimization.
Looks good. But I wanna make sure that we have sufficient testing such that things doesn't break if the storage layout changes.

@mergify mergify bot requested a review from Inphi June 3, 2022 20:19
@tynes
Copy link
Contributor Author

tynes commented Jun 7, 2022

solc can pack variables into the same storage slot if they are next to each other. I can remove the assembly if highly preferred but it is covered with fuzzing. I prefer this because the packing is very obvious and tests will fail on storage slot changes

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

lgtm. I did look into how solidity does slot packing and I'm happy with the how the yul is storing into the slot.

@mergify
Copy link
Contributor

mergify bot commented Jun 7, 2022

Hey @tynes! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Jun 7, 2022
tynes added 2 commits June 7, 2022 18:52
The amount of gas required to update the
L1 block values can be reduced by ~5000 gas by tightly
packing the `uint64`s into a single storage slot.
This is important because there will be a single
transaction at the beginning of each block that will
be updating these values. ~100 gas is saved by using
yul instead of straight solidity. I don't feel like the
yul is particularly difficult to read in this context,
and saving 100 gas per block will add up to a lot over
the history of the chain. This logic is covered by foundry
fuzzing.
@tynes tynes force-pushed the optimization/l1block branch from 98e1bb6 to f53fd07 Compare June 8, 2022 01:52
@mergify mergify bot removed the conflict label Jun 8, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2022

This PR has been added to the merge queue, and will be merged soon.

@mergify mergify bot merged commit 2d79130 into develop Jun 8, 2022
@mergify mergify bot deleted the optimization/l1block branch June 8, 2022 02:12
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2022

This PR is next in line to be merged, and will be merged as soon as checks pass.

@mergify mergify bot removed the on-merge-train label Jun 8, 2022
mslipper added a commit that referenced this pull request Jun 8, 2022
This was referenced Jun 8, 2022
theochap pushed a commit that referenced this pull request Dec 10, 2025
### Description

Introduces a `CLAUDE.md` file to give Claude Code better context when
using the `kona` repository.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants