p2p/discover: fix crash when revalidated node is removed

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)
+}

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 <http://www.gnu.org/licenses/>.
+
+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)
+	}
+}

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
 }