feat: cache for signature verification #18422

tac0turtle · 2023-11-09T11:26:46Z

Description

This pr adds a cache for signature verification. Since signature verification is one of, if not the most expensive part in the antehandler, caching sig verification to avoid checking it twice lends to greater performance. I set the cache size at 500, this only adds a potential for 37.5KB in size.

I did not set the signature verification cache to the multisig to avoid extra imports in auth, and since onchain multisigs are around the corner

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

coderabbitai · 2023-11-09T11:26:54Z

Warning

Rate Limit Exceeded

@tac0turtle has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 2 minutes and 47 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between 8a42f45 and f7ac82d.

Walkthrough

The changes involve the implementation of a SignatureCache across various files to optimize performance by caching the result of signature verifications. This reduces the need for repeated cryptographic operations when the same transaction is processed multiple times, addressing a known performance bottleneck. The cache is integrated into the transaction verification flow, with updates to function calls and method signatures to support the new caching mechanism.

Changes

File Path	Change Summary
`server/mock/tx.go`	Removed `ValidateBasic` and `GetSignBytes` functions from `KVStoreTx` struct.
`types/address.go` `crypto/types/cache.go` `x/auth/signing/cache.go` `x/auth/signing/verify.go` `x/auth/ante/sigverify.go` `x/auth/ante/ante.go` `x/auth/CHANGELOG.md` `x/auth/client/cli/tx_multisign.go` `x/auth/client/cli/validate_sigs.go` `x/group/testutil/expected_keepers_mocks.go` `simapp/ante.go` `x/auth/ante/sigverify_test.go`	Implemented and integrated `SignatureCache` for optimizing signature verification. Updated import paths, function signatures, and method calls to support caching.
`store/root/store_test.go`	Modified imports, including adding `storage/sqlite` and reorganizing others.

Assessment against linked issues

Objective	Addressed	Explanation
Reduce performance overhead of signature verification in delivering and checking transactions	✅	The implementation of `SignatureCache` directly addresses the objective by caching the results of signature verification, reducing the need for redundant cryptographic operations.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ?

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

You can reply to a review comment made by CodeRabbit.
You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

tac0turtle · 2023-11-09T13:33:20Z

rocksdb is failing to build because nix adds thousands of lines, cant make it work properly

coderabbitai

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between dab15ee and 04644c6.

Files selected for processing (2)

x/auth/CHANGELOG.md (1 hunks)
x/auth/signing/cache.go (1 hunks)

Additional comments: 3

x/auth/CHANGELOG.md (1)

34-34: The summary states there are no modifications to function signatures, which is inconsistent with the changelog entry that documents a new parameter for NewSigVerificationDecorator. This is an API breaking change and should be reflected accurately in the summary.

x/auth/signing/cache.go (2)

17-28: The NewSignatureCache function is designed to be called once to initialize the cache. Ensure that this behavior is documented and understood by the developers to prevent multiple initializations.

59-65: The validate method currently only checks for an empty string. Consider if additional validation is needed for keys, such as length or format constraints, depending on the use case.

x/auth/signing/cache.go

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 04644c6 and dc1bb85.

Files selected for processing (4)

crypto/types/cache.go (1 hunks)
x/auth/ante/sigverify.go (2 hunks)
x/auth/signing/bench_test.go (1 hunks)
x/auth/signing/verify.go (4 hunks)

Additional comments: 7

crypto/types/cache.go (1)

12-83: The code does not show any synchronization mechanisms for the Cache methods, which could lead to race conditions if accessed concurrently. The summary mentions the use of sync.Once for thread safety, but it is not visible in the provided code. Please ensure that the cache is thread-safe if it is intended to be accessed from multiple goroutines.

x/auth/ante/sigverify.go (4)

229-246: The addition of the cacheVerification field and the conditional initialization of the cryptotypes.Cache in the NewSigVerificationDecorator function are correctly implemented to support the new signature caching feature.

234-234: Verify that the cryptotypes.Cache is implemented using an LRU strategy as mentioned in the summary, and ensure it is compatible with the hashicorp/golang-lru/v2 package.

383-383: The update to the authsigning.VerifySignature function call to include the cacheVerification parameter aligns with the new caching mechanism.

237-237: Ensure that the NewAnteHandler function is updated to pass the appropriate boolean value to NewSigVerificationDecorator to enable or disable signature caching as intended.

x/auth/signing/bench_test.go (2)

204-228: The benchmark test BenchmarkVerifySignature correctly includes the new cache parameter in the VerifySignature function call, which aligns with the changes described in the summary.

179-192: The mismatched signature test cases are intentionally designed to produce a bad signature mismatch, and the linter changes have been addressed as per the existing comments.

crypto/types/cache.go

facundomedica

Looks good, left a few comments 👍

x/auth/signing/verify.go

crypto/types/cache.go

