BatchVerifier: Rename and unexport local functions in verify/txn #4578

algonautshant · 2022-09-23T15:12:35Z

Rename and make local functions which are internal to the package.
The purpose is to enhance the code readability.
Also removing a function which is not used.

brianolson

Good cleanup. I like it. This should make future changes much more clear about the scope of things in this package.

data/transactions/verify/txn.go

codecov · 2022-09-23T16:38:25Z

Codecov Report

Merging #4578 (9ff7853) into master (8cb8ec6) will increase coverage by 0.03%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #4578      +/-   ##
==========================================
+ Coverage   54.12%   54.16%   +0.03%     
==========================================
  Files         401      401              
  Lines       51642    51615      -27     
==========================================
+ Hits        27951    27955       +4     
+ Misses      21341    21312      -29     
+ Partials     2350     2348       -2

Impacted Files	Coverage Δ
data/transactions/verify/txn.go	`46.19% <47.05%> (+2.44%)`	⬆️
crypto/multisig.go	`44.65% <75.00%> (-0.70%)`	⬇️
crypto/batchverifier.go	`100.00% <100.00%> (ø)`
network/wsNetwork.go	`64.63% <0.00%> (ø)`
ledger/acctupdates.go	`70.19% <0.00%> (+0.59%)`	⬆️
network/wsPeer.go	`68.73% <0.00%> (+0.80%)`	⬆️
ledger/tracker.go	`74.89% <0.00%> (+0.85%)`	⬆️
ledger/roundlru.go	`96.22% <0.00%> (+5.66%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cce

I need access to TxnBatchVerify for external optimization (verifying more than one TxnGroup at a time)

data/transactions/verify/txn.go

cce · 2022-09-23T17:50:41Z

another function that needs renaming (e.g. from logicSigBatchVerify to logicSigVerify):

// logicSigBatchVerify checks that the signature is valid, executing the program.
// it is the caller responsibility to call batchVerifier.verify()
func logicSigBatchVerify(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext) error {

this is a copy-paste typo — logicSigBatchVerify does not take a batchVerifier argument, and the caller does not have the responsibility to call it after calling this function, since one is not provided

data/transactions/verify/txn_test.go

jannotti

I like these name changes, I was pretty scared callers would think the old things performed verification rather than just set it up.

…nt/verify_renamings

data/transactions/verify/txn.go

gmalouf

Not going to approve so others get eyes on, but reading through think it is correct.

brianolson

LGTM

crypto/multisig.go

brianolson · 2022-09-28T13:23:58Z

crypto/batchverifier.go

@@ -113,7 +112,7 @@ func (b *BatchVerifier) GetNumberOfEnqueuedSignatures() int {
 // if the batch is zero an appropriate error is return.
 func (b *BatchVerifier) Verify() error {
 	if b.GetNumberOfEnqueuedSignatures() == 0 {
-		return ErrZeroTransactionInBatch
+		return nil


this change means that TxnGroup will now add things to the cache where before it would just exit out early with the ctx and nil error

Yes. This is the intention, so that the logicsig transactions which were not added to BatchVerifier at the level of TxnGroup and do not contribute to this GetNumberOfEnqueuedSignatures, but were verified at local batches, also get cached.

I was wondering about that too, I looked up the version just before the BV refactor merged in #2578 and it looks like any verified txngroup (even those with 0 sigs) would get cached in the old version
https://github.com/algorand/go-algorand/blame/16bada1e2b185348a41e1590eefd9eebd4315064/data/transactions/verify/txn.go#L115

so are we OK this this change?

yes, we discussed it and it is the intention to make this change and then have logicsigs be cached like they used to.

crypto/multisig.go

data/transactions/verify/txn_test.go

data/transactions/verify/txn.go

crypto/multisig.go

rename and unexport local functions

3608cf9

algonautshant added tech debt Things that need re-work for simplification / sanitization to reduce implementation overhead Team Carbon-11 labels Sep 23, 2022

algonautshant requested review from jannotti, algobolson, algorandskiy and id-ms September 23, 2022 15:12

brianolson previously approved these changes Sep 23, 2022

View reviewed changes

algonautshant added the Enhancement label Sep 23, 2022

algonautshant changed the title ~~Rename and unexport local functions in verify/txn~~ BatchVerifier: Rename and unexport local functions in verify/txn Sep 23, 2022

update the tests

05d63ae

algonautshant dismissed brianolson’s stale review via 05d63ae September 23, 2022 15:52

update a test

58e4d3f

algorandskiy previously approved these changes Sep 23, 2022

View reviewed changes

data/transactions/verify/txn.go Show resolved Hide resolved

update test err msg

1e96dd6

algonautshant dismissed algorandskiy’s stale review via 1e96dd6 September 23, 2022 16:19

algorandskiy previously approved these changes Sep 23, 2022

View reviewed changes

cce requested changes Sep 23, 2022

View reviewed changes

data/transactions/verify/txn.go Outdated Show resolved Hide resolved

id-ms reviewed Sep 23, 2022

View reviewed changes

data/transactions/verify/txn_test.go Outdated Show resolved Hide resolved

jannotti reviewed Sep 26, 2022

View reviewed changes

CR updates

2db4ee5

algonautshant dismissed algorandskiy’s stale review via 2db4ee5 September 26, 2022 17:19

algonautshant requested a review from cce September 26, 2022 17:34

cce previously approved these changes Sep 27, 2022

View reviewed changes

algonautshant added 2 commits September 27, 2022 15:28

add test and refactoring

b0ef3a0

Merge remote-tracking branch 'origin/shant/verify_renamings' into sha…

530e3a3

…nt/verify_renamings

algonautshant dismissed cce’s stale review via 530e3a3 September 27, 2022 19:28

jannotti reviewed Sep 27, 2022

View reviewed changes

data/transactions/verify/txn.go Show resolved Hide resolved

gmalouf reviewed Sep 27, 2022

View reviewed changes

CR comments

caca719

brianolson previously approved these changes Sep 27, 2022

View reviewed changes

algonautshant requested review from jannotti and cce September 27, 2022 21:12

cce previously approved these changes Sep 27, 2022

View reviewed changes

cache txn when only logicsig

754f0fb

algonautshant dismissed stale reviews from cce and brianolson via 754f0fb September 28, 2022 01:40

fix comment

d2f8e16

algonautshant requested a review from id-ms September 28, 2022 01:57

id-ms reviewed Sep 28, 2022

View reviewed changes

crypto/multisig.go Outdated Show resolved Hide resolved

brianolson reviewed Sep 28, 2022

View reviewed changes

renaming per CR

dffe43e

jannotti reviewed Sep 28, 2022

View reviewed changes

crypto/multisig.go Outdated Show resolved Hide resolved

crypto/multisig.go Outdated Show resolved Hide resolved

data/transactions/verify/txn_test.go Outdated Show resolved Hide resolved

cleanups requested in CR

8f5df56

id-ms reviewed Sep 29, 2022

View reviewed changes

data/transactions/verify/txn.go Show resolved Hide resolved

id-ms reviewed Sep 29, 2022

View reviewed changes

crypto/multisig.go Outdated Show resolved Hide resolved

CR comments: rename and drop enqueued sig count

9ff7853

algonautshant requested review from cce, jannotti, gmalouf, brianolson, id-ms and algorandskiy September 29, 2022 17:07

cce approved these changes Sep 30, 2022

View reviewed changes

algorandskiy approved these changes Sep 30, 2022

View reviewed changes

algorandskiy merged commit d389196 into algorand:master Sep 30, 2022

This was referenced Sep 30, 2022

go-algorand 3.10.0-beta Release PR #4612

Merged

go-algorand 3.10.0-stable Release PR #4618

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BatchVerifier: Rename and unexport local functions in verify/txn #4578

BatchVerifier: Rename and unexport local functions in verify/txn #4578

algonautshant commented Sep 23, 2022 •

edited

Loading

brianolson left a comment

codecov bot commented Sep 23, 2022 •

edited

Loading

cce left a comment

cce commented Sep 23, 2022 •

edited

Loading

jannotti left a comment

gmalouf left a comment

brianolson left a comment

brianolson Sep 28, 2022

algonautshant Sep 28, 2022

cce Sep 28, 2022

algorandskiy Sep 29, 2022

cce Sep 30, 2022

BatchVerifier: Rename and unexport local functions in verify/txn #4578

BatchVerifier: Rename and unexport local functions in verify/txn #4578

Conversation

algonautshant commented Sep 23, 2022 • edited Loading

brianolson left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 23, 2022 • edited Loading

Codecov Report

cce left a comment

Choose a reason for hiding this comment

cce commented Sep 23, 2022 • edited Loading

jannotti left a comment

Choose a reason for hiding this comment

gmalouf left a comment

Choose a reason for hiding this comment

brianolson left a comment

Choose a reason for hiding this comment

brianolson Sep 28, 2022

Choose a reason for hiding this comment

algonautshant Sep 28, 2022

Choose a reason for hiding this comment

cce Sep 28, 2022

Choose a reason for hiding this comment

algorandskiy Sep 29, 2022

Choose a reason for hiding this comment

cce Sep 30, 2022

Choose a reason for hiding this comment

algonautshant commented Sep 23, 2022 •

edited

Loading

codecov bot commented Sep 23, 2022 •

edited

Loading

cce commented Sep 23, 2022 •

edited

Loading