madara icon indicating copy to clipboard operation
madara copied to clipboard

RPC cache system

Open tchataigner opened this issue 1 year ago • 3 comments

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Feature

What is the current behavior?

In the current implementation of the madara client, we fetch block data by going through the generated logs of our pallet. As the block data is important and we need fast retrieval, it seems important to:

  • Properly store the block data in a dedicated StorageValue in our pallet
  • When possible retrieve the block data through storage query instead of digest API
  • Add a cache layer on top of it to ease retrieval of recurrent queried data

Resolves: #947

What is the new behavior?

Right now, the update to the code base are as follow:

  • Added a CurrentBlock type in the starknet pallet, representing a StorageValue containing our block data. Also added a getter to retrieve it through our runtime API.
  • Added necessary method over our storage implementation to both retrieve the data through the runtime API and directly from our storage.
  • Implemented a base LRUCache limited by allocated memory, along with an asynchronous task responsible for the handling of the cache and retrieving data.
  • Updated the necessary calls in our RPC implementation, to use the cache task instead of directly retrieving the block data through our logs.
  • Added an argument over the run command to set the allocated memory size limit for the cache.

The new flow is:

  1. Run a madara run while specifying --starknet-log-block-cache-size. For now, the default value is 200 (most likely will cache little if not nothing)
  2. On RPC call, we will send the requested block hash to our data cache task.
  3. If the data cache has the block, returns it.
  4. Otherwise, we will directly query our storage for it.

Does this introduce a breaking change?

No

Other information

Original issue: #947

tchataigner avatar Sep 10 '23 20:09 tchataigner

Codecov Report

Patch coverage: 15.93% and project coverage change: -0.35% :warning:

Comparison is base (a6f6554) 40.71% compared to head (b63d1a8) 40.36%. Report is 1 commits behind head on main.

:exclamation: Current head b63d1a8 differs from pull request most recent head 59b7c99. Consider uploading reports for the commit 59b7c99 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1083      +/-   ##
==========================================
- Coverage   40.71%   40.36%   -0.35%     
==========================================
  Files          96       98       +2     
  Lines       11069    11281     +212     
  Branches    11069    11281     +212     
==========================================
+ Hits         4507     4554      +47     
- Misses       6050     6209     +159     
- Partials      512      518       +6     
Files Changed Coverage Δ
crates/client/rpc-core/src/lib.rs 0.00% <ø> (ø)
crates/client/rpc/src/cache/mod.rs 0.00% <0.00%> (ø)
crates/client/rpc/src/events/mod.rs 22.22% <0.00%> (-0.20%) :arrow_down:
crates/client/rpc/src/lib.rs 0.00% <0.00%> (ø)
crates/client/rpc/src/madara_backend_client.rs 0.00% <ø> (ø)
crates/client/storage/src/lib.rs 0.00% <ø> (ø)
crates/client/storage/src/overrides/mod.rs 0.00% <0.00%> (ø)
...client/storage/src/overrides/schema_v1_override.rs 0.00% <0.00%> (ø)
crates/node/src/cli.rs 0.00% <0.00%> (ø)
crates/node/src/command.rs 0.00% <0.00%> (ø)
... and 7 more

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 11 '23 08:09 codecov[bot]

Hey @tdelabro, here is a first draft for the RPC cache.

As we discussed under the issue, I've implemented the cache to work with allocated memory size instead of values size to make it more precise.

Right now, it seems to fail integration test, which is strange as it passes on my computer. I'll look into it.

The only problem that I have is that I think the benchmark tests to evaluate the cache efficiency are not there. I wanted to add some to try and measure it but I'm having trouble in identifying how to pull that off.

Could I ask for your input on that ? Ideally, I think that we want to try and spam a request on loop to our RPC methods that concern block fetch (e.g.: getblockWithTxHashes)

Also, I'd be interested in your opinion of the choice I made to implement a new StorageValue in pallet-starknet. Does it seem like a fair choice to you?

tchataigner avatar Sep 11 '23 09:09 tchataigner

There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. This PR will be closed and locked in 7 days if no further activity occurs. Thank you for your contributions!

github-actions[bot] avatar Oct 13 '23 00:10 github-actions[bot]

repository archived in favor of https://github.com/madara-alliance/madara

tdelabro avatar Aug 02 '24 18:08 tdelabro