Cache only correct eth_call data #4879
Conversation
| }); | ||
| result.0 | ||
| }), | ||
| .map(move |result| result.0), |
There was a problem hiding this comment.
I feel like this could take a bit of refactoring, but I'm not sure it's possible to make it more nice looking
| @@ -1276,7 +1276,8 @@ impl EthereumAdapterTrait for EthereumAdapter { | |||
| "block_hash" => call.block_ptr.hash_hex(), | |||
| "block_number" => call.block_ptr.block_number() | |||
| ); | |||
|
|
|||
| let cache_logger = logger.clone(); | |||
| let cache = cache.clone(); | |||
There was a problem hiding this comment.
I would like to move it to inner scope, but I cannot wrap my head on how to correctly clone it in and_then future.
| EthereumContractCallError::Revert(format!("failed to decode output: {}", e)) | ||
| }) | ||
| }); | ||
| let _ = graph::spawn_blocking_allow_panic(move || { |
There was a problem hiding this comment.
I think I could move it to dedicated and_then future using https://docs.rs/futures-async-combinators/latest/futures_async_combinators/future/fn.and_then.html
But I didn't see it usage in the graph so I'm hesitant to add this
There was a problem hiding this comment.
First off, congrats on making it through this code - it's really gnarly and would hugely benefit from asyncifying (though that might be quite a bit of an undertaking)
Before merging this, we need to find a way to not call set_call for calls that are already in the cache as that will have a bad effect on performance.
| "error" => e.to_string()) | ||
| }) | ||
| }); | ||
| return result; |
There was a problem hiding this comment.
As I understand it, this will always call set_call, even if we got the result from the cache, i.e., it's not necessary to cache it again. Why don't you just put the spawn_blocking_allow_panic in the existing code in the None branch into an if !result.0.is_empty() { .. } ? That will suppress caching these calls and the error will still be generated later in the and_then
|
hey @SozinM, thanks so much for this PR! Did you manage to take a look at David's comments? |
|
Hi! |
SozinM
force-pushed
the
bug/call_cache
branch
from
eef8dd1
to
5845d6c
Compare
This PR closes #4756
It changes caching logic to cache only correct and decodable eth_calls to prevent cache from providing faulty data