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.

Sign up for GitHub

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

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

Merged
merged 13 commits into from
Sep 30, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3608cf9
rename and unexport local functions
algonautshant Sep 23, 2022
05d63ae
update the tests
algonautshant Sep 23, 2022
58e4d3f
update a test
algonautshant Sep 23, 2022
1e96dd6
update test err msg
algonautshant Sep 23, 2022
2db4ee5
CR updates
algonautshant Sep 26, 2022
b0ef3a0
add test and refactoring
algonautshant Sep 27, 2022
530e3a3
Merge remote-tracking branch 'origin/shant/verify_renamings' into sha…
algonautshant Sep 27, 2022
caca719
CR comments
algonautshant Sep 27, 2022
754f0fb
cache txn when only logicsig
algonautshant Sep 28, 2022
d2f8e16
fix comment
algonautshant Sep 28, 2022
dffe43e
renaming per CR
algonautshant Sep 28, 2022
8f5df56
cleanups requested in CR
algonautshant Sep 28, 2022
9ff7853
CR comments: rename and drop enqueued sig count
algonautshant Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions crypto/batchverifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ const minBatchVerifierAlloc = 16
// Batch verifications errors
var (
ErrBatchVerificationFailed = errors.New("At least one signature didn't pass verification")
ErrZeroTransactionInBatch = errors.New("Could not validate empty signature set")
)

//export ed25519_randombytes_unsafe
Expand Down Expand Up @@ -104,19 +103,19 @@ func (b *BatchVerifier) expand() {
b.signatures = signatures
}

