Conversation
🤖 Kimi Code ReviewReview for PR #280: libp2p 0.57 upgrade This PR upgrades the libp2p networking stack from a pre-0.57 revision to 0.57.0 (commit Critical Observations1. Native async traits migration (Line 20,
2. Timer runtime change in
3. SSZ/Consensus safety
Security & Correctness
Minor Points
VerdictApprove with the caveat that you verify no other files require removal of Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewReview: chore: bump libp2p version (PR 280)This is a straightforward dependency bump from libp2p Changes summary
FindingsUnused dependency left behind ( async-trait = "0.1" # still declared, no longer usedThe Correctness of the
|
🤖 Codex Code ReviewFindings
No correctness, security, or consensus-layer regressions stood out from the diff itself. The change is effectively a I could not run a full Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryBumps the Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| crates/net/p2p/Cargo.toml | libp2p rev bumped from cd6cc3b to 2f14d0e; async-trait dependency is now unused and can be removed |
| crates/net/p2p/src/req_resp/codec.rs | Codec trait impl updated to use native async-in-traits (no #[async_trait]); logic is correct for the single-root BlocksByRoot pattern used in practice |
| Cargo.lock | Lock file updated to reflect the new libp2p rev; all fork sources point consistently to the new commit hash |
Sequence Diagram
sequenceDiagram
participant Requester as P2P Node (requester)
participant Codec
participant Responder as P2P Node (responder)
Requester->>Codec: write_request BlocksByRoot single root
Codec->>Responder: SSZ snappy-framed payload over QUIC substream
Responder->>Codec: write_response BlocksByRoot blocks
Note over Responder,Codec: Per block SUCCESS byte + varint + snappy frame
Codec->>Requester: read_response decode_blocks_by_root_response
Note over Codec,Requester: Reads response code byte then decode_payload
Comments Outside Diff (1)
-
crates/net/p2p/Cargo.toml, line 20 (link)async-traitis listed as a direct dependency but is not referenced anywhere in the crate's source. Now that the new libp2p commit uses Rust's native async-in-traits for therequest_response::Codecimplementation (no#[async_trait]attribute needed), this crate has no remaining callers of the macro. It can be removed.Prompt To Fix With AI
This is a comment left during a code review. Path: crates/net/p2p/Cargo.toml Line: 20 Comment: **Unused `async-trait` dependency** `async-trait` is listed as a direct dependency but is not referenced anywhere in the crate's source. Now that the new libp2p commit uses Rust's native async-in-traits for the `request_response::Codec` implementation (no `#[async_trait]` attribute needed), this crate has no remaining callers of the macro. It can be removed. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/net/p2p/Cargo.toml
Line: 20
Comment:
**Unused `async-trait` dependency**
`async-trait` is listed as a direct dependency but is not referenced anywhere in the crate's source. Now that the new libp2p commit uses Rust's native async-in-traits for the `request_response::Codec` implementation (no `#[async_trait]` attribute needed), this crate has no remaining callers of the macro. It can be removed.
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore: bump libp2p version" | Re-trigger Greptile
bab5bee to
e4f47c7
Compare
This PR bumps the libp2p version to 2f14d0ec9665a01cfb6a02326c90628c4bba521c (the commit is in our fork).
Changelog
Here's the summary of meaningful changes from upstream
master:Gossipsub Changes (likely fixed our issue)
5d47d9d- Port of 55e4a64 (biggest change)Multiple gossipsub fixes to
Instantarithmetic and backoff handling:Instantsubtraction (which can panic/overflow) withchecked_sub+saturating_duration_since. The old code computed(backoff_time + graft_flood_threshold) - prune_backoffwhich could panic if the arithmetic overflowed. This is likely the fix that resolved cross-client mesh issues: if a peer's GRAFT was incorrectly penalized due to arithmetic overflow, it would never join the mesh.checked_addto preventInstantoverflowInstantaddition withsaturating_duration_sinceMAX_REMOTE_PRUNE_BACKOFF_SECONDS = 3600to prevent a remote peer from requesting an absurdly long backoffa7d59cb- CVE fix (GHSA-gc42-3jg7-rxr2)Security fix: Ignore oversized PRUNE backoff values. A malicious peer could send a PRUNE with a backoff duration so large that
Instant::now() + timewould overflow, causing a panic. Now useschecked_addand ignores invalid values.7637c23- Optimize IDONTWANT sendOnly send IDONTWANT for first-seen large messages, deduplicating redundant messages.
aa7a9ec- Partial messages extensionNew gossipsub feature for partial message delivery (spec: libp2p/specs#704).
055186d- Fix duplicate metricsBug fix for double-counted metrics.
Other Changes
8541b83- Removeasync_traitfrom request_response (this caused our codec.rs compile fix)b6b79b2- MSRV bump to 1.88.0, Rust edition 2024aad1f8e- Remove unusedrpc.rs7cbf7c1- TLS key logging via SSLKEYLOGFILE3f88b30- Rendezvous protocol portRoot Cause Analysis
The GRAFT flood penalty fix in
5d47d9dis almost certainly what fixed our cross-client block propagation. The old code had unsafeInstantarithmetic that could overflow when zeam peers (with slightly different timing) sent GRAFT requests. The overflow would cause the penalty check to always trigger, causing ethlambda to PRUNE zeam peers from the block topic mesh. Attestations worked because they used fanout (bypasses mesh/GRAFT entirely).