From 3608cf9c1d74a09bcc87f8657810828cac5631e3 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Fri, 23 Sep 2022 11:07:54 -0400 Subject: [PATCH 01/12] rename and unexport local functions --- data/transactions/verify/txn.go | 48 +++++++++++---------------------- 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index 885d0882ec..4fbf312973 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -99,42 +99,26 @@ func (g *GroupContext) Equal(other *GroupContext) bool { g.minAvmVersion == other.minAvmVersion } -// Txn verifies a SignedTxn as being signed and having no obviously inconsistent data. +// txnBatchVerifyPrep verifies a SignedTxn having no obviously inconsistent data. // Block-assembly time checks of LogicSig and accounting rules may still block the txn. -func Txn(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext) error { - batchVerifier := crypto.MakeBatchVerifier() - - if err := TxnBatchVerify(s, txnIdx, groupCtx, batchVerifier); err != nil { - return err - } - - // this case is used for comapact certificate where no signature is supplied - if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 { - return nil - } - return batchVerifier.Verify() -} - -// TxnBatchVerify verifies a SignedTxn having no obviously inconsistent data. -// Block-assembly time checks of LogicSig and accounting rules may still block the txn. -// it is the caller responsibility to call batchVerifier.verify() -func TxnBatchVerify(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext, verifier *crypto.BatchVerifier) error { +// it is the caller responsibility to call batchVerifier.Verify() +func txnBatchVerifyPrep(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext, verifier *crypto.BatchVerifier) error { if !groupCtx.consensusParams.SupportRekeying && (s.AuthAddr != basics.Address{}) { - return errors.New("nonempty AuthAddr but rekeying not supported") + return errors.New("nonempty AuthAddr but rekeying is not supported") } if err := s.Txn.WellFormed(groupCtx.specAddrs, groupCtx.consensusParams); err != nil { return err } - return stxnVerifyCore(s, txnIdx, groupCtx, verifier) + return stxnCoreChecks(s, txnIdx, groupCtx, verifier) } // TxnGroup verifies a []SignedTxn as being signed and having no obviously inconsistent data. func TxnGroup(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, cache VerifiedTransactionCache, ledger logic.LedgerForSignature) (groupCtx *GroupContext, err error) { batchVerifier := crypto.MakeBatchVerifier() - if groupCtx, err = TxnGroupBatchVerify(stxs, contextHdr, cache, ledger, batchVerifier); err != nil { + if groupCtx, err = txnGroupBatchVerifyPrep(stxs, contextHdr, cache, ledger, batchVerifier); err != nil { return nil, err } @@ -149,9 +133,9 @@ func TxnGroup(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, return } -// TxnGroupBatchVerify verifies a []SignedTxn having no obviously inconsistent data. -// it is the caller responsibility to call batchVerifier.verify() -func TxnGroupBatchVerify(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, cache VerifiedTransactionCache, ledger logic.LedgerForSignature, verifier *crypto.BatchVerifier) (groupCtx *GroupContext, err error) { +// txnGroupBatchVerifyPrep verifies a []SignedTxn having no obviously inconsistent data. +// it is the caller responsibility to call batchVerifier.Verify() +func txnGroupBatchVerifyPrep(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, cache VerifiedTransactionCache, ledger logic.LedgerForSignature, verifier *crypto.BatchVerifier) (groupCtx *GroupContext, err error) { groupCtx, err = PrepareGroupContext(stxs, contextHdr, ledger) if err != nil { return nil, err @@ -160,7 +144,7 @@ func TxnGroupBatchVerify(stxs []transactions.SignedTxn, contextHdr bookkeeping.B minFeeCount := uint64(0) feesPaid := uint64(0) for i, stxn := range stxs { - err = TxnBatchVerify(&stxn, i, groupCtx, verifier) + err = txnBatchVerifyPrep(&stxn, i, groupCtx, verifier) if err != nil { err = fmt.Errorf("transaction %+v invalid : %w", stxn, err) return @@ -190,7 +174,7 @@ func TxnGroupBatchVerify(stxs []transactions.SignedTxn, contextHdr bookkeeping.B return } -func stxnVerifyCore(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext, batchVerifier *crypto.BatchVerifier) error { +func stxnCoreChecks(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext, batchVerifier *crypto.BatchVerifier) error { numSigs := 0 hasSig := false hasMsig := false @@ -246,7 +230,7 @@ func stxnVerifyCore(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContex func LogicSigSanityCheck(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext) error { batchVerifier := crypto.MakeBatchVerifier() - if err := LogicSigSanityCheckBatchVerify(txn, groupIndex, groupCtx, batchVerifier); err != nil { + if err := logicSigSanityCheckBatchVerifyPrep(txn, groupIndex, groupCtx, batchVerifier); err != nil { return err } @@ -258,10 +242,10 @@ func LogicSigSanityCheck(txn *transactions.SignedTxn, groupIndex int, groupCtx * return batchVerifier.Verify() } -// LogicSigSanityCheckBatchVerify checks that the signature is valid and that the program is basically well formed. +// logicSigSanityCheckBatchVerifyPrep checks that the signature is valid and that the program is basically well formed. // It does not evaluate the logic. -// it is the caller responsibility to call batchVerifier.verify() -func LogicSigSanityCheckBatchVerify(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext, batchVerifier *crypto.BatchVerifier) error { +// it is the caller responsibility to call batchVerifier.Verify() +func logicSigSanityCheckBatchVerifyPrep(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext, batchVerifier *crypto.BatchVerifier) error { lsig := txn.Lsig if groupCtx.consensusParams.LogicSigVersion == 0 { @@ -404,7 +388,7 @@ func PaysetGroups(ctx context.Context, payset [][]transactions.SignedTxn, blkHea batchVerifier := crypto.MakeBatchVerifierWithHint(len(payset)) for i, signTxnsGrp := range txnGroups { - groupCtxs[i], grpErr = TxnGroupBatchVerify(signTxnsGrp, blkHeader, nil, ledger, batchVerifier) + groupCtxs[i], grpErr = txnGroupBatchVerifyPrep(signTxnsGrp, blkHeader, nil, ledger, batchVerifier) // abort only if it's a non-cache error. if grpErr != nil { return grpErr From 05d63ae87cfd29c23721c653f202cf6c03f3d736 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Fri, 23 Sep 2022 11:52:05 -0400 Subject: [PATCH 02/12] update the tests --- data/transactions/verify/txn_test.go | 44 ++++++++++++++++++---------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/data/transactions/verify/txn_test.go b/data/transactions/verify/txn_test.go index 3998352114..44108ad5bf 100644 --- a/data/transactions/verify/txn_test.go +++ b/data/transactions/verify/txn_test.go @@ -51,6 +51,20 @@ var spec = transactions.SpecialAddresses{ RewardsPool: poolAddr, } +func verifyTxn(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext) error { + batchVerifier := crypto.MakeBatchVerifier() + + if err := txnBatchVerifyPrep(s, txnIdx, groupCtx, batchVerifier); err != nil { + return err + } + + // this case is used for comapact certificate where no signature is supplied + if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 { + return nil + } + return batchVerifier.Verify() +} + func keypair() *crypto.SignatureSecrets { var seed crypto.Seed crypto.RandBytes(seed[:]) @@ -117,14 +131,14 @@ func TestSignedPayment(t *testing.T) { groupCtx, err := PrepareGroupContext(stxns, blockHeader, nil) require.NoError(t, err) require.NoError(t, payment.WellFormed(spec, proto), "generateTestObjects generated an invalid payment") - require.NoError(t, Txn(&stxn, 0, groupCtx), "generateTestObjects generated a bad signedtxn") + require.NoError(t, verifyTxn(&stxn, 0, groupCtx), "generateTestObjects generated a bad signedtxn") stxn2 := payment.Sign(secret) require.Equal(t, stxn2.Sig, stxn.Sig, "got two different signatures for the same transaction (our signing function is deterministic)") stxn2.MessUpSigForTesting() require.Equal(t, stxn.ID(), stxn2.ID(), "changing sig caused txid to change") - require.Error(t, Txn(&stxn2, 0, groupCtx), "verify succeeded with bad sig") + require.Error(t, verifyTxn(&stxn2, 0, groupCtx), "verify succeeded with bad sig") require.True(t, crypto.SignatureVerifier(addr).Verify(payment, stxn.Sig), "signature on the transaction is not the signature of the hash of the transaction under the spender's key") } @@ -137,7 +151,7 @@ func TestTxnValidationEncodeDecode(t *testing.T) { for _, txn := range signed { groupCtx, err := PrepareGroupContext([]transactions.SignedTxn{txn}, blockHeader, nil) require.NoError(t, err) - if Txn(&txn, 0, groupCtx) != nil { + if verifyTxn(&txn, 0, groupCtx) != nil { t.Errorf("signed transaction %#v did not verify", txn) } @@ -145,7 +159,7 @@ func TestTxnValidationEncodeDecode(t *testing.T) { var signedTx transactions.SignedTxn protocol.Decode(x, &signedTx) - if Txn(&signedTx, 0, groupCtx) != nil { + if verifyTxn(&signedTx, 0, groupCtx) != nil { t.Errorf("signed transaction %#v did not verify", txn) } } @@ -159,14 +173,14 @@ func TestTxnValidationEmptySig(t *testing.T) { for _, txn := range signed { groupCtx, err := PrepareGroupContext([]transactions.SignedTxn{txn}, blockHeader, nil) require.NoError(t, err) - if Txn(&txn, 0, groupCtx) != nil { + if verifyTxn(&txn, 0, groupCtx) != nil { t.Errorf("signed transaction %#v did not verify", txn) } txn.Sig = crypto.Signature{} txn.Msig = crypto.MultisigSig{} txn.Lsig = transactions.LogicSig{} - if Txn(&txn, 0, groupCtx) == nil { + if verifyTxn(&txn, 0, groupCtx) == nil { t.Errorf("transaction %#v verified without sig", txn) } } @@ -205,13 +219,13 @@ func TestTxnValidationStateProof(t *testing.T) { groupCtx, err := PrepareGroupContext([]transactions.SignedTxn{stxn}, blockHeader, nil) require.NoError(t, err) - err = Txn(&stxn, 0, groupCtx) + err = verifyTxn(&stxn, 0, groupCtx) require.NoError(t, err, "state proof txn %#v did not verify", stxn) stxn2 := stxn stxn2.Txn.Type = protocol.PaymentTx stxn2.Txn.Header.Fee = basics.MicroAlgos{Raw: proto.MinTxnFee} - err = Txn(&stxn2, 0, groupCtx) + err = verifyTxn(&stxn2, 0, groupCtx) require.Error(t, err, "payment txn %#v verified from StateProofSender", stxn2) secret := keypair() @@ -219,28 +233,28 @@ func TestTxnValidationStateProof(t *testing.T) { stxn2.Txn.Header.Sender = basics.Address(secret.SignatureVerifier) stxn2.Txn.Header.Fee = basics.MicroAlgos{Raw: proto.MinTxnFee} stxn2 = stxn2.Txn.Sign(secret) - err = Txn(&stxn2, 0, groupCtx) + err = verifyTxn(&stxn2, 0, groupCtx) require.Error(t, err, "state proof txn %#v verified from non-StateProofSender", stxn2) // state proof txns are not allowed to have non-zero values for many fields stxn2 = stxn stxn2.Txn.Header.Fee = basics.MicroAlgos{Raw: proto.MinTxnFee} - err = Txn(&stxn2, 0, groupCtx) + err = verifyTxn(&stxn2, 0, groupCtx) require.Error(t, err, "state proof txn %#v verified", stxn2) stxn2 = stxn stxn2.Txn.Header.Note = []byte{'A'} - err = Txn(&stxn2, 0, groupCtx) + err = verifyTxn(&stxn2, 0, groupCtx) require.Error(t, err, "state proof txn %#v verified", stxn2) stxn2 = stxn stxn2.Txn.Lease[0] = 1 - err = Txn(&stxn2, 0, groupCtx) + err = verifyTxn(&stxn2, 0, groupCtx) require.Error(t, err, "state proof txn %#v verified", stxn2) stxn2 = stxn stxn2.Txn.RekeyTo = basics.Address(secret.SignatureVerifier) - err = Txn(&stxn2, 0, groupCtx) + err = verifyTxn(&stxn2, 0, groupCtx) require.Error(t, err, "state proof txn %#v verified", stxn2) } @@ -258,7 +272,7 @@ func TestDecodeNil(t *testing.T) { // This used to panic when run on a zero value of SignedTxn. groupCtx, err := PrepareGroupContext([]transactions.SignedTxn{st}, blockHeader, nil) require.NoError(t, err) - Txn(&st, 0, groupCtx) + verifyTxn(&st, 0, groupCtx) } } @@ -425,7 +439,7 @@ func BenchmarkTxn(b *testing.B) { groupCtx, err := PrepareGroupContext(txnGroup, blk.BlockHeader, nil) require.NoError(b, err) for i, txn := range txnGroup { - err := Txn(&txn, i, groupCtx) + err := verifyTxn(&txn, i, groupCtx) require.NoError(b, err) } } From 58e4d3f71e0b7bd936cae54a330adbcbe42ab207 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Fri, 23 Sep 2022 12:11:31 -0400 Subject: [PATCH 03/12] update a test --- test/e2e-go/upgrades/rekey_support_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e-go/upgrades/rekey_support_test.go b/test/e2e-go/upgrades/rekey_support_test.go index 4988b79c4c..0032e569dc 100644 --- a/test/e2e-go/upgrades/rekey_support_test.go +++ b/test/e2e-go/upgrades/rekey_support_test.go @@ -128,8 +128,8 @@ func TestRekeyUpgrade(t *testing.T) { _, err = client.BroadcastTransaction(rekeyed) // non empty err means the upgrade have not happened yet (as expected), ensure the error if err != nil { - // should be either "nonempty AuthAddr but rekeying not supported" or "txn dead" - if !strings.Contains(err.Error(), "nonempty AuthAddr but rekeying not supported") && + // should be either "nonempty AuthAddr but rekeying is not supported" or "txn dead" + if !strings.Contains(err.Error(), "nonempty AuthAddr but rekeying is not supported") && !strings.Contains(err.Error(), "txn dead") { a.NoErrorf(err, "error message should be one of :\n%s\n%s", "nonempty AuthAddr but rekeying not supported", "txn dead") } From 1e96dd683d346d53a8b96d40c19db34e68cb37b6 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Fri, 23 Sep 2022 12:19:30 -0400 Subject: [PATCH 04/12] update test err msg --- test/e2e-go/upgrades/rekey_support_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e-go/upgrades/rekey_support_test.go b/test/e2e-go/upgrades/rekey_support_test.go index 0032e569dc..56f5b6408b 100644 --- a/test/e2e-go/upgrades/rekey_support_test.go +++ b/test/e2e-go/upgrades/rekey_support_test.go @@ -131,7 +131,7 @@ func TestRekeyUpgrade(t *testing.T) { // should be either "nonempty AuthAddr but rekeying is not supported" or "txn dead" if !strings.Contains(err.Error(), "nonempty AuthAddr but rekeying is not supported") && !strings.Contains(err.Error(), "txn dead") { - a.NoErrorf(err, "error message should be one of :\n%s\n%s", "nonempty AuthAddr but rekeying not supported", "txn dead") + a.NoErrorf(err, "error message should be one of :\n%s\n%s", "nonempty AuthAddr but rekeying is not supported", "txn dead") } } else { // if we had no error it must mean that we've upgraded already. Verify that. From 2db4ee5454ce7016e9b3909c9bb03b430282927d Mon Sep 17 00:00:00 2001 From: algonautshant Date: Mon, 26 Sep 2022 13:18:51 -0400 Subject: [PATCH 05/12] CR updates --- data/transactions/verify/txn.go | 6 ++---- data/transactions/verify/txn_test.go | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index 4fbf312973..49d56ea5c8 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -220,7 +220,7 @@ func stxnCoreChecks(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContex return errors.New("multisig validation failed") } if hasLogicSig { - return logicSigBatchVerify(s, txnIdx, groupCtx) + return logicSigVerify(s, txnIdx, groupCtx) } return errors.New("has one mystery sig. WAT?") } @@ -314,9 +314,7 @@ func logicSigSanityCheckBatchVerifyPrep(txn *transactions.SignedTxn, groupIndex return nil } -// 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 { +func logicSigVerify(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext) error { err := LogicSigSanityCheck(txn, groupIndex, groupCtx) if err != nil { return err diff --git a/data/transactions/verify/txn_test.go b/data/transactions/verify/txn_test.go index 44108ad5bf..6cec7f238d 100644 --- a/data/transactions/verify/txn_test.go +++ b/data/transactions/verify/txn_test.go @@ -58,7 +58,6 @@ func verifyTxn(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext) er return err } - // this case is used for comapact certificate where no signature is supplied if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 { return nil } From b0ef3a0228e8802cde04bf0bed1b328c74aeafda Mon Sep 17 00:00:00 2001 From: algonautshant Date: Tue, 27 Sep 2022 15:28:20 -0400 Subject: [PATCH 06/12] add test and refactoring --- data/transactions/verify/txn.go | 19 ++++--- data/transactions/verify/txn_test.go | 76 +++++++++++++++++++++++++++- 2 files changed, 84 insertions(+), 11 deletions(-) diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index 4fbf312973..781669fb93 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -118,7 +118,7 @@ func txnBatchVerifyPrep(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupCo func TxnGroup(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, cache VerifiedTransactionCache, ledger logic.LedgerForSignature) (groupCtx *GroupContext, err error) { batchVerifier := crypto.MakeBatchVerifier() - if groupCtx, err = txnGroupBatchVerifyPrep(stxs, contextHdr, cache, ledger, batchVerifier); err != nil { + if groupCtx, err = txnGroupBatchVerifyPrep(stxs, contextHdr, ledger, batchVerifier); err != nil { return nil, err } @@ -130,12 +130,16 @@ func TxnGroup(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, return nil, err } + if cache != nil { + cache.Add(stxs, groupCtx) + } + return } // txnGroupBatchVerifyPrep verifies a []SignedTxn having no obviously inconsistent data. // it is the caller responsibility to call batchVerifier.Verify() -func txnGroupBatchVerifyPrep(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, cache VerifiedTransactionCache, ledger logic.LedgerForSignature, verifier *crypto.BatchVerifier) (groupCtx *GroupContext, err error) { +func txnGroupBatchVerifyPrep(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, ledger logic.LedgerForSignature, verifier *crypto.BatchVerifier) (groupCtx *GroupContext, err error) { groupCtx, err = PrepareGroupContext(stxs, contextHdr, ledger) if err != nil { return nil, err @@ -168,9 +172,6 @@ func txnGroupBatchVerifyPrep(stxs []transactions.SignedTxn, contextHdr bookkeepi return } - if cache != nil { - cache.Add(stxs, groupCtx) - } return } @@ -220,7 +221,7 @@ func stxnCoreChecks(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContex return errors.New("multisig validation failed") } if hasLogicSig { - return logicSigBatchVerify(s, txnIdx, groupCtx) + return logicSigVerify(s, txnIdx, groupCtx) } return errors.New("has one mystery sig. WAT?") } @@ -314,9 +315,7 @@ func logicSigSanityCheckBatchVerifyPrep(txn *transactions.SignedTxn, groupIndex return nil } -// 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 { +func logicSigVerify(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext) error { err := LogicSigSanityCheck(txn, groupIndex, groupCtx) if err != nil { return err @@ -388,7 +387,7 @@ func PaysetGroups(ctx context.Context, payset [][]transactions.SignedTxn, blkHea batchVerifier := crypto.MakeBatchVerifierWithHint(len(payset)) for i, signTxnsGrp := range txnGroups { - groupCtxs[i], grpErr = txnGroupBatchVerifyPrep(signTxnsGrp, blkHeader, nil, ledger, batchVerifier) + groupCtxs[i], grpErr = txnGroupBatchVerifyPrep(signTxnsGrp, blkHeader, ledger, batchVerifier) // abort only if it's a non-cache error. if grpErr != nil { return grpErr diff --git a/data/transactions/verify/txn_test.go b/data/transactions/verify/txn_test.go index 44108ad5bf..7bd3c68279 100644 --- a/data/transactions/verify/txn_test.go +++ b/data/transactions/verify/txn_test.go @@ -58,7 +58,6 @@ func verifyTxn(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext) er return err } - // this case is used for comapact certificate where no signature is supplied if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 { return nil } @@ -445,3 +444,78 @@ func BenchmarkTxn(b *testing.B) { } b.StopTimer() } + +// TestTxnGroupCacheUpdate uses TxnGroup to verify txns and add them to the +// cache. Then makes sure that only the valid txns are verified and added to +// the cache. +func TestTxnGroupCacheUpdate(t *testing.T) { + partitiontest.PartitionTest(t) + + _, signedTxn, secrets, addrs := generateTestObjects(100, 20, 50) + blkHdr := bookkeeping.BlockHeader{ + Round: 50, + GenesisHash: crypto.Hash([]byte{1, 2, 3, 4, 5}), + UpgradeState: bookkeeping.UpgradeState{ + CurrentProtocol: protocol.ConsensusCurrentVersion, + }, + RewardsState: bookkeeping.RewardsState{ + FeeSink: feeSink, + RewardsPool: poolAddr, + }, + } + + txnGroups := generateTransactionGroups(signedTxn, secrets, addrs) + cache := MakeVerifiedTransactionCache(1000) + + // break the signature and see if it fails. + txnGroups[0][0].Sig[0] = txnGroups[0][0].Sig[0] + 1 + + _, err := TxnGroup(txnGroups[0], blkHdr, cache, nil) + require.Error(t, err) + + // The txns should not be in the cache + unverifiedGroups := cache.GetUnverifiedTransactionGroups(txnGroups[:1], spec, protocol.ConsensusCurrentVersion) + require.Equal(t, 1, len(unverifiedGroups)) + + unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups[:2], spec, protocol.ConsensusCurrentVersion) + require.Equal(t, 2, len(unverifiedGroups)) + + _, err = TxnGroup(txnGroups[1], blkHdr, cache, nil) + require.NoError(t, err) + + // Only the second txn should be in the cache + unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups[:2], spec, protocol.ConsensusCurrentVersion) + require.Equal(t, 1, len(unverifiedGroups)) + + // Fix the signature + txnGroups[0][0].Sig[0] = txnGroups[0][0].Sig[0] - 1 + + _, err = TxnGroup(txnGroups[0], blkHdr, cache, nil) + require.NoError(t, err) + + // Both transactions should be in the cache + unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups[:2], spec, protocol.ConsensusCurrentVersion) + require.Equal(t, 0, len(unverifiedGroups)) + + // Break a random signature + txgIdx := rand.Intn(len(txnGroups)) + txIdx := rand.Intn(len(txnGroups[txgIdx])) + txnGroups[txgIdx][txIdx].Sig[0] = txnGroups[0][0].Sig[0] + 1 + + numFailed := 0 + + // Add them to the cache by verifying them + for _, txng := range txnGroups { + _, err = TxnGroup(txng, blkHdr, cache, nil) + if err != nil { + numFailed++ + } + } + require.Equal(t, 1, numFailed) + + // Onle one transaction should not be in cache + unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups, spec, protocol.ConsensusCurrentVersion) + require.Equal(t, 1, len(unverifiedGroups)) + + require.Equal(t, unverifiedGroups[0], txnGroups[txgIdx]) +} From caca719dc80ca6aceb99cd7fbc4afe5132ee8b32 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Tue, 27 Sep 2022 16:43:03 -0400 Subject: [PATCH 07/12] CR comments --- data/transactions/verify/txn.go | 1 + data/transactions/verify/txn_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index 781669fb93..cd951346e8 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -315,6 +315,7 @@ func logicSigSanityCheckBatchVerifyPrep(txn *transactions.SignedTxn, groupIndex return nil } +// logicSigVerify checks that the signature is valid, executing the program. func logicSigVerify(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext) error { err := LogicSigSanityCheck(txn, groupIndex, groupCtx) if err != nil { diff --git a/data/transactions/verify/txn_test.go b/data/transactions/verify/txn_test.go index 7bd3c68279..05ad1e4659 100644 --- a/data/transactions/verify/txn_test.go +++ b/data/transactions/verify/txn_test.go @@ -475,17 +475,17 @@ func TestTxnGroupCacheUpdate(t *testing.T) { // The txns should not be in the cache unverifiedGroups := cache.GetUnverifiedTransactionGroups(txnGroups[:1], spec, protocol.ConsensusCurrentVersion) - require.Equal(t, 1, len(unverifiedGroups)) + require.Len(t, unverifiedGroups, 1) unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups[:2], spec, protocol.ConsensusCurrentVersion) - require.Equal(t, 2, len(unverifiedGroups)) + require.Len(t, unverifiedGroups, 2) _, err = TxnGroup(txnGroups[1], blkHdr, cache, nil) require.NoError(t, err) // Only the second txn should be in the cache unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups[:2], spec, protocol.ConsensusCurrentVersion) - require.Equal(t, 1, len(unverifiedGroups)) + require.Len(t, unverifiedGroups, 1) // Fix the signature txnGroups[0][0].Sig[0] = txnGroups[0][0].Sig[0] - 1 @@ -495,7 +495,7 @@ func TestTxnGroupCacheUpdate(t *testing.T) { // Both transactions should be in the cache unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups[:2], spec, protocol.ConsensusCurrentVersion) - require.Equal(t, 0, len(unverifiedGroups)) + require.Len(t, unverifiedGroups, 0) // Break a random signature txgIdx := rand.Intn(len(txnGroups)) @@ -515,7 +515,7 @@ func TestTxnGroupCacheUpdate(t *testing.T) { // Onle one transaction should not be in cache unverifiedGroups = cache.GetUnverifiedTransactionGroups(txnGroups, spec, protocol.ConsensusCurrentVersion) - require.Equal(t, 1, len(unverifiedGroups)) + require.Len(t, unverifiedGroups, 1) require.Equal(t, unverifiedGroups[0], txnGroups[txgIdx]) } From 754f0fbf83f9658414293aa88fa1b83d479701cb Mon Sep 17 00:00:00 2001 From: algonautshant Date: Tue, 27 Sep 2022 21:40:45 -0400 Subject: [PATCH 08/12] cache txn when only logicsig --- crypto/batchverifier.go | 3 +-- crypto/batchverifier_test.go | 2 +- crypto/multisig.go | 4 ++-- crypto/multisig_test.go | 12 ++++++------ data/transactions/verify/txn.go | 8 ++------ 5 files changed, 12 insertions(+), 17 deletions(-) diff --git a/crypto/batchverifier.go b/crypto/batchverifier.go index cce8e06d7e..bb02075910 100644 --- a/crypto/batchverifier.go +++ b/crypto/batchverifier.go @@ -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 @@ -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 } var messages = make([][]byte, b.GetNumberOfEnqueuedSignatures()) diff --git a/crypto/batchverifier_test.go b/crypto/batchverifier_test.go index 781a80e174..8e76580db9 100644 --- a/crypto/batchverifier_test.go +++ b/crypto/batchverifier_test.go @@ -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()) } diff --git a/crypto/multisig.go b/crypto/multisig.go index 53386ebc97..e5470f51cd 100644 --- a/crypto/multisig.go +++ b/crypto/multisig.go @@ -219,7 +219,7 @@ func MultisigAssemble(unisig []MultisigSig) (msig MultisigSig, err error) { func MultisigVerify(msg Hashable, addr Digest, sig MultisigSig) (verified bool, err error) { batchVerifier := MakeBatchVerifier() - if verified, err = MultisigBatchVerify(msg, addr, sig, batchVerifier); err != nil { + if verified, err = MultisigBatchVerifyPrep(msg, addr, sig, batchVerifier); err != nil { return } if !verified { @@ -236,7 +236,7 @@ func MultisigVerify(msg Hashable, addr Digest, sig MultisigSig) (verified bool, // 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) { +func MultisigBatchVerifyPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier *BatchVerifier) (verified bool, err error) { verified = false // short circuit: if msig doesn't have subsigs or if Subsigs are empty // then terminate (the upper layer should now verify the unisig) diff --git a/crypto/multisig_test.go b/crypto/multisig_test.go index 28eec24598..0e070e550b 100644 --- a/crypto/multisig_test.go +++ b/crypto/multisig_test.go @@ -142,7 +142,7 @@ func TestMultisig(t *testing.T) { //test3: use the batch verification br := MakeBatchVerifier() - verify, err = MultisigBatchVerify(txid, addr, msig, br) + verify, err = MultisigBatchVerifyPrep(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() @@ -258,7 +258,7 @@ func TestEmptyMultisig(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerify(txid, addr, emptyMutliSig, br) + verify, err = MultisigBatchVerifyPrep(txid, addr, emptyMutliSig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") } @@ -286,7 +286,7 @@ func TestIncorrectAddrresInMultisig(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerify(txid, addr, MutliSig, br) + verify, err = MultisigBatchVerifyPrep(txid, addr, MutliSig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") @@ -325,7 +325,7 @@ func TestMoreThanMaxSigsInMultisig(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerify(txid, addr, msig, br) + verify, err = MultisigBatchVerifyPrep(txid, addr, msig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") } @@ -364,7 +364,7 @@ func TestOneSignatureIsEmpty(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerify(txid, addr, msig, br) + verify, err = MultisigBatchVerifyPrep(txid, addr, msig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") } @@ -405,7 +405,7 @@ func TestOneSignatureIsInvalid(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerify(txid, addr, msig, br) + verify, err = MultisigBatchVerifyPrep(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() diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index cd951346e8..a9417489b8 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -122,10 +122,6 @@ func TxnGroup(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, return nil, err } - if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 { - return groupCtx, nil - } - if err := batchVerifier.Verify(); err != nil { return nil, err } @@ -212,7 +208,7 @@ func stxnCoreChecks(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContex return nil } if hasMsig { - if ok, _ := crypto.MultisigBatchVerify(s.Txn, + if ok, _ := crypto.MultisigBatchVerifyPrep(s.Txn, crypto.Digest(s.Authorizer()), s.Msig, batchVerifier); ok { @@ -308,7 +304,7 @@ func logicSigSanityCheckBatchVerifyPrep(txn *transactions.SignedTxn, groupIndex batchVerifier.EnqueueSignature(crypto.PublicKey(txn.Authorizer()), &program, lsig.Sig) } else { program := logic.Program(lsig.Logic) - if ok, _ := crypto.MultisigBatchVerify(&program, crypto.Digest(txn.Authorizer()), lsig.Msig, batchVerifier); !ok { + if ok, _ := crypto.MultisigBatchVerifyPrep(&program, crypto.Digest(txn.Authorizer()), lsig.Msig, batchVerifier); !ok { return errors.New("logic multisig validation failed") } } From d2f8e16484fb381f2ebb6bb1cf0c3d09ad42ecab Mon Sep 17 00:00:00 2001 From: algonautshant Date: Tue, 27 Sep 2022 21:48:24 -0400 Subject: [PATCH 09/12] fix comment --- crypto/multisig.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/multisig.go b/crypto/multisig.go index e5470f51cd..c1bb6b9abd 100644 --- a/crypto/multisig.go +++ b/crypto/multisig.go @@ -234,7 +234,7 @@ func MultisigVerify(msg Hashable, addr Digest, sig MultisigSig) (verified bool, return true, nil } -// MultisigBatchVerify verifies an assembled MultisigSig. +// MultisigBatchVerifyPrep verifies an assembled MultisigSig. // it is the caller responsibility to call batchVerifier.verify() func MultisigBatchVerifyPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier *BatchVerifier) (verified bool, err error) { verified = false From dffe43e9dd939315d5d818bcf3db3caca661b7e9 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Wed, 28 Sep 2022 13:00:41 -0400 Subject: [PATCH 10/12] renaming per CR --- crypto/multisig.go | 6 +++--- crypto/multisig_test.go | 12 ++++++------ data/transactions/verify/txn.go | 24 ++++++++++++------------ data/transactions/verify/txn_test.go | 2 +- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/crypto/multisig.go b/crypto/multisig.go index c1bb6b9abd..fcb04b5f94 100644 --- a/crypto/multisig.go +++ b/crypto/multisig.go @@ -219,7 +219,7 @@ func MultisigAssemble(unisig []MultisigSig) (msig MultisigSig, err error) { func MultisigVerify(msg Hashable, addr Digest, sig MultisigSig) (verified bool, err error) { batchVerifier := MakeBatchVerifier() - if verified, err = MultisigBatchVerifyPrep(msg, addr, sig, batchVerifier); err != nil { + if verified, err = MultisigBatchPrep(msg, addr, sig, batchVerifier); err != nil { return } if !verified { @@ -234,9 +234,9 @@ func MultisigVerify(msg Hashable, addr Digest, sig MultisigSig) (verified bool, return true, nil } -// MultisigBatchVerifyPrep verifies an assembled MultisigSig. +// MultisigBatchPrep verifies an assembled MultisigSig. // it is the caller responsibility to call batchVerifier.verify() -func MultisigBatchVerifyPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier *BatchVerifier) (verified bool, err error) { +func MultisigBatchPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier *BatchVerifier) (verified bool, err error) { verified = false // short circuit: if msig doesn't have subsigs or if Subsigs are empty // then terminate (the upper layer should now verify the unisig) diff --git a/crypto/multisig_test.go b/crypto/multisig_test.go index 0e070e550b..8bd263818b 100644 --- a/crypto/multisig_test.go +++ b/crypto/multisig_test.go @@ -142,7 +142,7 @@ func TestMultisig(t *testing.T) { //test3: use the batch verification br := MakeBatchVerifier() - verify, err = MultisigBatchVerifyPrep(txid, addr, msig, br) + verify, 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() @@ -258,7 +258,7 @@ func TestEmptyMultisig(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerifyPrep(txid, addr, emptyMutliSig, br) + verify, err = MultisigBatchPrep(txid, addr, emptyMutliSig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") } @@ -286,7 +286,7 @@ func TestIncorrectAddrresInMultisig(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerifyPrep(txid, addr, MutliSig, br) + verify, err = MultisigBatchPrep(txid, addr, MutliSig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") @@ -325,7 +325,7 @@ func TestMoreThanMaxSigsInMultisig(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerifyPrep(txid, addr, msig, br) + verify, err = MultisigBatchPrep(txid, addr, msig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") } @@ -364,7 +364,7 @@ func TestOneSignatureIsEmpty(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerifyPrep(txid, addr, msig, br) + verify, err = MultisigBatchPrep(txid, addr, msig, br) require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") } @@ -405,7 +405,7 @@ func TestOneSignatureIsInvalid(t *testing.T) { require.False(t, verify, "Multisig: verification succeeded, it should failed") require.Error(t, err, "Multisig: did not return error as expected") br := MakeBatchVerifier() - verify, err = MultisigBatchVerifyPrep(txid, addr, msig, br) + verify, 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() diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index a9417489b8..129219ec26 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -99,10 +99,10 @@ func (g *GroupContext) Equal(other *GroupContext) bool { g.minAvmVersion == other.minAvmVersion } -// txnBatchVerifyPrep verifies a SignedTxn having no obviously inconsistent data. +// txnBatchPrep verifies a SignedTxn having no obviously inconsistent data. // Block-assembly time checks of LogicSig and accounting rules may still block the txn. // it is the caller responsibility to call batchVerifier.Verify() -func txnBatchVerifyPrep(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext, verifier *crypto.BatchVerifier) error { +func txnBatchPrep(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext, verifier *crypto.BatchVerifier) error { if !groupCtx.consensusParams.SupportRekeying && (s.AuthAddr != basics.Address{}) { return errors.New("nonempty AuthAddr but rekeying is not supported") } @@ -118,7 +118,7 @@ func txnBatchVerifyPrep(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupCo func TxnGroup(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, cache VerifiedTransactionCache, ledger logic.LedgerForSignature) (groupCtx *GroupContext, err error) { batchVerifier := crypto.MakeBatchVerifier() - if groupCtx, err = txnGroupBatchVerifyPrep(stxs, contextHdr, ledger, batchVerifier); err != nil { + if groupCtx, err = txnGroupBatchPrep(stxs, contextHdr, ledger, batchVerifier); err != nil { return nil, err } @@ -133,9 +133,9 @@ func TxnGroup(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, return } -// txnGroupBatchVerifyPrep verifies a []SignedTxn having no obviously inconsistent data. +// txnGroupBatchPrep verifies a []SignedTxn having no obviously inconsistent data. // it is the caller responsibility to call batchVerifier.Verify() -func txnGroupBatchVerifyPrep(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, ledger logic.LedgerForSignature, verifier *crypto.BatchVerifier) (groupCtx *GroupContext, err error) { +func txnGroupBatchPrep(stxs []transactions.SignedTxn, contextHdr bookkeeping.BlockHeader, ledger logic.LedgerForSignature, verifier *crypto.BatchVerifier) (groupCtx *GroupContext, err error) { groupCtx, err = PrepareGroupContext(stxs, contextHdr, ledger) if err != nil { return nil, err @@ -144,7 +144,7 @@ func txnGroupBatchVerifyPrep(stxs []transactions.SignedTxn, contextHdr bookkeepi minFeeCount := uint64(0) feesPaid := uint64(0) for i, stxn := range stxs { - err = txnBatchVerifyPrep(&stxn, i, groupCtx, verifier) + err = txnBatchPrep(&stxn, i, groupCtx, verifier) if err != nil { err = fmt.Errorf("transaction %+v invalid : %w", stxn, err) return @@ -208,7 +208,7 @@ func stxnCoreChecks(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContex return nil } if hasMsig { - if ok, _ := crypto.MultisigBatchVerifyPrep(s.Txn, + if ok, _ := crypto.MultisigBatchPrep(s.Txn, crypto.Digest(s.Authorizer()), s.Msig, batchVerifier); ok { @@ -227,7 +227,7 @@ func stxnCoreChecks(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContex func LogicSigSanityCheck(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext) error { batchVerifier := crypto.MakeBatchVerifier() - if err := logicSigSanityCheckBatchVerifyPrep(txn, groupIndex, groupCtx, batchVerifier); err != nil { + if err := logicSigSanityCheckBatchPrep(txn, groupIndex, groupCtx, batchVerifier); err != nil { return err } @@ -239,10 +239,10 @@ func LogicSigSanityCheck(txn *transactions.SignedTxn, groupIndex int, groupCtx * return batchVerifier.Verify() } -// logicSigSanityCheckBatchVerifyPrep checks that the signature is valid and that the program is basically well formed. +// logicSigSanityCheckBatchPrep checks that the signature is valid and that the program is basically well formed. // It does not evaluate the logic. // it is the caller responsibility to call batchVerifier.Verify() -func logicSigSanityCheckBatchVerifyPrep(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext, batchVerifier *crypto.BatchVerifier) error { +func logicSigSanityCheckBatchPrep(txn *transactions.SignedTxn, groupIndex int, groupCtx *GroupContext, batchVerifier *crypto.BatchVerifier) error { lsig := txn.Lsig if groupCtx.consensusParams.LogicSigVersion == 0 { @@ -304,7 +304,7 @@ func logicSigSanityCheckBatchVerifyPrep(txn *transactions.SignedTxn, groupIndex batchVerifier.EnqueueSignature(crypto.PublicKey(txn.Authorizer()), &program, lsig.Sig) } else { program := logic.Program(lsig.Logic) - if ok, _ := crypto.MultisigBatchVerifyPrep(&program, crypto.Digest(txn.Authorizer()), lsig.Msig, batchVerifier); !ok { + if ok, _ := crypto.MultisigBatchPrep(&program, crypto.Digest(txn.Authorizer()), lsig.Msig, batchVerifier); !ok { return errors.New("logic multisig validation failed") } } @@ -384,7 +384,7 @@ func PaysetGroups(ctx context.Context, payset [][]transactions.SignedTxn, blkHea batchVerifier := crypto.MakeBatchVerifierWithHint(len(payset)) for i, signTxnsGrp := range txnGroups { - groupCtxs[i], grpErr = txnGroupBatchVerifyPrep(signTxnsGrp, blkHeader, ledger, batchVerifier) + groupCtxs[i], grpErr = txnGroupBatchPrep(signTxnsGrp, blkHeader, ledger, batchVerifier) // abort only if it's a non-cache error. if grpErr != nil { return grpErr diff --git a/data/transactions/verify/txn_test.go b/data/transactions/verify/txn_test.go index 05ad1e4659..ae4a3aeab7 100644 --- a/data/transactions/verify/txn_test.go +++ b/data/transactions/verify/txn_test.go @@ -54,7 +54,7 @@ var spec = transactions.SpecialAddresses{ func verifyTxn(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext) error { batchVerifier := crypto.MakeBatchVerifier() - if err := txnBatchVerifyPrep(s, txnIdx, groupCtx, batchVerifier); err != nil { + if err := txnBatchPrep(s, txnIdx, groupCtx, batchVerifier); err != nil { return err } From 8f5df56c1ad63a9e6960426bcee2db93828b9336 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Wed, 28 Sep 2022 16:29:03 -0400 Subject: [PATCH 11/12] cleanups requested in CR --- crypto/multisig.go | 43 +++++----------- crypto/multisig_test.go | 51 +++++++------------ data/transactions/verify/txn.go | 13 ++--- data/transactions/verify/txn_test.go | 4 -- .../kmd/e2e_kmd_wallet_multisig_test.go | 3 +- 5 files changed, 36 insertions(+), 78 deletions(-) diff --git a/crypto/multisig.go b/crypto/multisig.go index fcb04b5f94..6231053f52 100644 --- a/crypto/multisig.go +++ b/crypto/multisig.go @@ -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 = MultisigBatchPrep(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 } -// MultisigBatchPrep verifies an assembled MultisigSig. -// it is the caller responsibility to call batchVerifier.verify() -func MultisigBatchPrep(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 @@ -251,20 +241,17 @@ func MultisigBatchPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier 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 @@ -275,8 +262,7 @@ func MultisigBatchPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier } } if counter < sig.Threshold { - err = errInvalidNumberOfSignature - return + return errInvalidNumberOfSignature } // checks individual signature verifies @@ -291,11 +277,8 @@ func MultisigBatchPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier // 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 + return errInvalidNumberOfSignature } - - verified = true return } diff --git a/crypto/multisig_test.go b/crypto/multisig_test.go index 8bd263818b..5035300d21 100644 --- a/crypto/multisig_test.go +++ b/crypto/multisig_test.go @@ -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 = MultisigBatchPrep(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") } @@ -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} @@ -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} @@ -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 } @@ -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 = MultisigBatchPrep(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") } @@ -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 = MultisigBatchPrep(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") } @@ -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 = MultisigBatchPrep(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") } @@ -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 = MultisigBatchPrep(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") } @@ -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 = MultisigBatchPrep(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") @@ -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") } diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index 129219ec26..7d744fdb9d 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -208,13 +208,10 @@ func stxnCoreChecks(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContex return nil } if hasMsig { - if ok, _ := crypto.MultisigBatchPrep(s.Txn, - crypto.Digest(s.Authorizer()), - s.Msig, - batchVerifier); ok { - return nil + if err := crypto.MultisigBatchPrep(s.Txn, crypto.Digest(s.Authorizer()), s.Msig, batchVerifier); err != nil { + return fmt.Errorf("multisig validation failed: %w", err) } - return errors.New("multisig validation failed") + return nil } if hasLogicSig { return logicSigVerify(s, txnIdx, groupCtx) @@ -304,8 +301,8 @@ func logicSigSanityCheckBatchPrep(txn *transactions.SignedTxn, groupIndex int, g batchVerifier.EnqueueSignature(crypto.PublicKey(txn.Authorizer()), &program, lsig.Sig) } else { program := logic.Program(lsig.Logic) - if ok, _ := crypto.MultisigBatchPrep(&program, crypto.Digest(txn.Authorizer()), lsig.Msig, batchVerifier); !ok { - return errors.New("logic multisig validation failed") + if err := crypto.MultisigBatchPrep(&program, crypto.Digest(txn.Authorizer()), lsig.Msig, batchVerifier); err != nil { + return fmt.Errorf("logic multisig validation failed: %w", err) } } return nil diff --git a/data/transactions/verify/txn_test.go b/data/transactions/verify/txn_test.go index ae4a3aeab7..ea3f05b87e 100644 --- a/data/transactions/verify/txn_test.go +++ b/data/transactions/verify/txn_test.go @@ -57,10 +57,6 @@ func verifyTxn(s *transactions.SignedTxn, txnIdx int, groupCtx *GroupContext) er if err := txnBatchPrep(s, txnIdx, groupCtx, batchVerifier); err != nil { return err } - - if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 { - return nil - } return batchVerifier.Verify() } diff --git a/test/e2e-go/kmd/e2e_kmd_wallet_multisig_test.go b/test/e2e-go/kmd/e2e_kmd_wallet_multisig_test.go index 3b43606d6d..be8f27e766 100644 --- a/test/e2e-go/kmd/e2e_kmd_wallet_multisig_test.go +++ b/test/e2e-go/kmd/e2e_kmd_wallet_multisig_test.go @@ -440,7 +440,6 @@ func TestMultisigSignProgram(t *testing.T) { err = protocol.Decode(resp3.Multisig, &msig) a.NoError(err) - ok, err := crypto.MultisigVerify(logic.Program(program), crypto.Digest(msigAddr), msig) + err = crypto.MultisigVerify(logic.Program(program), crypto.Digest(msigAddr), msig) a.NoError(err) - a.True(ok) } From 9ff7853d7222b3f24737edef1b46cd50c983c2b8 Mon Sep 17 00:00:00 2001 From: algonautshant Date: Thu, 29 Sep 2022 13:06:55 -0400 Subject: [PATCH 12/12] CR comments: rename and drop enqueued sig count --- crypto/batchverifier.go | 8 ++++---- crypto/batchverifier_test.go | 2 +- crypto/multisig.go | 6 +++--- data/transactions/verify/txn.go | 14 +++----------- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/crypto/batchverifier.go b/crypto/batchverifier.go index bb02075910..db8ffe6426 100644 --- a/crypto/batchverifier.go +++ b/crypto/batchverifier.go @@ -103,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 { + if b.getNumberOfEnqueuedSignatures() == 0 { return nil } - var messages = make([][]byte, b.GetNumberOfEnqueuedSignatures()) + var messages = make([][]byte, b.getNumberOfEnqueuedSignatures()) for i, m := range b.messages { messages[i] = HashRep(m) } diff --git a/crypto/batchverifier_test.go b/crypto/batchverifier_test.go index 8e76580db9..4469da400b 100644 --- a/crypto/batchverifier_test.go +++ b/crypto/batchverifier_test.go @@ -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()) } diff --git a/crypto/multisig.go b/crypto/multisig.go index 6231053f52..62ec187a2b 100644 --- a/crypto/multisig.go +++ b/crypto/multisig.go @@ -266,17 +266,17 @@ func MultisigBatchPrep(msg Hashable, addr Digest, sig MultisigSig, batchVerifier } // 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) { + if sigCount < int(sig.Threshold) { return errInvalidNumberOfSignature } return diff --git a/data/transactions/verify/txn.go b/data/transactions/verify/txn.go index 7d744fdb9d..4d0bd2717c 100644 --- a/data/transactions/verify/txn.go +++ b/data/transactions/verify/txn.go @@ -227,12 +227,6 @@ func LogicSigSanityCheck(txn *transactions.SignedTxn, groupIndex int, groupCtx * if err := logicSigSanityCheckBatchPrep(txn, groupIndex, groupCtx, batchVerifier); err != nil { return err } - - // in case of contract account the signature len might 0. that's ok - if batchVerifier.GetNumberOfEnqueuedSignatures() == 0 { - return nil - } - return batchVerifier.Verify() } @@ -387,11 +381,9 @@ func PaysetGroups(ctx context.Context, payset [][]transactions.SignedTxn, blkHea return grpErr } } - if batchVerifier.GetNumberOfEnqueuedSignatures() != 0 { - verifyErr := batchVerifier.Verify() - if verifyErr != nil { - return verifyErr - } + verifyErr := batchVerifier.Verify() + if verifyErr != nil { + return verifyErr } cache.AddPayset(txnGroups, groupCtxs) return nil