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

[alesforz]

Context

Closes #156.

Changes

The:

• Iterator.Key() API implementations of levelDB, badgerDB, and pebbleDB no longer return a copy of the key.
• Iterator.Value() API implementations of levelDB and pebbleDB no longer return a copy of the value.

Rather, as the updated docs reflect, the caller is responsible for creating a copy of the slice if they want to modify it.

See my further comments about rocksdb and badgerdb below.

[alesforz]

Unfortunately, I don't think we can avoid the copy in rocksdb Key() and Value() APIs implementations.
Under the hood, the library we use allocates and returns a C array, which is subject to C's memory resource policy.
But Go's memory management can't keep track of any memory allocated in the C library.
Therefore, to ensure there are no memory leaks, we call Free() to deallocate the C array's allocated memory.

In short, my understanding is that we need the copy of the key/value in order to free its memory allocated in the C library, thus avoiding possible memory leaks.

[alesforz]

We can't avoid the copy in the Value() API of badgerDB either.
Namely, badgerDB exposes 2 APIs to retrieve a value:

func (item *Item) Value(fn func(val []byte) error) error
func (item *Item) ValueCopy(dst []byte) ([]byte, error)

Note:

• Item.Value does not return the value. Instead, it wants a closure that works on it directly inside the library's boundaries. The Value method will call fn passing the value we want to retrieve as an argument []byte. To extract it we must do something like:

  var valueCopy []byte
  err = item.Value(func(val []byte) error {
      // Make a copy of the value.
      valueCopy = append([]byte{}, val...)
      return nil
  })

  That is, we have to manually copy the value.
• Item.ValueCopy creates a copy for us and returns it.

In both cases, the library returns as a copy of the original value; as far as I can tell, it offers no possibility of accessing the original value directly.

[alesforz]

Given the discussion above, addressing #168 makes the library behave inconsistently depending on the underlying database used. Namely, some databases require callers to make copies of key and value, while others do not. Consequently, users cannot switch between databases without first checking the behavior of their Key() and Value() APIs and adapting their code accordingly.

[melekes]

looks good so far 👍

[melekes]

Suggested change

// Key implements Iterator.

I'd remove these comments.

[alesforz]

🤔 I would leave them, because I think they give readers some additional context about the functions.
Granted, in the context of this repo it's clear what the functions refer to.
Still, I would argue it doesn't hurt to leave extra-info in.

[melekes]

Note: this is a breaking change.

That's debatable because, in the Iterator documentation we say read-only []byte, although we don't mention anything about the lifetime of the pointer.

[alesforz]

Note: this is a breaking change.

That's debatable because, in the Iterator documentation we say read-only []byte, although we don't mention anything about the lifetime of the pointer.

I see your point.
The Iterator interface docs states:

// As with DB, keys and values should be considered read-only, and must be copied before they are
// modified.

therefore, users of the library know that they must create a copy, thus these changes shouldn't break anything.

[melekes]

👍

[melekes]

Could you add a changelog entry? btw we're using https://github.com/informalsystems/unclog