Skip to content

feat(beacon): score gossipsub peers with lighthouse's parameters - #656

Merged
MegaRedHand merged 1 commit into
perf/beacon-epoch-precomputefrom
feat/beacon-gossipsub-scoring
Oct 27, 2026
Merged

MegaRedHand merged 1 commit into
perf/beacon-epoch-precomputefrom
feat/beacon-gossipsub-scoring

Conversation

@MegaRedHand

@MegaRedHand MegaRedHand commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Motivation

The beacon wire had no gossipsub peer scoring (// TODO: set peer scoring params). Gossipsub kept exchanging with every peer, including ones that send invalid messages, break IWANT promises or cannot keep up, and the connection cap was the only bound on bad peers.

Slow peers are the visible case on the followers. The mainnet follower logged 96k gossipsub Send Queue full warnings in 22 h, from only 8 peers (one of them 60k). The Hoodi follower currently logs about 5000 a minute, almost all from 2 peers. Without scoring, nothing penalizes or drops those peers.

Changes

Area Change
p2p/src/beacon/scoring.rs (new) Lighthouse v8.2.2's gossipsub_scoring_parameters.rs, ported formula for formula: thresholds, peer score parameters, and per-topic parameters for beacon_block, beacon_aggregate_and_proof, all 64 beacon_attestation_{n} subnets, exits and slashings. MeshDeliveryGate (the P3 sync gate below). DigestRole and PeerScoring::hold, which score every held fork digest by its role (below). Unit tests for the derived parameters against lighthouse's values.
p2p/src/lib.rs Scoring is enabled on the beacon wire only (lean is unchanged). The p2p actor rebuilds the validator-count-dependent topic parameters every slot (RefreshPeerScoring, first run at startup) from the head's current-epoch shuffling, under every held fork digest, and sends them to the swarm.
p2p/src/beacon/transition.rs apply hands the swarm every held digest's parameters before it subscribes to any of them.
p2p/src/swarm_adapter.rs SwarmCommand::SetTopicScoreParams. On the existing 10 s tick, peers below the graylist are disconnected and peers are counted by score band.
storage/src/committee_cache.rs, types/beacon/committees.rs CommitteeCache::head_current_committees reads the pinned head's current-epoch shuffling (never builds one), and EpochCommittees::active_validator_count takes the count from its length, so the refresh needs no registry scan.
Metrics lean_gossipsub_peers_by_score{band}, lean_gossipsub_score_disconnects_total, lean_gossipsub_mesh_delivery_scoring.
Docs beacon_wire.md "Peer scoring" section; metrics.md.

Design decisions

  • Lighthouse's parameters. Teku, Lodestar and Grandine run the same ones, so a score here means what it means on most of mainnet. Thresholds: gossip -4000, publish -8000, graylist -16000. The slow-peer penalty (-10 per dropped message) is kept, since slow peers are what the followers see most.

  • mesh_n is ours (8), not lighthouse's 5. It only enters the first-message-delivery cap.

  • P3 (mesh deliveries) is gated on our own sync. Lighthouse subscribes to these topics only once synced. This node subscribes at startup and, while catching up, ignores every aggregate voting for a block it has not imported. A mesh peer credited with no aggregates scores about -19.5k on aggregates alone (below the graylist), so a restart would graylist our whole aggregate mesh for our own lag. P3 is off while the head lags the wall clock by more than 4 slots, and back on only after one epoch within that lag, so the counters have refilled. While off, its threshold and weight are zero but its cap is not, so the counters keep counting to their real ceiling.

  • The gate reads head lag directly, not SyncStatus. On the mainnet follower SyncStatus reported synced through a 10-minute catch-up up to 98 slots behind: its network-stall rule reads a lagging freshest-imported block as a stalled network.

  • Graylisted peers are disconnected; no ban list. Gossipsub alone only ignores a graylisted peer's RPCs, and the peer keeps a connection slot. A peer's negative score is retained for retain_score (100 epochs) after it leaves, so one that reconnects comes back graylisted and is dropped again on the next pass.

  • Columns, sync committee contributions and BLS changes stay unscored. This node Ignores every contribution and change (no consumer), and a delivery is only credited once accepted, so P3 there would penalize the mesh for a gap that is ours. Only Prysm scores columns.

  • All 64 attestation subnets get parameters, not just the backbone ones, because aggregator duties join others at runtime.

  • Every held fork digest is scored, by role. Around a boundary the node holds two digests' topics, and each gets the whole parameter set (exits and slashings included):

    Digest Topic weights P3
    upcoming, joined SUBSCRIBE_LEAD_EPOCHS ahead full off: nothing is published under it yet
    current full per the gate above
    previous, held UNSUBSCRIBE_LAG_EPOCHS after 0 off
    • The parameters go out before the subscriptions, on the swarm's one command channel. Gossipsub tracks a mesh peer on a topic only if the topic had parameters when the peer was grafted; parameters installed later never reach that mesh.
    • The previous digest's traffic moves away at the boundary, so its delivery counters drain. Still scored, the aggregate topic alone would graylist an honest mesh peer 53 slots after the boundary (1M validators), before the topic is left, and leaving it would charge every mesh peer the sticky P3b. Lighthouse zeroes the same weights (remove_topic_weight_except). The cost, as on lighthouse: invalid messages on the previous digest's topics stop costing P4 while it is held.
    • A switch restarts the P3 warmup. The new digest's counters start empty, while its mesh, formed before the boundary, can already be past the deliveries' activation (one epoch for blocks and aggregates, the same as the subscription lead). Lighthouse joins two slots ahead, so activation alone covers it there.

Not in this PR

  • Fixing SyncStatus during beacon catch-up.

Testing

Run on the branch after merging beacon-chain-integration (125dce1).

Command Result
cargo clippy --workspace --all-targets --profile release-fast -- -D warnings clean
cargo fmt --all --check clean
cargo test --profile release-fast -p ethlambda-p2p --lib 218 passed, 1 ignored
cargo test --profile release-fast -p ethlambda-storage --lib 139 passed, 1 ignored

After the fork-rollover fix (dadac7a), on the branch merged with main:

Command Result
make lint (cargo clippy --locked --workspace --all-targets -- -D warnings) clean
cargo fmt --all --check clean
cargo test --profile release-fast -p ethlambda-p2p --lib 360 passed, 1 ignored

The rollover tests in beacon::scoring: a_rollover_parametrizes_each_digest_before_joining_it runs the real transition::apply across a boundary with the swarm's commands captured in order; a_mesh_grafted_ahead_of_its_fork_is_scored connects two real swarms and checks the mesh grafted before the fork earns a score, against a control topic joined before its parameters that stays at 0; a_previous_digest_never_graylists_its_silent_mesh is a tick-by-tick model of gossipsub's mesh-delivery arithmetic (a live run would need 53 slots of decay). The first two fail if the parameters are sent after the subscriptions, and the first fails if the switch skips the warmup restart.

The beacon spec suites were not run: the only state-transition change makes committee_count_per_slot public, and fork choice is untouched.

@MegaRedHand MegaRedHand added the beacon Ethereum Beacon Chain client label Oct 2, 2026
MegaRedHand added a commit that referenced this pull request Oct 2, 2026
…-636-638-gloas-live

The scoring refresh names the dynamic topics for the digest current at
each slot, so on this branch's runtime fork schedule they follow a switch
within a slot; the docs say what does not follow (exit and slashing
parameters, old-digest weights).
MegaRedHand added a commit that referenced this pull request Oct 5, 2026
@MegaRedHand MegaRedHand mentioned this pull request Oct 5, 2026
4 tasks
MegaRedHand added a commit that referenced this pull request Oct 5, 2026
…36-638-gloas-live

Brings gloas validator duties (produceBlockV4, envelope publication, PTC
duties and payload attestations, gloas attestation data and aggregates, VC
gloas support) onto the deployment branch, keeping every behavior of #626,
#633, #636, #638, #646, #647-#652, #656, #658-#660 and the sync-committee and
liveness endpoints.

Conflict resolutions keep both sides: the attestation pool stays in Store
(tmp) while the payload attestation pool is threaded through P2P and the RPC
handles (feature); the aggregate endpoints keep tmp's liveness recording and
attesting indices and add the feature's fork-header check and gloas pooling;
the VC tests and fake execution client serve both fulu blobs and gloas V6.

Semantic fixes:
 a. POST /eth/v2/beacon/blocks (gloas) calls publish_beacon_block(block,
    Vec::new()): gloas columns travel with the envelope. RecordingNetwork
    implements publish_beacon_block(block, sidecars) and both new methods.
 b. produceBlockV4 appends client versions to the graffiti exactly like
    produceBlockV3 (graffiti::execution_client_version run alongside the
    payload build, with_client_versions, Extension<OwnVersion>) and logs it.
 c. Attestation data, aggregate_attestation keep require_execution_client and
    require_validated for gloas slots; payload_attestation_data now applies
    the same two rules (503 without an execution client, or when the voted
    block's payload is unvalidated).
 d. Proposer duties v1 and v2 serve gloas epochs from a fulu or gloas state's
    proposer_lookahead; nothing refuses gloas any more; v2 keeps its
    dependent root.
 e. The VC's per-validator ProposerSettings apply to gloas proposals (graffiti
    in the BlockRequest, fee recipient compared with the bid's); a test pins
    the graffiti. VC tests updated to the ProposerSettings constructors.
 f. gloas production reads the attestation pool from Store and calls the
    stf with the ActiveBalanceCache the perf work added; fulu production and
    pack_operations are untouched (gloas blocks carry no pooled operations).
 g. Chain events are emitted by the chain actor only, so nothing on the RPC
    publish paths needed to move; gloas imports reach it unchanged.
 h. Cargo.lock unchanged; cargo check --locked passes.
Base automatically changed from beacon-chain-integration to main October 20, 2026 16:19
@MegaRedHand
MegaRedHand force-pushed the feat/beacon-gossipsub-scoring branch from 4434d77 to 9c18979 Compare October 20, 2026 17:32
MegaRedHand added a commit that referenced this pull request Oct 21, 2026
Squash of feat/beacon-gossipsub-scoring at 9c18979 (merge-base 35c074b).

Adapted onto the runtime fork-digest switch: the module doc says how the
scored topics follow the current digest.
zexoverz pushed a commit to zexoverz/ethlambda that referenced this pull request Oct 22, 2026
## 🗒️ Description / Motivation

Merges `beacon-chain-integration` into `main`. This adds an **Ethereum
Beacon Chain** client (phase0 through fulu) to the ethlambda binary,
alongside the Lean consensus client:

- **`ethlambda beacon`**: a beacon node. It checkpoint-syncs, follows
the chain over gossip and req/resp, validates execution payloads through
an execution client's Engine API, and serves the Beacon API, including
the validator endpoints.
- **`ethlambda validator`**: a beacon validator client (attest,
aggregate, propose) that talks to any beacon node over the standard REST
API.
- **`ethlambda node`** (lean) keeps its behavior, apart from the items
under "Behavior changes for lean" below.

Both chains run on the same storage layer, P2P actor, RPC crate and
chain actor. `BeaconState` has a `Lean` variant holding lean's `State`,
and each shared handler dispatches on it once, at the top.

Each piece was reviewed as its own PR into the integration branch
(listed under "Related Issues / PRs"). This PR merges that branch.

## What Changed

| Area | Change |
|---|---|
| `ethlambda-types` (`beacon/`) | Containers for phase0 to fulu, presets
(compile-time `preset-minimal` feature), runtime fork config, and a
`Lean` variant on `ForkName` and `BeaconState` |
| `ethlambda-state-transition` (`beacon/`) | State transition, epoch
processing, fork upgrades, fork choice, BLS (`blst`), KZG (`c-kzg`),
gossip validation rules, DAS custody math |
| `ethlambda-ssz-tree` (**new**) | Persistent Merkle `List`/`Vector` for
the validator registry and balances, so consecutive states share
unchanged subtrees |
| `ethlambda-storage` | One `Store` for both chains. Beacon states are
stored as snapshots plus VCDIFF deltas and written on a background
thread. Adds a committee cache, and `db_version`/`chain`/`preset`
metadata that resume checks |
| `ethlambda-blockchain` | Beacon path in the chain actor
(`BlockChain::spawn_beacon`): import, holding blocks until their parent
or columns arrive, applying gossip aggregates to fork choice, block
production and aggregation |
| `ethlambda-engine` (**new**) | Engine API client with JWT auth:
`newPayload`, `forkchoiceUpdated`, `getPayload`, `getBlobs` |
| `ethlambda-p2p` | Beacon wire: blocks, aggregates, subnet attestations
and data columns are validated before relay;
`beacon_blocks_by_{range,root}/2` served and requested; range sync;
column custody and sampling; attestation subnet backbone; discv5 always
on for `beacon` |
| `ethlambda-rpc` | Beacon API under `/eth/v1` and `/eth/v2`: JSON by
default, SSZ on request. The router is picked by `Store::chain()`, so
`/lean/v0` and the Beacon API are never served together |
| `ethlambda-validator` (**new**) | Validator client: duties,
attestations, aggregation selection, proposals, keymanager API. It has
**no slashing protection** (recorded in `docs/spec_deviations.md`) |
| `bin/ethlambda` | `beacon` and `validator` sub-commands; `--network`
takes a built-in name (`mainnet`, `sepolia`, `hoodi`) or a directory of
published network files; beacon checkpoint sync; `benchmark import`
replays a real block corpus offline; `run_node` is one startup path for
both chains |
| CI / Makefile | One beacon spec-test job per preset, minimal-preset
check, `make test` split into `test-consensus` and `test-node` (one
runner's disk no longer fits both), benchmark-smoke and tooling jobs,
disk-cleanup action |
| Docs | New: `beacon_stf.md`, `beacon_wire.md`, `beacon_engine.md`,
`cli.md`. Updated: `data_storage.md`, `metrics.md`, `rpc.md`,
`checkpoint_sync.md`, `discovery.md`, `benchmarking.md`,
`spec_deviations.md`, README (beacon quickstart with ethrex) |

## Correctness / Behavior Guarantees

- **Separation:** beacon behavior lives in `beacon` modules that lean
code never reads, and those modules never read lean's. The shared crates
dispatch on the chain once per handler.
- **Lean state at a beacon accessor:** this panics, naming the function
it reached (`lean_state_unreachable` / `lean_fork_unreachable`), so the
caller never gets a wrong answer back. `ForkName::Lean` is left out of
`ForkName::ALL`, so neither `parse` nor a fork upgrade can reach it.
- **Spec conformance:** every consensus-specs `v1.6.1` fixture case
passes on both presets: 5705 mainnet and 40009 minimal, including 150
mainnet `fork_choice` cases. Gossip fixtures are pinned to
`v1.7.0-beta.1`.
- **Resume safety:** a data directory written by another chain, preset
or format version is refused at startup, before any state is read.

### ⚠️ Behavior changes for lean (`ethlambda node`)

- **Existing data directories are refused on resume.** A directory
written by `main` has no `db_version` key, so `Store::from_db_state`
returns `DbVersionMismatch { found: 0, expected: 4 }`. There is no
migration: an upgraded node needs a fresh data directory (or checkpoint
sync).
- `blst` and `c-kzg` are now dependencies of the lean binary too, since
the state-transition crate holds both chains' rules. They are not
feature-gated.
- libssz is a git dependency pinned by `rev`: the beacon containers need
`DerefMut`/`IndexMut` on `SszList` (lambdaclass/libssz#33), which no
crates.io release has yet.
- The `release-fast` profile (used for tests) now sets `debug-assertions
= true`.
- `P2P::spawn` now runs before the chain actor is spawned, so gossip
arriving in that short window is dropped.
- `/lean/v0/blocks/finalized` answers JSON when asked for it by name.
SSZ stays the default.

## Tests Added / Run

- **Beacon spec tests:** `make test-beacon` (or `test-beacon-mainnet` /
`test-beacon-minimal`). Each fixture case is a separately named test,
e.g. `cargo test -p ethlambda-state-transition --test beacon_spec_tests
--features beacon-spec-tests -- electra/attester_slashing`. CI runs one
job per preset.
- **`make test`** still needs no consensus-spec download: the beacon
spec target is behind the `beacon-spec-tests` feature, and the BLS/KZG
vector tests report as ignored without it.
- **Minimal preset:** the `Test minimal preset` CI job builds and runs
the `ethlambda-types` unit tests with `preset-minimal` on.
- **Benchmark smoke:** a CI job runs the `benchmark` sub-command end to
end.
- Unit and integration tests in every new crate (`ssz-tree`, `engine`,
`validator`) and in the beacon paths of storage, p2p, rpc and the chain
actor.

## Related Issues / PRs

<details>
<summary>PRs merged into the integration branch (reviewed on
<code>lambdaclass/ethlambda_private</code>)</summary>

**Types and state transition**
- lambdaclass/ethlambda_private#1 Beacon Chain containers under a
`beacon` namespace
- lambdaclass/ethlambda_private#2 Beacon Chain state transition, phase0
through fulu
- lambdaclass/ethlambda_private#37 keep the registry and balances in
persistent Merkle trees

**Follower, sync and storage**
- lambdaclass/ethlambda_private#3 follow mainnet gossip behind an
`ethlambda beacon` sub-command
- lambdaclass/ethlambda_private#6 unify the lean and beacon storage
layer
- lambdaclass/ethlambda_private#7 checkpoint sync for the beacon chain
- lambdaclass/ethlambda_private#8 serve beacon blocks by range and by
root
- lambdaclass/ethlambda_private#9 give the follower a chain, from the
anchor to the tip
- lambdaclass/ethlambda_private#11 fulu data availability sampling,
custody through the availability gate
- lambdaclass/ethlambda_private#13 validate execution payloads over the
Engine API
- lambdaclass/ethlambda_private#15 follow a network described by files
on disk
- lambdaclass/ethlambda_private#26 add sepolia and hoodi as built-in
networks
- lambdaclass/ethlambda_private#48, lambdaclass/ethlambda_private#52,
lambdaclass/ethlambda_private#53, lambdaclass/ethlambda_private#54: held
blocks, their columns and range batches

**P2P**
- lambdaclass/ethlambda_private#18 stop outbound req/resp requests
crossing protocols
- lambdaclass/ethlambda_private#23 rank dial candidates by custody
redundancy
- lambdaclass/ethlambda_private#28 count why peers drop us
- lambdaclass/ethlambda_private#38 validate beacon gossip before
propagating it
- lambdaclass/ethlambda_private#43 run every data column check in p2p
- lambdaclass/ethlambda_private#47 validate beacon aggregates and subnet
attestations before relaying
- lambdaclass/ethlambda_private#19 apply gossip aggregates to fork
choice, backbone two attestation subnets

**API and validator**
- lambdaclass/ethlambda_private#21 serve the Beacon API from `ethlambda
beacon`
- lambdaclass/ethlambda_private#44 report the preset and constants from
`/eth/v1/config/spec`
- lambdaclass/ethlambda_private#16 beacon-chain validator client
- lambdaclass/ethlambda_private#34 serve the validator API
- lambdaclass/ethlambda_private#40 aggregate and propose through
`ethlambda beacon`
- lambdaclass/ethlambda_private#56 pack other nodes' attestations into
produced blocks

**Performance and metrics**
- lambdaclass/ethlambda_private#10, lambdaclass/ethlambda_private#20,
lambdaclass/ethlambda_private#22, lambdaclass/ethlambda_private#24,
lambdaclass/ethlambda_private#33, lambdaclass/ethlambda_private#49:
registry scans, parallel pubkey validation, committee cache, background
state writes, BLS key validation
- lambdaclass/ethlambda_private#17, lambdaclass/ethlambda_private#31:
import timing by phase
- lambdaclass/ethlambda_private#25 benchmark block import by replaying a
real corpus

**CI**
- lambdaclass/ethlambda_private#12, lambdaclass/ethlambda_private#30,
lambdaclass/ethlambda_private#45: split lint/test jobs, benchmark smoke
job, runner selection

</details>

Open PRs based on `beacon-chain-integration`, **not** included here:
lambdaclass#626, lambdaclass#627, lambdaclass#629, lambdaclass#630, lambdaclass#632, lambdaclass#634, lambdaclass#636, lambdaclass#638, lambdaclass#642, lambdaclass#644 to lambdaclass#652,
lambdaclass#656, lambdaclass#658.

## ✅ 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
- [ ] Ran `make test-beacon` (both presets), all passing

---------

Co-authored-by: Pablo Deymonnaz <pdeymon@fi.uba.ar>
Co-authored-by: Klaus Lungwitz <klaus.lungwitz@lambdaclass.com>
Co-authored-by: Klaus @ LambdaClass <18153834+klaus993@users.noreply.github.com>
Co-authored-by: Chase Wright <chase.wright@ethereum.org>
@MegaRedHand
MegaRedHand marked this pull request as ready for review October 26, 2026 16:11
@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

Looks good overall. I did not spot a consensus-critical bug in fork choice / STF / attestation processing here; the PR is narrowly scoped to beacon-wire gossipsub scoring, and the math/tests are generally careful.

  • crates/net/p2p/src/swarm_adapter.rs:327 / crates/net/p2p/src/swarm_adapter.rs:329:
    The metric update happens before disconnect attempts. If disconnect_peer_id fails for any graylisted peer, lean_gossipsub_peers_by_score{band="below_graylist"} can report peers that were not actually disconnected, which weakens the operational signal the docs promise. Consider either recomputing after disconnects or documenting that this is a pre-disconnect snapshot.

  • crates/net/p2p/src/beacon/scoring.rs:549:
    active_validator_count() is taken from head_current_committees(), i.e. the head’s current epoch, while wall_slot drives freshness gating. If the node’s wall clock has crossed an epoch boundary but committee_cache has not yet been refreshed to the new epoch, you can spend up to one slot using stale validator-count-derived rates. This is probably harmless operationally, but worth calling out because the whole feature claims “refreshed every slot from the head”; the actual source is “latest cached head current-epoch committees.”

  • crates/net/p2p/src/beacon/scoring.rs:135 and crates/common/types/src/beacon/committees.rs:110:
    u64 casts from usize (self.shuffled.len() as u64) are fine on current targets, but this is one of the few places where consensus-adjacent counts cross abstraction boundaries for networking math. Using u64::try_from(...) would be a bit more defensive/idiomatic if you want to avoid silent truncation assumptions.

  • crates/net/p2p/src/beacon/scoring.rs:417:
    invalid_message_deliveries_weight divides by topic_weight. Today every caller passes a positive nonzero weight, so it is safe, but this helper now underpins a large scoring table and is easy to reuse incorrectly later. A debug_assert!(topic_weight > 0.0) would make that invariant explicit.

  • crates/net/p2p/src/lib.rs:838 and crates/net/p2p/src/lib.rs:1481:
    Startup behavior is sensible: placeholder params first, then an immediate refresh. I like that mesh-delivery scoring is gated while lagging; that avoids self-inflicted graylisting during catch-up, which is the main correctness/security concern in this area.

If you want, I can do a second pass focused only on the scoring formulas against Lighthouse semantics, or scan for any test gaps around epoch-boundary refresh behavior.


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

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Review of PR 656: gossipsub peer scoring on the beacon wire

I read the full diff but did not build it or run the tests. The design is sound and well documented. I found no consensus-affecting bugs. Scoring is beacon-only, lean is untouched, and the only change to state-transition code is making committee_count_per_slot public. The points below are robustness and operational concerns.

What looks right

  • The parameters follow lighthouse's formulas. The tests pin them to lighthouse's numbers, and a parameter-validity sweep covers active = 0, the placeholder, and slots before and after the decay windows.
  • expected_aggregators_per_slot cannot divide by zero. active is floored at 32 and committees_per_slot is at least 1, so smaller is at least 1.
  • Closing the mesh-delivery gate keeps the cap but zeroes the threshold and weight. That is consistent: a zero threshold means no deficit, so the P3b failure penalty (mesh_failure_penalty_weight) set unconditionally in topic_params cannot fire. This is worth a short comment next to that line.
  • MeshDeliveryGate handles a backwards clock with saturating_sub. Tests cover both the warmup and the close-and-restart paths.
  • head_current_committees only reads the cache and never builds a shuffling. That is the right trade-off for a p2p-side read.

Points to consider

  1. build_swarm panics on bad scoring parameters.

    • .expect("valid peer score parameters") in crates/net/p2p/src/lib.rs (the build_swarm hunk) can crash the node at startup.
    • The tests validate only Config::mainnet() with the mainnet preset.
    • A loaded network with a different slot_duration_ms or preset feeds decay_interval = slot.max(1s), time_in_mesh_cap = 3600 / slot and the preset's SLOTS_PER_EPOCH into the formulas. A sub-second slot would also break the "one decay tick per slot" assumption.
    • Return a SwarmBuildError variant instead of panicking, or add a validity test over a couple of non-mainnet configs.
  2. Topic-parameter update failures are logged at debug in execute_command (SwarmCommand::SetTopicScoreParams).

    • If set_topic_params ever rejects a set, the swarm silently keeps stale or placeholder rates. Those rates are 32 validators with P3 off.
    • Log at warn, or at least dedupe the message, since this fires 66 times per slot.
  3. The refresh can stay on placeholder rates for a long time.

    • scoring.active_validators stays at 32 until head_current_committees() returns Some.
    • That happens only after the chain actor has built and pinned the current-epoch shuffling.
    • Until then the first-message-delivery caps are far too small for the real traffic, so P2 saturates immediately. This is probably harmless, but a debug log or a metric showing which validator count is in use would make it observable.
  4. Graylist disconnect churn.

    • A graylisted peer that redials is dropped again within 10 s. The docs acknowledge this, but each cycle costs a full handshake.
    • Consider also feeding the existing peer-ban or backoff logic, if there is any.
    • below_graylist in the metrics will usually read 0 because the same pass disconnects those peers. The docs say so, which is fine.
  5. The scoring-on check is indirect.

    • gossipsub.peer_score(swarm.local_peer_id()).is_none() in swarm_adapter.rs relies on peer_score returning Some for any peer id when scoring is on. It works and is commented, but it depends on libp2p-fork behaviour.
    • A bool captured at swarm build time and passed to swarm_loop would be more robust.
  6. Minor.

    • PeerScoreParams::topics is rebuilt each slot, with 64 identical attestation TopicScoreParams clones. That is cheap, so not an issue.
    • The test only_the_beacon_swarm_scores_peers binds 127.0.0.1:0 but never listens in the test. That is fine.

Overall this is good to merge once Point 1 is addressed or consciously accepted. Points 2 and 5 are cheap polish.


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

@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

I'll review this PR focusing on correctness, security, performance, and Rust best practices. This is a substantial peer scoring implementation for gossipsub.

Overall Assessment

The code is well-structured, thoroughly documented, and includes comprehensive tests. The port from Lighthouse appears careful. However, I've identified several issues ranging from minor correctness concerns to potential security/availability issues.


Critical Issues

1. Integer Division Truncation in expected_aggregators_per_slot (scoring.rs:787-798)

let smaller = active / committees;
let larger_count = active - smaller * committees;
let smaller_modulo = (smaller / TARGET_AGGREGATORS_PER_COMMITTEE).max(1);
let larger_modulo = ((smaller + 1) / TARGET_AGGREGATORS_PER_COMMITTEE).max(1);

Problem: When smaller is 0 (which happens when active < committees), smaller_modulo becomes max(1, 0) = 1, but larger_modulo uses smaller + 1 = 1, also giving 1. However, the actual committee size is 0 or 1, and the aggregator selection formula len / max(1, len / TARGET) would give 0 for empty committees and 1 for single-validator committees. The approximation diverges from the real protocol for very small validator counts.

Impact: Low for mainnet, but the placeholder PLACEHOLDER_ACTIVE_VALIDATORS = 32 with 1 committee per slot gives smaller = 32, which is fine. More concerning: if active = 0 (tested in every_parameter_set_is_valid), committees_per_slot is clamped to 1, committees = 32, smaller = 0, and the function returns a positive number of aggregators despite zero validators.

Suggested fix: Add an early return:

if active == 0 {
    return (0.0, 0);
}

Or at minimum, document that active is expected to be at least PLACEHOLDER_ACTIVE_VALIDATORS in practice.


2. max_positive_score Calculation Doesn't Match Lighthouse (scoring.rs:138-146)

let max_positive_score = (MAX_IN_MESH_SCORE + MAX_FIRST_MESSAGE_DELIVERIES_SCORE)
    * (BEACON_BLOCK_WEIGHT
        + BEACON_AGGREGATE_PROOF_WEIGHT
        + attestation_subnet_weight * ATTESTATION_SUBNET_COUNT as f64
        + VOLUNTARY_EXIT_WEIGHT
        + PROPOSER_SLASHING_WEIGHT
        + ATTESTER_SLASHING_WEIGHT);

Problem: This sums to (10 + 40) * (0.5 + 0.5 + 1.0 + 0.15) = 50 * 2.15 = 107.5, but the test expects 53.75 (half that). The test passes because topic_score_cap = max_positive_score * 0.5 = 53.75.

Verification: Looking at Lighthouse's source, max_positive_score should indeed be the cap before halving. The naming is confusing but the math checks out: topic_score_cap is 0.5 * max_positive_score in Lighthouse too. This is correct, just worth noting the naming divergence.

Status: Correct, but the naming is misleading. Consider a comment explaining that Lighthouse's topic_score_cap is half of what it calls max_positive_score.


3. Missing use std::collections::HashMap in swarm_adapter.rs

At line 291 in the new disconnect_graylisted_peers function:

let mut bands: HashMap<&'static str, i64> =
    scoring::SCORE_BANDS.iter().map(|&band| (band, 0)).collect();

But HashMap is not imported in swarm_adapter.rs. Looking at the file's imports (lines 1-15), there's no HashMap. This is a compile error.

Suggested fix: Add use std::collections::HashMap; to the imports.


Security / Availability Issues

4. Score Manipulation via Clock Skew (scoring.rs:372-380)

pub fn update(&mut self, wall_slot: u64, head_slot: u64) -> bool {
    if wall_slot.saturating_sub(head_slot) > MESH_DELIVERY_MAX_HEAD_LAG {
        self.keeping_up_since = None;
        return false;
    }
    let since = *self.keeping_up_since.get_or_insert(wall_slot);
    // Saturating: the wall clock can step backwards under an NTP correction.
    wall_slot.saturating_sub(since) >= MESH_DELIVERY_WARMUP_SLOTS
}

Problem: If wall_slot steps backward due to NTP correction, saturating_sub returns 0, so the warmup condition 0 >= 32 is false. But if keeping_up_since was already Some(old_value) where old_value > wall_slot, then wall_slot.saturating_sub(since) returns 0 (not a large negative), which is correct.

However, a more subtle issue: if NTP corrects forward significantly, wall_slot jumps ahead, and the warmup completes instantly. This is acceptable (faster activation is safe).

More concerning: A malicious local clock could deliberately set wall_slot far in the past to keep P3 off, or far in the future to instantly complete warmup. But this requires compromising the local system clock, which is outside the threat model.

Status: Acceptable with the saturating arithmetic comment.


5. No Rate Limiting on set_topic_score_params Calls (swarm_adapter.rs:436-442)

SwarmCommand::SetTopicScoreParams(topics) => {
    let gossipsub = &mut swarm.behaviour_mut().gossipsub;
    for (topic, params) in topics {
        let _ = gossipsub
            .set_topic_params(topic, params)
            .inspect_err(|err| debug!(%err, "Swarm adapter: topic score update failed"));
    }
}

Problem: This is called every slot (every 12 seconds on mainnet) with 69 topics. If set_topic_params is expensive in libp2p-gossipsub, this could cause jitter. More importantly, if the swarm task is backlogged, these commands queue up.

Mitigation: The current design seems fine for 12s slots, but consider documenting the complexity. Also, inspect_err with debug! level means failures are invisible in production—consider warn! at least.


Correctness Issues

6. refresh Uses Stale active_validators After committees is None (scoring.rs:830-845)

pub(crate) fn refresh(server: &mut P2PServer) {
    // ...
    if let Some(committees) = server.store.committee_cache().head_current_committees() {
        scoring.active_validators = committees.active_validator_count();
    }

    let inputs = ScoreInputs {
        active_validators: scoring.active_validators,  // Uses stale value if None
        current_slot: wall_slot,
        mesh_deliveries_scored,
    };

Problem: If head_current_committees() returns None (e.g., during startup before the first shuffling is built), scoring.active_validators retains its previous value. This is intentional per the comment ("kept for the ticks where its shuffling is not built yet"), but during initial sync this could be the placeholder value for many slots.

Question: Is the placeholder value of 32 validators safe for scoring? With 32 validators, expected aggregate traffic is very low, so mesh peers would be held to a low threshold. This seems conservative (won't falsely graylist peers), but verify.

Status: Design choice, acceptable.


7. topic_params mesh_scored Logic Allows Negative Threshold When Scored (scoring.rs:410-413)

if mesh_scored {
    params.mesh_message_deliveries_threshold = threshold;
    params.mesh_message_deliveries_weight = -topic_weight;
}

Problem: When mesh_scored is true, threshold could theoretically be negative if rate is negative (impossible here) or if decay_convergence produces negative values (impossible with positive inputs). The clamp on active prevents zero.

However, threshold is decay_convergence(decay, rate / 50.0) * decay. With very small rate, this could underflow to subnormal or zero. Gossipsub's TopicScoreParams::validate() should catch this, and indeed the test every_parameter_set_is_valid tests active = 0 and active = 1.

Verification needed: Does libp2p-gossipsub's validate() reject threshold = 0.0 with weight < 0? The test says it passes, so presumably yes or the weight is 0 in that case.


Performance Issues

8. dynamic_topic_params Allocates 69 Topics Every Slot (scoring.rs:251-305)

let mut topics = vec![
    (IdentTopic::new(topic_name(fork_digest, BEACON_BLOCK)), block),
    (IdentTopic::new(topic_name(fork_digest, BEACON_AGGREGATE_AND_PROOF)), aggregate),
];
topics.extend((0..ATTESTATION_SUBNET_COUNT).map(|subnet_id| {
    let topic = IdentTopic::new(attestation_topic_name(fork_digest, subnet_id));
    (topic, attestation.clone())
}));

Problem: This allocates 69 IdentTopic strings and a Vec every slot. The topics are identical every time except for the parameters. Consider caching the IdentTopic objects.

Suggested optimization: Pre-build the topic list once in ScoreSettings and clone it, or store Arc<str> for topic names.

Priority: Low—69 small allocations every 12 seconds is negligible.


9. disconnect_graylisted_peers Iterates All Peers Synchronously (swarm_adapter.rs:312-335)

for (peer_id, _topics) in gossipsub.all_peers() {
    let Some(score) = gossipsub.peer_score(peer_id) else {
        continue;
    };
    // ...
    if score < scoring::GRAYLIST_THRESHOLD {
        graylisted.push((*peer_id, score));
    }
}

Problem: With thousands of peers, this could take milliseconds. Called every 10 seconds in the swarm task, which must also process network I/O.

Mitigation: The iteration is likely fast enough (peer scores are in memory), but consider if all_peers() has O(n) cost with large meshes. No change needed unless profiling shows an issue.


Rust Best Practices

10. use std::collections::HashSet in scoring.rs is Unused (scoring.rs:56)

use std::collections::HashSet;

Used only for ip_colocation_factor_whitelist: HashSet::new() at line 228. This is correct.


11. pub(crate) vs pub Inconsistency (scoring.rs:97)

pub fn thresholds() -> PeerScoreThresholds {

This is pub but only used within the crate. Consider pub(crate) for consistency with peer_score_params visibility (which is pub on ScoreSettings but ScoreSettings itself may not be exported).

Status: Minor style issue.


12. beacon_wall_slot Changed from fn to pub(crate) fn (gossipsub/handler.rs:682)

Correct and necessary for the new use in scoring::refresh.


Documentation and Maintainability

13. Excellent Documentation

The module-level documentation in scoring.rs (lines 1-55) is outstanding. The comparison table, the explicit list of differences from Lighthouse, and the rationale for design choices make this maintainable.


14. Test Coverage is Comprehensive

Tests cover:

  • Lighthouse numerical compatibility
  • Parameter validation for edge cases
  • Gate behavior
  • Young chain behavior
  • Topic coverage
  • Swarm integration

Gap: No test for the actual disconnection logic in swarm_adapter.rs. The disconnect_graylisted_peers function has no unit test.


Summary Table

Priority Item File:Line Issue
Compile error 3 swarm_adapter.rs:291 Missing HashMap import
High 1 scoring.rs:787-798 active = 0 gives incorrect aggregator count
Medium 5 swarm_adapter.rs:436-442 set_topic_params failure silently logged at debug
Medium 7 scoring.rs:410-413 Verify gossipsub accepts very small thresholds
Low 8 scoring.rs:251-305 Allocates 69 topics every slot
Low 9 swarm_adapter.rs:312-335 Peer iteration in hot loop
Low 11 scoring.rs:97 Visibility inconsistency
Style — — Consider caching topic names

Recommended Changes

  1. Fix the compile error in swarm_adapter.rs:

    use std::collections::HashMap;  // Add this import
  2. Add guard for active = 0 in expected_aggregators_per_slot:

    fn expected_aggregators_per_slot(active: u64) -> (f64, u64) {
        if active == 0 {
            return (0.0, 0);
        }
        // ... rest
    }
  3. Consider warn! level for set_topic_params failures, or at least document why debug! is sufficient.

  4. Add a test for disconnect_graylisted_peers behavior, perhaps by mocking the gossipsub peer list.

The core scoring logic appears correct and the Lighthouse port is faithful. The MeshDeliveryGate is a thoughtful addition that addresses a real availability concern for startup/catch-up scenarios.


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

@ilitteri ilitteri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two fork-rollover issues need to be addressed before merge.

  1. High: old-fork topics retain live P3 scoring and can graylist honest peers. refresh updates dynamic parameters only for wire.fork_digest (crates/net/p2p/src/beacon/scoring.rs:561-564), leaving the previous digest's nonzero threshold and weight active. Old topics remain subscribed for two epochs while honest peers move traffic to the new digest; their old-topic counters then decay without deliveries until disconnect_graylisted_peers acts on the resulting score (crates/net/p2p/src/swarm_adapter.rs:318-330). Using this PR's mainnet parameters with one million validators and a previously full aggregate counter, the old aggregate topic alone crosses the -16,000 graylist threshold after 53 silent slots, within the 64-slot retention window. Please retire/zero old-digest weights and P3 parameters, and apply gate changes to every held digest.

  2. Medium: new-fork topics have incomplete scoring. Full startup parameters include exit and slashing topics (scoring.rs:230-234, :263-284), but refresh installs only dynamic topics, so those fixed topics receive no parameters under subsequent digests and invalid messages escape P4. Also, future-digest topics can be GRAFTed during the one-epoch pre-fork subscription window before parameters exist; pinned gossipsub does not initialize mesh state when parameters are later installed, leaving those peers without P1/P3 until re-GRAFT or reconnect. Please install complete parameters for current and upcoming held digests before subscriptions/GRAFTs.

Please add runtime rollover coverage for both cases, including graylist behavior and a mesh formed before activation.

@ilitteri ilitteri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional specification confirmation for the old-fork P3 finding: specs/altair/p2p-interface.md:442-445 says that, post-fork, peers propagating messages on pre-fork topics MUST NOT be scored negatively. Keeping the old digest's P3 threshold and weight active therefore is not only a graylist risk, it also violates the gossip-transition rule.

Lighthouse explicitly calls remove_topic_weight_except(new_fork_digest) at the transition to prevent peers on old topics from being penalized (beacon_node/network/src/service.rs:921-923). Prysm currently disables P3 mesh-delivery scoring, and installs fixed-topic parameters on every new subscription, which is consistent with the two recommended changes in the existing review.

@MegaRedHand

Copy link
Copy Markdown
Collaborator Author

Both fixed in dadac7a.

Every held digest now gets the whole parameter set (exits and slashings included), by role, sent by transition::apply before it subscribes. Both travel the swarm's one command channel, so the parameters exist at the first GRAFT.

Digest Topic weights P3
upcoming full off
current full per gate
previous 0 off
  • The gate only reaches the current digest, since no other digest ever has P3 on; that is how "apply gate changes to every held digest" is covered.
  • Added: a switch restarts the P3 warmup. SUBSCRIBE_LEAD_EPOCHS equals the block/aggregate mesh_message_deliveries_activation, so peers grafted when the next digest is joined become active exactly at the boundary, with empty counters. Lighthouse avoids this by joining only two slots ahead.
  • With the old parameters, leaving the old topics after two epochs would also have charged every old-mesh peer the sticky P3b (make_prune → prune).
  • Trade-off, same as lighthouse: with the previous digest's weight at 0, invalid messages on its topics stop costing P4 during those two epochs.

Tests:

  • a_rollover_parametrizes_each_digest_before_joining_it: the real apply across the boundary, with the swarm's commands captured in order.
  • a_mesh_grafted_ahead_of_its_fork_is_scored: two real swarms, with a control topic joined before its parameters that stays at 0. The positive case checks P1 > 0, since graft() sets the same mesh_status that P3 activation reads.
  • a_previous_digest_never_graylists_its_silent_mesh: a tick-by-tick model of the pinned peer_score.rs arithmetic rather than a live run, since a live one needs 53 slots of decay. It reproduces your 53 slots for the old parameters, and zero P3 and P3b for the new ones.

The first two fail if the parameters are sent after the subscriptions, and the first fails if the switch skips the warmup restart.

@pablodeymo
pablodeymo force-pushed the feat/beacon-gossipsub-scoring branch from dadac7a to 8e90e57 Compare October 27, 2026 19:38
@pablodeymo
pablodeymo changed the base branch from main to perf/beacon-epoch-precompute October 27, 2026 19:38
@MegaRedHand
MegaRedHand added this pull request to stack #711 October 27, 2026 19:59
@MegaRedHand
MegaRedHand merged commit 6cf0310 into feat/prune-finalized-states Oct 27, 2026
12 checks passed
@MegaRedHand
MegaRedHand deleted the feat/beacon-gossipsub-scoring branch October 27, 2026 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beacon Ethereum Beacon Chain client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants