R4R: Swap start/end in ReverseIterator (#2913)

* Swap start/end in ReverseIterator

* update CHANGELOG_PENDING

* fixes from review

CHANGELOG_PENDING.md

@@ -17,6 +17,8 @@ program](https://hackerone.com/tendermint).
 
 * Go API
 
+- [db] [\#2913](https://github.com/tendermint/tendermint/pull/2913) ReverseIterator API change -- start < end, and end is exclusive.
+
 * Blockchain Protocol
 
 * P2P Protocol

libs/db/backend_test.go

@@ -180,13 +180,13 @@ func testDBIterator(t *testing.T, backend DBBackendType) {
 	verifyIterator(t, db.ReverseIterator(nil, nil), []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator")
 
 	verifyIterator(t, db.Iterator(nil, int642Bytes(0)), []int64(nil), "forward iterator to 0")
-	verifyIterator(t, db.ReverseIterator(nil, int642Bytes(10)), []int64(nil), "reverse iterator 10")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(10), nil), []int64(nil), "reverse iterator from 10 (ex)")
 
 	verifyIterator(t, db.Iterator(int642Bytes(0), nil), []int64{0, 1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 0")
 	verifyIterator(t, db.Iterator(int642Bytes(1), nil), []int64{1, 2, 3, 4, 5, 7, 8, 9}, "forward iterator from 1")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(10), nil), []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 10")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(9), nil), []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 9")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(8), nil), []int64{8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 8")
+	verifyIterator(t, db.ReverseIterator(nil, int642Bytes(10)), []int64{9, 8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 10 (ex)")
+	verifyIterator(t, db.ReverseIterator(nil, int642Bytes(9)), []int64{8, 7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 9 (ex)")
+	verifyIterator(t, db.ReverseIterator(nil, int642Bytes(8)), []int64{7, 5, 4, 3, 2, 1, 0}, "reverse iterator from 8 (ex)")
 
 	verifyIterator(t, db.Iterator(int642Bytes(5), int642Bytes(6)), []int64{5}, "forward iterator from 5 to 6")
 	verifyIterator(t, db.Iterator(int642Bytes(5), int642Bytes(7)), []int64{5}, "forward iterator from 5 to 7")
@@ -195,20 +195,20 @@ func testDBIterator(t *testing.T, backend DBBackendType) {
 	verifyIterator(t, db.Iterator(int642Bytes(6), int642Bytes(8)), []int64{7}, "forward iterator from 6 to 8")
 	verifyIterator(t, db.Iterator(int642Bytes(7), int642Bytes(8)), []int64{7}, "forward iterator from 7 to 8")
 
-	verifyIterator(t, db.ReverseIterator(int642Bytes(5), int642Bytes(4)), []int64{5}, "reverse iterator from 5 to 4")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(6), int642Bytes(4)), []int64{5}, "reverse iterator from 6 to 4")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(7), int642Bytes(4)), []int64{7, 5}, "reverse iterator from 7 to 4")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(6), int642Bytes(5)), []int64(nil), "reverse iterator from 6 to 5")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(7), int642Bytes(5)), []int64{7}, "reverse iterator from 7 to 5")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(7), int642Bytes(6)), []int64{7}, "reverse iterator from 7 to 6")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(4), int642Bytes(5)), []int64{4}, "reverse iterator from 5 (ex) to 4")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(4), int642Bytes(6)), []int64{5, 4}, "reverse iterator from 6 (ex) to 4")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(4), int642Bytes(7)), []int64{5, 4}, "reverse iterator from 7 (ex) to 4")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(5), int642Bytes(6)), []int64{5}, "reverse iterator from 6 (ex) to 5")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(5), int642Bytes(7)), []int64{5}, "reverse iterator from 7 (ex) to 5")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(6), int642Bytes(7)), []int64(nil), "reverse iterator from 7 (ex) to 6")
 
 	verifyIterator(t, db.Iterator(int642Bytes(0), int642Bytes(1)), []int64{0}, "forward iterator from 0 to 1")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(9), int642Bytes(8)), []int64{9}, "reverse iterator from 9 to 8")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(8), int642Bytes(9)), []int64{8}, "reverse iterator from 9 (ex) to 8")
 
 	verifyIterator(t, db.Iterator(int642Bytes(2), int642Bytes(4)), []int64{2, 3}, "forward iterator from 2 to 4")
 	verifyIterator(t, db.Iterator(int642Bytes(4), int642Bytes(2)), []int64(nil), "forward iterator from 4 to 2")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(4), int642Bytes(2)), []int64{4, 3}, "reverse iterator from 4 to 2")