x/auth/signing/verify.go

alexanderbez

Some superfulous comments we can remove, but otherwise, LGTM! Nice work @tac0turtle

crypto/types/cache.go

coderabbitai

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between dc1bb85 and a1af317.

Files selected for processing (4)

crypto/types/cache.go (1 hunks)
x/auth/ante/sigverify.go (2 hunks)
x/auth/signing/bench_test.go (1 hunks)
x/auth/signing/verify.go (4 hunks)

Additional comments: 5

x/auth/ante/sigverify.go (2)

229-254: The SigVerificationDecorator struct and NewSigVerificationDecorator function have been updated to include a cacheVerification field and parameter, respectively. This aligns with the PR's objective to introduce a caching mechanism for signature verification. The cacheVerification field is of type *cryptotypes.SignatureCache, which suggests that it is a pointer to a cache object. The NewSigVerificationDecorator function initializes this cache if the cacheVerification parameter is true. The use of panic in the error handling of cryptotypes.NewSignatureCache() is appropriate here because it's a critical error that should not occur during normal operation, and if it does, it indicates a non-recoverable error that should stop the program execution.

386-392: The AnteHandle method of SigVerificationDecorator has been updated to include the use of the signature cache when verifying signatures. The authsigning.VerifySignature function now accepts the cacheVerification field as an argument. This change is crucial for the performance improvement targeted by the PR, as it allows for the reuse of signature verification results, reducing the computational overhead of redundant checks. The error handling within this method is robust, providing clear error messages that include account number, sequence, and chain-id when signature verification fails. This is important for debugging and user feedback.

x/auth/signing/bench_test.go (3)

209-210: The code correctly initializes the new SignatureCache and checks for errors, which aligns with the PR's objective to introduce a caching mechanism for signature verification.

180-193: The test cases for mismatched signatures are still present, which is consistent with the previous review comments that intentional mismatches were used to produce bad signature errors for testing purposes.

205-230: The BenchmarkVerifySignature function has been updated to include the new signature cache in the benchmark tests, which is in line with the PR's objective and the AI-generated summary.

x/auth/signing/verify.go

store/root/store_test.go

crypto/types/cache.go

facundomedica

lgtm, I left a single comment

types/address.go

crypto/types/cache.go

alexanderbez · 2023-12-21T04:15:56Z

x/auth/ante/ante.go

