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

Update to brotli v1.1.0 #2625

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Update to brotli v1.1.0 #2625

wants to merge 10 commits into from

Conversation

eljobe
Copy link
Member

@eljobe eljobe commented Sep 2, 2024

The previous pin was almost 3 years old, and was causing some ugly warnings during builds.

This change also deals with google/brotli@641bec0 which stopped creating the static libraries by default and switched to only building them with -DBUILD_SHARED_LIBS=OFF.

Also, now that the static libraries are unambiguous, the -static suffix was removed from the library names.

The previous pin was almost 3 years old, and was causing some ugly
warnings during builds.

This change also deals with google/brotli@641bec0 which stopped
creating the static libraries by default and switched to only
building them with `-DBUILD_SHARED_LIBS=OFF`.

Also, now that the static libraries are unambiguous, the `-static`
suffix was removed from the library names.
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Sep 2, 2024
diegoximenes
diegoximenes previously approved these changes Sep 2, 2024
@PlasmaPower
Copy link
Collaborator

I was a bit worried at first because brotli up to v1.0.5 kept improving the compression, which would be a breaking consensus chain because we measure how large compressed txs are to charge for them. But it seems like since then compression has been stable, so this should work in theory. I don't think we should include it in v3.1.2 though, and I think we should look at doing more specialized testing such as rerunning it against the Arbitrum One chain history to make sure this doesn't change behavior.

@eljobe
Copy link
Member Author

eljobe commented Sep 3, 2024

I'd be happy to run it against the Arbitrum One chain history to make sure that this doesn't change behavior.
Do you have any pointers to practical steps which would be required to do that?

The first thing that comes to my mind is:

  1. Wait for Validation Inputs wiring #2604 to be submitted and released to produciton.
  2. Call the ValidationInputsAt RPC call with a view different blocks to get some bock_inputs_*.json files.
  3. Validate those block_inputs_*.json files with the prover built from this branch and confirm the final global state is the same as what it was on mainnet.

Is that the idea, or did you have a different process in mind?

@eljobe eljobe self-assigned this Sep 24, 2024
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.96%. Comparing base (019b70b) to head (ab082be).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2625      +/-   ##
==========================================
- Coverage   23.07%   22.96%   -0.12%     
==========================================
  Files         260      260              
  Lines       37719    37719              
==========================================
- Hits         8704     8662      -42     
- Misses      27571    27607      +36     
- Partials     1444     1450       +6     

@tsahee
Copy link
Contributor

tsahee commented Oct 1, 2024

specifically, what we want to know is that brotli compression level 1 did not change.
If it did, changing brotli wold require arbos-upgrade andd split validation again which will not be fun.
I think we want a fuzzing campaign to be convinnced behavior didn't change..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants