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

fjord: Add FastLZ L1-Cost function specs #25

Merged
merged 6 commits into from
May 15, 2024

Conversation

mdehoog
Copy link
Contributor

@mdehoog mdehoog commented Feb 2, 2024

@tynes
Copy link
Contributor

tynes commented Feb 2, 2024

Are we going to want to adjust intercept, fastlzCoef, and uncompressedTxCoef dynamically? Or do you think that hardcoding these values for all networks will be sufficient? If we want to make them dynamic, then we will need a file system-config.md that describes the setters and a l1-attributes.md that describes the updates to the L1 Attributes transaction. We just optimized the L1 Attributes tx as part of ecotone to make it smaller so I am fine with adding new fields to it, I also have a diff based proposal here that could be helpful in allowing us to add many possible fields to the L1 Attributes tx without it adding bloat to the history: #13

specs/protocol/deposits.md Outdated Show resolved Hide resolved
@mdehoog
Copy link
Contributor Author

mdehoog commented Feb 3, 2024

Are we going to want to adjust intercept, fastlzCoef, and uncompressedTxCoef dynamically? Or do you think that hardcoding these values for all networks will be sufficient? If we want to make them dynamic, then we will need a file system-config.md that describes the setters and a l1-attributes.md that describes the updates to the L1 Attributes transaction. We just optimized the L1 Attributes tx as part of ecotone to make it smaller so I am fine with adding new fields to it, I also have a diff based proposal here that could be helpful in allowing us to add many possible fields to the L1 Attributes tx without it adding bloat to the history: #13

We plan to make then configurable. I've updated the relevant parts of the spec.

EDIT: we met about this and decided to derisk the upgrade by hardcoding the constants for now. We may want to introduce ability for this to be configured, either by L1 attributes or by putting the logic in an L2 contract, but for now hardcoding is fine.

@ajsutton ajsutton removed their request for review February 4, 2024 21:57
specs/protocol/deposits.md Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor

tynes commented Feb 6, 2024

Generally looks good

@tynes
Copy link
Contributor

tynes commented Feb 21, 2024

This looks great! Thank you. We would like to modularize the specs a bit so the diff that comes with a particular network upgrade is very obvious. If you could rebase and move the new specs into the fjord directory, that would be great. We will be going through and making directories for each network upgrade in the future

@mdehoog mdehoog force-pushed the fastlz branch 3 times, most recently from cc67dc6 to 72eaf9d Compare February 23, 2024 21:34
specs/fjord/deposits.md Outdated Show resolved Hide resolved
specs/fjord/exec-engine.md Outdated Show resolved Hide resolved
specs/fjord/exec-engine.md Outdated Show resolved Hide resolved
specs/fjord/exec-engine.md Outdated Show resolved Hide resolved
specs/fjord/exec-engine.md Outdated Show resolved Hide resolved
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! Also confirmed that the new GPO deployment code matches the monorepo PR.

mdehoog and others added 3 commits May 14, 2024 09:02
Co-authored-by: Michael de Hoog <michael.dehoog@coinbase.com>
Co-authored-by: Yukai Tu <yukai.tu@coinbase.com>
Co-authored-by: Sebastian Stammler <stammler.s@gmail.com>
@sebastianst sebastianst merged commit ede0bdf into ethereum-optimism:main May 15, 2024
1 check passed
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.

7 participants