feat: signature verification cache #41

jinsan-line · 2020-12-08T11:10:06Z

Closes: https://github.com/line/link/issues/1135, ~~https://github.com/line/link/issues/1136~~

Description

During checking a tx, if it succeed to verify the signatures, keep it in txHashCache as [account_number:sequence]: tx_hash
When delivering or rechecking a tx, we could look up txHashCache to check whether it's already verified.
When delivering a tx, if txHashCache doesn't have a entry or the txHash is not same, we'll verify it again w/ VerifyBytes()
~~When rechecking a tx, SigVerificationDecorator is executed as well for https://github.com/line/link/issues/1136~~

Motivation and context

Signature verification is cpu intensive task. It could be executed repeatedly for the same transaction so, if we keep the verified the transactions, it could save cpu time and improve the performance.

How has this been tested?

make test
~~I'll revise cli_test/multi_test.TestMultiValidatorAddNodeAndFailedTransactions() in link for https://github.com/line/link/issues/1136~~

Checklist:

I followed the contributing guidelines.
I have updated the documentation accordingly.
I have added tests to cover my changes.

whylee259

LGTM

wetcod

LGTM

egonspace · 2020-12-09T01:04:40Z

x/auth/ante/sigverify.go

+		if !pubKey.VerifyBytes(sigTx.GetSignBytes(ctx, signerAcc), sig) {
+			return false
+		}
+		svd.txHashCache.Store(sigKey, txHash)


It seems like some tx's signature will not be removed from the cache if it does not meet DeliverTx. If the mempool is full, the tx will not be able to enter the mempool after passing checkTx, so it will not be able to meet DeliverTx.

You're correct. A tx couldn't be removed from the cache if the tx is not added to mempool. But I'd like to care it w/ missing transaction issue. Currently, I assume that the tx must be added to mempool if it passes checkTx.

As another approach, we could remove a tx from the cache when we fail to add a tx to mempool.

Another thought, even though we fail to add a tx to mempool after adding it to the cache, it could be removed when same account w/ the same sequence broadcasts a tx again. So, finally after thinking deeply, I have a conclusion that it's not a big deal. Could you agree on it?

x/auth/ante/sigverify.go

kukugi

LGTM 👍

kfangw

I am afraid 2 point.

a cached key may not be removed
-> can we add ttl feature to the cache?
the assumption cause potential fault
-> please see the details in the below comment

x/auth/ante/sigverify.go

kfangw · 2020-12-24T02:59:11Z

I strongly recommend you get a review from @whylee259 before merging it.

whylee259 · 2020-12-24T04:25:46Z

I've thrown approve before.

During the review, I worried about cache clear logic.

After checkTx passed, deliverTx or recheckTx will be always called?
What if checkTx is not passed after adding cache?

From the mempool logic.

TXs will be removed from the mempool
1. when deliverTx has been done.
2. when recheckTx returns failure.
so the cache will be clear before TXs is removed from the mempool.

From the Ante Chain

SigVerificationDecorator is the last verification.

For these two reason, I've clicked the approve button.

However, after hearing @egonspace's comments, I agree with his opinion.
The current implementation is coupled with the mempool logic and the ante order.

jinsan-line · 2020-12-24T05:31:19Z

I think it's a key topic to discuss.

The current implementation is coupled with the mempool logic and the ante order.

I think it need to be coupled with the mempool because signature verification cache is conceptually a part of mempool. On the other hand, I think we could implement this cache in tendermint to more clearly be bound. But I suspect whether it's good idea because cosmos-sdk should let tendermint know account number and sequence.
I think checkTx (ante handlers) is already bound with the mempool. For example, if the mempool doesn't add the tx that is passed checkTx, the sequence is already increased during ante so an account might be broken. I think it's better to set up CONTRACT explicitly that we must add a tx if it's passed checkTx. It looks like already assumed in cosmos-sdk implicitly.

egonspace · 2020-12-24T05:58:50Z

The problem I raised is relatively easy to reproduce. If we fire a large number of txs continuously with a client such as a vulcan after setting the mempool small, it will continue to accumulate signatures in the cache, but txs will fail to reach the DeliveryTx. How can we keep this code when we can't defend this problem?

jinsan-line · 2020-12-24T06:12:03Z

