add test and refactoring

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

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])
+}