Web3.py: The simple cache caches unmined transactions

[carver]

What was wrong?

w3.eth.getTransactionByHash is caching the result, if the transaction is unmined. I believe some clients return None on an unmined transaction, and others return a transaction without a block number or hash.

Discovered by @palango in https://github.com/ethereum/web3.py/issues/1066#issuecomment-427321034

How can it be fixed?

We can introspect the transaction in the simple cache and skip caching it when the block number or block hash is missing.

Should be backported to v4.

[palango]

I believe some clients return None on an unmined transaction, and others return a transaction without a block number or hash.

geth returns the transaction info with the blockNumber set to None

We can introspect the transaction in the simple cache and skip caching it when the block number or block hash is missing.

This would handle the transaction got mined case, but a transaction could still end up in a reorg. Maybe it makes sense to add a confirmations argument.

[pipermerriam]

This would handle the transaction got mined case, but a transaction could still end up in a reorg. Maybe it makes sense to add a confirmations argument.

We can call it something like required_confirmations and we only cache values that are beyond that depth. :+1:

[ldub]

Hey, is there any update on this? I was looking into simple_cache_middleware and had the same concern. Adding the required_confirmations argument makes sense.

[carver]

Thanks for the ping @ldub

cc @kclowes - are you up for taking a look?

[kclowes]

Yep, I can take a look!

[ldub]

One thing that came to mind with the required_confirmations argument: when using a gas_strategy that queries the last 120 blocks for every tx sent by web3py, even a 5 or 10 block required_confirmations would end up in a lot of unnecessary requests. This is also especially painful now that infura has quotas.

I would suggest accepting both a number or a map for the required_confirmations argument so that a map of {"eth_rpcMethod": <required_confirmations>} can be passed to configure some sane defaults:

• cache eth_getBlockByHash, eth_getBlockTransactionCountByHash after 0/1 confirmations (a block will never change once it has a hash)
• cache eth_getBlockByNumber, eth_getTransactionByHash, etc after 10 confirmations

[kclowes]

Good point. That makes sense to me. @pipermerriam or @carver any thoughts?

[pipermerriam]

I think a confirmations based mechanism makes sense, however, if added directly to the cache muiddleware that would result in the Middleware not being usable for other non eth rpc methods. I think we can still implement something that does confirmation based conditional caching but we should try to find a way to do it such that it doesn't tie the middleware directly to the eth rpc methods.

Sent from ProtonMail mobile

-------- Original Message --------
On Oct 25, 2019, 3:00 PM, kclowes wrote:

Good point. That makes sense to me. @pipermerriam or @carver any thoughts?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[kclowes]

@pipermerriam did you have an API in mind? I had two ideas, but I think I like the second one better.

1) Add ability to pass in a dict with: {‘method_name’: required_confirmations}, and then also have a default required_confirmations that gets applied to the other methods, but is configurable.
or
2) Split out caching middleware so that we have one for methods like eth_getBlockByHash, and eth_getBlockTransactionCountByHash that should stay the same once they have a hash (and not cache them until they're mined). Then, we'd have a separate caching middleware for other eth transaction methods, with only one required_confirmations arg that would apply to all the methods that may get shuffled in a reorg.

I'm also open to other ideas!

[pipermerriam]

Option 2 seems simpler. I like it.