[MegaRedHand]

🗒️ Description / Motivation

On a 7-node all-ethlambda kurtosis devnet (ethlambda-all.yaml from our ethereum-package fork, image 1.0.0), every CL logged this at the start of epoch 5, against the same two peers:

WARN libp2p_gossipsub::behaviour: [Penalty] Peer attempted graft within backoff time, penalizing peer=...

The two penalized nodes had each left an attestation subnet when an aggregator duty expired at the end of epoch 4. At the epoch 5 boundary, the VC's duties for epoch 6 asked for that subnet again, and the node rejoined about 23 s (and, on the other node, 55 s) after leaving. The trigger is a duty on a subnet in the last slots of epoch N, none in N+1, and one in N+2.

The cause is an asymmetry in rust-libp2p gossipsub (verified at our pinned rev 2f14d0e):

Side	What it does	Backoff
Leaver	Sends UNSUBSCRIBE, then PRUNE carrying unsubscribe_backoff; won't graft for that long	10 s default
Receiver	Handling the UNSUBSCRIBE, removes the peer from the mesh with remove_peer_from_mesh(.., None, ..); None means prune_backoff. Backoffs are only ever extended, so the PRUNE's shorter value can't shorten it	60 s default

So the leaver regrafts after 10 s into peers still holding a 60 s backoff. They reject the graft and add a behaviour penalty (P7).

What Changed

• crates/net/p2p/src/lib.rs: a PRUNE_BACKOFF constant (60 s, gossipsub's default) set as both prune_backoff and unsubscribe_backoff in gossipsub_config, so this node waits as long after leaving a topic as its peers do. Plus a unit test checking that the unsubscribe backoff is at least a default peer's prune backoff.
• docs/beacon_wire.md: a third "difference from lighthouse" bullet in Peer scoring explaining the choice.

Correctness / Behavior Guarantees

• The mesh on a rejoined subnet comes back no later than before. The early grafts were rejected anyway, so the subnet's mesh was empty for the peers' 60 s either way; this only stops the node from being penalized for trying.
• No effect on lean: its subscriptions are fixed at startup and never left.
• Severity was low. One penalty per peer per event, and the behaviour penalty only counts past its threshold, so it would take about 10 such events per epoch, sustained, to reach the gossip threshold. Lighthouse would penalize us the same way, since it uses the same rust-libp2p defaults.

What the specs say

• consensus-specs (v1.7.0-beta.2, phase0/p2p-interface.md) fixes D, D_low, D_high, D_lazy, the heartbeat, fanout_ttl, mcache_* and seen_ttl, and leaves gossipsub v1.1's extra parameters (backoffs, scoring) "under investigation". For subnets, it asks for SUBNETS_PER_NODE long-lived subscriptions held for EPOCHS_PER_SUBNET_SUBSCRIPTION epochs. An aggregator joins its duty's subnet (phase0/validator.md), but nothing says how long to stay.
• The libp2p gossipsub v1.1 spec recommends a 1-minute PruneBackoff and a 10 s UnsubscribeBackoff, and expects the leaver's PRUNE to carry the shorter one. rust-libp2p's receiver doesn't honor it, which is the asymmetry above.
• Lighthouse sets neither backoff, and unsubscribes 2 slots after an aggregator duty (UNSUBSCRIBE_AFTER_AGGREGATOR_DUTY).

An alternative is to keep an expired aggregator subnet until the next epoch's duty refresh, so the leave never happens in the first place. I didn't take it because it changes subscription logic rather than one parameter, and still leaves other leave-and-rejoin paths exposed.

Tests Added / Run

• rejoining_a_topic_waits_out_the_backoff_peers_apply_to_a_leaver (new)
• cargo test -p ethlambda-p2p --lib --profile release-fast: 361 passed, 1 ignored (an existing devnet3 fixture)
• make fmt, make lint

Related Issues / PRs

• Found while running lambdaclass/ethereum-package's ethlambda-all.yaml after lambdaclass/ethereum-package@1b7aa4d6, which lets an ethlambda CL be the bootnode.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — all passing (not run: only ethlambda-p2p's lib tests above)

[github-actions]

🤖 Claude Code Review

Review of PR 728: gossipsub backoff alignment

The change is small, correct for the problem it describes, and I have no blocking concerns.

What it does

• gossipsub_config now sets prune_backoff and unsubscribe_backoff to one PRUNE_BACKOFF constant (60 s).
• It adds a unit test and a docs bullet.

