Why do we create this itInvalid object a few lines above? Since it's an invalid iterator, any method call on it will cause a panic, thus making it useless. Why not return an error here instead?

I think we should return an error

After some testing, I now understand why itInvalid is created.
The current design allows the creation of an Iterator with the key range [nil:nil], which we interpret as "from the first to the last key in the database." However, there is a corner case: creating an Iterator with the range [nil:some_key] when there is only one key in the database (or only one key with the given prefix, i.e., prefix+some_key). Because the iterator's upper bound is exclusive, this results in an empty key range. There might be other corner cases that I haven't found, though.

In the case of Pebble (and possibly other databases), such a scenario leads to an invalid iterator state. According to Pebble's documentation, this situation produces an iterator that "has a non-exhausted internalIterator, but has reached a limit without any key for the caller." In other words, a call to Valid() will fail, and therefore the creation of the Iterator itself will fail.

I think cometbft’s code depends on newPrefixDBIterator returning an invalid iterator rather than an error. For example, in the evidence package, returning an invalid iterator that returns false on the first call to iterator.Valid() allows the related functions to proceed normally. They end up creating an empty Evidence slice, which cometbft interprets as "no evidence to broadcast." If we returned an error instead, these functions would fail, breaking the process.

This design is not ideal, but changing it would likely require revisiting a large portion of cometbft’s codebase.

Is the purpose of these if statements to double-check that the current key of the wrapped Iterator in prefixDBIterator is correct?

We already call Iterator.Valid on the wrapped iterator to check the validity of the key. Assuming a prefixDBIterator is correctly initialized with the key range [prefix|start_key : prefix|end_key], in what scenarios are these additional checks needed?

For example, consider our implementation of the Pebble iterator pebbleDBIterator. Its Valid() method ensures that the iterator's current key is between start and end. Now, suppose we create a prefixDBIterator wrapping a pebbleDBIterator with prefix foo, start key a, and end key f. The key range of this iterator will be [fooa : fooe] (since the end is exclusive, foof won't be included). If I understand correctly, a call to pebbleDBIterator.Valid() should be sufficient to verify that the current key is valid, as any key outside [fooa : fooe] will be considered invalid.

Therefore, it seems that checking the prefix again in the prefixDBIterator methods might be unnecessary.

Returning to my initial question: Are we performing these additional checks to guard against third-party Iterator implementations that might not strictly enforce key validity and could return keys we should consider invalid? If not, what am I missing?

Agree that it's not needed.

After some thinking, we have decided to leave these checks in place. It is very likely that the reason they are there is to guard against third-party Iterator implementations that might not strictly enforce key validity and could return keys we should consider invalid. Changing this requires some in-depth testing to make sure we aren't screwing up validation checks on the keys. We, unfortunately, don't have enough time to do it at the moment.

I think we should return an error

Agree that it's not needed.

I think we should rethink the policy of ignoring the case when the key is not found. We'll discuss synchronously, but as we are now using only one DB we can check whether this still makes sense.

We are already ignoring that case. If pebble returns a pebble.ErrNotFound error, we ignore it and return an empty slice. CometBFT's code always checks if a len(returnedSlice) == 0 to see if the key was found.