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: adds costs test for completing withdrawals #314

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Jul 11, 2024

This PR adds a unit tests that measures the amount of cost used when "maxing out" the number of individual withdrawals completed in a single transaction.

At the moment, the unit test completes 500 withdrawals in one tx. To view the costs of a transaction, run pnpm test:report, then open contracts/costs-report.json. Search for "test_name": "tests/sbtc-withdrawal.test.ts__optimization tests to see that transaction's costs. The result with 500 withdrawals is:

"cost_result": {
  "total": {
    "write_length": 10500,
    "write_count": 1500,
    "read_length": 17978964,
    "read_count": 12009,
    "runtime": 52492065
  },
  "limit": {
    "write_length": 15000000,
    "write_count": 15000,
    "read_length": 100000000,
    "read_count": 15000,
    "runtime": 5000000000
  },
  "memory": 91500,
  "memory_limit": 100000000
}

As you can see, the read_count is pretty close to the full block budget, so we can assume that ~600 is the upper limit without optimization. In general, read_count is the metric that most blocks max out, so the fact that it's also our highest relative metric is important.

On the other hand, I'm not so sure that making a batch call to .sbtc-registry would help that much, because (I think) each contract-call? is only 1 extra read_count. Additionally, sbtc-registry will need to validate the contract caller each time, so that's another single read per call. So, we'd reduce the read count by 1000, but that's not that much of an optimization (compared to what I thought it could be).

@hstove hstove requested a review from setzeus July 11, 2024 00:22
@hstove hstove force-pushed the feat/withdrawal-optimization branch from 17e4f17 to b5f8a8b Compare July 11, 2024 00:23
Copy link
Collaborator

@setzeus setzeus left a comment

Choose a reason for hiding this comment

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

Copy copy. Thank you for the great work here! Can confirm the same results. Happy to approve overall just have two small requirements:

  1. Can we change the list limit in parameter to 600? (the max amount), no point in leaving it at 1000
  2. CI currently failing over a mistype error

@hstove
Copy link
Contributor Author

hstove commented Jul 15, 2024

@setzeus I've made some changes:

  • Wrote a similar test for deposits - it has a very similar maximum (650), which I've updated the argument for.
  • Fixed the type error
  • Updated the max withdrawals to 600 as you mentioned

@hstove hstove requested a review from setzeus July 15, 2024 19:57
@hstove hstove merged commit f2a12f7 into main Jul 22, 2024
4 checks 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.

Clarity withdrawal completion optimization
2 participants