Correctness

• The 60 s value matches gossipsub's default prune_backoff, so prune_backoff is unchanged in effect. Setting it explicitly only pins it against a future default change.
• The substantive change is unsubscribe_backoff going from 10 s to 60 s. That matches the receiver-side backoff, so a leaver's regraft no longer lands inside a window where peers reject it and apply the P7 penalty.
• The trade-off is that a node which leaves a topic and wants it back sooner than 60 s must wait. The PR argues this costs nothing, because the early grafts were rejected anyway. That holds against rust-libp2p peers, but not against peers with a shorter prune backoff. For a non-rust-libp2p client, the node would wait longer than needed. Given the target peers (lighthouse and ethlambda), this is acceptable.
• Lean subscriptions are static, so the lean side is unaffected, as the PR says.

Nits (non-blocking)

1. The new test's first assertion compares against Config::default().prune_backoff(). If upstream changes that default, the test fails with an unclear message. A short assert message would help.
2. The second assertion (unsubscribe_backoff >= prune_backoff) is redundant, since both come from the same constant. It does guard against the two drifting apart, so it can stay.
3. The test does not cover the real behavior, which is that regrafting inside the window is penalized. That is hard to unit test, so a config-level check is reasonable.
4. The doc comment on PRUNE_BACKOFF and the docs bullet repeat much of the same explanation. Trimming one would reduce drift risk. The docs entry sits correctly under "differences from lighthouse".

The code and docs are consistent, and the PR is ready to merge as is. I did not build or run the tests, and I did not independently check the claim about rust-libp2p's receiver-side handling at rev 2f14d0e.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR focusing on the areas you specified. This is a targeted change to gossipsub backoff parameters with documentation and test updates.

Overall Assessment

The PR is conceptually sound and addresses a real interoperability issue with Lighthouse/rust-libp2p peers. The core logic is correct: aligning unsubscribe_backoff with prune_backoff prevents a node from re-grafting into peers that are still backing off from the previous unsubscribe.

However, I found several issues ranging from documentation clarity to test robustness.

---

Detailed Findings

1. Constant naming is misleading (crates/net/p2p/src/lib.rs:820-833)

The doc comment and constant name PRUNE_BACKOFF conflate two semantically different backoffs:

• prune_backoff: Time to wait before grafting a peer after being pruned by or pruning that peer
• unsubscribe_backoff: Time to wait before grafting after leaving a topic

The comment acknowledges they're "the same on purpose" but the naming suggests they serve the same purpose. This hurts maintainability.

Suggestion: Rename to BACKOFF_DURATION or GRAFT_BACKOFF, and clarify in docs that this value is applied to both settings intentionally.

---

2. Test assertion is weaker than the documented intent (crates/net/p2p/src/lib.rs:3378-3385)

assert!(config.unsubscribe_backoff() >= peers_backoff);
assert!(config.unsubscribe_backoff() >= config.prune_backoff());

The test name says "waits out the backoff peers apply to a leaver," but the first assertion only checks >= against Lighthouse's default prune backoff. The documented behavior requires equality with the prune backoff (60s), not merely >=.

Line 3382: This should assert exact equality or at least verify the specific expected duration:

assert_eq!(config.unsubscribe_backoff(), Duration::from_secs(60));
assert_eq!(config.prune_backoff(), Duration::from_secs(60));

The current >= would pass if unsubscribe_backoff were 300s, which would be suboptimal (unnecessarily delayed rejoining).

---

3. Missing test for the actual penalty scenario (crates/net/p2p/src/lib.rs:3378-3385)

The test validates configuration values but doesn't test the behavioral outcome: that rejoining after the configured backoff avoids penalties. Consider adding an integration-level test or at least a comment referencing where this is tested (if anywhere).

If no such test exists, this is a gap—configuration correctness doesn't guarantee the gossipsub state machine behaves as expected.

---

4. Documentation inconsistency (docs/beacon_wire.md:299-312)

The new section states:

"Gossipsub's default backoff after unsubscribing (10 s) is shorter than its prune backoff (60 s)"

But the rust-libp2p gossipsub::Config::default() values should be verified. In current rust-libp2p versions, prune_backoff default is actually 60 seconds, and unsubscribe_backoff is 10 seconds—so the statement appears correct, but this is an implicit dependency on external defaults.

