Skip to content

p2p/discover: evict unresponsive nodes topdisc#71

Open
srene wants to merge 19 commits into
topdiscfrom
feat/topic-failure-blacklist
Open

p2p/discover: evict unresponsive nodes topdisc#71
srene wants to merge 19 commits into
topdiscfrom
feat/topic-failure-blacklist

Conversation

@srene

@srene srene commented Jun 2, 2026

Copy link
Copy Markdown
Member

Removes nodes that have gone away from the topic-discovery state. Failures are observed where topic RPC responses are already handled — on the goroutine that owns each state — so there is no global failure tracking and no shared counters.

Event Reg tables Search table Ad cache
REGTOPIC timeout attempt removed (this topic) ads removed
TOPICQUERY timeout attempt removed (all topics) node removed ads removed
DHT node removed attempt removed (all topics) ads removed

Only errTimeout is treated as a liveness signal. A call can also fail with errClosed, which means we cancelled it from our own side — node shutdown, a topic deregistration, or a search iterator Close — so the node's liveness is unknown; it is ignored everywhere and never triggers eviction, matching lookup.go which likewise skips failure recording on cancel.

A timeout is a global liveness signal (the node is dead, not just bad for one topic), so errTimeout in the search loop routes through evictNode — the same fan-out a DHT removal uses — evicting the node from the ad cache and every registration table. The reg loop still handles its own REGTOPIC failures locally (own attempt + ads) rather than via evictNode, to avoid a blocking self-send into its own eviction channel.

Registration table. A failed REGTOPIC drops the attempt via Registration.HandleErrorResponse (pre-existing behavior). New: DHT removals and failed TOPICQUERYs also evict parked attempts (Registration.RemoveNode), because those can sit unprobed for a long time — Standby nodes are never contacted and Registered ones only at ad expiry, so a dead registrar would otherwise squat in its bucket holding a phantom ad. In-flight attempts are left for their pending response handling.

