rpc: fix tx_search pagination with ordered results (tendermint#4437)

Author: erikgrinaker

Diff for: CHANGELOG_PENDING.md

@@ -21,4 +21,6 @@ program](https://hackerone.com/tendermint).
 
 ### BUG FIXES:
 
+- [rpc] [\#4437](https://github.com/tendermint/tendermint/pull/4437) Fix tx_search pagination with ordered results (@erikgrinaker)
+
 - [rpc] [\#4406](https://github.com/tendermint/tendermint/pull/4406) Fix issue with multiple subscriptions on the websocket (@antho1404)

Diff for: rpc/client/rpc_test.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/tendermint/tendermint/crypto/ed25519"
 	"github.com/tendermint/tendermint/crypto/tmhash"
+	tmbytes "github.com/tendermint/tendermint/libs/bytes"
 	"github.com/tendermint/tendermint/libs/log"
 	tmmath "github.com/tendermint/tendermint/libs/math"
 	mempl "github.com/tendermint/tendermint/mempool"
@@ -414,14 +415,25 @@ func TestTx(t *testing.T) {
 }
 
 func TestTxSearch(t *testing.T) {
-	// first we broadcast a tx
 	c := getHTTPClient()
-	_, _, tx := MakeTxKV()
-	bres, err := c.BroadcastTxCommit(tx)
-	require.Nil(t, err)
 
-	txHeight := bres.Height
-	txHash := bres.Hash
+	// first we broadcast a few txs
+	var tx []byte
+	var txHeight int64
+	var txHash tmbytes.HexBytes
+	for i := 0; i < 10; i++ {
+		_, _, tx = MakeTxKV()
+		res, err := c.BroadcastTxCommit(tx)
+		require.NoError(t, err)
+		txHeight = res.Height
+		txHash = res.Hash
+	}
+
+	// Since we're not using an isolated test server, we'll have lingering transactions
+	// from other tests as well
+	result, err := c.TxSearch("tx.height >= 0", true, 1, 100, "asc")
+	require.NoError(t, err)
+	txCount := len(result.Txs)
 
 	anotherTxHash := types.Tx("a different tx").Hash()
 
@@ -433,6 +445,7 @@ func TestTxSearch(t *testing.T) {
 		result, err := c.TxSearch(fmt.Sprintf("tx.hash='%v'", txHash), true, 1, 30, "asc")
 		require.Nil(t, err)
 		require.Len(t, result.Txs, 1)
+		require.Equal(t, txHash, result.Txs[0].Hash)
 
 		ptx := result.Txs[0]
 		assert.EqualValues(t, txHeight, ptx.Height)
@@ -476,12 +489,7 @@ func TestTxSearch(t *testing.T) {
 		require.Nil(t, err)
 		require.Len(t, result.Txs, 0)
 
-		// broadcast another transaction to make sure we have at least two.
-		_, _, tx2 := MakeTxKV()
-		_, err = c.BroadcastTxCommit(tx2)
-		require.Nil(t, err)
-
-		// chech sorting
+		// check sorting
 		result, err = c.TxSearch(fmt.Sprintf("tx.height >= 1"), false, 1, 30, "asc")
 		require.Nil(t, err)
 		for k := 0; k < len(result.Txs)-1; k++ {
@@ -495,6 +503,31 @@ func TestTxSearch(t *testing.T) {
 			require.GreaterOrEqual(t, result.Txs[k].Height, result.Txs[k+1].Height)
 			require.GreaterOrEqual(t, result.Txs[k].Index, result.Txs[k+1].Index)
 		}
+
+		// check pagination
+		seen := map[int64]bool{}
+		maxHeight := int64(0)
+		perPage := 3
+		pages := txCount/perPage + 1
+		for page := 1; page <= pages; page++ {
+			result, err = c.TxSearch("tx.height >= 1", false, page, perPage, "asc")
+			require.NoError(t, err)
+			if page < pages {
+				require.Len(t, result.Txs, perPage)
+			} else {
+				require.LessOrEqual(t, len(result.Txs), perPage)
+			}
+			require.Equal(t, txCount, result.TotalCount)
+			for _, tx := range result.Txs {
+				require.False(t, seen[tx.Height],
+					"Found duplicate height %v in page %v", tx.Height, page)
+				require.Greater(t, tx.Height, maxHeight,
+					"Found decreasing height %v (max seen %v) in page %v", tx.Height, maxHeight, page)
+				seen[tx.Height] = true
+				maxHeight = tx.Height
+			}
+		}
+		require.Len(t, seen, txCount)
 	}
 }
 

Diff for: rpc/core/tx.go

@@ -72,56 +72,54 @@ func TxSearch(ctx *rpctypes.Context, query string, prove bool, page, perPage int
 		return nil, err
 	}
 
+	// sort results (must be done before pagination)
+	switch orderBy {
+	case "desc":
+		sort.Slice(results, func(i, j int) bool {
+			if results[i].Height == results[j].Height {
+				return results[i].Index > results[j].Index
+			}
+			return results[i].Height > results[j].Height
+		})
+	case "asc", "":
+		sort.Slice(results, func(i, j int) bool {
+			if results[i].Height == results[j].Height {
+				return results[i].Index < results[j].Index
+			}
+			return results[i].Height < results[j].Height
+		})
+	default:
+		return nil, errors.New("expected order_by to be either `asc` or `desc` or empty")
+	}
+
+	// paginate results
 	totalCount := len(results)
 	perPage = validatePerPage(perPage)
 	page, err = validatePage(page, perPage, totalCount)
 	if err != nil {
 		return nil, err
 	}
 	skipCount := validateSkipCount(page, perPage)
+	pageSize := tmmath.MinInt(perPage, totalCount-skipCount)
 
-	apiResults := make([]*ctypes.ResultTx, tmmath.MinInt(perPage, totalCount-skipCount))
-	var proof types.TxProof
-	// if there's no tx in the results array, we don't need to loop through the apiResults array
-	for i := 0; i < len(apiResults); i++ {
-		r := results[skipCount+i]
-		height := r.Height
-		index := r.Index
+	apiResults := make([]*ctypes.ResultTx, 0, pageSize)
+	for i := skipCount; i < skipCount+pageSize; i++ {
+		r := results[i]
 
+		var proof types.TxProof
 		if prove {
-			block := blockStore.LoadBlock(height)
-			proof = block.Data.Txs.Proof(int(index)) // XXX: overflow on 32-bit machines
+			block := blockStore.LoadBlock(r.Height)
+			proof = block.Data.Txs.Proof(int(r.Index)) // XXX: overflow on 32-bit machines
 		}
 
-		apiResults[i] = &ctypes.ResultTx{
+		apiResults = append(apiResults, &ctypes.ResultTx{
 			Hash:     r.Tx.Hash(),
-			Height:   height,
-			Index:    index,
+			Height:   r.Height,
+			Index:    r.Index,
 			TxResult: r.Result,
 			Tx:       r.Tx,
 			Proof:    proof,
-		}
-	}
-
-	if len(apiResults) > 1 {
-		switch orderBy {
-		case "desc":
-			sort.Slice(apiResults, func(i, j int) bool {
-				if apiResults[i].Height == apiResults[j].Height {
-					return apiResults[i].Index > apiResults[j].Index
-				}
-				return apiResults[i].Height > apiResults[j].Height
-			})
-		case "asc", "":
-			sort.Slice(apiResults, func(i, j int) bool {
-				if apiResults[i].Height == apiResults[j].Height {
-					return apiResults[i].Index < apiResults[j].Index
-				}
-				return apiResults[i].Height < apiResults[j].Height
-			})
-		default:
-			return nil, errors.New("expected order_by to be either `asc` or `desc` or empty")
-		}
+		})
 	}
 
 	return &ctypes.ResultTxSearch{Txs: apiResults, TotalCount: totalCount}, nil