Add explore state machine to expand population of routing table #934

iand · 2023-09-21T14:01:22Z

No description provided.

* Move coord and kadt packages to internal * go mod tidy * go fmt * Move kadt out of internal and add RoutingTable interface

dennis-tra · 2023-09-22T15:36:35Z

v2/internal/coord/coordinator.go

+	exploreCfg.Clock = cfg.Clock
+	exploreCfg.Timeout = cfg.QueryTimeout
+
+	schedule, err := routing.NewDynamicExploreSchedule(14, cfg.Clock.Now(), time.Hour, 1, 0)


also: // TODO: expose more config

dennis-tra · 2023-09-22T15:41:53Z

v2/internal/coord/routing.go

-// A RoutingBehaviour provices the behaviours for bootstrapping and maintaining a DHT's routing table.
+const (
+	// IncludeQueryID is the id for connectivity checks performed by the include state machine.
+	// This identifier used for routing network responses to the state machine.


Suggested change

// This identifier used for routing network responses to the state machine.

// This identifier is used for routing network responses to the state machine.

dennis-tra · 2023-09-22T15:42:00Z

v2/internal/coord/routing.go

+	IncludeQueryID = coordt.QueryID("include")
+
+	// ProbeQueryID is the id for connectivity checks performed by the probe state machine
+	// This identifier used for routing network responses to the state machine.


Suggested change

// This identifier used for routing network responses to the state machine.

// This identifier is used for routing network responses to the state machine.

dennis-tra · 2023-09-22T15:42:30Z

v2/internal/coord/routing.go

@@ -28,6 +38,9 @@ type RoutingBehaviour struct {
 	// probe is the node probing state machine, responsible for periodically checking connectivity of nodes in the routing table
 	probe coordt.StateMachine[routing.ProbeEvent, routing.ProbeState]

+	// probe is the routing table explore state machine, responsible for increasing the occupanct of the routing table


Suggested change

// probe is the routing table explore state machine, responsible for increasing the occupanct of the routing table

// explore is the routing table explore state machine, responsible for increasing the occupancy of the routing table

dennis-tra · 2023-09-22T15:43:55Z

v2/internal/coord/routing.go

@@ -112,7 +127,7 @@ func (r *RoutingBehaviour) notify(ctx context.Context, ev BehaviourEvent) {
 	case *EventGetCloserNodesSuccess:
 		span.SetAttributes(attribute.String("event", "EventGetCloserNodesSuccess"), attribute.String("queryid", string(ev.QueryID)), attribute.String("nodeid", ev.To.String()))
 		switch ev.QueryID {
-		case "bootstrap":
+		case routing.BootstrapQueryID:


possible to also lift this constant out of the routing package or move all constants into that? just to be consistent

It depends where the query or request is initiated. The connectivity checks performed by probe and include are initiated by the routing behaviour so the constants belong there. The bootstrap and explore queries are initiated by the state machines.

I'd like to move the behaviours into the packages with the state machines at some point so the constants would all end up there too.

dennis-tra · 2023-09-22T15:47:30Z

v2/internal/coord/routing/bootstrap.go

-		queryID := coordt.QueryID("bootstrap")
-
-		qry, err := query.NewFindCloserQuery[K, N, any](b.self, queryID, b.self.Key(), iter, tev.KnownClosestNodes, qryCfg)
+		qry, err := query.NewFindCloserQuery[K, N, any](b.self, BootstrapQueryID, b.self.Key(), iter, tev.KnownClosestNodes, qryCfg)


I don't see a great benefit now but there's a coordt.Message type

Suggested change

qry, err := query.NewFindCloserQuery[K, N, any](b.self, BootstrapQueryID, b.self.Key(), iter, tev.KnownClosestNodes, qryCfg)

qry, err := query.NewFindCloserQuery[K, N, coordt.Message](b.self, BootstrapQueryID, b.self.Key(), iter, tev.KnownClosestNodes, qryCfg)

dennis-tra · 2023-09-22T15:48:56Z

v2/internal/coord/routing/explore.go

+// before proceeding to the next bucket.
+//
+// The frequency of running an explore varies by bucket distance, such that closer buckets are processed more frequently.
+type Explore[K kad.Key[K], N kad.NodeID[K]] struct {


will an expore operation also be triggered when we are low on peers in the routing table?

Not automatically. But it can be added. I will create an issue for it

dennis-tra · 2023-09-22T16:00:43Z

v2/internal/coord/routing/explore.go

+			Target:  st.Target,
+		}
+	case *query.StateQueryFinished[K, N]:
+		span.SetAttributes(attribute.String("out_state", "StateExploreFinished"))


in other places I used tele.AttrOutEvent(...). I also used the defer to capture the event that's passed out of this function.

dennis-tra · 2023-09-22T16:07:28Z

v2/internal/coord/routing/explore.go

+func (s *DynamicExploreSchedule) NextCpl(ts time.Time) (int, bool) {
+	// is an explore due yet?
+	next := (*s.cpls)[0]
+	if !next.Due.After(ts) {
+		// update its schedule
+
+		interval := float64(s.interval) + float64(s.interval)*float64(s.maxCpl-next.Cpl)*s.multiplier
+		interval *= 1 + rand.Float64()*s.jitter
+
+		next.Due = ts.Add(s.cplInterval(next.Cpl))
+		heap.Fix(s.cpls, 0) // update the heap
+		return next.Cpl, true
+	}
+
+	return -1, false
+}


I think it would be more go-like to unnest the happy-path:

func (s *DynamicExploreSchedule) NextCpl(ts time.Time) (int, bool) { // is an explore due yet? next := (*s.cpls)[0] if next.Due.After(ts) { return -1, false } ... }

Agree, will make that change

dennis-tra · 2023-09-22T16:07:50Z

v2/internal/coord/routing/explore.go

+		interval := float64(s.interval) + float64(s.interval)*float64(s.maxCpl-next.Cpl)*s.multiplier
+		interval *= 1 + rand.Float64()*s.jitter
+


Moved to cplInterval

Suggested change

interval := float64(s.interval) + float64(s.interval)*float64(s.maxCpl-next.Cpl)*s.multiplier

interval *= 1 + rand.Float64()*s.jitter

dennis-tra · 2023-09-22T16:18:12Z

Two general comments:

we can only have one explore query at a time, right?
the v1 logic actually explores from lowest CPL (bucket 0) to highest CPL (bucket 15). The description of the NewDynamicExploreSchedule and the code suggest that the logic in here does it the other way around. I think it's more reasonable to do it the way v1 does it because virtually half of the queries to the routing table will hit bucket 0 (assuming uniform keys we want to query) so it's important to have that bucket filled early.
Given the ExploreSchedule interface, it would just be a matter of adding a new implementation so we could also add it in a separate PR.

iand · 2023-09-25T09:58:18Z

Two general comments:

we can only have one explore query at a time, right?

Yes, by design. Do you think that is an issue?

the v1 logic actually explores from lowest CPL (bucket 0) to highest CPL (bucket 15). The description of the NewDynamicExploreSchedule and the code suggest that the logic in here does it the other way around. I think it's more reasonable to do it the way v1 does it because virtually half of the queries to the routing table will hit bucket 0 (assuming uniform keys we want to query) so it's important to have that bucket filled early.

We had some discussion on this in the design issue and @guillaumemichel seemed to be of the opinion that exploring closer buckets more frequently was useful (one comment here).

My reasoning is that since every peer discovered is a candidate for inclusion in the routing table, querying for nearby peers will almost certainly discover peers that populate further buckets first. Bucket 0 is likely to fill up with every explore we do.

Given the ExploreSchedule interface, it would just be a matter of adding a new implementation so we could also add it in a separate PR.

Yes, this would be quite simple to add.

dennis-tra · 2023-09-25T10:06:20Z

Cool, thanks for the clarification! Makes sense 👍

Yes, by design. Do you think that is an issue?

Nope, all good! Just wanted to verify my understanding of what's going on.

iand added 6 commits September 20, 2023 00:52

Improve query capabilities

86285a0

go mod tidy

d5f37fe

Review feedback

ac0e316

go mod tidy

a9f130e

Move coord packages to internal (#933)

13934e2

* Move coord and kadt packages to internal * go mod tidy * go fmt * Move kadt out of internal and add RoutingTable interface

Add explore state machine to expand population of routing table

41ec1f2

iand added the v2 All issues related to the v2 rewrite label Sep 21, 2023

iand changed the base branch from v2-routing-getvalue2 to v2-develop September 21, 2023 14:05

iand added 5 commits September 21, 2023 15:18

Merge branch 'v2-develop' into v2-sm-explore

9c57a11

Refactor schedule into separate type

d57064e

Add generation of random peer id for a given cpl

cc04196

go mod tidy

5544ee3

Add prefixmap generator

6a314fb

iand marked this pull request as ready for review September 22, 2023 08:23

iand requested a review from dennis-tra as a code owner September 22, 2023 08:23

iand marked this pull request as draft September 22, 2023 08:26

iand added 5 commits September 22, 2023 12:04

Use constants for various query ids

5e163c5

Merge branch 'v2-develop' into v2-sm-explore

85413a7

go mod tidy

32f1920

Wire explore state machine into routing behaviour

bc0ec84

Remove some unnecessary conversions

9927b5f

iand marked this pull request as ready for review September 22, 2023 14:45

dennis-tra approved these changes Sep 22, 2023

View reviewed changes

PR review updates

b9cf374

iand mentioned this pull request Sep 25, 2023

Explore routing table to increase occupancy when it is low on peers #936

Closed

iand merged commit ae5a094 into v2-develop Sep 25, 2023
11 checks passed

iand deleted the v2-sm-explore branch September 25, 2023 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explore state machine to expand population of routing table #934

Add explore state machine to expand population of routing table #934

iand commented Sep 21, 2023

dennis-tra Sep 22, 2023

dennis-tra Sep 22, 2023

dennis-tra Sep 22, 2023

dennis-tra Sep 22, 2023

dennis-tra Sep 22, 2023

iand Sep 25, 2023

dennis-tra Sep 22, 2023

dennis-tra Sep 22, 2023

iand Sep 25, 2023

iand Sep 25, 2023

dennis-tra Sep 22, 2023

dennis-tra Sep 22, 2023

iand Sep 25, 2023

iand Sep 25, 2023

dennis-tra Sep 22, 2023

dennis-tra commented Sep 22, 2023 •

edited

Loading

iand commented Sep 25, 2023

dennis-tra commented Sep 25, 2023

	// This identifier used for routing network responses to the state machine.
	// This identifier is used for routing network responses to the state machine.

	// probe is the routing table explore state machine, responsible for increasing the occupanct of the routing table
	// explore is the routing table explore state machine, responsible for increasing the occupancy of the routing table

	qry, err := query.NewFindCloserQuery[K, N, any](b.self, BootstrapQueryID, b.self.Key(), iter, tev.KnownClosestNodes, qryCfg)
	qry, err := query.NewFindCloserQuery[K, N, coordt.Message](b.self, BootstrapQueryID, b.self.Key(), iter, tev.KnownClosestNodes, qryCfg)

		interval := float64(s.interval) + float64(s.interval)float64(s.maxCpl-next.Cpl)s.multiplier
		interval = 1 + rand.Float64()s.jitter

Add explore state machine to expand population of routing table #934

Add explore state machine to expand population of routing table #934

Conversation

iand commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennis-tra commented Sep 22, 2023 • edited Loading

iand commented Sep 25, 2023

dennis-tra commented Sep 25, 2023

dennis-tra commented Sep 22, 2023 •

edited

Loading