MetaMask / eth-json-rpc-middleware

Ethereum middleware for composing an Ethereum provider using json-rpc-engine. Intended to replace provider-engine
ISC License
180 stars 88 forks source link

Some Infura RPC methods should be cached but are not #143

Open mcmire opened 2 years ago

mcmire commented 2 years ago

The following RPC methods take block parameters and yet are not covered in blockTagParamIndex and so are not being cached the same as other RPC methods which take block parameters:

mcmire commented 2 years ago

Look into eth_getUncleByBlockNumberAndIndex and eth_getUncleCountByBlockNumber — how are these computed and are these cacheable?

mcmire commented 2 years ago

I did some research and it looks like when a block is attested to be a part of the canonical chain, if there are any other competing blocks that should be marked as uncles, those are associated with the canonical block. For instance, see this block which became an uncle after this block became the canonical (the uncle is listed on the block page).

Interestingly, according to the Ethereum docs, uncles (ommers) are not possible in Ethereum 2.0. In fact, according to Etherscan, the last uncle was 18 days ago. So caching uncle-related RPC methods isn't going to be as important in the future as it is now.

In any case, in terms of whether these methods are safe to cache in the first place, my thought is that the possibility that a block that was once canonical then can be an uncle now if the network says so is a consequence of the overall ephemeralness of Ethereum that affects all kinds of blocks, not just uncle blocks. In other words, if we think that eth_getUncleByBlockNumberAndIndex is unreliable because the information this method returns may change depending on when we call it, I feel like the same thing would apply for eth_getBlockByNumber, because the block in question may change if the block gets replaced in the canonical chain. But it seems like in allowing eth_getBlockByNumber to be cached, we assume that this will never happen. So we ought to apply the same assumption to eth_getUncleByBlockNumberAndIndex and eth_getUncleCountByBlockNumber and go ahead and cache them.

Or, we ignore these methods and only cache eth_getBlockTransactionCountByNumber and eth_getTransactionByBlockNumberAndIndex, as it is increasingly likely that the uncle-related methods will no longer be used.

Thoughts?

Gudahtt commented 2 years ago

Interesting; I hadn't realized this changed post-merge. I guess it's still an issue for PoW chains though.

Ideally we would detect chain reorganizations and refresh any affected caches, across the board. Until then, chain reorganizations might be very confusing.

In the meantime, I'm unsure whether caching these would make these situations more confusing or not. Maybe better to err on the side of caution and leave them uncached for now.

mcmire commented 2 years ago

Okay, so I'll just focus on caching eth_getBlockTransactionCountByNumber and eth_getTransactionByBlockNumberAndIndex then.