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

[Feature]: add SignatureCache for checkTx and DeliverTx #15780

Open
chengzhinei opened this issue Apr 11, 2023 · 4 comments
Open

[Feature]: add SignatureCache for checkTx and DeliverTx #15780

chengzhinei opened this issue Apr 11, 2023 · 4 comments
Labels

Comments

@chengzhinei
Copy link

Summary

Problem Definition

For a TX, when it is called through abci to deliverTx and checkTx, verification of the signature is used. However, it was found through pprof that the performance overhead of verifying the signature is still significant.
To save performance, the result of verifying the signature for the same TX can be cached during the first verification. This way, when the signature is needed for verification again, no decrypting and encrypting operations need to be performed, thereby saving performance.

Proposal

The code change is relatively simple, and this Pull-Request can be used as a reference.
https://github.com/cosmos/cosmos-sdk/pull/15778/files

image
https://github.com/okex

@alexanderbez
Copy link
Contributor

Can you post benchmarks please? This is going to be dwarfed by the fact that apps might have to verify 100+ signatures in ProcessProposal, which shouldn't take that much time.

@chengzhinei
Copy link
Author

chengzhinei commented Apr 14, 2023

Can you post benchmarks please? This is going to be dwarfed by the fact that apps might have to verify 100+ signatures in ProcessProposal, which shouldn't take that much time.

thanks for your reply.
In our okc(based on cosmos-sdk), we supported the evm, and we found that evm-tx verify the signature for many times, and it cost so much time. While for cosmos-tx, it only verify twice in DeliverTx and CheckTx, so maybe it has little effect.
Maybe it is not obvious when a block has not many txs, but when we have the pressure test, the performance is greatly affected.

These conclusions are based on pressure testing data, and haven't yet been compared to benchmarks. I'll perform some benchmarks next and provide feedback as soon as results are available.

Besides, you said you already use Hashi's LRU cache, and the LRU cache is used for getting the signBytes, but it still has to do the pubkey.VerifySignature, and this operation of verifying signatures has a performance overhead.

@IdaTucker
Copy link

Hi @chengzhinei, any updates about the benchmarks? If not, someone from Zondax could look into doing them, so that a final decision can be made whether the code change is worth it?

@educlerici-zondax educlerici-zondax added the S:zondax Squad: Zondax label Oct 9, 2023
@tac0turtle tac0turtle added T:Sprint and removed S:zondax Squad: Zondax labels Nov 8, 2023
@github-project-automation github-project-automation bot moved this to 👀 To Do in Cosmos-SDK Nov 8, 2023
@tac0turtle tac0turtle moved this from 👀 To Do to ✍ In Progress in Cosmos-SDK Nov 13, 2023
@tac0turtle tac0turtle self-assigned this Nov 16, 2023
@educlerici-zondax
Copy link
Contributor

this will be on hold until the implementation of ADR071 is completed.
After that, we can assess the possibility of applying these changes.

@tac0turtle tac0turtle removed their assignment Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🤸‍♂️ In Progress
Development

Successfully merging a pull request may close this issue.

5 participants