Why are Iterate/IterateRange and IterateRangeInclusive expecting different callback functions

[UnitylChaos]

Iterate/IterateRange expect a function from (key, value) to boolean, while IterateRangeInclusive expects a function from (key, value, version) to boolean.

[UnitylChaos]

It looks like the IterateRangeInclusive function isn't being called from anywhere in here, SDK or Burrow. So I think we would be fine to simply change it's callback signature to be in line with the other iterators. We could also go the other way and make the other iterators expect a version parameter, but that would require small changes to downstream software.

[silasdavis]

I think it would be a good idea if we standardised our iteration to use the same style as libs/db and the cosmos-sdk.

The cosmos-sdk defines KVStore: https://github.com/cosmos/cosmos-sdk/blob/develop/types/store.go#L121-L164. This is okay, but I would like something more minimal embedded in that interface that we can use, and also one that avoids unnecessarily coupling us to named types in the cosmos sdk. I have defined a subset interface: https://github.com/silasdavis/burrow/blob/state/storage/kvstore.go:

// KVStore is a simple interface to get/set data
type KVStore interface {
	// Get returns nil iff key doesn't exist. Panics on nil key.
	Get(key []byte) []byte

	// Has checks if a key exists. Panics on nil key.
	Has(key []byte) bool

	// Set sets the key. Panics on nil key.
	Set(key, value []byte)

	// Delete deletes the key. Panics on nil key.
	Delete(key []byte)

	// Iterator over a domain of keys in ascending order. End is exclusive.
	// Start must be less than end, or the Iterator is invalid.
	// Iterator must be closed by caller.
	// To iterate over entire domain, use store.Iterator(nil, nil)
	// CONTRACT: No writes may happen within a domain while an iterator exists over it.
	Iterator(start, end []byte) dbm.Iterator

	// Iterator over a domain of keys in descending order. End is exclusive.
	// Start must be greater than end, or the Iterator is invalid.
	// Iterator must be closed by caller.
	// CONTRACT: No writes may happen within a domain while an iterator exists over it.
	ReverseIterator(start, end []byte) dbm.Iterator
}

The advantage of this interface is it avoids self-reference so you could implement it with just depending on db and not cosmos. As well as generally not including unnecessary methods for our purposes. We would need to implement Has and rename Remove to Delete...

In particular I think we should switch to the stateful iterator exposed by db.