Repository navigation
fix(p2p): stop warning on every gossipsub IHAVE dropped by a full send queue - #717
pablodeymo wants to merge 1 commit into
Conversation
…d queue A beacon node logs `WARN libp2p_gossipsub::behaviour: Send Queue full. Could not send Rpc.` once per heartbeat for every slow peer, each line listing every IHAVE message id for an attestation subnet. The drop is already counted in the peer's `failed_messages` and charged to its score, so the warning adds noise, not information. The line is emitted inside the gossipsub crate, so this bumps the libp2p fork to lambdaclass/rust-libp2p#1, which logs non-priority drops (IHAVE, IWANT, publish, forward) at DEBUG and keeps WARN for control drops (GRAFT, PRUNE, IDONTWANT). The fix commit is based on the revision pinned so far, so the dependency tree differs from today's by those lines only; the branch head is a full upstream merge ahead and was deliberately not taken. The lockfile's 38 fork source lines are rewritten in place rather than through `cargo update -p libp2p`, which also re-resolved two unrelated dependency edges; `cargo metadata --locked` accepts the rewritten lock unchanged.
🤖 Kimi Code ReviewI'll review this PR which updates the SummaryThis PR bumps the ReviewSecurity & Supply ChainCritical concern: No verification of fork changes
Dependency ManagementMinor issue:
Consensus-Layer ImpactNo direct consensus code affected
Networking ConsiderationsItem 1: Protocol compatibility risk
Item 2:
Rust Best PracticesThe change itself is idiomatic — git dependencies with explicit revs are the correct approach for fork pinning. No code quality issues. Verdict
Required Actions
The diff itself is correct; the concern is operational/process-related for a blockchain client. Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt |
🤖 Claude Code ReviewReview of PR 717: bump the libp2p fork pin to The change is mechanical and consistent. I found no blocking issues.
Notes, none blocking:
Approve. Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewLooks good overall. I don’t see a correctness, consensus, or memory-safety issue in this PR itself.
This PR is only a pinned What I checked:
Minor note:
If you want, I can also do a deeper follow-up pass on the effective upstream change by comparing the old and new Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
|
Superseded by #719, which caps gossipsub's log output in ethlambda's own filter instead of bumping the libp2p fork. The branch stays for reference. |
…ambdaclass#719) ## 🗒️ Description / Motivation - A beacon node on Hoodi logs `WARN libp2p_gossipsub::behaviour: Send Queue full. Could not send Rpc.` once per heartbeat for every slow peer, each line listing every IHAVE id dropped on an attestation subnet. The drop is already counted in the peer's `failed_messages` and charged to its score, so the line adds noise, not information. - The line is emitted inside the gossipsub crate. Rather than patching the libp2p fork, the node's default log filter now caps that crate at ERROR, which is how an operator sees the dependency's noise anyway. Lighthouse goes further and routes all `libp2p_gossipsub` and `discv5` output to separate files. ## What Changed - `bin/ethlambda/src/main.rs`: `init_node_logging` builds its filter from `node_log_filter(RUST_LOG)`. With no `RUST_LOG` the directives are `info,libp2p_gossipsub=error`; with one they are `libp2p_gossipsub=error,<RUST_LOG>`. A `RUST_LOG` directive on `libp2p_gossipsub` replaces the cap, one on any other target adds to it, and `RUST_LOG` keeps replacing the `info` floor the way `from_env_lossy` always did. The floor is spelled out because `EnvFilter` only applies a default directive when it was given no directive at all. - Besides the send-queue line, the cap hides gossipsub's other warnings (bad signatures, substream limits, anonymous-mode field checks), all per-peer protocol violations that peer scoring already handles. Its 19 error sites still show. - The benchmark and keygen filters are untouched; neither runs a network. - `docs/beacon_wire.md`: one sentence next to the existing `RUST_LOG` example, saying how to see gossipsub's warnings. ## Correctness / Behavior Guarantees - Default output changes only for the `libp2p_gossipsub` target: WARN and below are dropped, ERROR stays. - `RUST_LOG` semantics are preserved, including the pre-existing quirk that a `RUST_LOG` without its own `info` silences other targets. Verified against tracing-subscriber 0.3.23: an equal-specificity directive added later replaces the earlier one, and `Ordering::Equal` implies the same target, so no other directive is clobbered. - A bare `RUST_LOG=debug` keeps the cap, since the more specific directive wins; the doc comment says to name the crate. ## Tests Added / Run - Three tests in `main.rs` drive real events through the filter: default behaviour (INFO floor, gossipsub WARN dropped, gossipsub ERROR kept), the lift via `RUST_LOG=…,libp2p_gossipsub=warn,…` alongside another directive, and the floor-replacement quirk. - `cargo test --locked --profile release-fast -p ethlambda --bin ethlambda -- tests::node_logging tests::rust_log`: 3 passed. - `cargo clippy --locked -p ethlambda --all-targets -- -D warnings`: clean. `cargo fmt --all -- --check`: clean. ## Related Issues / PRs - Supersedes lambdaclass#717 and lambdaclass/rust-libp2p#1, which fixed the same symptom in the fork; both are being closed. - Follow-up worth an issue: ethlambda does not handle gossipsub's `Event::SlowPeer`. Lighthouse reports a peer after more than 10 priority or 10 non-priority drops, so the peer producing these lines gets disconnected instead of staying in the mesh. ## ✅ Verification Checklist - [x] Ran `make fmt` — clean - [ ] Ran `make lint` (clippy with `-D warnings`) — run as `cargo clippy -p ethlambda --all-targets -- -D warnings`, the only crate changed; CI runs the workspace - [ ] Ran `make test` (`test-consensus` plus `test-node`, at `release-fast`) — the three new tests run at `release-fast`; CI runs the rest
🗒️ Description / Motivation
WARN libp2p_gossipsub::behaviour: Send Queue full. Could not send Rpc.once per heartbeat for every slow peer, each line listing every IHAVE message id of an attestation subnet. The drop is already counted in the peer'sfailed_messagesand charged to its score, so the warning adds noise, not information.What Changed
crates/net/p2p/Cargo.toml: bothrevfields (libp2p,libp2p-mplex) move from2f14d0ectoda141177, the fix commit, whose parent is2f14d0ec. The dependency tree therefore differs from today's by those lines only. The branch head was deliberately not taken: it is 51 commits and a full upstream merge ahead (protobuf back to prost, gossipsub behaviour and config changes), which is not a change to make during the 1.0 window.Cargo.lock: the 38 fork source lines, rewritten in place.cargo update -p libp2pwas not used because it also re-resolved two unrelated edges (data-encoding-macro-internal→syn,iana-time-zone→windows-core);cargo metadata --lockedaccepts the rewritten lock without changes.Correctness / Behavior Guarantees
Tests Added / Run
cargo clippy --locked -p ethlambda-p2p --all-targets -- -D warnings: clean.cargo test --locked --profile release-fast -p ethlambda-p2p: 360 passed, 0 failed, 1 ignored.da141177carries the WARN/DEBUG split.Related Issues / PRs
Event::SlowPeer. Lighthouse penalizes a peer once it has dropped more than 10 priority messages; we only let the score decay do the work.✅ Verification Checklist
make fmt— no Rust source changed in this repomake lint(clippy with-D warnings) — run forethlambda-p2p, the only crate consuming the fork, rather than the whole workspacemake test(test-consensusplustest-node, atrelease-fast) —ethlambda-p2psuite run atrelease-fast; CI runs the rest