Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add persistent top-down finality cache #897

Open
wants to merge 6 commits into
base: external-materializer
Choose a base branch
from

Conversation

fridrik01
Copy link
Contributor

@fridrik01 fridrik01 commented May 7, 2024

This PR replaces the current in-memory finality cache with a CacheStore implementation that is stored in a finality_cache namespace in Rocksdb.

The PR also introduces a new CLI for deleting all the key/values in the CacheStore:

fendermint debug delete-cache-store --data-dir=/home/fridrik/.fendermint/data

Implementation details:

  • The CacheStore implementation is not generic as suggested in the approach suggested here. After battling with that approach I ended up just making it simple and non-generic as there is also not really a need for it being generic (though it would be nice).

@fridrik01 fridrik01 force-pushed the cache-store-hack branch 2 times, most recently from bccc236 to 0454d84 Compare May 8, 2024 13:23
@fridrik01 fridrik01 requested a review from a team May 8, 2024 13:27
@fridrik01 fridrik01 marked this pull request as ready for review May 8, 2024 13:36
@fridrik01 fridrik01 force-pushed the external-materializer branch from 2df08f9 to f0355f4 Compare May 8, 2024 13:53
@fridrik01 fridrik01 force-pushed the cache-store-hack branch from 0454d84 to 8760091 Compare May 8, 2024 13:54
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a shame you gave up on trying to work with the abstractions in the storage crate, the ability to iterate over keys would have been a nice addition.

What is problematic is the combination of persistent database operations which do not participate in transactions with Stm. STM transactions can be retried if there is a version conflict, which would cause something like the append operation that already inserted and flushed the data into RocksDB to fail.

This is why the STM library contains the atomically_or_err_aux function which takes a factory function for an Aux interface, which represents a database transaction that gets committed if the STM transaction can successfully be committed itself, and rolled back otherwise.

This can be used to keep the DB and the in-memory data structures in sync with each other. The RocksDB implementations of storage abstractions are all executed in transactions, so implementing Aux for them is entirely possible.

If I had to do this I would have used the RocksDB backend in a different way. Instead of re-implementing the whole cache with all its operations in terms of RocksDB operations, I would have turned it into a log of what happened to the in-memory cache, and kept the in-memory one serving the queries while it's alive. As I understand the persistence is only required so data isn't lost in restarts, so it would be enough to 1) initialise the in-memory cache at start and 2) do inserts and deletions when data is removed from it.

That can even be achieved with the current get/put/delete operations, although there is no doubt that iteration would be a boon.

If the storage abstractions aren't used so the data can be committed together in-memory and in-database, the operations to maintain the state of the database could be done after the STM transaction is committed, e.g. let ops = atomically(...); cache.update(ops);. The Aux stuff was added to avoid this, although there are some rough edges around how to deal with errors in that context.

@aakoshh
Copy link
Contributor

aakoshh commented May 9, 2024

I gave using KVStore a try on the https://github.com/consensus-shipyard/ipc/tree/cache-store-generic branch to see what the issue is with the compiler. It's a lot of new generic constraints for sure, but in the end where I got stuck is that the RocksDbWriteTx implementation is not Send and Sync, and the compiler insists that it should be. I know that it's not used across await by STM, but the compiler doesn't seem to recognise this.

I am not sure how this can be resolved :(

@aakoshh
Copy link
Contributor

aakoshh commented May 9, 2024

I think I managed to get around it with aakoshh/async-stm-rs@113be84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants