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

dont' recover signer in a hot loop #212

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

dvush
Copy link
Contributor

@dvush dvush commented Oct 9, 2024

📝 Summary

This is pretty serious regression that I've missed while reviewing external PR on one of the reth upgrades.

We need to be careful to not allow something like that slip in the future.

💡 Motivation and Context

calling recover_signer verifies transaction signature every time we try to executing something in the vm.


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

Copy link

github-actions bot commented Oct 9, 2024

Benchmark results for dce6f79

Report: https://flashbots-rbuilder-ci-stats.s3.us-east-2.amazonaws.com/benchmark/dce6f79-66613c9/report/index.html

Date (UTC) 2024-10-09T11:02:26+00:00
Commit dce6f799d2634aac35fccd09ea79cdbd32f18782
Base SHA 66613c9eb68ec0072e5ede5641a53b023ae3d834

Significant changes

Benchmark Mean Status
MEV-Boost SubmitBlock serialization/JSON encoding -5.68% Performance has improved.

Copy link
Contributor

@ferranbt ferranbt left a comment

Choose a reason for hiding this comment

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

Do you feel we can add some unit test or protection to this?

@dvush
Copy link
Contributor Author

dvush commented Oct 9, 2024

@ferranbt I think microbenchmark for commit_tx can catch that.

@dvush dvush merged commit f60a4ec into develop Oct 9, 2024
5 checks passed
@dvush dvush deleted the dont_recover_signer_in_hot_loop branch October 9, 2024 17:54
astarinmymind pushed a commit that referenced this pull request Oct 15, 2024
## 📝 Summary

This is pretty serious regression that I've missed while reviewing
external PR on one of the reth upgrades.

We need to be careful to not allow something like that slip in the
future.

## 💡 Motivation and Context

calling recover_signer verifies transaction signature every time we try
to executing something in the vm.

---

## ✅ I have completed the following steps:

* [ ] Run `make lint`
* [ ] Run `make test`
* [ ] Added tests (if applicable)
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.

2 participants