Search table. New Search.HandleErrorResponse: a failed TOPICQUERY evicts the queried node, freeing its bucket slot and IP-limit entry. The failure does not count as a response: the bucket keeps gating the query walk while it has remaining candidates; once they all fail it empties and no longer blocks the walk (#60 semantics), and a search whose nodes all fail becomes IsDone and rolls over. A query that delivered some nodes before erroring is treated as a response (kept, with the error logged). Search tables ignore DHT removals: their state is short-lived and every node in it is queried within seconds.

Ad cache. It has no liveness probe of its own — advertisers are never the target of topic RPCs — so it is cleaned from both external signals: topic RPC timeouts observed by the reg/search loops, and DHT routing-table removals (Table.subscribeRemovedNodes, #21). The DHT-removal signal is drained off removedFeed (sent under the table lock) on a dedicated goroutine that only forwards events to a worker, so the eviction work never backs up the feed under the lock.

Closes #21.

@srene
srene requested a review from fjl as a code owner June 2, 2026 07:47
@srene
srene marked this pull request as draft June 2, 2026 08:04
@srene
srene force-pushed the feat/topic-failure-blacklist branch from 430165e to 5906bf0 Compare June 2, 2026 08:32
srene added a commit that referenced this pull request Jun 2, 2026
Adds a churn workload (-churn-interval / -churn-frac) that kills a fraction of
the active nodes each round during the search phase, exercising the
failure-driven blacklist/eviction path (#71): killed nodes stop answering RPCs,
peers' reval pings/queries time out, and after MaxNodeFailures they are
blacklisted and evicted. Reports blacklist growth and killed-registrant
visibility decay over time. Adds Blacklist.Len() + UDPv5.BlacklistLen() for
observability.
srene added a commit that referenced this pull request Jun 4, 2026
Adds a churn workload (-churn-interval / -churn-frac) that kills a fraction of
the active nodes each round during the search phase, exercising the
failure-driven blacklist/eviction path (#71): killed nodes stop answering RPCs,
peers' reval pings/queries time out, and after MaxNodeFailures they are
blacklisted and evicted. Reports blacklist growth and killed-registrant
visibility decay over time. Adds Blacklist.Len() + UDPv5.BlacklistLen() for
observability.
@srene srene changed the title p2p/discover: blacklist nodes that repeatedly fail to respond p2p/discover: topic-discovery liveness eviction (failure blacklist + DHT-eviction hook) Jun 6, 2026
srene added a commit that referenced this pull request Jun 15, 2026
Adds a count accessor on the topic failure blacklist (#71) and a UDPv5.BlacklistLen() wrapper, so the churn testbed can sample how many nodes are blacklisted over time as a failure-eviction metric.
@srene srene changed the title p2p/discover: topic-discovery liveness eviction (failure blacklist + DHT-eviction hook) p2p/discover: topic-discovery liveness eviction Jun 16, 2026
@srene srene changed the title p2p/discover: topic-discovery liveness eviction p2p/discover: topic-discovery liveness check Jun 23, 2026
@srene srene changed the title p2p/discover: topic-discovery liveness check p2p/discover: topic-discovery liveness verifications Jun 23, 2026
@srene
srene force-pushed the feat/topic-failure-blacklist branch from e0f6e2c to 0d75e20 Compare June 23, 2026 06:37
srene added a commit that referenced this pull request Jun 23, 2026
Reworks #71 from "failure blacklist" to liveness eviction with no ban. A
node that has gone away is removed from the topic state by two cooperating
but separate mechanisms, so it leaves both the topic tables and the DHT:

  - TopDisc eviction (membership-independent): the topic system counts
    consecutive failed topic-discovery RPCs per node in an in-memory map and,
    at maxNodeFailures, evicts the node from the registration/search tables
    and ad cache directly. This covers nodes that are NOT in the routing
    table — advertisers without a bucket slot, candidates learned from query
    responses — which a routing-table-only approach would never clean.
  - topic -> DHT: the same call outcomes are fed into tab.trackRequest, so a
    node failing topic RPCs is also dropped from the routing table.
  - DHT -> topic (#21): Table.subscribeRemovedNodes evicts a node from the
    topic state when the core DHT drops it (its own revalidation or the hook
    above).

Removes the Blacklist type, BlacklistTTL/Blacklist config, the Contains()
gating and banAndEvict. No nodeDB change: the per-node counter is in-memory
(consecutive-failure counts are ephemeral), so FindFailsV5 is left untouched.
maxFindnodeFailures is renamed maxNodeFailures since it now bounds both
findnode and topic-discovery failures. Failure tests assert eviction and the
FindFails feed instead of banning.

Evicted nodes are not remembered — once the DHT re-learns a node it can
re-enter — matching discv5's "evict, don't remember" model. Simnet (5k,
sustained churn) shows the ban adds nothing over eviction (dead-results
~11.5% either way) while removing eviction is measurably worse (~15%).

Closes #21.
@srene
srene force-pushed the feat/topic-failure-blacklist branch 6 times, most recently from fec145b to bf8887a Compare June 23, 2026 09:42
@srene srene self-assigned this Jun 23, 2026
@srene
srene marked this pull request as ready for review June 23, 2026 09:49
@srene
srene force-pushed the feat/topic-failure-blacklist branch 2 times, most recently from 7680d1a to 247dbd7 Compare June 23, 2026 09:55
@srene srene changed the title p2p/discover: topic-discovery liveness verifications p2p/discover: evict unresponsive nodes in topic RPC response handling Jul 7, 2026
@srene
srene force-pushed the feat/topic-failure-blacklist branch 2 times, most recently from 985c44f to df2575f Compare July 7, 2026 08:49
@srene
srene marked this pull request as draft July 7, 2026 09:16
Failed topic RPCs drop the node from the reg/search table that sent
them. Timeouts also evict the node's ads. DHT removals (#21) evict
ads and parked registration attempts; search tables are short-lived
and clean themselves.

Closes #21.
@srene
srene force-pushed the feat/topic-failure-blacklist branch from df2575f to 0faded3 Compare July 7, 2026 09:19
srene added 2 commits July 13, 2026 10:56
…arch node

The search bucket 'asked' set stored only node IDs, so removeNode could
release the bucket's IP-limit entry only for nodes still in 'new'.
Removing a node that had moved to 'asked' leaked its /24 reservation
(searchBucketIPLimit is 1 per /24), blocking same-subnet replacements.

Store the node in 'asked' too and free the IP regardless of which set it
was in. Not reachable today (failures only hit 'new' nodes) but removes a
latent footgun.

Refs #88.
removedFeed.Send runs while the table holds its lock, and the eviction
work it triggers (an onDispatchCh round-trip plus a fan-out to every
registration's evictCh) can block on goroutines that themselves take the
table lock via tab.allNodes(). Draining the feed on the same goroutine as
that work therefore forms a lock-ordering cycle: once the removed-node
buffer fills, the table's lock-held Send blocks, wedging all routing-table
mutation.

Split the two: a worker goroutine performs the blocking eviction from an
internal queue while the subscribing goroutine only forwards feed events
into the queue, so the feed is always drained promptly and Send never
backs up under the table lock.
srene added 3 commits July 13, 2026 12:10
A TOPICQUERY timeout means the queried node is dead globally, not only
for that search. Route it through evictNode (ad cache + every registration
table) — the same liveness path a DHT removal uses — instead of evicting
ads alone. Registration attempts can sit unprobed for a long time (Standby
never contacted, Registered only at ad expiry), so a search failure may be
the first liveness signal they get.

The reg loop still handles its own REGTOPIC failures locally rather than
via evictNode, to avoid a blocking self-send into its own evictCh.

Adds TestTopicEvictNodeRemovesRegistration, which also covers the
evictNode -> registration fan-out (previously only the ad-cache half was
tested).
…-off

Replace the mutex + unbounded slice + wake/done signaling with a buffered
work channel and a non-blocking forward. The feed is still drained on a
dedicated goroutine (so removedFeed's lock-held Send never backs up) and
the blocking eviction runs on a joined worker, but a removal is dropped if
the worker falls >1024 behind. Eviction is best-effort — a missed one is
recovered by ad expiry or the node's own RPC failure — and drops are
logged.
@srene
srene marked this pull request as ready for review July 13, 2026 10:44
srene added a commit that referenced this pull request Jul 13, 2026
@srene srene changed the title p2p/discover: evict unresponsive nodes in topic RPC response handling p2p/discover: evict unresponsive nodes topdisc Jul 20, 2026
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.

Passive revalidation of registration & search tables via DHT eviction callbacks

1 participant