feat!: Iterator Key() and Value() no longer return a copy (#168)

* updated the Iterator interface docs for methods Key() and Value()

* changes in goLevelDBIterator Key() impl:
- returns the key itself, not a copy.
- updated docs to reflect the change

* changes in badgerDBIterator Key() implementation
- returns the key itself, not a copy.
- updated docs to reflect the change

* changes in pebbleDBIterator Key() implementation
- returns the key itself, not a copy.
- updated docs to reflect the change.

* updated backend tests:
To reflect the changes, the tests now copy the iterator's key for further use

* changes in goLevelDBIterator Value() impl:
- returns the value itself, not a copy
- updated tests to reflect the change
- updated docs to reflect the change

* updated docs of badgerdb Key() and Value() APIs implementations.

* changes in pebbleDBIterator Value() impl:
- returns the value itself, not a copy
- updated docs to reflect the change

* updated the Iterator interface docs for methods Key() and Value()

* updated badgerDBIterator docs for the Value() method

* updated badgerdb Value(), and rocksdb Key() and Value() API docs

* added changelog entry

.changelog/v0.13.0/features/168-iter-key.md

@@ -0,0 +1,2 @@
+- Iterator Key and Value APIs now return an object that must be copied before
+  use ([\#168](https://github.com/cometbft/cometbft-db/pull/168))

.changelog/v0.13.0/summary.md

@@ -0,0 +1,7 @@
+<!--
+    Add a summary for the release here.
+
+    If you don't change this message, or if this file is empty, the release
+    will not be created. -->
+This release changes the contract of the Iterator Key() and Value() APIs.
+Namely, the caller is now responsible for creating a copy of their returned value if they want to modify it.

backend_test.go

@@ -348,7 +348,8 @@ func verifyIterator(t *testing.T, itr Iterator, expected []int64, msg string) {
 
 	var list []int64
 	for itr.Valid() {
-		key := itr.Key()
+		key := make([]byte, len(itr.Key()))
+		copy(key, itr.Key())
 		list = append(list, bytes2Int64(key))
 		itr.Next()
 	}
@@ -450,7 +451,10 @@ func assertKeyValues(t *testing.T, db DB, expect map[string][]byte) {
 	actual := make(map[string][]byte)
 	for ; iter.Valid(); iter.Next() {
 		require.NoError(t, iter.Error())
-		actual[string(iter.Key())] = iter.Value()
+
+		value := make([]byte, len(iter.Value()))
+		copy(value, iter.Value())
+		actual[string(iter.Key())] = value
 	}
 
 	assert.Equal(t, expect, actual)

badger_db.go

@@ -279,19 +279,23 @@ func (i *badgerDBIterator) Valid() bool {
 	return true
 }
 
+// Key implements Iterator.
+// The caller should not modify the contents of the returned slice.
+// Instead, the caller should make a copy and work on the copy.
 func (i *badgerDBIterator) Key() []byte {
 	if !i.Valid() {
 		panic("iterator is invalid")
 	}
-	// Note that we don't use KeyCopy, so this is only valid until the next
-	// call to Next.
-	return i.iter.Item().KeyCopy(nil)
+	return i.iter.Item().Key()
 }
 
+// Value implements Iterator.
+// The returned slice is a copy of the original data, therefore it is safe to modify.
 func (i *badgerDBIterator) Value() []byte {
 	if !i.Valid() {
 		panic("iterator is invalid")
 	}
+
 	val, err := i.iter.Item().ValueCopy(nil)
 	if err != nil {
 		i.lastErr = err

common_test.go

@@ -72,7 +72,9 @@ func checkKeyPanics(t *testing.T, itr Iterator) {
 
 func checkValuePanics(t *testing.T, itr Iterator) {
 	t.Helper()
-	assert.Panics(t, func() { itr.Value() })
+
+	msg := "checkValuePanics expected panic but didn't"
+	assert.Panics(t, func() { itr.Value() }, msg)
 }
 
 func newTempDB(t *testing.T, backend BackendType) (db DB, dbDir string) {

goleveldb_iterator.go

@@ -93,19 +93,19 @@ func (itr *goLevelDBIterator) Valid() bool {
 }
 
 // Key implements Iterator.
+// The caller should not modify the contents of the returned slice.
+// Instead, the caller should make a copy and work on the copy.
 func (itr *goLevelDBIterator) Key() []byte {
-	// Key returns a copy of the current key.
-	// See https://github.com/syndtr/goleveldb/blob/52c212e6c196a1404ea59592d3f1c227c9f034b2/leveldb/iterator/iter.go#L88
 	itr.assertIsValid()
-	return cp(itr.source.Key())
+	return itr.source.Key()
 }
 
 // Value implements Iterator.
+// The caller should not modify the contents of the returned slice.
+// Instead, the caller should make a copy and work on the copy.
 func (itr *goLevelDBIterator) Value() []byte {
-	// Value returns a copy of the current value.
-	// See https://github.com/syndtr/goleveldb/blob/52c212e6c196a1404ea59592d3f1c227c9f034b2/leveldb/iterator/iter.go#L88
 	itr.assertIsValid()
-	return cp(itr.source.Value())
+	return itr.source.Value()
 }
 
 // Next implements Iterator.

pebble.go

@@ -396,19 +396,19 @@ func (itr *pebbleDBIterator) Valid() bool {
 }
 
 // Key implements Iterator.
+// The caller should not modify the contents of the returned slice.
+// Instead, the caller should make a copy and work on the copy.
 func (itr *pebbleDBIterator) Key() []byte {
-	// Key returns a copy of the current key.
-	// See https://github.com/cockroachdb/pebble/blob/v1.0.0/iterator.go#L2106
 	itr.assertIsValid()
-	return cp(itr.source.Key())
+	return itr.source.Key()
 }
 
 // Value implements Iterator.
+// The caller should not modify the contents of the returned slice.
+// Instead, the caller should make a copy and work on the copy.
 func (itr *pebbleDBIterator) Value() []byte {
-	// Value returns a copy of the current value.
-	// See https://github.com/cockroachdb/pebble/blob/v1.0.0/iterator.go#L2116
 	itr.assertIsValid()
-	return cp(itr.source.Value())
+	return itr.source.Value()
 }
 
 // Next implements Iterator.

rocksdb_iterator.go

@@ -94,12 +94,14 @@ func (itr *rocksDBIterator) Valid() bool {
 }
 
 // Key implements Iterator.
+// The returned slice is a copy of the original data, therefore it is safe to modify.
 func (itr *rocksDBIterator) Key() []byte {
 	itr.assertIsValid()
 	return moveSliceToBytes(itr.source.Key())
 }
 
 // Value implements Iterator.
+// The returned slice is a copy of the original data, therefore it is safe to modify.
 func (itr *rocksDBIterator) Value() []byte {
 	itr.assertIsValid()
 	return moveSliceToBytes(itr.source.Value())

types.go

@@ -136,11 +136,17 @@ type Iterator interface {
 	Next()
 
 	// Key returns the key at the current position. Panics if the iterator is invalid.
-	// CONTRACT: key readonly []byte
+	// Key returns the key of the current key/value pair, or nil if done.
+	// The caller should not modify the contents of the returned slice, and
+	// its contents may change on the next call to any 'seeks method'.
+	// Instead, the caller should make a copy and work on the copy.
 	Key() (key []byte)
 
 	// Value returns the value at the current position. Panics if the iterator is invalid.
-	// CONTRACT: value readonly []byte
+	// Value returns the value of the current key/value pair, or nil if done.
+	// The caller should not modify the contents of the returned slice, and
+	// its contents may change on the next call to any 'seeks method'.
+	// Instead, the caller should make a copy and work on the copy.
 	Value() (value []byte)
 
 	// Error returns the last error encountered by the iterator, if any.