Skip to content

fix(p2p): stop warning on every gossipsub IHAVE dropped by a full send queue - #717

Closed
pablodeymo wants to merge 1 commit into
mainfrom
fix/gossipsub-send-queue-log-level
Closed

pablodeymo wants to merge 1 commit into
mainfrom
fix/gossipsub-send-queue-log-level

Conversation

@pablodeymo

Copy link
Copy Markdown
Collaborator

🗒️ 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 message id of 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 the fix is in the libp2p fork: fix(gossipsub): log non-priority send-queue drops at debug rust-libp2p#1 logs non-priority drops (IHAVE, IWANT, publish, forward) at DEBUG and keeps WARN for control drops (GRAFT, PRUNE, IDONTWANT), which do leave the peer's mesh view out of step with ours. This PR picks that commit up.

What Changed

  • crates/net/p2p/Cargo.toml: both rev fields (libp2p, libp2p-mplex) move from 2f14d0ec to da141177, the fix commit, whose parent is 2f14d0ec. 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 libp2p was not used because it also re-resolved two unrelated edges (data-encoding-macro-internal → syn, iana-time-zone → windows-core); cargo metadata --locked accepts the rewritten lock without changes.

Correctness / Behavior Guarantees

  • No behaviour change beyond the level of one log line. The failed-message counters and the slow-peer scoring are untouched, and control-message drops still warn.
  • Nothing else pins the fork revision: the event-monitor lockfile and the Shadow patch do not reference libp2p.

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.
  • Checked that cargo's checkout of da141177 carries the WARN/DEBUG split.

Related Issues / PRs

  • fix(gossipsub): log non-priority send-queue drops at debug rust-libp2p#1 (the fix). This PR does not wait for it: the commit stays reachable on the fork either way, and the pin was already a non-head commit of that branch.
  • Follow-up worth a look: ethlambda does not handle gossipsub's 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

  • Ran make fmt — no Rust source changed in this repo
  • Ran make lint (clippy with -D warnings) — run for ethlambda-p2p, the only crate consuming the fork, rather than the whole workspace
  • Ran make test (test-consensus plus test-node, at release-fast) — ethlambda-p2p suite run at release-fast; CI runs the rest

…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.
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR which updates the rust-libp2p fork revision used by the project.

Summary

This PR bumps the lambdaclass/rust-libp2p fork from revision 2f14d0ec9665a01cfb6a02326c90628c4bba521c to da14117765c24f7a90634d841f649fb4adf82bfb. The change is mechanical and limited to dependency version pinning.

Review

Security & Supply Chain

Critical concern: No verification of fork changes

  • crates/net/p2p/Cargo.toml lines 29 and 37: The PR updates to a new git revision without any description of what changed in the fork. For a consensus-critical blockchain client, this is a significant supply chain risk.

    Recommendation: Add a comment in Cargo.toml or in the PR description documenting:

    1. What bug fixes or features this revision contains
    2. Whether this corresponds to an upstream libp2p release or is a custom LambdaClass patch
    3. Security implications of the changes (especially for libp2p-noise, libp2p-tls, libp2p-quic)

Dependency Management

Minor issue: Cargo.lock bloat

  • The Cargo.lock diff shows ~40 lines changed with identical patterns. This is expected for a git revision bump, but verify that no transitive dependency versions changed unexpectedly.

Consensus-Layer Impact

No direct consensus code affected

  • This change is isolated to the networking layer (crates/net/p2p). No state transition, fork choice, attestation, or XMSS code is modified.

Networking Considerations

Item 1: Protocol compatibility risk

  • libp2p revision changes can affect wire protocol behavior. Verify that da14117765c24f7a90634d841f649fb4adf82bfb maintains compatibility with:
    • GossipSub message encoding (affects attestation/block propagation)
    • Request-Response codec behavior (affects sync protocol)
    • Noise/ TLS handshake behavior (affects peer authentication)

Item 2: mplex deprecation status

  • Cargo.toml line 34-37: The comment notes mplex is deprecated in upstream. Confirm this fork revision doesn't introduce breaking changes to the deprecated API that the muxers module relies on.

