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.

Sign up for GitHub

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

Jump to bottom

Cache milagro g1 pubkeys #811

Merged
merged 18 commits into from
May 4, 2023
Merged

Cache milagro g1 pubkeys #811

merged 18 commits into from
May 4, 2023

Conversation

yrong
Copy link
Contributor

@yrong yrong commented Apr 20, 2023 •
edited
Loading

@yrong yrong marked this pull request as ready for review April 21, 2023 03:48
@yrong
Copy link
Contributor Author

yrong commented Apr 21, 2023 •
edited
Loading

Benchmark

https://github.com/Snowfork/cumulus/pull/19/files#diff-db08d74f3fe259c221df3ac86f73c1e06ebafc90e2907e95b98ff9cf71018ced

  • execution time decreased by almost 4 times less than before as following.
Extrinsic Minimum Execution Time before optimization(us) Minimum Execution Time after optimization(us)
sync_committee_period_update 125_394 138_632
import_finalized_header 119_688 30_960
import_execution_header 119_457 30_946
  • storage proof size increased by almost 3 times more than before as following.
Extrinsic Storage Proof Size Estimated before optimization(bytes) Storage Proof Size Estimated after optimization(bytes)
sync_committee_period_update 72217 207289
import_finalized_header 43615 111151
import_execution_header 37729 105265

@yrong yrong force-pushed the ron/cache-milagro-g1-pubkeys branch from bba6dc6 to 21ba6dd Compare April 21, 2023 04:51
doubledup
doubledup approved these changes Apr 21, 2023
View reviewed changes
Copy link
Contributor

@doubledup doubledup left a comment

Choose a reason for hiding this comment

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

Looks good! Going through the companion PRs now

parachain/pallets/ethereum-beacon-client/Cargo.toml Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/tests.rs Outdated Show resolved Hide resolved
doubledup
doubledup approved these changes Apr 24, 2023
View reviewed changes
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/benchmarking/mod.rs Outdated Show resolved Hide resolved
parachain/rustfmt.toml Outdated Show resolved Hide resolved
@yrong
Copy link
Contributor Author

yrong commented Apr 24, 2023 •
edited
Loading

In recent commit I've added the optimization about substract absent G1 points as discussed before, benchmark here shows only a minor improvement achieved.

Benchmark also shows that the milagro fast_aggregate_verify_pre_aggregated consumes almost all of the weight(>90%) so is the major bottleneck now.

@vgeddes
Copy link
Collaborator

vgeddes commented Apr 25, 2023 •
edited
Loading

@vgeddes In recent commit I've added the optimization about substract absent G1 points as we discussed before, benchmark here shows only a minor improvement achieved(kind of unexpected). Seems not a key point and I'll double check.

Ah, too bad. Let's keep the optimization though. It does shave off some
Weight. 👍

@yrong yrong force-pushed the ron/cache-milagro-g1-pubkeys branch 2 times, most recently from e72b394 to ae8a983 Compare April 26, 2023 14:21
@yrong yrong changed the title cache milagro g1 pubkeys Cache milagro g1 pubkeys Apr 27, 2023
@yrong yrong mentioned this pull request May 1, 2023
Merged
@yrong yrong force-pushed the ron/cache-milagro-g1-pubkeys branch from e8540d5 to 4e92feb Compare May 2, 2023 08:07
vgeddes
vgeddes reviewed May 2, 2023
View reviewed changes
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/primitives/beacon/src/types.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
vgeddes
vgeddes reviewed May 2, 2023
View reviewed changes
Copy link
Collaborator

@vgeddes vgeddes left a comment •
edited by yrong
Loading

Choose a reason for hiding this comment

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

Looks better. Though I see you are returning Error instead of DispatchError from utility functions, which is unusual.

Is there a reason for this style?

No special, just change to DispatchError as suggest

parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
parachain/pallets/ethereum-beacon-client/src/lib.rs Outdated Show resolved Hide resolved
@yrong yrong mentioned this pull request May 3, 2023
Merged
@yrong
Copy link
Contributor Author

yrong commented May 4, 2023 •
edited
Loading

@doubledup For this PR to work we need to upgrade rust toolchain to nightly-2023-04-22, I would suspect Nix setup will just respect config here and no special handling required, but correct me if I'm wrong.

@yrong
Copy link
Contributor Author

yrong commented May 4, 2023

@vgeddes The only issue left is we need to add --release for unit test or it will just failed, CI changed here accordingly. I would assume still related to rust compiler and not a block issue.

@vgeddes
Copy link
Collaborator

vgeddes commented May 4, 2023 •
edited
Loading

@doubledup For this PR to work we need to upgrade rust toolchain to nightly-2023-04-22, I would suspect Nix setup will just respect config here and no special handling required, but correct me if I'm wrong.

Nope, I tried this, and we first need to update the rust-overlay input package for our Nix env:

nix flake lock --update-input rust-overlay

cc @doubledup

vgeddes
vgeddes approved these changes May 4, 2023
View reviewed changes
Copy link
Collaborator

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

OK, approved! lets finally merge this in. First run this though to get the newer rust working:

nix flake lock --update-input rust-overlay

@yrong yrong merged commit 23f9de6 into main May 4, 2023
@yrong yrong deleted the ron/cache-milagro-g1-pubkeys branch May 4, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doubledup doubledup doubledup approved these changes

@alistair-singh alistair-singh alistair-singh approved these changes

@vgeddes vgeddes vgeddes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yrong @vgeddes @alistair-singh @doubledup