chose readability over performance on Validation method

types/tx.go

@@ -182,13 +182,11 @@ func (t TxRecordSet) Validate(maxSizeBytes int64, otxs Txs) error {
 	// and once by sorting the set of the added, removed, and unmodified transactions indexes,
 	// which, when combined, comprise the complete list of modified transactions.
 	//
-	// The original list is iterated once and the modified list is iterated multiple times,
-	// one time alongside each of the 3 indexes for a total of 4 iterations of the modified list.
-	// Asymptotically, this yields a total runtime of O(N*log(N) + N + 2*M*log(M) + 4*M),
-	// in the input size of the original list and the input size of the new list respectively.
-	// A 2 * M performance gain is possible by iterating all of the indexes simultaneously
-	// alongside the full list, but the multiple iterations were preferred to be more
-	// readable and maintainable.
+	// Each of the added, removed, and unmodified indices is then iterated and once
+	// and each value index is checked against the sorted original list for containment.
+	// Asymptotically, this yields a total runtime of O(N*log(N) + 2*M*log(M) + M*log(N)).
+	// in the input size of the original list, N, and the input size of the new list, M, respectively.
+	// Performance gains are likely possible, but this was prefered for readability and maintainability.
 
 	// Sort a copy of the complete transaction slice so we can check for
 	// duplication. The copy is so we do not change the original ordering.
@@ -218,14 +216,14 @@ func (t TxRecordSet) Validate(maxSizeBytes int64, otxs Txs) error {
 	// the caller's data is not altered.
 	otxsCopy := sortedCopy(otxs)
 
-	if ix, ok := containsAllTxs(otxsCopy, unmodifiedCopy); !ok {
+	if ix, ok := containsAll(otxsCopy, unmodifiedCopy); !ok {
 		return fmt.Errorf("new transaction incorrectly marked as removed, transaction hash: %x", unmodifiedCopy[ix].Hash())
 	}
 
-	if ix, ok := containsAllTxs(otxsCopy, removedCopy); !ok {
+	if ix, ok := containsAll(otxsCopy, removedCopy); !ok {
 		return fmt.Errorf("new transaction incorrectly marked as removed, transaction hash: %x", removedCopy[ix].Hash())
 	}
-	if ix, ok := containsAnyTxs(otxsCopy, addedCopy); ok {
+	if ix, ok := containsAny(otxsCopy, addedCopy); ok {
 		return fmt.Errorf("existing transaction incorrectly marked as added, transaction hash: %x", addedCopy[ix].Hash())
 	}
 	return nil
@@ -238,71 +236,42 @@ func sortedCopy(txs Txs) Txs {
 	return cp
 }
 
-// containsAnyTxs checks that list a contains one of the transactions in list
+// containsAny checks that list a contains one of the transactions in list
 // b. If a match is found, the index in b of the matching transaction is returned.
 // Both lists must be sorted.
-func containsAnyTxs(a, b []Tx) (int, bool) {
-	aix, bix := 0, 0
-
-nextA:
-	for ; aix < len(a); aix++ {
-		for bix < len(b) {
-			switch bytes.Compare(b[bix], a[aix]) {
-			case 0:
-				return bix, true
-			case -1:
-				bix++
-				// we've reached the end of b, and the last value in b was
-				// smaller than the value under the iterator of a.
-				// a's values never get smaller, so we know there are no more matches
-				// in the list. We can terminate the iteration here.
-				if bix == len(b) {
-					return -1, false
-				}
-			case 1:
-				continue nextA
-			}
+func containsAny(a, b []Tx) (int, bool) {
+	for i, cur := range b {
+		if _, ok := contains(a, cur); ok {
+			return i, true
 		}
 	}
 	return -1, false
 }
 
-// containsAllTxs checks that super contains all of the transactions in the sub
+// containsAll checks that super contains all of the transactions in the sub
 // list. If not all values in sub are present in super, the index in sub of the
 // first Tx absent from super is returned.
-func containsAllTxs(super, sub []Tx) (int, bool) {
-	// The following iteration assumes sorted lists.
-	// The checks ensure that all the values in the sorted sub list are present in the sorted super list.
-	//
-	// We compare the value under the sub iterator to the value
-	// under the super iterator. If they match, we advance the
-	// sub iterator one position. If they don't match, then the value under
-	// the sub iterator should be greater.
-	// If it is not, then there is a value in the the sub list that is not present in the
-	// super list.
-	subIx := 0
-	for _, cur := range super {
-		if subIx == len(sub) {
-			return -1, true
+func containsAll(super, sub Txs) (int, bool) {
+	for i, cur := range sub {
+		if _, ok := contains(super, cur); !ok {
+			return i, false
 		}
-		switch bytes.Compare(sub[subIx], cur) {
-		case 0:
-			subIx++
-		case -1:
-			return subIx, false
-		}
-	}
-
-	// Check that the loop visited all values of the transactions from sub.
-	// If it did not, then there are values present in these indexes that were not
-	// present in the super list of transactions.
-
-	if subIx != len(sub) {
-		return subIx, false
 	}
 	return -1, true
 }
 
+// contains checks that the sorted list, set contains elem. If set does contain elem, then the
+// index in set of elem is returned.
+func contains(set []Tx, elem Tx) (int, bool) {
+	n := sort.Search(len(set), func(i int) bool {
+		return bytes.Compare(elem, set[i]) <= 0
+	})
+	if n == len(set) || !bytes.Equal(elem, set[n]) {
+		return -1, false
+	}
+	return n, true
+}
+
 // TxProof represents a Merkle proof of the presence of a transaction in the Merkle tree.
 type TxProof struct {
 	RootHash tmbytes.HexBytes `json:"root_hash"`