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

feat: deposit_v4 with cheaper depositTopUp #2102

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

86667
Copy link
Contributor

@86667 86667 commented Jan 7, 2025

Work done by updateLatestComputedEpoch() within functions called by delegators, ie depositTopUp(), unstake() and withdraw() is expensive in some circumstances, ie when a deposit has occurred and one or more epochs passed since. This is because the new staker key is written to future committees.

This work aims to remove costly writes to _committee[] from the above functions and instead have all writes done in deposit(). To achieve this i've changed how _committee works slightly such that we only write to _committee[] if there are changes and do not copy data if data hasnt changed.

Results

Before

deposit owner1: Gas used 410503

Rolling ahead 2 epochs
deposit owner2: Gas used 644369

Rolling ahead 1 epochs
1st top up owner 1: Gas used 179666
1st top up owner 2: Gas used 16149

Rolling ahead 2 epochs
2nd top up owner 1: Gas used 273779
2nd top up owner 2: Gas used 16158

After

deposit owner1: Gas used 334749

Rolling ahead 2 epochs
deposit owner2: Gas used 446585

Rolling ahead 1 epochs
1st top up owner 1: Gas used 14174
1st top up owner 2: Gas used 14174

Rolling ahead 2 epochs
2nd top up owner 1: Gas used 13987
2nd top up owner 2: Gas used 13990

@86667 86667 changed the base branch from main to deposit_v4 January 7, 2025 11:34
@86667 86667 force-pushed the 1991-incorrect-estimation-and-high-gas-costs-on-first-call-to-deposittopupdeposit branch 2 times, most recently from 6911fa3 to 58da64d Compare January 7, 2025 11:41
Copy link
Contributor

github-actions bot commented Jan 7, 2025

🐰 Bencher Report

Branch1991-incorrect-estimation-and-high-gas-costs-on-first-call-to-deposittopupdeposit
Testbedself-hosted

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
process-empty/process-empty📈 view plot
⚠️ NO THRESHOLD
9.63
produce-full/produce-full📈 view plot
⚠️ NO THRESHOLD
1,964.70
🐰 View full continuous benchmarking report in Bencher

* build(deps): bump foundry-compilers from 0.12.8 to 0.12.9 (#2096)

Bumps [foundry-compilers](https://github.com/foundry-rs/compilers) from 0.12.8 to 0.12.9.
- [Changelog](https://github.com/foundry-rs/compilers/blob/main/CHANGELOG.md)
- [Commits](foundry-rs/compilers@v0.12.8...v0.12.9)

---
updated-dependencies:
- dependency-name: foundry-compilers
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: remove _stakerKeys from deposit contract

* fix: use onlyControlAddress()

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@86667 86667 force-pushed the 1991-incorrect-estimation-and-high-gas-costs-on-first-call-to-deposittopupdeposit branch from 0128502 to bfd580a Compare January 8, 2025 17:13
@86667 86667 force-pushed the 1991-incorrect-estimation-and-high-gas-costs-on-first-call-to-deposittopupdeposit branch from bfd580a to a73922a Compare January 8, 2025 18:05
@86667 86667 force-pushed the 1991-incorrect-estimation-and-high-gas-costs-on-first-call-to-deposittopupdeposit branch from 44900a5 to 575142b Compare January 9, 2025 09:17
Base automatically changed from deposit_v4 to main January 9, 2025 19:07
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.

Incorrect estimation and high gas costs on first call to deposit::topupDeposit()
1 participant