Fix reverse iterator in RocksDB

[AurelienFT]

Linked Issues/PRs

Closes #2044

Description

Problem analysis

Our database columns use the prefix extractor configuration which allow rocksdb to make optimizations on storage based on prefix. However, this breaks the lexicographic order between prefixs and we need to pass a special option to the read iterator to bypass the "prefix sharding" (source: https://github.com/facebook/rocksdb/wiki/Prefix-Seek/3a5e28faf6c2d0ec1bdb99763043e1e3322007e9#how-to-ignore-prefix-bloom-filters-in-read).

Current solution

I took the same approach as before and started iteration from the next prefix using the bypass read option argument.

However I change the behavior to make only iterator to simplify and optimize the solution.

Better solution for the future

I think that breaking the "prefix sharding" from RocksDB to iterate can make the iterator way more costly.

There is a way to avoid it because we can iterate in reverse order inside a prefix (source: https://github.com/facebook/rocksdb/wiki/SeekForPrev) but we need to get the maximum value for a key in a given prefix to use it as a starting point of our iteration which is not possible to get in the current code because we don't have any information about the key.

Happy to here your thoughts @FuelLabs/client :)

EDIT: Not done : #2405

Checklist

• Breaking changes are clearly marked as such in the PR description and changelog
• New behavior is reflected in tests
• The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

• I have reviewed the code myself
• I have created follow-up issues caused by this PR and linked them here

[xgreenx]

It would be nice to have tests for reverse iteration with prefix. If can use examples from the mainnet, would be nice

[AurelienFT]

@xgreenx I have added a test at rocksdb level and integration level.

[netrome]

Nice stuff. Thanks for spotting and fixing this!

[xgreenx]

It seems that final solution doesn't match the description of the PR=D

[xgreenx]

I think the current solution is more elegant. But it's still not clear to me why the old one was not working

[xgreenx]

Ohh, I see. We used iter_store with the next prefix, not the key, and because of that, if it returns None, we will fall back into the End branch.

[AurelienFT]

@xgreenx Yes the description had one sentence that was outdated, I removed.
It was failing before, essentially because we wasn't using the read option total_order_seek that is needed if you want to iterate correctly from outside of a prefix befined in column creation. We could change to not use this option anymore with my futur proposition.

[xgreenx]

All tests are passing even without total_order_seek, I think the problem were in the old logic=D

BTW, documentation says that it is better to use auto_prefix_mode

[AurelienFT]

The auto_prefix_mode doesn't seems to exists in rust-rocksdb (rust-rocksdb/rust-rocksdb#624).

You are right all the tests are passing but the test on the playground on the mainnet data isn't working without total_order_seek. I think the unit test it's because they are not using a column with prefix set and the integration test maybe don't have enough addresses defined.
Will try to change some tests to fail without total_order_seek

[AurelienFT]

I don't find a way to make one of the integration or unit test fails :/

[xgreenx]

You are right all the tests are passing but the test on the playground on the mainnet data isn't working without total_order_seek

So, did I get right that If you use the current fix but without total_order_seek, the query from the issue will fail?

[AurelienFT]

@xgreenx yes.