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

feat(provider): LRUCache Layer #954

Merged
merged 57 commits into from
Oct 8, 2024
Merged

feat(provider): LRUCache Layer #954

merged 57 commits into from
Oct 8, 2024

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Jun 21, 2024

Motivation

Ref: #770

Solution

  • Adds a provider caching layer that dumps cache to fs.
  • Responses are saved as key-value pairs where key is the hash of the request and its params.

Overrides the following RPC methods

  • get_block_by_number
  • get_block_by_hash
  • get_block_receipts
  • get_proof
  • get_storage_at
  • get_code_at
  • get_transaction_count
  • get_logs
  • get_transaction_by_hash
  • get_raw_transaction_by_hash
  • get_transaction_receipt

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya mentioned this pull request Jun 27, 2024
10 tasks
@yash-atreya yash-atreya changed the title feat(providers): LRUCache Layer feat(transport): LRUCache Layer Aug 5, 2024
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

some suggestions

crates/transport/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/transport/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/transport/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/transport/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/transport/src/layers/cache.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya marked this pull request as ready for review August 6, 2024 11:49
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

some questions

crates/transport/Cargo.toml Outdated Show resolved Hide resolved
crates/provider/src/provider/trait.rs Outdated Show resolved Hide resolved
crates/transport/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
) -> TransportResult<Option<Block>> {
let hash = RequestType::new("eth_getBlockByNumber", (number, hydrate));

cache_get_or_fetch!(self, hash, self.inner.get_block_by_number(number, hydrate))
Copy link
Member

Choose a reason for hiding this comment

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

this is problematic because of reorgs, we'd need to track chain height somehow, but out of scope for this pr imo

crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

almost there

few more suggestions

Cargo.toml Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya enabled auto-merge (squash) October 5, 2024 16:03
@yash-atreya
Copy link
Member Author

@mattsse addressed the suggestions

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

last suggestions

let req = RequestType::new("eth_getBlockReceipts", (block,));

let redirect =
!matches!(block, BlockId::Hash(_) | BlockId::Number(BlockNumberOrTag::Number(_)));
Copy link
Member

Choose a reason for hiding this comment

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

the number case is also slightly problematic, but we can worry about this separately

async fn get_logs(&self, filter: &Filter) -> TransportResult<Vec<Log>> {
let req = RequestType::new("eth_getLogs", filter.clone());

let params_hash = req.params_hash().ok();
Copy link
Member

Choose a reason for hiding this comment

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

this is also a bit nuanced because the blockrange variants allows the server to chose the range for example if upper bound is missing

crates/provider/src/layers/cache.rs Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
crates/provider/src/layers/cache.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya merged commit 1decf94 into main Oct 8, 2024
26 checks passed
@yash-atreya yash-atreya deleted the yash/provider-cache branch October 8, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

2 participants