-	verifyIterator(t, db.ReverseIterator(int642Bytes(2), int642Bytes(4)), []int64(nil), "reverse iterator from 2 to 4")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(2), int642Bytes(4)), []int64{3, 2}, "reverse iterator from 4 (ex) to 2")
+	verifyIterator(t, db.ReverseIterator(int642Bytes(4), int642Bytes(2)), []int64(nil), "reverse iterator from 2 (ex) to 4")
 
 }
 

libs/db/c_level_db.go

@@ -205,13 +205,13 @@ type cLevelDBIterator struct {
 
 func newCLevelDBIterator(source *levigo.Iterator, start, end []byte, isReverse bool) *cLevelDBIterator {
 	if isReverse {
-		if start == nil {
+		if end == nil {
 			source.SeekToLast()
 		} else {
-			source.Seek(start)
+			source.Seek(end)
 			if source.Valid() {
-				soakey := source.Key() // start or after key
-				if bytes.Compare(start, soakey) < 0 {
+				eoakey := source.Key() // end or after key
+				if bytes.Compare(end, eoakey) <= 0 {
 					source.Prev()
 				}
 			} else {
@@ -255,10 +255,11 @@ func (itr cLevelDBIterator) Valid() bool {
 	}
 
 	// If key is end or past it, invalid.
+	var start = itr.start
 	var end = itr.end
 	var key = itr.source.Key()
 	if itr.isReverse {
-		if end != nil && bytes.Compare(key, end) <= 0 {
+		if start != nil && bytes.Compare(key, start) < 0 {
 			itr.isInvalid = true
 			return false
 		}

libs/db/fsdb.go

@@ -161,7 +161,7 @@ func (db *FSDB) MakeIterator(start, end []byte, isReversed bool) Iterator {
 
 	// We need a copy of all of the keys.
 	// Not the best, but probably not a bottleneck depending.
-	keys, err := list(db.dir, start, end, isReversed)
+	keys, err := list(db.dir, start, end)
 	if err != nil {
 		panic(errors.Wrapf(err, "Listing keys in %s", db.dir))
 	}
@@ -229,7 +229,7 @@ func remove(path string) error {
 
 // List keys in a directory, stripping of escape sequences and dir portions.
 // CONTRACT: returns os errors directly without wrapping.
-func list(dirPath string, start, end []byte, isReversed bool) ([]string, error) {
+func list(dirPath string, start, end []byte) ([]string, error) {
 	dir, err := os.Open(dirPath)
 	if err != nil {
 		return nil, err
@@ -247,7 +247,7 @@ func list(dirPath string, start, end []byte, isReversed bool) ([]string, error)
 			return nil, fmt.Errorf("Failed to unescape %s while listing", name)
 		}
 		key := unescapeKey([]byte(n))
-		if IsKeyInDomain(key, start, end, isReversed) {
+		if IsKeyInDomain(key, start, end) {
 			keys = append(keys, string(key))
 		}
 	}

libs/db/go_level_db.go

@@ -213,13 +213,13 @@ var _ Iterator = (*goLevelDBIterator)(nil)
 
 func newGoLevelDBIterator(source iterator.Iterator, start, end []byte, isReverse bool) *goLevelDBIterator {
 	if isReverse {
-		if start == nil {
+		if end == nil {
 			source.Last()
 		} else {
-			valid := source.Seek(start)
+			valid := source.Seek(end)
 			if valid {
-				soakey := source.Key() // start or after key
-				if bytes.Compare(start, soakey) < 0 {
+				eoakey := source.Key() // end or after key
+				if bytes.Compare(end, eoakey) <= 0 {
 					source.Prev()
 				}
 			} else {
@@ -265,11 +265,12 @@ func (itr *goLevelDBIterator) Valid() bool {
 	}
 
 	// If key is end or past it, invalid.
+	var start = itr.start
 	var end = itr.end
 	var key = itr.source.Key()
 
 	if itr.isReverse {
-		if end != nil && bytes.Compare(key, end) <= 0 {
+		if start != nil && bytes.Compare(key, start) < 0 {
 			itr.isInvalid = true
 			return false
 		}

libs/db/mem_db.go

@@ -237,7 +237,7 @@ func (itr *memDBIterator) assertIsValid() {
 func (db *MemDB) getSortedKeys(start, end []byte, reverse bool) []string {
 	keys := []string{}
 	for key := range db.db {
-		inDomain := IsKeyInDomain([]byte(key), start, end, reverse)
+		inDomain := IsKeyInDomain([]byte(key), start, end)
 		if inDomain {
 			keys = append(keys, key)
 		}

libs/db/prefix_db.go

@@ -131,27 +131,13 @@ func (pdb *prefixDB) ReverseIterator(start, end []byte) Iterator {
 	defer pdb.mtx.Unlock()
 
 	var pstart, pend []byte
-	if start == nil {
-		// This may cause the underlying iterator to start with
-		// an item which doesn't start with prefix.  We will skip
-		// that item later in this function. See 'skipOne'.
-		pstart = cpIncr(pdb.prefix)
-	} else {
-		pstart = append(cp(pdb.prefix), start...)
-	}
+	pstart = append(cp(pdb.prefix), start...)
 	if end == nil {
-		// This may cause the underlying iterator to end with an
-		// item which doesn't start with prefix.  The
-		// prefixIterator will terminate iteration
-		// automatically upon detecting this.
-		pend = cpDecr(pdb.prefix)
+		pend = cpIncr(pdb.prefix)
 	} else {
 		pend = append(cp(pdb.prefix), end...)
 	}
 	ritr := pdb.db.ReverseIterator(pstart, pend)
-	if start == nil {
-		skipOne(ritr, cpIncr(pdb.prefix))
-	}
 	return newPrefixIterator(
 		pdb.prefix,
 		start,
@@ -310,7 +296,6 @@ func (itr *prefixIterator) Next() {
 	}
 	itr.source.Next()
 	if !itr.source.Valid() || !bytes.HasPrefix(itr.source.Key(), itr.prefix) {
-		itr.source.Close()
 		itr.valid = false
 		return
 	}
@@ -345,13 +330,3 @@ func stripPrefix(key []byte, prefix []byte) (stripped []byte) {
 	}
 	return key[len(prefix):]
 }
-
-// If the first iterator item is skipKey, then
-// skip it.
-func skipOne(itr Iterator, skipKey []byte) {
-	if itr.Valid() {
-		if bytes.Equal(itr.Key(), skipKey) {
-			itr.Next()
-		}
-	}
-}

libs/db/prefix_db_test.go

@@ -113,8 +113,46 @@ func TestPrefixDBReverseIterator2(t *testing.T) {
 	db := mockDBWithStuff()
 	pdb := NewPrefixDB(db, bz("key"))
 
+	itr := pdb.ReverseIterator(bz(""), nil)
+	checkDomain(t, itr, bz(""), nil)
+	checkItem(t, itr, bz("3"), bz("value3"))
+	checkNext(t, itr, true)
+	checkItem(t, itr, bz("2"), bz("value2"))
+	checkNext(t, itr, true)
+	checkItem(t, itr, bz("1"), bz("value1"))
+	checkNext(t, itr, true)
+	checkItem(t, itr, bz(""), bz("value"))
+	checkNext(t, itr, false)
+	checkInvalid(t, itr)
+	itr.Close()
+}
+
+func TestPrefixDBReverseIterator3(t *testing.T) {
+	db := mockDBWithStuff()
+	pdb := NewPrefixDB(db, bz("key"))
+
 	itr := pdb.ReverseIterator(nil, bz(""))
 	checkDomain(t, itr, nil, bz(""))
+	checkInvalid(t, itr)
+	itr.Close()
+}
+
+func TestPrefixDBReverseIterator4(t *testing.T) {
+	db := mockDBWithStuff()
+	pdb := NewPrefixDB(db, bz("key"))
+
+	itr := pdb.ReverseIterator(bz(""), bz(""))
+	checkDomain(t, itr, bz(""), bz(""))
+	checkInvalid(t, itr)
+	itr.Close()
+}
+
+func TestPrefixDBReverseIterator5(t *testing.T) {
+	db := mockDBWithStuff()
+	pdb := NewPrefixDB(db, bz("key"))
+
+	itr := pdb.ReverseIterator(bz("1"), nil)
+	checkDomain(t, itr, bz("1"), nil)
 	checkItem(t, itr, bz("3"), bz("value3"))
 	checkNext(t, itr, true)
 	checkItem(t, itr, bz("2"), bz("value2"))
@@ -125,23 +163,30 @@ func TestPrefixDBReverseIterator2(t *testing.T) {
 	itr.Close()
 }
 
-func TestPrefixDBReverseIterator3(t *testing.T) {
+func TestPrefixDBReverseIterator6(t *testing.T) {
 	db := mockDBWithStuff()
 	pdb := NewPrefixDB(db, bz("key"))
 
-	itr := pdb.ReverseIterator(bz(""), nil)
-	checkDomain(t, itr, bz(""), nil)
-	checkItem(t, itr, bz(""), bz("value"))
+	itr := pdb.ReverseIterator(bz("2"), nil)
+	checkDomain(t, itr, bz("2"), nil)
+	checkItem(t, itr, bz("3"), bz("value3"))
+	checkNext(t, itr, true)
+	checkItem(t, itr, bz("2"), bz("value2"))
 	checkNext(t, itr, false)
 	checkInvalid(t, itr)
 	itr.Close()
 }
 
-func TestPrefixDBReverseIterator4(t *testing.T) {
+func TestPrefixDBReverseIterator7(t *testing.T) {
 	db := mockDBWithStuff()
 	pdb := NewPrefixDB(db, bz("key"))
 
-	itr := pdb.ReverseIterator(bz(""), bz(""))
+	itr := pdb.ReverseIterator(nil, bz("2"))
+	checkDomain(t, itr, nil, bz("2"))
+	checkItem(t, itr, bz("1"), bz("value1"))
+	checkNext(t, itr, true)
+	checkItem(t, itr, bz(""), bz("value"))
+	checkNext(t, itr, false)
 	checkInvalid(t, itr)
 	itr.Close()
 }

libs/db/types.go

@@ -34,9 +34,9 @@ type DB interface {
 	Iterator(start, end []byte) Iterator
 
 	// Iterate over a domain of keys in descending order. End is exclusive.
-	// Start must be greater than end, or the Iterator is invalid.
-	// If start is nil, iterates from the last/greatest item (inclusive).
-	// If end is nil, iterates up to the first/least item (inclusive).
+	// Start must be less than end, or the Iterator is invalid.
+	// If start is nil, iterates up to the first/least item (inclusive).
+	// If end is nil, iterates from the last/greatest item (inclusive).
 	// CONTRACT: No writes may happen within a domain while an iterator exists over it.
 	// CONTRACT: start, end readonly []byte
 	ReverseIterator(start, end []byte) Iterator

libs/db/util.go

@@ -33,46 +33,13 @@ func cpIncr(bz []byte) (ret []byte) {
 	return nil
 }
 
-// Returns a slice of the same length (big endian)
-// except decremented by one.
-// Returns nil on underflow (e.g. if bz bytes are all 0x00)
-// CONTRACT: len(bz) > 0
-func cpDecr(bz []byte) (ret []byte) {
-	if len(bz) == 0 {
-		panic("cpDecr expects non-zero bz length")
-	}
-	ret = cp(bz)
-	for i := len(bz) - 1; i >= 0; i-- {
-		if ret[i] > byte(0x00) {
-			ret[i]--
-			return
-		}
-		ret[i] = byte(0xFF)
-		if i == 0 {
-			// Underflow
-			return nil
-		}
-	}
-	return nil
-}
-
 // See DB interface documentation for more information.
-func IsKeyInDomain(key, start, end []byte, isReverse bool) bool {
-	if !isReverse {
-		if bytes.Compare(key, start) < 0 {
-			return false
-		}
-		if end != nil && bytes.Compare(end, key) <= 0 {
-			return false
-		}
-		return true
-	} else {
-		if start != nil && bytes.Compare(start, key) < 0 {
-			return false
-		}
-		if end != nil && bytes.Compare(key, end) <= 0 {
-			return false
-		}
-		return true
+func IsKeyInDomain(key, start, end []byte) bool {
+	if bytes.Compare(key, start) < 0 {
+		return false
 	}
+	if end != nil && bytes.Compare(end, key) <= 0 {
+		return false
+	}
+	return true
 }

lite/dbprovider.go

@@ -105,8 +105,8 @@ func (dbp *DBProvider) LatestFullCommit(chainID string, minHeight, maxHeight int
 	}
 
 	itr := dbp.db.ReverseIterator(
-		signedHeaderKey(chainID, maxHeight),
-		signedHeaderKey(chainID, minHeight-1),
+		signedHeaderKey(chainID, minHeight),
+		append(signedHeaderKey(chainID, maxHeight), byte(0x00)),
 	)
 	defer itr.Close()
 
@@ -190,8 +190,8 @@ func (dbp *DBProvider) deleteAfterN(chainID string, after int) error {
 	dbp.logger.Info("DBProvider.deleteAfterN()...", "chainID", chainID, "after", after)
 
 	itr := dbp.db.ReverseIterator(
-		signedHeaderKey(chainID, 1<<63-1),
-		signedHeaderKey(chainID, 0),
+		signedHeaderKey(chainID, 1),
+		append(signedHeaderKey(chainID, 1<<63-1), byte(0x00)),
 	)
 	defer itr.Close()