// GetNumberOfEnqueuedSignatures returns the number of signatures current enqueue onto the bacth verifier object
func (b *BatchVerifier) GetNumberOfEnqueuedSignatures() int {
// getNumberOfEnqueuedSignatures returns the number of signatures current enqueue onto the bacth verifier object
func (b *BatchVerifier) getNumberOfEnqueuedSignatures() int {
return len(b.messages)
}

// Verify verifies that all the signatures are valid. in that case nil is returned
// if the batch is zero an appropriate error is return.
func (b *BatchVerifier) Verify() error {
if b.GetNumberOfEnqueuedSignatures() == 0 {
return ErrZeroTransactionInBatch
if b.getNumberOfEnqueuedSignatures() == 0 {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so are we OK this this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

var messages = make([][]byte, b.GetNumberOfEnqueuedSignatures())
var messages = make([][]byte, b.getNumberOfEnqueuedSignatures())
for i, m := range b.messages {
messages[i] = HashRep(m)
}
Expand Down
4 changes: 2 additions & 2 deletions crypto/batchverifier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func TestBatchVerifierBulk(t *testing.T) {
sig := sigSecrets.Sign(msg)
bv.EnqueueSignature(sigSecrets.SignatureVerifier, msg, sig)
}
require.Equal(t, n, bv.GetNumberOfEnqueuedSignatures())
require.Equal(t, n, bv.getNumberOfEnqueuedSignatures())
require.NoError(t, bv.Verify())
}

Expand Down Expand Up @@ -122,5 +122,5 @@ func BenchmarkBatchVerifier(b *testing.B) {
func TestEmpty(t *testing.T) {
partitiontest.PartitionTest(t)
bv := MakeBatchVerifier()
require.Error(t, bv.Verify())
require.NoError(t, bv.Verify())
}
49 changes: 16 additions & 33 deletions crypto/multisig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,33 +216,23 @@ func MultisigAssemble(unisig []MultisigSig) (msig MultisigSig, err error) {
}

// MultisigVerify verifies an assembled MultisigSig
func MultisigVerify(msg Hashable, addr Digest, sig MultisigSig) (verified bool, err error) {
func MultisigVerify(msg Hashable, addr Digest, sig MultisigSig) (err error) {
batchVerifier := MakeBatchVerifier()

if verified, err = MultisigBatchVerify(msg, addr, sig, batchVerifier); err != nil {
if err = MultisigBatchPrep(msg, addr, sig, batchVerifier); err != nil {
return
}
if !verified {
return
}
if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 {
return true, nil
}
if err = batchVerifier.Verify(); err != nil {
return false, err
}
return true, nil
err = batchVerifier.Verify()
return
}

// MultisigBatchVerify verifies an assembled MultisigSig.
// it is the caller responsibility to call batchVerifier.verify()
func MultisigBatchVerify(msg Hashable, addr Digest, sig MultisigSig, batchVerifier *BatchVerifier) (verified bool, err error) {
verified = false
// MultisigBatchPrep performs checks on the assembled MultisigSig and adds to the batch.
// The caller must call batchVerifier.verify() to verify it.
func MultisigBatchPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier *BatchVerifier) (err error) {
// short circuit: if msig doesn't have subsigs or if Subsigs are empty
// then terminate (the upper layer should now verify the unisig)
if (len(sig.Subsigs) == 0 || sig.Subsigs[0] == MultisigSubsig{}) {
err = errInvalidNumberOfSignature
return
return errInvalidNumberOfSignature
}

// check the address is correct
Expand All @@ -251,20 +241,17 @@ func MultisigBatchVerify(msg Hashable, addr Digest, sig MultisigSig, batchVerifi
return
}
if addr != addrnew {
err = errInvalidAddress
return
return errInvalidAddress
}

// check that we don't have too many multisig subsigs
if len(sig.Subsigs) > maxMultisig {
err = errInvalidNumberOfSignature
return
return errInvalidNumberOfSignature
}

// check that we don't have too few multisig subsigs
if len(sig.Subsigs) < int(sig.Threshold) {
err = errInvalidNumberOfSignature
return
return errInvalidNumberOfSignature
}

// checks the number of non-blank signatures is no less than threshold
Expand All @@ -275,27 +262,23 @@ func MultisigBatchVerify(msg Hashable, addr Digest, sig MultisigSig, batchVerifi
}
}
if counter < sig.Threshold {
err = errInvalidNumberOfSignature
return
return errInvalidNumberOfSignature
}

// checks individual signature verifies
var verifiedCount int
var sigCount int
for _, subsigi := range sig.Subsigs {
if (subsigi.Sig != Signature{}) {
batchVerifier.EnqueueSignature(subsigi.Key, msg, subsigi.Sig)
verifiedCount++
sigCount++
}
}

// sanity check. if we get here then every non-blank subsig should have
// been verified successfully, and we should have had enough of them
if verifiedCount < int(sig.Threshold) {
err = errInvalidNumberOfSignature
return
if sigCount < int(sig.Threshold) {
return errInvalidNumberOfSignature
}

verified = true
return
}

Expand Down
51 changes: 17 additions & 34 deletions crypto/multisig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,13 @@ func TestMultisig(t *testing.T) {
require.NoError(t, err, "Multisig: unexpected failure in generating sig from pk 2")
msig, err = MultisigAssemble(sigs)
require.NoError(t, err, "Multisig: unexpected failure when assembling multisig")
verify, err := MultisigVerify(txid, addr, msig)
err = MultisigVerify(txid, addr, msig)
require.NoError(t, err, "Multisig: unexpected verification failure with err")
require.True(t, verify, "Multisig: verification failed, verify flag was false")

//test3: use the batch verification
br := MakeBatchVerifier()
verify, err = MultisigBatchVerify(txid, addr, msig, br)
err = MultisigBatchPrep(txid, addr, msig, br)
require.NoError(t, err, "Multisig: unexpected verification failure with err")
require.True(t, verify, "Multisig: verification failed, verify flag was false")
res := br.Verify()
require.NoError(t, res, "Multisig: batch verification failed")
}
Expand Down Expand Up @@ -200,8 +198,7 @@ func TestMultisigAddAndMerge(t *testing.T) {
require.NoError(t, err, "Multisig: unexpected failure signing with pk 2")
err = MultisigAdd(sigs, &msig1)
require.NoError(t, err, "Multisig: unexpected err adding pk 2 signature to that of pk 0 and 1")
verify, err := MultisigVerify(txid, addr, msig1)
require.True(t, verify, "Multisig: verification failed, verify flag was false")
err = MultisigVerify(txid, addr, msig1)
require.NoError(t, err, "Multisig: unexpected verification failure with err")

// msig2 = {sig3, sig4}
Expand All @@ -215,9 +212,8 @@ func TestMultisigAddAndMerge(t *testing.T) {
// merge two msigs and then verify
msigt, err := MultisigMerge(msig1, msig2)
require.NoError(t, err, "Multisig: unexpected failure merging multisig messages {0, 1, 2} and {3, 4}")
verify, err = MultisigVerify(txid, addr, msigt)
err = MultisigVerify(txid, addr, msigt)
require.NoError(t, err, "Multisig: unexpected verification failure with err")
require.True(t, verify, "Multisig: verification failed, verify flag was false")

// create a valid duplicate on purpose
// msig1 = {sig0, sig1, sig2}
Expand All @@ -230,9 +226,8 @@ func TestMultisigAddAndMerge(t *testing.T) {
require.NoError(t, err, "Multisig: unexpected failure adding pk 2 signature to that of pk 3 and 4")
msigt, err = MultisigMerge(msig1, msig2)
require.NoError(t, err, "Multisig: unexpected failure merging multisig messages {0, 1, 2} and {2, 3, 4}")
verify, err = MultisigVerify(txid, addr, msigt)
err = MultisigVerify(txid, addr, msigt)
require.NoError(t, err, "Multisig: unexpected verification failure with err")
require.True(t, verify, "Multisig: verification failed, verify flag was false")

return
}
Expand All @@ -254,12 +249,10 @@ func TestEmptyMultisig(t *testing.T) {
addr, err := MultisigAddrGen(version, threshold, pks)
require.NoError(t, err, "Multisig: unexpected failure generating message digest")
emptyMutliSig := MultisigSig{Version: version, Threshold: threshold, Subsigs: make([]MultisigSubsig, 0)}
verify, err := MultisigVerify(txid, addr, emptyMutliSig)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigVerify(txid, addr, emptyMutliSig)
require.Error(t, err, "Multisig: did not return error as expected")
br := MakeBatchVerifier()
verify, err = MultisigBatchVerify(txid, addr, emptyMutliSig, br)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigBatchPrep(txid, addr, emptyMutliSig, br)
require.Error(t, err, "Multisig: did not return error as expected")
}

Expand All @@ -282,12 +275,10 @@ func TestIncorrectAddrresInMultisig(t *testing.T) {
MutliSig, err := MultisigSign(txid, addr, version, threshold, pks, *secrets)
require.NoError(t, err, "Multisig: could not create mutlisig")
addr[0] = addr[0] + 1
verify, err := MultisigVerify(txid, addr, MutliSig)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigVerify(txid, addr, MutliSig)
require.Error(t, err, "Multisig: did not return error as expected")
br := MakeBatchVerifier()
verify, err = MultisigBatchVerify(txid, addr, MutliSig, br)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigBatchPrep(txid, addr, MutliSig, br)
require.Error(t, err, "Multisig: did not return error as expected")

}
Expand Down Expand Up @@ -321,12 +312,10 @@ func TestMoreThanMaxSigsInMultisig(t *testing.T) {

msig, err := MultisigAssemble(sigs)
require.NoError(t, err, "Multisig: error assmeble multisig")
verify, err := MultisigVerify(txid, addr, msig)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigVerify(txid, addr, msig)
require.Error(t, err, "Multisig: did not return error as expected")
br := MakeBatchVerifier()
verify, err = MultisigBatchVerify(txid, addr, msig, br)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigBatchPrep(txid, addr, msig, br)
require.Error(t, err, "Multisig: did not return error as expected")
}

Expand Down Expand Up @@ -360,12 +349,10 @@ func TestOneSignatureIsEmpty(t *testing.T) {
msig, err := MultisigAssemble(sigs)
require.NoError(t, err, "Multisig: error assmeble multisig")
msig.Subsigs[0].Sig = Signature{}
verify, err := MultisigVerify(txid, addr, msig)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigVerify(txid, addr, msig)
require.Error(t, err, "Multisig: did not return error as expected")
br := MakeBatchVerifier()
verify, err = MultisigBatchVerify(txid, addr, msig, br)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigBatchPrep(txid, addr, msig, br)
require.Error(t, err, "Multisig: did not return error as expected")
}

Expand Down Expand Up @@ -401,13 +388,11 @@ func TestOneSignatureIsInvalid(t *testing.T) {
sigs[1].Subsigs[1].Sig[5] = sigs[1].Subsigs[1].Sig[5] + 1
msig, err := MultisigAssemble(sigs)
require.NoError(t, err, "Multisig: error assmeble multisig")
verify, err := MultisigVerify(txid, addr, msig)
require.False(t, verify, "Multisig: verification succeeded, it should failed")
err = MultisigVerify(txid, addr, msig)
require.Error(t, err, "Multisig: did not return error as expected")
br := MakeBatchVerifier()
verify, err = MultisigBatchVerify(txid, addr, msig, br)
err = MultisigBatchPrep(txid, addr, msig, br)
require.NoError(t, err, "Multisig: did not return error as expected")
require.True(t, verify, "Multisig: verification succeeded, it should failed")
res := br.Verify()
require.Error(t, res, "Multisig: batch verification passed on broken signature")

Expand Down Expand Up @@ -455,15 +440,13 @@ func TestMultisigLessThanTrashold(t *testing.T) {
msig, err = MultisigAssemble(sigs)
require.NoError(t, err, "should be able to detect insufficient signatures for assembling")
msig.Subsigs[1].Sig = BlankSignature
verify, err := MultisigVerify(txid, addr, msig)
require.False(t, verify, "Multisig: verification passed, should have failed")
err = MultisigVerify(txid, addr, msig)
require.Error(t, err, "Multisig: expected verification failure with err")

msig, err = MultisigAssemble(sigs)
require.NoError(t, err, "should be able to detect insufficient signatures for assembling")
msig.Subsigs = msig.Subsigs[:len(msig.Subsigs)-1]
verify, err = MultisigVerify(txid, addr, msig)
require.False(t, verify, "Multisig: verification passed, should have failed")
err = MultisigVerify(txid, addr, msig)
require.Error(t, err, "Multisig: expected verification failure with err")

}
Loading