From 416d143bf72237c026cc289e5505ad943b88944b Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Wed, 28 Nov 2018 22:49:24 +0900 Subject: [PATCH] R4R: Swap start/end in ReverseIterator (#2913) * Swap start/end in ReverseIterator * update CHANGELOG_PENDING * fixes from review --- CHANGELOG_PENDING.md | 2 ++ libs/db/backend_test.go | 26 +++++++++--------- libs/db/c_level_db.go | 11 ++++---- libs/db/fsdb.go | 6 ++--- libs/db/go_level_db.go | 11 ++++---- libs/db/mem_db.go | 2 +- libs/db/prefix_db.go | 29 ++------------------ libs/db/prefix_db_test.go | 57 ++++++++++++++++++++++++++++++++++----- libs/db/types.go | 6 ++--- libs/db/util.go | 47 +++++--------------------------- lite/dbprovider.go | 8 +++--- 11 files changed, 98 insertions(+), 107 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 2a2626a4f..dfa2c1ce5 100644 --- a/CHANGELOG_PENDING.md +++ b/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 diff --git a/libs/db/backend_test.go b/libs/db/backend_test.go index 2aebde1c6..fb2a3d0b3 100644 --- a/libs/db/backend_test.go +++ b/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") } diff --git a/libs/db/c_level_db.go b/libs/db/c_level_db.go index 307461261..7f74b2a71 100644 --- a/libs/db/c_level_db.go +++ b/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 } diff --git a/libs/db/fsdb.go b/libs/db/fsdb.go index b1d40c7b4..2d82e7749 100644 --- a/libs/db/fsdb.go +++ b/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)) } } diff --git a/libs/db/go_level_db.go b/libs/db/go_level_db.go index 8a4887921..fd487a4dd 100644 --- a/libs/db/go_level_db.go +++ b/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 } diff --git a/libs/db/mem_db.go b/libs/db/mem_db.go index 580123017..ff516bc7d 100644 --- a/libs/db/mem_db.go +++ b/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) } diff --git a/libs/db/prefix_db.go b/libs/db/prefix_db.go index 9dc4ee97d..40d72560c 100644 --- a/libs/db/prefix_db.go +++ b/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() - } - } -} diff --git a/libs/db/prefix_db_test.go b/libs/db/prefix_db_test.go index 60809f157..e3e37c7d1 100644 --- a/libs/db/prefix_db_test.go +++ b/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() } diff --git a/libs/db/types.go b/libs/db/types.go index ad78859a7..9b9c6d0b9 100644 --- a/libs/db/types.go +++ b/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 diff --git a/libs/db/util.go b/libs/db/util.go index 51277ac42..e927c3548 100644 --- a/libs/db/util.go +++ b/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 } diff --git a/lite/dbprovider.go b/lite/dbprovider.go index e0c4e65b4..9f4b264f7 100644 --- a/lite/dbprovider.go +++ b/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()