@@ -20,6 +20,7 @@ type HandlerOptions struct {
 	SignModeHandler        *txsigning.HandlerMap
 	SigGasConsumer         func(meter storetypes.GasMeter, sig signing.SignatureV2, params types.Params) error
 	TxFeeChecker           TxFeeChecker
+	Cachesignature         bool


Suggested change

Cachesignature bool

CacheSignature bool

alexanderbez · 2023-12-21T04:16:04Z

crypto/types/cache.go

+
+// sigkey is the key used to store the signature in the cache
+type sigkey struct {
+	signbytes []byte


Suggested change

signbytes []byte

signBytes []byte

alexanderbez · 2023-12-21T04:17:45Z

x/auth/signing/verify.go

+		return pubKey.VerifySignature(signBytes, sig)
+	}
+
+	bz := cryptotypes.NewSigKey(signBytes, sig).String()


Suggested change

bz := cryptotypes.NewSigKey(signBytes, sig).String()

sigKey := cryptotypes.NewSigKey(signBytes, sig).String()

alexanderbez · 2023-12-21T04:20:16Z

crypto/types/cache.go

+}
+
+// Add adds a signature to the cache
+func (c *SignatureCache) Add(key string, value []byte) {


My only gripe with this is that I think Add should actually take a cryptotypes.SIgKey as an argument instead of a string. The reason I recommend this is that the SignatureCache doesn't really pertain to a signature cache, the API suggests it can be used for any [string,[]byte] mapping. So I'd recommend either changing the argument.

Note, this isn't to block, but just a suggestion.

github-actions · 2024-01-21T00:05:41Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions bot added C:x/staking C:CLI C:x/gov C:x/slashing C:x/distribution distribution module related C:x/mint C:x/bank C:x/params C:x/auth C:x/evidence C:x/upgrade C:x/feegrant C:x/authz C:x/group C:x/nft C:x/circuit C:x/accounts C:x/protocolpool labels Nov 9, 2023

add cache for sig verification

d6f5195

tac0turtle force-pushed the marko/15780 branch from c37023f to d6f5195 Compare November 9, 2023 11:27

fix init function

4ff9d56

tac0turtle marked this pull request as ready for review November 9, 2023 13:34

tac0turtle requested a review from a team as a code owner November 9, 2023 13:34

github-prbot requested review from a team, atheeshp and likhita-809 and removed request for a team November 9, 2023 13:34

This comment has been minimized.

Sign in to view

coderabbitai bot reviewed Nov 30, 2023

View reviewed changes

x/auth/signing/cache.go Outdated Show resolved Hide resolved

x/auth/signing/cache.go Outdated Show resolved Hide resolved

x/auth/signing/cache.go Outdated Show resolved Hide resolved

x/auth/signing/cache.go Outdated Show resolved Hide resolved

tac0turtle requested review from alexanderbez and ValarDragon December 1, 2023 10:25

tac0turtle added 2 commits December 1, 2023 14:57

Merge branch 'main' into marko/15780

a94863d

move cache to types

dc1bb85

coderabbitai bot reviewed Dec 1, 2023

View reviewed changes

crypto/types/cache.go Outdated Show resolved Hide resolved

facundomedica reviewed Dec 4, 2023

View reviewed changes

x/auth/signing/verify.go Outdated Show resolved Hide resolved

crypto/types/cache.go Outdated Show resolved Hide resolved

x/auth/signing/verify.go Outdated Show resolved Hide resolved

x/auth/signing/verify.go Outdated Show resolved Hide resolved

alexanderbez approved these changes Dec 4, 2023

View reviewed changes

crypto/types/cache.go Outdated Show resolved Hide resolved

crypto/types/cache.go Outdated Show resolved Hide resolved

tac0turtle added 2 commits December 9, 2023 22:57

return error on new cache, rename cache to signatureCache

a1af317

Merge branch 'main' into marko/15780

9f35046

tac0turtle requested a review from facundomedica December 9, 2023 21:57

coderabbitai bot reviewed Dec 9, 2023

View reviewed changes

x/auth/signing/verify.go Outdated Show resolved Hide resolved

x/auth/signing/verify.go Outdated Show resolved Hide resolved

x/auth/signing/verify.go Outdated Show resolved Hide resolved

tac0turtle added 2 commits December 11, 2023 10:29

fix build

42b6466

address comments

74a6008

coderabbitai bot reviewed Dec 11, 2023

View reviewed changes

store/root/store_test.go Outdated Show resolved Hide resolved

facundomedica reviewed Dec 15, 2023

View reviewed changes

crypto/types/cache.go Outdated Show resolved Hide resolved

facundomedica approved these changes Dec 15, 2023

View reviewed changes

Merge branch 'main' into marko/15780

34d2743

coderabbitai bot reviewed Dec 21, 2023

View reviewed changes

types/address.go Show resolved Hide resolved

cleanup

8a42f45

coderabbitai bot reviewed Dec 21, 2023

View reviewed changes

crypto/types/cache.go Outdated Show resolved Hide resolved

crypto/types/cache.go Show resolved Hide resolved

simplfy validate

f7ac82d

tac0turtle enabled auto-merge December 21, 2023 01:20

alexanderbez reviewed Dec 21, 2023

View reviewed changes

170210 mentioned this pull request Jan 15, 2024

Triage finschia-sdk#143 Finschia/cosmos-sdk#10

Closed

github-actions bot added the Stale label Jan 21, 2024

github-actions bot closed this Jan 26, 2024

auto-merge was automatically disabled January 26, 2024 00:05
Pull request was closed

tac0turtle deleted the marko/15780 branch September 17, 2024 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cache for signature verification #18422

feat: cache for signature verification #18422

tac0turtle commented Nov 9, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 9, 2023 •

edited

Loading

Rate Limit Exceeded

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

tac0turtle commented Nov 9, 2023

This comment has been minimized.

coderabbitai bot left a comment

coderabbitai bot left a comment

facundomedica left a comment

alexanderbez left a comment

coderabbitai bot left a comment

facundomedica left a comment

alexanderbez Dec 21, 2023

alexanderbez Dec 21, 2023

alexanderbez Dec 21, 2023

alexanderbez Dec 21, 2023

github-actions bot commented Jan 21, 2024

	bz := cryptotypes.NewSigKey(signBytes, sig).String()
	sigKey := cryptotypes.NewSigKey(signBytes, sig).String()

feat: cache for signature verification #18422

feat: cache for signature verification #18422

Conversation

tac0turtle commented Nov 9, 2023 • edited by coderabbitai bot Loading

Description

Author Checklist

Reviewers Checklist

coderabbitai bot commented Nov 9, 2023 • edited Loading

Rate Limit Exceeded

Walkthrough

Changes

Assessment against linked issues

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

tac0turtle commented Nov 9, 2023

This comment has been minimized.

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

facundomedica left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

facundomedica left a comment

Choose a reason for hiding this comment

alexanderbez Dec 21, 2023

Choose a reason for hiding this comment

alexanderbez Dec 21, 2023

Choose a reason for hiding this comment

alexanderbez Dec 21, 2023

Choose a reason for hiding this comment

alexanderbez Dec 21, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2024

tac0turtle commented Nov 9, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 9, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)