Fix block cache deserialization for old ethereum blocks#6330
Fix block cache deserialization for old ethereum blocks#6330incrypto32 merged 7 commits intomasterfrom
Conversation
Add a dedicated module for JSON patching utilities that handle missing `type` fields in Ethereum transactions and receipts. This consolidates patching logic that will be used by both the HTTP transport layer (for RPC responses) and cache deserialization (for stored blocks). The module provides: - patch_type_field: Adds "type": "0x0" to JSON objects missing the field - patch_block_transactions: Patches all transactions in a block - patch_receipts: Patches single receipts or arrays of receipts
Refactor the HTTP transport's receipt patching to use the shared json_patch module instead of duplicating the patching logic. This removes the patch_receipt and patch_result methods from PatchingHttp and replaces them with calls to json_patch::patch_receipts. The patch_rpc_response and patch_response methods remain as they handle RPC-specific JSON-RPC response structure.
Add patching for cached blocks before deserialization to handle blocks that were cached before March 2022 when graph-node's rust-web3 fork didn't capture the transaction type field. This patches transactions and receipts in cached blocks at two locations: - ancestor_block(): For full block deserialization (EthereumBlock), patches both transactions and transaction_receipts - parent_ptr(): For light block deserialization (LightEthereumBlock), patches only transactions (light blocks don't include receipts) The patching adds type: 0x0 (legacy) to transactions/receipts missing the field, allowing alloy to deserialize blocks that would otherwise fail due to the missing required field.
incrypto32
force-pushed
the
krishna/unified-json-patching
branch
from
5eaa074 to
c52a748
Compare
lutter
left a comment
lutter
left a comment
There was a problem hiding this comment.
Looks good, but I would tighten up the code by introducing a new JsonBlock type that helps with determining what the block is and conversion into one of our blocks.
chain/ethereum/src/chain.rs
Outdated
| if json.get("block").is_none() { | ||
| warn!( | ||
| // Shallow blocks have "data": null - no block data to deserialize | ||
| if json.get("data") == Some(&json::Value::Null) { |
There was a problem hiding this comment.
Since this check is repeated a few times and a little noisy, I wonder if it wouldn't be better to introduce a newtype JsonBlock(json::Value) so that you could have methods like is_shallow(&self) and patch_transactions on that
chain/ethereum/src/chain.rs
Outdated
| let mut json = json; | ||
| if let Some(block) = json.get_mut("block") { | ||
| json_patch::patch_block_transactions(block); | ||
| } |
There was a problem hiding this comment.
I would put the entire if statement into a json_block.path_block_transactions() method
chain/ethereum/src/chain.rs
Outdated
| } | ||
| if let Some(receipts) = json.get_mut("transaction_receipts") { | ||
| json_patch::patch_receipts(receipts); | ||
| } |
chain/ethereum/src/chain.rs
Outdated
| if let Some(receipts) = json.get_mut("transaction_receipts") { | ||
| json_patch::patch_receipts(receipts); | ||
| } | ||
| match json::from_value::<EthereumBlock>(json) { |
There was a problem hiding this comment.
or since patching happens here and on line 1205, just have a JsonBlock::patched_block<T>(self) -> Result<T, SomeError> that does the patching and conversion
Introduce EthereumJsonBlock newtype with helper methods for format detection (is_shallow, is_legacy_format) and deserialization (into_full_block, into_light_block). Cleans up repeated logic and avoids unnecessary clones by taking ownership of the JSON data.
incrypto32
force-pushed
the
krishna/unified-json-patching
branch
from
1b3569c to
7fa4a54
Compare
| use crate::json_patch; | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct EthereumJsonBlock(Value); |
There was a problem hiding this comment.
Let me know if we should call it just JsonBlock named it this because we already have a JsonBlock in chainstore and this one is very ethereum specific
There was a problem hiding this comment.
I am fine with either, but since we are in the ethereum crate, the Ethereum prefix isn't strictly necessary. But not a big deal either way.
lutter
left a comment
lutter
left a comment
There was a problem hiding this comment.
Nice! Left some suggestions for minor improvements, but totally fine to merge as-is
| pub fn into_full_block<T: DeserializeOwned>(mut self) -> Result<T, json::Error> { | ||
| self.patch(); | ||
| json::from_value(self.0) | ||
| } |
There was a problem hiding this comment.
When I suggested the type parameter T, I assumed we'd use this for different block forms. But since we need to have different methods for light and full blocks, I think we don't need T.
chain/ethereum/src/json_block.rs
Outdated
| fn from(value: Value) -> Self { | ||
| Self::new(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is definitely a matter of taste, but I generally try to not have From implementations that just call new or other constructors, since when you're reading code value.into() is much less clear than EthereumJsonBlock::new(value)
Remove generic type parameters from into_full_block() and into_light_block(), returning EthereumBlock and LightEthereumBlock directly instead.
2 participants
No description provided.