The problem I raised is relatively easy to reproduce

As I already said, I couldn't agree on it especially on easy. I think the idea assumes that account never send a tx again. We need huge accounts to impact on the memory size practically and also the idea depends on race condition or bug behavior of mempool. I think it's better to fix bug behavior of mempool.

If it's really easy to produce it and impact on the memory, please could you give me an example? I think it might really help me to try to defense it. Thanks!

whylee259 · 2020-12-24T06:20:12Z

If recheckTx is failed due to another ante decor(fee deduction, etc.), it does not reach svd.txHashCache.Delete, isn't it?

jinsan-line · 2020-12-24T06:41:03Z

If recheckTx is failed due to another ante decor(fee deduction, etc.), it does not reach svd.txHashCache.Delete, isn't it?

No (Yes or no is not intuitive for Korean, at least to me. T_T), it doesn't reach to delete even though the tx is deleted from the mempool. But I think, when some time the account will send tx again, the cache will be deleted finally. I think the key challenge is whether this idea is acceptable or not.

egonspace · 2020-12-24T07:17:43Z

If it's really easy to produce it and impact on the memory, please could you give me an example?

Isn't that exactly how the vulcan works?

jinsan-line · 2020-12-24T08:46:20Z

Isn't that exactly how the vulcan works?

I don't think so because vulcan uses same account (address) by HDWallet between test and test.

jinsan-line · 2020-12-24T08:48:03Z

If recheckTx is failed due to another ante decor(fee deduction, etc.), it does not reach svd.txHashCache.Delete, isn't it?

#41 (comment)

I think we could remove the signature verification cache when a tx is removed from mempool by a new abci. If you'd like to review it all together, I'll try it.

# Conflicts: # x/auth/ante/sigverify.go

jinsan-line · 2021-01-06T09:20:29Z

w/ https://github.com/line/link/issues/1153, a tx must be added to mempool if checkTx is passed so we don't need to care of missing tx case any more.

The only case that we should revert cache is failure of ante handler. I implement it at 865bc14.

Please take a look at again.
Thanks,

x/auth/ante/sigverify.go

egonspace

Good job!

wetcod

LGTM

whylee259

LGTM

* feat: signature verification cache * chore: recover to skip to sigverify * chore: rename `ok` to `exist` of `checkCache()` return var * chore: remove txHashCash if got an error * chore: revise `verifySignatureWithCache()` for readability * chore: remove naked return * fix: lint error Co-authored-by: KIm, JinSan <jinsan@linecorp.com>

feat: signature verification cache

bdac6d6

jinsan-line requested review from kfangw, kukugi, egonspace and wetcod December 8, 2020 11:10

jinsan-line self-assigned this Dec 8, 2020

jinsan-line requested a review from whylee259 December 8, 2020 11:10

whylee259 approved these changes Dec 8, 2020

View reviewed changes

wetcod approved these changes Dec 8, 2020

View reviewed changes

egonspace reviewed Dec 9, 2020

View reviewed changes

x/auth/ante/sigverify.go Show resolved Hide resolved

kukugi approved these changes Dec 9, 2020

View reviewed changes

jinsan-line changed the base branch from develop to feat/perf December 9, 2020 03:57

jinsan-line requested review from kukugi, wetcod and whylee259 December 9, 2020 05:52

kfangw reviewed Dec 24, 2020

View reviewed changes

x/auth/ante/sigverify.go Outdated Show resolved Hide resolved

x/auth/ante/sigverify.go Outdated Show resolved Hide resolved

jinsan-line added 3 commits January 5, 2021 20:03

chore: recover to skip to sigverify

3ac69bd

chore: rename ok to exist of checkCache() return var

dd56ff5

Merge remote-tracking branch 'upstream/feat/perf' into sig-cache

cfc5dca

# Conflicts: # x/auth/ante/sigverify.go

jinsan-line marked this pull request as draft January 6, 2021 07:59

chore: remove txHashCash if got an error

865bc14

jinsan-line marked this pull request as ready for review January 6, 2021 09:21

egonspace reviewed Jan 7, 2021

View reviewed changes

x/auth/ante/sigverify.go Show resolved Hide resolved

egonspace reviewed Jan 7, 2021

View reviewed changes

x/auth/ante/sigverify.go Show resolved Hide resolved

