diff --git a/p2p/discover/table_reval.go b/p2p/discover/table_reval.go index 9a13900ebc..9a3b69cfb0 100644 --- a/p2p/discover/table_reval.go +++ b/p2p/discover/table_reval.go @@ -43,6 +43,14 @@ type revalidationResponse struct { didRespond bool } +type revalStatus byte + +const ( + revalStatusGone revalStatus = iota + revalStatusFailed + revalStatusOK +) + func (tr *tableRevalidation) init(cfg *Config) { tr.activeReq = make(map[enode.ID]struct{}) tr.fast.nextTime = never @@ -119,12 +127,27 @@ func (tab *Table) doRevalidate(resp revalidationResponse, node *enode.Node) { } // handleResponse processes the result of a revalidation request. -func (tr *tableRevalidation) handleResponse(tab *Table, resp revalidationResponse) { - now := tab.cfg.Clock.Now() - n := resp.n - b := tab.bucket(n.ID()) +func (tr *tableRevalidation) handleResponse(tab *Table, resp revalidationResponse) revalStatus { + var ( + now = tab.cfg.Clock.Now() + n = resp.n + b = tab.bucket(n.ID()) + ) delete(tr.activeReq, n.ID()) + if !resp.list.contains(n) { + tab.log.Debug("Revalidated node is gone", "b", b.index, "id", n.ID(), "checks", "q", resp.list.name) + return revalStatusGone + } + // Store potential seeds in database. + // This is done via defer to avoid holding Table lock while writing to DB. + defer func() { + if n.isValidatedLive && n.livenessChecks > 5 { + tab.db.UpdateNode(resp.n.Node) + } + }() + + // Remaining logic needs access to Table internals. tab.mutex.Lock() defer tab.mutex.Unlock() @@ -136,7 +159,7 @@ func (tr *tableRevalidation) handleResponse(tab *Table, resp revalidationRespons } else { tr.moveToList(&tr.fast, resp.list, n, now, &tab.rand) } - return + return revalStatusFailed } // The node responded. @@ -159,11 +182,7 @@ func (tr *tableRevalidation) handleResponse(tab *Table, resp revalidationRespons } else { tr.moveToList(&tr.fast, resp.list, n, now, &tab.rand) } - - // Store potential seeds in database. - if n.isValidatedLive && n.livenessChecks > 5 { - tab.db.UpdateNode(resp.n.Node) - } + return revalStatusOK } func (tr *tableRevalidation) moveToList(dest, source *revalidationList, n *node, now mclock.AbsTime, rand randomSource) { @@ -221,3 +240,7 @@ func (list *revalidationList) remove(n *node) bool { } return true } + +func (list *revalidationList) contains(n *node) bool { + return slices.Contains(list.nodes, n) +} diff --git a/p2p/discover/table_reval_test.go b/p2p/discover/table_reval_test.go new file mode 100644 index 0000000000..4b66d27bbf --- /dev/null +++ b/p2p/discover/table_reval_test.go @@ -0,0 +1,65 @@ +// Copyright 2024 The go-ethereum Authors +// This file is part of the go-ethereum library. +// +// The go-ethereum library is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// The go-ethereum library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with the go-ethereum library. If not, see . + +package discover + +import ( + "net" + "testing" + "time" + + "github.com/ethereum/go-ethereum/common/mclock" +) + +// This test checks that revalidation can handle a node disappearing while +// a request is active. +func TestRevalidationNodeRemoved(t *testing.T) { + var ( + clock mclock.Simulated + transport = newPingRecorder() + tab, db = newInactiveTestTable(transport, Config{Clock: &clock}) + tr = &tab.revalidation + ) + defer db.Close() + + // Fill a bucket. + node := nodeAtDistance(tab.self().ID(), 255, net.IP{77, 88, 99, 1}) + tab.handleAddNode(addNodeOp{node: node}) + + // Start a revalidation request. Schedule once to get the next start time, + // then advance the clock to that point and schedule again to start. + next := tr.run(tab, clock.Now()) + clock.Run(time.Duration(next + 1)) + next = tr.run(tab, clock.Now()) + if len(tr.activeReq) != 1 { + t.Fatal("revalidation request did not start:", tr.activeReq) + } + + // Delete the node. + tab.deleteInBucket(tab.bucket(node.ID()), node.ID()) + + // Now finish the revalidation request. + var resp revalidationResponse + select { + case resp = <-tab.revalResponseCh: + case <-time.After(1 * time.Second): + t.Fatal("timed out waiting for revalidation") + } + status := tr.handleResponse(tab, resp) + if status != revalStatusGone { + t.Fatal("wrong revalidation status: got", status, ", want", revalStatusGone) + } +} diff --git a/p2p/discover/table_util_test.go b/p2p/discover/table_util_test.go index 86c6048094..59045bf2a8 100644 --- a/p2p/discover/table_util_test.go +++ b/p2p/discover/table_util_test.go @@ -43,9 +43,15 @@ func init() { } func newTestTable(t transport, cfg Config) (*Table, *enode.DB) { + tab, db := newInactiveTestTable(t, cfg) + go tab.loop() + return tab, db +} + +// newInactiveTestTable creates a Table without running the main loop. +func newInactiveTestTable(t transport, cfg Config) (*Table, *enode.DB) { db, _ := enode.OpenDB("") tab, _ := newTable(t, db, cfg) - go tab.loop() return tab, db }