Fix seek in TrieDB iterator

[ngotchac]

It seems to me that the behavior of seek on a TrieDB iterator was broken, looking at the tests.
Seeking to "A" which is in the Trie, calling next should return "A" and not the next value.
This was particularly broken when in a trie with [16, 8] and [16, 16], seeking to [16, 0] would start the iterator at [16, 16].

[ordian]

Looking at the TrieIterator docs, it says >:

parity-common/patricia_trie/src/lib.rs

Line 205 in 4481687

/// Position the iterator on the first element with key > `key`

So, I'm not sure whether it was a typo in the TrieDBIterator docs or the actual bug.

[ngotchac]

Hm, yeah there are two contradicting comments... IMO >= makes the most sense.

[pepyakin]

Nice find @ngotchac ! I think that not a bug, since the comment found by @ordian and tests are good evidences of otherwise, and this is more of a documentation issue.

And I agree with you that >= makes more sense. If you don't need an element that stricly equals to the one that passed into seek as an argument then you can freely skip it. On the other hand, you can't just go back one item with this iterator.

UPDATE

To clarify: there is actually code in polkadot that relies on >=

parity-common/patricia_trie/src/triedb.rs

Line 338 in 4481687

/// Position the iterator on the first element with key >= `key`

The use-case is to delete a range of entries keys of which share the given common prefix. E.g. the trie contains the following keys:

cat
dog
dogger
doggest
shark

If I deleted keys with prefix dog I would expected that all dog, dogger and the doggest would have been deleted. But with > the value with the key dog would have been skipped.

[ngotchac]

I fully agree with you @pepyakin ! And actually the current implementation is still broken with > running my example described above.
(I updated the lib.rs comment to reflect that)

[dvdplm]

I agree that >= is the more natural logic, but I am worried about breakage in code relying on the == behavior. Did you review other code in parity-ethereum that calls seek? If yes, lgtm.

[ngotchac]

So the only uses of seek in parity-ethereum are in list_accounts and list_storage methods of ethcore/src/client/client.rs, which are only used in execute_export_state in parity/blockchain.rs. Updating to the new implementation is trivial, but I'm not sure how to make atomic PRs :/

[ordian]

LGTM, although I don't fully understand the implementation.