Create a unified transaction verification cache #1757

tsachiherman · 2020-12-08T22:16:47Z

Summary

The existing transaction cache was always around the transaction entries that we had in our transaction pool. That has been working well for scenarios where the transaction pool is not congested. However, when we get to situations where the transaction pool is congested, it creates issues in the following two scenarios:

A transaction is being received while the transaction pool is full. After we verify it's signature, we find that we won't be able to insert it into the transaction pool, and drop it. A subsequent block that we attempt to verify could include this transaction, and we will need to re-validate it's signature again.
A node receive a proposal for verification. After verifying the proposal, the node receive a second proposal with a lower hash value ( and with a similar set of transactions ). At that point, the node would attempt to re-verify all the repeated transactions ( assuming they aren't present in the transaction pool ).

To address both issues, I've extracted the verified transaction cache out of the transaction pool into a separate object that is being held by the ledger. This object is always being used when verifying a transaction, and any verified transaction is being "set" in that object.

Test Plan

Unit tests were added and updated.

Performance Testing

The changes were tested using scenario1 and scenario2 networks; no regression was noted.

…acheSize is too small

tsachiherman · 2020-12-16T16:11:45Z

data/transactions/verify/txn_test.go

 		r := rand.Intn(numAccs)
 		a := rand.Intn(1000)
-		f := config.Consensus[protocol.ConsensusCurrentVersion].MinTxnFee + uint64(rand.Intn(10))
+		f := config.Consensus[protocol.ConsensusCurrentVersion].MinTxnFee + uint64(rand.Intn(10)) + u


this was done to ensure that we don't end up with identical transactions when generating large number of txns.

ledger/eval.go

algorandskiy

Som initial minor remarks. I need to take another look later.

algorandskiy · 2020-12-17T21:46:57Z

data/transactions/verify/txn.go

-							}
+					groupCtxs := make([]*GroupContext, len(txnGroups))
+					for i, signTxnsGrp := range txnGroups {
+						groupCtxs[i], grpErr = TxnGroup(signTxnsGrp, blkHeader, nil)


why is the cache param nil here? AddPayset is used only here, so... maybe let TxnGroup add a group?
I do not think few additional locks make a difference there

many transaction groups will be of size 1. I think that we shouldn't take the lock if we don't have to..
after all, taking the lock takes 3000-5000 ns; multiply this by 10000 and you'll end up with some notable delay.

data/transactions/verify/verifiedTxnCache.go

algorandskiy

as gerrit says "+1, Looks good to me, but someone else must approve"

data/pools/transactionPool.go

algorandskiy · 2020-12-18T01:06:12Z

data/transactions/verify/verifiedTxnCache.go

+	if len(v.buckets[v.base])+len(txgroup) > entriesPerBucket {
+		// move to the next bucket while deleting the content of the next bucket.
+		v.base = (v.base + 1) % len(v.buckets)
+		v.buckets[v.base] = make(map[transactions.Txid]*GroupContext, entriesPerBucket)


probably it is better pre-allocate to max(entriesPerBucket, len(txgroup))

the number of transaction in a group is 16. the entriesPerBucket is in the order of several thousands.
when allocating a new bucket, we want to have large buckets, and have each bucket contain all the transactions of a single txn group.

data/transactions/verify/verifiedTxnCache.go

algorandskiy · 2020-12-18T01:12:18Z

data/transactions/verify/verifiedTxnCache.go

+			}
+		}
+		if !found {
+			transcationMissing = true


break? since we going to error anyway

ahh yes.. Failing to pin a transaction within group ( or part of it ) isn't a good thing, but it shouldn't prevent us from pinning the rest of the entries. ( i.e. in the worst case scenario, we will need to verify the signature again for that particular transaction ).
The caller should log this, but there is nothing that really can be done at that point. ( and it's not really harmful either )

algorandskiy · 2020-12-18T01:14:33Z

data/transactions/verify/verifiedTxnCache.go

+		// we use the (base + W) % W trick here so we can go backward and wrap around the zero.
+		for offsetBucketIdx := baseBucket + len(v.buckets); offsetBucketIdx > baseBucket; offsetBucketIdx-- {
+			bucketIdx := offsetBucketIdx % len(v.buckets)
+			if ctx, has := v.buckets[bucketIdx][txID]; has {


nit: we might stop earlier if we track how many buckets are in use. Maybe not a big deal, will only help on non-full cache.

I think that after the first cycle, all the bucket will be in use ( although they might contain "old" entries ).
My intent here was to try and avoid deleting the old maps entries.

data/transactions/verify/verifiedTxnCache.go

algonautshant

Looks great.
I have some clarification questions.

data/transactions/verify/txn.go

algonautshant · 2020-12-19T02:16:21Z

data/transactions/verify/txn.go

 // LogicSigSanityCheck checks that the signature is valid and that the program is basically well formed.
 // It does not evaluate the logic.
-func LogicSigSanityCheck(txn *transactions.SignedTxn, ctx *Context) error {
+func LogicSigSanityCheck(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext) error {


Do we have a test for this function?

algonautshant · 2020-12-19T05:05:57Z

data/transactions/verify/verifiedTxnCache.go

+// errMissingPinnedEntry is being generated when we're trying to pin a transaction that does not appear in the cache
+var errMissingPinnedEntry = &VerifiedTxnCacheError{errors.New("Missing pinned entry")}
+
+// VerifiedTransactionCache provides a cached store of recently verified transactions. The cache is desiged two have two separate "levels". On the


typo: designed two have -> designed to have

data/transactions/verify/verifiedTxnCache.go

algonautshant · 2020-12-19T16:05:22Z

data/transactions/verify/verifiedTxnCache.go

+		// entry isn't in pinned; maybe we have it in one of the buckets ?
+		found := false
+		// we use the (base + W) % W trick here so we can go backward and wrap around the zero.
+		for offsetBucketIdx := v.base + len(v.buckets); offsetBucketIdx > v.base; offsetBucketIdx-- {


Most of the buckets are expected to be non-empty most of the time right?

It depends on the usage. Proposal validation would cause full buckets, transaction gossiping that goes into the txpool would first go into the buckets and then moved into the pinned map.

data/transactions/verify/verifiedTxnCache.go

Create a unified transaction verification cache

tsachiherman added 21 commits December 6, 2020 19:56

parallel verify

e661386

handle len(payset)=0

36f3b57

few changes.

dd64f12

Merge branch 'master' into tsachi/parallel_tnx_verification

5c791b6

Fix alive test

3c0ebb6

fix typo

8c68b7a

Merge branch 'master' into tsachi/parallel_tnx_verification

3a84c8d

Add unit test

0b19b99

Add unit test

b767b7a

Merge branch 'master' into tsachi/parallel_tnx_verification

6b04ce6

Fix datarace

9699c3a

check compilation.

36673a9

step

a7314a3

undo unwanted changes.

602135f

fix compilation

2a8f555

Add txgroup validation

be9eb43

few bugfixes

7069326

Fix compilation

4a328db

Merge branch 'master' into tsachi/txn_cache

20f5efc

rollback unwanted changes.

3326978

Merge branch 'master' into tsachi/txn_cache

f99999e

ian-algorand added this to the Sprint 15 milestone Dec 11, 2020

tsachiherman added 8 commits December 11, 2020 18:13

Merge branch 'master' into tsachi/txn_cache

76835db

removed unneeded methods.

e9567fa

update PaysetGroups

280bbab

Merge branch 'master' into tsachi/txn_cache

bad6515

merge with master

764ccb2

Fix datarace

47dfb8d

bugfix

ad5b9ac

Take cache lock only once per workset

3a1d92b

tsachiherman added 3 commits December 15, 2020 19:27

some perf tuning

e0c81f0

Move parameters to config file.

d9c26fb

Add verifiedCacheSize fallback to TxPoolSize if VerifiedTranscationsC…

bd94cc4

…acheSize is too small

tsachiherman commented Dec 16, 2020

View reviewed changes

simplify error handling

594191b

tsachiherman changed the title ~~Implement node-wide transaction verification cache~~ Create a unified transaction verification cache Dec 16, 2020

tsachiherman marked this pull request as ready for review December 16, 2020 16:37

tsachiherman requested review from algobolson, algonautshant and algorandskiy December 16, 2020 16:37

tsachiherman self-assigned this Dec 16, 2020

tsachiherman added the Infrastructure label Dec 16, 2020

Add missing file.

464a401

tsachiherman requested a review from a user December 16, 2020 19:28

algonautshant reviewed Dec 17, 2020

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

algorandskiy reviewed Dec 17, 2020

View reviewed changes

Update per reviewer's comments.

857ad55

tsachiherman requested review from algonautshant and algorandskiy December 17, 2020 22:36

algorandskiy reviewed Dec 18, 2020

View reviewed changes

tsachiherman added 2 commits December 18, 2020 09:51

fix typo.

31a5af3

update comment

c7f2bd0

tsachiherman requested a review from algorandskiy December 18, 2020 16:15

algonautshant approved these changes Dec 19, 2020

View reviewed changes

tsachiherman mentioned this pull request Dec 21, 2020

bug fix: goal doesn't evaluate logic signature correctly when signing a transaction group #1766

Closed

algorandskiy approved these changes Dec 21, 2020

View reviewed changes

tsachiherman merged commit 3af8232 into algorand:master Dec 21, 2020

tsachiherman deleted the tsachi/txn_cache branch December 21, 2020 19:10

tsachiherman added a commit to tsachiherman/go-algorand that referenced this pull request Jul 7, 2021

Create a unified transaction verification cache (algorand#1757)

a4f3ef5

Create a unified transaction verification cache

cce mentioned this pull request Sep 17, 2025

transactions: remove redundant arg from VerifiedTransactionCache #6444

Merged

3 tasks

Create a unified transaction verification cache #1757

Create a unified transaction verification cache #1757

Uh oh!

Conversation

tsachiherman commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Performance Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

algonautshant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tsachiherman commented Dec 8, 2020 •

edited

Loading