fix: Preload txn receipt from eth_api for historical deposit txns #28
Conversation
|
The codebase has several other places where unsafe |
| }; | ||
| Ok(Some(self.transform_tx(transaction, tx_info))) | ||
| // preload transaction receipt if it's a deposit transaction | ||
| if transaction.is_deposit() { |
There was a problem hiding this comment.
Why did all this move to this method and pass in the receipt? Why not keep this logic in the transform_tx and prevent the addition of the receipt param? That way if the method is used anywhere else we won't have to replicate this logic. Otherwise looks good
There was a problem hiding this comment.
pretty much to avoid marking a bunch of other functions async because rust has colored functions
|
The diff looks crazy, does this need a rebase or something? I came back to approve this morning and looks like a bunch of cache stuff is in there. |
haardikk21
force-pushed
the
haardik/cache-read-race-condition
branch
from
337d51c to
4bec491
Compare
messed up previous rebase a bit. should be fine now. |
…se#28) * preload txn receipt from eth_api for historical deposit txns * error handle being unable to load txn receipt * propogate upstream RPC error to client separately * update cache to use structured keys * preload txn receipt from eth_api for historical deposit txns * remove old comment * clean up match statement
The Flashblocks RPC had a bug where it would panic if
getTransactionByHashwas used to load historical deposit transactions.These transactions did not exist in the mempool (system transaction) and did not exist in the cache (historical transaction) but in the case of deposit transactions we would always try to fetch a receipt from the cache to populate
deposit_receipt_versionanddeposit_nonce.Since we did
unwrap()without an error check in that path, it would cause a panic.This PR attempts to fix that by pre-loading the deposit transaction receipt from
eth_apiin case a historical deposit transaction is attempted to be fetched viagetTransactionByHash.P.S. Opened this PR as the forked repo PR did not allow me to trigger a
base-buildssepolia alpha deployment to test without more changes than necessary to those Dockerfiles.Edit; ignore the branch name. i thought the root cause was a race condition, it was not.