egonspace approved these changes Jan 7, 2021

View reviewed changes

wetcod approved these changes Jan 7, 2021

View reviewed changes

whylee259 approved these changes Jan 7, 2021

View reviewed changes

jinsan-line added 3 commits January 7, 2021 17:24

chore: revise verifySignatureWithCache() for readability

f369205

chore: remove naked return

9531633

fix: lint error

a77c79a

jinsan-line merged commit 4946a48 into Finschia:feat/perf Jan 7, 2021

jinsan-line deleted the sig-cache branch January 7, 2021 08:41

wetcod mentioned this pull request Feb 4, 2021

feat: signature verification cache #63

Merged

3 tasks

jinsan-line added a commit that referenced this pull request Apr 23, 2021

feat: signature verification cache (#41)

38044c2

jinsan-line mentioned this pull request Apr 23, 2021

feat: signature verification cache (#41) #143

Merged

9 tasks

jinsan-line added a commit that referenced this pull request Apr 26, 2021

feat: signature verification cache (#41) (#143)

60ec2db

egonspace unassigned jinsan-line Jun 28, 2021

0Tech mentioned this pull request Dec 22, 2023

[Epic]: triage the PRs from Finschia/finschia-sdk Finschia/cosmos-sdk#1

Open

96 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: signature verification cache #41

feat: signature verification cache #41

jinsan-line commented Dec 8, 2020 •

edited

Loading

whylee259 left a comment

wetcod left a comment

egonspace Dec 9, 2020 •

edited

Loading

jinsan-line Dec 9, 2020 •

edited

Loading

jinsan-line Dec 9, 2020 •

edited

Loading

jinsan-line Dec 9, 2020

kukugi left a comment

kfangw left a comment

kfangw commented Dec 24, 2020

whylee259 commented Dec 24, 2020

jinsan-line commented Dec 24, 2020 •

edited

Loading

egonspace commented Dec 24, 2020 •

edited

Loading

jinsan-line commented Dec 24, 2020 •

edited

Loading

whylee259 commented Dec 24, 2020

jinsan-line commented Dec 24, 2020

egonspace commented Dec 24, 2020 •

edited

Loading

jinsan-line commented Dec 24, 2020

jinsan-line commented Dec 24, 2020

jinsan-line commented Jan 6, 2021 •

edited

Loading

egonspace left a comment

wetcod left a comment

whylee259 left a comment

feat: signature verification cache #41

feat: signature verification cache #41

Conversation

jinsan-line commented Dec 8, 2020 • edited Loading

Description

Motivation and context

How has this been tested?

Checklist:

whylee259 left a comment

Choose a reason for hiding this comment

wetcod left a comment

Choose a reason for hiding this comment

egonspace Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

jinsan-line Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

jinsan-line Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

jinsan-line Dec 9, 2020

Choose a reason for hiding this comment

kukugi left a comment

Choose a reason for hiding this comment

kfangw left a comment

Choose a reason for hiding this comment

kfangw commented Dec 24, 2020

whylee259 commented Dec 24, 2020

jinsan-line commented Dec 24, 2020 • edited Loading

egonspace commented Dec 24, 2020 • edited Loading

jinsan-line commented Dec 24, 2020 • edited Loading

whylee259 commented Dec 24, 2020

jinsan-line commented Dec 24, 2020

egonspace commented Dec 24, 2020 • edited Loading

jinsan-line commented Dec 24, 2020

jinsan-line commented Dec 24, 2020

jinsan-line commented Jan 6, 2021 • edited Loading

egonspace left a comment

Choose a reason for hiding this comment

wetcod left a comment

Choose a reason for hiding this comment

whylee259 left a comment

Choose a reason for hiding this comment

jinsan-line commented Dec 8, 2020 •

edited

Loading

egonspace Dec 9, 2020 •

edited

Loading

jinsan-line Dec 9, 2020 •

edited

Loading

jinsan-line Dec 9, 2020 •

edited

Loading

jinsan-line commented Dec 24, 2020 •

edited

Loading

egonspace commented Dec 24, 2020 •

edited

Loading

jinsan-line commented Dec 24, 2020 •

edited

Loading

egonspace commented Dec 24, 2020 •

edited

Loading

jinsan-line commented Jan 6, 2021 •

edited

Loading