Skip to content

Comments

Pin rust_eth_kzg to 0.5.1#6608

Merged
mergify[bot] merged 2 commits intosigp:unstablefrom
michaelsproul:pin-rust-eth-kzg
Nov 26, 2024
Merged

Pin rust_eth_kzg to 0.5.1#6608
mergify[bot] merged 2 commits intosigp:unstablefrom
michaelsproul:pin-rust-eth-kzg

Conversation

@michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Nov 25, 2024

Proposed Changes

Temporarily pin rust_eth_kzg to 0.5.1 as 0.5.2 seems to have regressed some loading performance enough for our CI to start timing out in the simulator tests (32s timeout). See:

@michaelsproul michaelsproul added ready-for-review The code is ready for review v6.0.0 New major release for hierarchical state diffs labels Nov 25, 2024
@michaelsproul michaelsproul mentioned this pull request Nov 25, 2024
Closed
@michaelsproul
Copy link
Member Author

michaelsproul commented Nov 25, 2024

Benchmarks for kzg crate for 0.5.2:

Initialize context rust_eth_kzg
                        time:   [3.8753 s 3.8973 s 3.9200 s]

Benchmarking Initialize context c-kzg (4844): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.3s, or reduce sample count to 90.
Initialize context c-kzg (4844)
                        time:   [53.268 ms 53.289 ms 53.310 ms]
Found 11 outliers among 100 measurements (11.00%)
  6 (6.00%) high mild
  5 (5.00%) high severe

@michaelsproul
Copy link
Member Author

michaelsproul commented Nov 25, 2024

Context initialisation is 33% faster with 0.5.1:

Initialize context rust_eth_kzg
                        time:   [2.6043 s 2.6138 s 2.6226 s]
                        change: [-33.399% -32.932% -32.494%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 30 outliers among 100 measurements (30.00%)
  21 (21.00%) low severe
  1 (1.00%) low mild
  7 (7.00%) high mild
  1 (1.00%) high severe

Benchmarking Initialize context c-kzg (4844): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.4s, or reduce sample count to 90.
Initialize context c-kzg (4844)
                        time:   [55.333 ms 55.380 ms 55.429 ms]
                        change: [+3.8292% +3.9248% +4.0237%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

michaelsproul added a commit that referenced this pull request Nov 25, 2024
@michaelsproul
Pin rust_eth_kzg to 0.5.1 (#6608)
efc7a17 
Squashed commit of the following:

commit adc86cd
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Nov 25 15:39:27 2024 +1100

    Pin crate_crypto transitive deps

commit e87210a
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Nov 25 15:05:34 2024 +1100

    Pin rust_eth_kzg to 0.5.1
@michaelsproul michaelsproul mentioned this pull request Nov 25, 2024
Merged
jxs
jxs approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 26, 2024
@michaelsproul
Copy link
Member Author

michaelsproul commented Nov 26, 2024

@mergify queue

@mergify
Copy link

mergify bot commented Nov 26, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

Details

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

mergify bot added a commit that referenced this pull request Nov 26, 2024
@mergify
Merge of #6608
8887384
@mergify mergify bot mentioned this pull request Nov 26, 2024
Closed
5 tasks
@michaelsproul
Copy link
Member Author

michaelsproul commented Nov 26, 2024

@mergify requeue

@mergify
Copy link

mergify bot commented Nov 26, 2024

requeue

✅ This pull request will be re-embarked automatically

Details

The followup queue command will be automatically executed to re-embark the pull request

@mergify
Copy link

mergify bot commented Nov 26, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

Details

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

mergify bot added a commit that referenced this pull request Nov 26, 2024
@mergify
Merge of #6608
fc20328
@mergify mergify bot mentioned this pull request Nov 26, 2024
Closed
5 tasks
@michaelsproul
Copy link
Member Author

michaelsproul commented Nov 26, 2024

@mergify requeue

@mergify
Copy link

mergify bot commented Nov 26, 2024

requeue

✅ This pull request will be re-embarked automatically

Details

The followup queue command will be automatically executed to re-embark the pull request

@mergify
Copy link

mergify bot commented Nov 26, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 720f596

mergify bot added a commit that referenced this pull request Nov 26, 2024
@mergify
Merge of #6608
2111296
@mergify mergify bot mentioned this pull request Nov 26, 2024
Closed
5 tasks
@mergify mergify bot merged commit 720f596 into sigp:unstable Nov 26, 2024
@michaelsproul michaelsproul deleted the pin-rust-eth-kzg branch January 22, 2025 04:49
This was referenced Jan 22, 2025
Closed
Merged
mergify bot pushed a commit that referenced this pull request Jan 30, 2025
@michaelsproul
Cargo update without rust_eth_kzg (#6848)
d47b3e3 
Update cargo dependencies while keeping `rust_eth_kzg` pinned to `0.5.1` due to the regression described in:

- #6608

The changes from that PR were not sufficient to actually pin the dependencies of `rust_eth_kzg`, because the dependencies from the workspace Cargo.toml file were not being used anywhere. To fix this, I've added them as explicit dependencies in `crypto/kzg/Cargo.toml`. With this change, `cargo update` no longer tries to update them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jxs jxs jxs approved these changes

Assignees

No one assigned

Labels

ready-for-merge This PR is ready to merge. v6.0.0 New major release for hierarchical state diffs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@michaelsproul @jxs