Risk: If rust-libp2p changes defaults, the documentation becomes incorrect without any code change in this repo.

Suggestion: Add a code comment or doc note that these defaults are rust-libp2p version-dependent, or reference the specific rust-libp2p version verified.

---

5. No Clone/Copy concern for Duration (not an issue)

The Duration::from_secs(60) usage is correct and idiomatic. No issues here.

---

6. Potential consensus interaction: attestation subnet timing (crates/net/p2p/src/lib.rs:820-833)

The comment mentions "a duty in the last slots of epoch N, none in N+1, one in N+2" as the problematic case. With 32 slots per epoch and 12 seconds per slot:

• Epoch N: slots N32 through N32+31
• Duty in last slots: say slot N*32+30
• Next duty epoch N+2: slot (N+2)32 = N32+64
• Gap: ~34 slots = ~6.8 minutes

With PRUNE_BACKOFF = 60s, rejoining at slot N*32+64 (4 minutes later) is well past the backoff. This seems correct.

But what if the gap is shorter? With PRUNE_BACKOFF = 60s, a duty at slot N*32+31 and next at (N+1)*32+1 (33 slots = ~6.6 min gap) also clears. The minimum gap that triggers this issue is any gap where rejoin happens within 60s of leaving—possible with subnet quick-switching.

Question: Is 60s always sufficient? The comment says "two epochs later" which is ~6.4 min, so the safety margin is large. Consider documenting the worst-case timing analysis.

---

7. Error handling: None in this PR (not applicable)

This PR only touches configuration constants and tests. No error paths to review.

---

8. Memory safety: None in this PR (not applicable)

No unsafe code or manual memory management.

---

Positive Acknowledgments

1. Good test coverage for configuration: The new test verifies the invariant that unsubscribe_backoff >= peers_backoff, which is the core correctness property.

2. Excellent documentation: The doc comment on PRUNE_BACKOFF and the beacon_wire.md update clearly explain the interoperability motivation. This is critical for consensus code where "why" matters as much as "what."

3. Correct fix direction: The analysis of rust-libp2p's behavior (ignoring the shorter PRUNE-carried backoff) is accurate per the gossipsub v1.1 spec and rust-libp2p implementation.

---

Summary Table

Item	Severity	File	Lines	Issue
1	Low	lib.rs	820-833	Misleading constant name
2	Medium	lib.rs	3382-3383	Weak test assertions (>= vs ==)
3	Low	lib.rs	3378-3385	Missing behavioral test
4	Low	beacon_wire.md	299-312	Implicit dependency on external defaults
6	Informational	lib.rs	820-833	Timing analysis could be explicit

Recommended changes before merge:

• Item 2: Strengthen test assertions to exact equality
• Item 1: Consider renaming PRUNE_BACKOFF to clarify dual use

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall. The change is small, targeted, and the reasoning matches libp2p gossipsub behavior for unsubscribe vs prune backoff.

• crates/net/p2p/src/lib.rs:863 / crates/net/p2p/src/lib.rs:864 — Setting unsubscribe_backoff to match prune_backoff is a sensible correctness fix for dynamic aggregator subnet rejoin behavior. I don’t see a consensus or safety regression from this alone.
• crates/net/p2p/src/gossipsub/handler.rs:1251 with crates/net/p2p/src/gossipsub/handler.rs:1156 — The change aligns with the actual join/leave flow: subnets are explicitly unsubscribed after duty expiry and later re-subscribed, so avoiding premature re-graft attempts should reduce self-inflicted mesh penalties rather than create them.
• crates/net/p2p/src/lib.rs:3380 — The new test is directionally useful, but it is a bit brittle because it depends on libp2p::gossipsub::Config::default().prune_backoff() staying at Lighthouse-compatible defaults. A more robust assertion would check the explicit intended value (Duration::from_secs(60)) for both fields, which makes the contract clearer and avoids false failures if upstream defaults change.
• docs/beacon_wire.md:299 — Documentation is clear and matches the implementation; good to have the operational rationale written down.

I did not spot:

• consensus-layer correctness risks in fork choice / attestation / finalization / state transition logic, since this PR does not touch those paths directly;
• memory-safety concerns;
• obvious security issues beyond normal gossip behavior;
• meaningful performance regressions.

If you want, I can also do a second pass specifically on whether the 60s constant should be derived from libp2p defaults or exposed as a named networking parameter.

---

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