Repository navigation
Conversation
Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
tmigone
changed the title
Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
There was a problem hiding this comment.
🟡 Changes recommended
The patch is applied to every HTTP chain rather than being restricted to Celo as described.
1 open finding
What changed in this PR
Fixes Celo pre-Gingerbread block deserialization when sha3Uncles is absent.
Changes:
- Adds a zero-hash JSON patch for missing
sha3Uncles. - Applies response patching to HTTP block RPC calls and adds tests.
| File | Description |
|---|---|
graph/src/components/ethereum/json_patch.rs |
Adds the missing-uncles-hash patch. |
chain/ethereum/src/transport.rs |
Integrates patching into HTTP responses and tests it. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tmigone
changed the title
Signed-off-by: Tomás Migone <tomas@thegraph.foundation>
tmigone
changed the title
dimitrovmaksim
left a comment
dimitrovmaksim
left a comment
There was a problem hiding this comment.
nit: Please use the {crate}: {description} format for the commits as well
| if let Value::Object(fields) = &mut header | ||
| && !fields.contains_key("sha3Uncles") | ||
| { | ||
| fields.insert("sha3Uncles".to_string(), B256::ZERO.to_string().into()); | ||
| } |
There was a problem hiding this comment.
From what i'm seeing Celo adds some fields in their rpc responses for compatibility with ethers (and other web3 libs) https://github.com/celo-org/celo-blockchain/blob/b1b88599dae455ccc08a1084acea6ccdcb817612/internal/ethapi/api.go#L738-L781 but it can be disabled with --disablerpcethcompatibility https://github.com/celo-org/celo-blockchain/blob/b1b88599dae455ccc08a1084acea6ccdcb817612/cmd/geth/usage.go#L237 (not sure if anyone does this). The rust-web3 fix defaulted sha3Uncles, gasLimit, difficulty and uncles. Alloy itself defaults gasLimit and Block.uncles, but not difficulty , so maybe difficulty should be added too.
| }; | ||
|
|
||
| // Re-export Alloy network types for convenience | ||
| pub use alloy::network::{AnyHeader, AnyRpcHeader, AnyTxEnvelope}; |
There was a problem hiding this comment.
nit: I believe AnyRpcHeader export is now unused and can be removed
| pub use alloy::network::{AnyHeader, AnyTxEnvelope}; |
| } | ||
|
|
||
| #[test] | ||
| fn serializes_like_any_header() { |
There was a problem hiding this comment.
Non-blocking nit: this test only serializes a header whose sha3Uncles is the zero default, so it can't catch a serializer that loses a real uncles hash on the way into the block cache. I checked with a mutant whose Serialize drops sha3Uncles only when it's non-zero: all three tests still pass, and a normal Ethereum block (0x1dcc…) would come back from the cache as zero.
A round-trip test with a real hash catches it (it fails on that mutant and passes on this head):
#[test]
fn round_trips_through_json() {
let uncles = "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347";
let json =
CELO_PRE_GINGERBREAD_BLOCK.replacen('{', &format!(r#"{{"sha3Uncles":"{uncles}","#), 1);
let block: AnyBlock = serde_json::from_str(&json).unwrap();
let back: AnyBlock =
serde_json::from_value(serde_json::to_value(&block).unwrap()).unwrap();
assert_eq!(back.header, block.header);
}
Overview
Since v0.42.0 (
rust-web3->alloymigratiuon) blocks with missingsha3Unclesfail to deserialize since alloy'sAnyHeaderrequires it. Block fetches retry forever, so affected subgraphs stall. Block headers now deserialize throughLenientHeader, which fills in the zero hash when the field is missing.LenientHeaderwrapsAnyHeader. It serializes identically, derefs to it, and delegates the header-trait methods to it. The other edits only convert headers built by hand.Motivation
Regression from #6063. graph-node's rust-web3 fork defaulted
sha3Uncles(bfba0af, pulled in by #2284), and the alloy migration lost that. Fixes #6738.