p2p/discover: schedule revalidation also when all nodes are excluded

[dknopik]

Issue

If nextTime has passed, but all nodes are excluded, get would return nil and run would therefore not invoke schedule. Then, we schedule a timer for the past, as neither nextTime value has been updated. This creates a busy loop, as the timer immediately returns.

Fix

With this PR, revalidation will be also rescheduled when all nodes are excluded.

[lightclient]

I would apply a patch like this:

diff --git a/p2p/discover/table_reval.go b/p2p/discover/table_reval.go
index c043dbd14..2465fee90 100644
--- a/p2p/discover/table_reval.go
+++ b/p2p/discover/table_reval.go
@@ -77,12 +77,18 @@ func (tr *tableRevalidation) nodeEndpointChanged(tab *Table, n *tableNode) {
 // It returns the next time it should be invoked, which is used in the Table main loop
 // to schedule a timer. However, run can be called at any time.
 func (tr *tableRevalidation) run(tab *Table, now mclock.AbsTime) (nextTime mclock.AbsTime) {
-       if n := tr.fast.getAndSchedule(now, &tab.rand, tr.activeReq); n != nil {
-               tr.startRequest(tab, n)
-       }
-       if n := tr.slow.getAndSchedule(now, &tab.rand, tr.activeReq); n != nil {
-               tr.startRequest(tab, n)
+       reval := func(list *revalidationList) {
+               if list.nextTime <= now {
+                       if n := list.get(now, &tab.rand, tr.activeReq); n != nil {
+                               tr.startRequest(tab, n)
+                       }
+                       // Update nextTime regardless if any requests were started because
+                       // current value has passed.
+                       list.schedule(now, &tab.rand)
+               }
        }
+       reval(&tr.fast)
+       reval(&tr.slow)

        return min(tr.fast.nextTime, tr.slow.nextTime)
 }
@@ -196,13 +202,8 @@ type revalidationList struct {
        name     string
 }

-// getAndSchedule returns a random node from the queue. Nodes in the 'exclude' map are not returned.
-// It also schedules the next invocation if nextTime has already passed.
-func (list *revalidationList) getAndSchedule(now mclock.AbsTime, rand randomSource, exclude map[enode.ID]struct{}) *tableNode {
-       if now < list.nextTime {
-               return nil
-       }
-       list.schedule(now, rand)
+// get returns a random node from the queue. Nodes in the 'exclude' map are not returned.
+func (list *revalidationList) get(now mclock.AbsTime, rand randomSource, exclude map[enode.ID]struct{}) *tableNode {
        if len(list.nodes) == 0 {
                return nil
        }

The current PR is forcing more logic into get(..) whereas I think it's easier to understand what's happening from a top-down perspective in run(..).

[fjl]

I agree with @lightclient but in his patch the condition seems inverted. We only want to perform work and reschedule if list.nextTime >= now.

[dknopik]

@lightclient
Good idea, applied!

@fjl
Matt's suggestion of list.nextTime <= now (as condition for rescheduling) should be equivalent to the previous condition now < list.nextTime (for NOT rescheduling)

[fjl]

Oh right.