Rust Best Practices

The change itself is idiomatic — git dependencies with explicit revs are the correct approach for fork pinning. No code quality issues.

Verdict

Aspect Assessment
Code correctness ✓ Mechanical change, no bugs introduced
Security ⚠ Supply chain transparency lacking
Performance Neutral (depends on fork changes)
Idiomatic Rust ✓
Consensus safety ✓ No consensus code touched

Required Actions

  1. Before merge: Document what changed in the lambdaclass/rust-libp2p@da141177 revision. Link to the fork's commit diff or provide a summary in the PR description.

  2. Recommended: Consider adding a CHANGELOG.network or similar to track fork divergence from upstream libp2p, especially for security-relevant crates (libp2p-noise, libp2p-tls).

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

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review of PR 717: bump the libp2p fork pin to da141177

The change is mechanical and consistent. I found no blocking issues.

  • crates/net/p2p/Cargo.toml moves both rev fields, libp2p and libp2p-mplex, to da14117765c24f7a90634d841f649fb4adf82bfb. They stay in sync, so cargo won't resolve two copies of the fork.
  • Cargo.lock has 38 source lines rewritten, all to the new rev. No version or checksum lines changed and no unrelated crates were re-resolved. That matches the description and is the minimal lock diff.
  • Taking the single fix commit, whose parent is the old pin, instead of the branch head is the right call during the 1.0 window. It limits the risk to one log-level change in gossipsub.

Notes, none blocking:

  1. Check that the commit is on a branch. The commit is reachable today because the old pin was already a non-head commit of that branch. If fix(gossipsub): log non-priority send-queue drops at debug rust-libp2p#1 is closed and its branch deleted, GitHub can garbage-collect the commit and break fresh builds and CI caches. Merging the fork PR, or tagging or branching da141177, would make the pin durable.
  2. Dropped IHAVE and IWANT now log at DEBUG. failed_messages and the peer score still record them, as the description says. Nothing in this repo reads the WARN line, so this is fine. Operators lose that signal unless they enable debug logging. The SlowPeer follow-up in the description would give a better signal than a log line.
  3. Description checkboxes. The make fmt and make test boxes are unchecked and the explanations are given inline. That is reasonable here because no Rust source changed. CI covers the rest of the workspace.
  4. Out of scope. The description's follow-up on handling Event::SlowPeer is a sensible separate issue. I'd file it rather than fold it into this PR.

Approve.


Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Looks good overall. I don’t see a correctness, consensus, or memory-safety issue in this PR itself.

  • crates/net/p2p/Cargo.toml:29
  • crates/net/p2p/Cargo.toml:37

This PR is only a pinned rust-libp2p revision bump plus the corresponding Cargo.lock refresh, so none of the consensus-critical paths you listed are directly modified here: fork choice, attestation validation, justification/finalization, state transition, XMSS, and SSZ logic remain untouched.

What I checked:

  • Dependency coherence: libp2p and libp2p-mplex are bumped to the same fork revision, which avoids mixed-tree ABI/API surprises.
  • Lockfile consistency: Cargo.lock updates appear to be a pure source-revision rollover for the lambdaclass/rust-libp2p packages, not a broader dependency graph change.
  • Security/correctness surface: the stated intent is a logging-behavior fix for gossipsub send-queue drops, and I don’t see evidence in this PR diff of application-level behavior changes in peer scoring, validation, or message acceptance.

Minor note:

  • Because this is a git revision bump to a forked networking stack, the real risk is behavioral drift outside the visible diff. If CI does not already cover it, I’d want at least one targeted integration check around gossipsub backpressure / dropped IHAVE handling before merging. That’s not a blocker from the diff alone, just the main residual risk with dependency-only PRs.

If you want, I can also do a deeper follow-up pass on the effective upstream change by comparing the old and new rust-libp2p revisions.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@pablodeymo

Copy link
Copy Markdown
Collaborator Author

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.

@pablodeymo pablodeymo closed this Oct 28, 2026
@MegaRedHand
MegaRedHand deleted the fix/gossipsub-send-queue-log-level branch October 28, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant