[RFC] Delay dials to relay addresses #57

magik6k · 2018-02-15T20:41:33Z

This is a somewhat naive way to do this, but I can't think of an easy way to make this smarter without making this ugly in some way.

One way this could be improved would be to switch from chan to an array in dialAddrs, I just wasn't sure what is the reason behind it being a channel, so I decided to not touch that for now.

vyzo

so what happens if a peer only has a relay address?
shouldn't we dial that immediately instead of waiting for 2s?

Stebalien · 2018-02-15T20:50:01Z

I see the issue but I'd like to discuss this thoroughly first (in an issue). More generally, we need some way to prefer some transports over others.

Note: We're using a channel because, technically, we'd like to dial and discover addresses in parallel. However, we don't currently do that. This does make things a bit annoying... but we can always try reading off what's on the channel into an array and then sorting there. We can even, e.g., introduce delays if we have different "preference" tiers.

whyrusleeping · 2018-02-15T21:04:31Z

Yeah, ideally we are able to discover new addresses and inform the dial process of them during a dial. I did the refactor to move towards that, but the other side (the dht address finder) hasnt caught up yet. This is a hard problem.

magik6k · 2018-02-15T21:09:53Z

so what happens if a peer only has a relay address?
shouldn't we dial that immediately instead of waiting for 2s?

That's the hard part

Stebalien · 2018-02-15T21:17:17Z

That's the hard part

Expanding on my previous comment, I think the better (although certainly not perfect) solution here is to:

Read off the channel until it blocks (using a select). Maybe with a timeout (e.g., wait until it blocks for more than a millisecond).
Sort that slice of addresses into preference tiers.
Add delays between preference tiers (iff there exists an address in the preference tier).

magik6k · 2018-02-15T23:48:25Z

Ok, I think I pushed something that makes sense, please review.

todo:

tests
a bit more abstracted dialOffset
- dialLimiter doesn't exactly abstract it's checks.
- Should I PR p2p-circuit entry to go-multiaddr and to mafmt?

Stebalien

So, I don't think we really need sorting. Really, we just want to bucket. Also, preempting would be nice...

Here's a quick sketch of what I'm thinking (could probably use some optimizations (also, not tested at all)). Do you think this is too complicated? Probably overkill for what we need now but we'll want something like this eventually.

(also, mixing context stuff and probably buggy as hell)

	out := make(chan ma.Multiaddr)
	go func() {
		// We have a preset number of "tiers"
		var pending [NumTiers][]ma.Multiaddr
		lastTier := 0

		// put enqueues the mutliaddr
		put := func(addr ma.Multiaddr) {
			tier := getTier(addr)
			pending[tier] = append(pending[tier], addr)
		}

		// get gets the best multiaddr available.
		get := func() (ma.Multiaddr, int) {
			for i, tier := range pending[:] {
				if len(tier) > 0 {
					addr := tier[len(tier)-1]
					tier[len(tier)-1] = nil
					pending[i] = tier[:len(tier)-1]
					return addr, i
				}
			}
		}

		// Always delay 2 seconds between tiers.
		delay := timer.Timeout(time.Second * 2)
        defer delay.Stop()

	outer:
		for {
		fill:
			for {
				select {
				case addr, ok := <-addrs:
					if !ok {
						break outer
					}
					put(addr)
				default:
					break fill
				}
			}

			next, tier = get()

			// Nothing? Block!
			if next == nil {
				addr, ok := <-addrs
				if !ok {
					break outer
				}
				put(addr)
				continue
			}

			// Jumping a tier?
			if tier > lastTier {
            	// Wait the delay (preempt with new addresses)
				select {
				case addr, ok := <-addrs:
					put(addr)
					continue
				case <-delay.C:
					// So we always get a zero delay, there
					// are better ways to deal with this...
					delay.Reset(0)
				}
			}

			lastTier = tier

			select {
			case addr, ok := <-addrs:
				put(next)
				if !ok {
					break outer
				}
				put(addr)
				continue
			case out<-addr:
            	// Always count the timeout since the last dial.
				delay.Reset(time.Second * 2)
			}
		}

		// finish sending
		for {
			next, tier := get()
			if next == nil {
				return
			}
			if tier > lastTier {
				<-delay.C
				delay.Reset(time.Second * 2)
			}
			tier = lastTier
			out<-next
		}
	}

whyrusleeping · 2018-02-16T01:54:50Z

cc @diasdavid. Something something relay dialing logic.

magik6k · 2018-02-16T02:43:20Z

Refactored to use buckets

vyzo

this is crazy complicated...

vyzo · 2018-02-16T08:20:44Z

dial_delay.go

+const p_circuit = 290
+
+const numTiers = 2
+const tierDelay = 2 * time.Second


can we make it just 1s? 2s might already be too much for a dial.

magik6k · 2018-02-16T11:33:18Z

dial_delay.go

+				}
+			}
+
+			next, tier := get()


Note that with the current setup we don't ever reach this point in this loop as the channel is closed by now. All sending will happen in the loop in L127. I'll add some tests later today to test that it works.

Stebalien · 2018-02-16T17:39:03Z

With a test, LGTM. You even covered the "out of addresses" case. Nice!

(I kind of expected you to tell me to tell me my idea was way too complicated and to take a hike...)

whyrusleeping · 2018-02-16T17:56:04Z

dial_delay.go

+	delay := time.NewTimer(tierDelay)
+	triggerNext := make(chan struct{}, 1)
+
+	go func() {


would be nice to have this broken out into a separate function

whyrusleeping · 2018-02-16T18:19:12Z

dial_delay.go

+		}
+
+		// get gets the best (lowest tier) multiaddr available
+		get := func() (ma.Multiaddr, int) {


worth noting that we're using a stack within each tier. Might not be the best approach (though definitely simpler and cheaper to implement)

magik6k · 2018-02-19T02:34:32Z

Broken the code into some more functions
Added tests for all cases I could imagine
- almost all cases covered (except contexts)
- ran the tests 20 times, none have failed (not even on windows)

whyrusleeping · 2018-02-22T20:17:00Z

I would also like @diasdavid to review this. This is a libp2p design decision I think he should have a say in.

Stebalien · 2018-02-19T23:07:40Z

dial_delay.go

+		var pending [numTiers][]ma.Multiaddr
+		lastTier := -1
+
+		// put enqueues the mutliaddr


s/mutliaddr/multiaddr/

Stebalien · 2018-02-19T23:09:51Z

dial_delay.go

+					}
+					put(addr)
+				default:
+					break loop


Nit: Given that we have a function here, I'd just move the return true statement here and get rid of the named break.

magik6k · 2018-06-08T18:28:30Z

swarm_dial.go

+//        swarm and agrees on the                                        |
+//            muxer to use                                             /---\
+//                                        The result is distributed -> |||||
+//                                            to callers of Dial       |||||


@Stebalien after the transport refactor only the part after 'dialConnSetup' changed, right?

magik6k · 2018-06-08T18:32:59Z

dial_delay.go

+
+var TierDelay = 1 * time.Second
+
+var relay = mafmt.Or(mafmt.And(mafmt.Base(p_circuit), mafmt.Base(ma.P_IPFS)), mafmt.And(mafmt.Base(ma.P_IPFS), mafmt.Base(p_circuit), mafmt.Base(ma.P_IPFS)))


I wonder if we could have the transports have some way to indicate that they should be delayed, possibly an optional interface like

type ExpensiveTransport interface { TransportCost() int //basically the 'tier' to use here }

Sounds like a good idea.

We should be aware of potential problems with changes in the circuit addressing here -- cf libp2p/specs#72

This might break with the changes in libp2p/go-libp2p-circuit#48
We should drop the trailing /ipfs part from the filter and just rely on the presence of /p2p-circuit

Also problematic for explicit relay addresses.

bigs · 2018-09-04T23:06:35Z

per @diasdavid's suggestion should we close this temporarily and move to an RFC?

edit: i actually mean move it to a discussion and specification session in specs?

Stebalien · 2018-09-04T23:14:24Z

I really can't remember what this was blocked on other than "holy shit this is complicated".

vyzo · 2018-09-27T18:02:02Z

Reopening this, as it's a high priority issue.

vyzo · 2018-09-27T18:02:56Z

So where are we blocked here?

magik6k · 2018-09-27T18:26:57Z

#57 (comment)

holy shit this is complicated

No, but seriously, IIRC the core logic here is done, one thing that would be nice would be to have some method in the transport interface saying that the transport is lower tier so we don't hardcode relay here - https://github.com/libp2p/go-libp2p-swarm/pull/57/files#diff-46c472072de3b864e2fabb74ab784758R202, but I think this can by done separate to this PR

vyzo · 2018-09-27T19:23:12Z

dial_delay.go

+	mafmt "github.com/whyrusleeping/mafmt"
+)
+
+const p_circuit = 290


we should probably capitalize this to P_CIRCUIT -- that's what we do everywhere else.

vyzo · 2018-09-27T19:24:46Z

dial_delay.go

+
+var TierDelay = 1 * time.Second
+
+var relay = mafmt.Or(mafmt.And(mafmt.Base(p_circuit), mafmt.Base(ma.P_IPFS)), mafmt.And(mafmt.Base(ma.P_IPFS), mafmt.Base(p_circuit), mafmt.Base(ma.P_IPFS)))


We should be aware of potential problems with changes in the circuit addressing here -- cf libp2p/specs#72

vyzo · 2018-09-27T19:34:43Z

it's still quite complicated, but it looks ok; will reread in the morning.

@Stebalien care for a refresher review? Let's move it forward.

Just the presence of /p2p-circuit in the addr is enough to warrant delayed dialing. This accepts both explicit relay addresses and is also future-proof for changes in go-libp2p-circuit#48 and specs#72.

vyzo · 2018-09-28T16:28:42Z

Fixed issues with the relay address filter and future-proofed it for Change transport address to not include /p2p part go-libp2p-circuit#48 and P2P Circuit Addressing specs#72
Broke out the delay logic to its own top-level function, per @whyrusleeping's suggestion.

magik6k requested review from Stebalien and whyrusleeping February 15, 2018 20:41

ghost assigned magik6k Feb 15, 2018

ghost added the status/in-progress In progress label Feb 15, 2018

vyzo reviewed Feb 15, 2018

View reviewed changes

magik6k force-pushed the feat/dial-priority branch 3 times, most recently from 454d440 to 2012f5d Compare February 15, 2018 23:36

magik6k force-pushed the feat/dial-priority branch from 2012f5d to 19ab72d Compare February 15, 2018 23:53

Stebalien reviewed Feb 16, 2018

View reviewed changes

magik6k force-pushed the feat/dial-priority branch from 19ab72d to 3699629 Compare February 16, 2018 02:35

vyzo reviewed Feb 16, 2018

View reviewed changes

magik6k commented Feb 16, 2018

View reviewed changes

whyrusleeping reviewed Feb 16, 2018

View reviewed changes

magik6k force-pushed the feat/dial-priority branch from e2b10f6 to ef507ae Compare February 19, 2018 02:19

magik6k force-pushed the feat/dial-priority branch from ef507ae to 35bbd29 Compare February 19, 2018 02:45

magik6k mentioned this pull request Mar 18, 2018

Announce relay addresses by default ipfs/kubo#4830

Closed

Stebalien reviewed Mar 20, 2018

View reviewed changes

magik6k force-pushed the feat/dial-priority branch from 35bbd29 to 4757f91 Compare March 20, 2018 20:47

whyrusleeping mentioned this pull request Jun 5, 2018

QUIC Transport libp2p/libp2p#35

Closed

3 tasks

magik6k mentioned this pull request Jun 7, 2018

swarm: potential issue with dial limiter libp2p/go-libp2p#1553

Open

magik6k added 4 commits June 8, 2018 20:20

Delay dials to realy addrs

973940e

dial delay: tests and fixes

2e22aa8

dial delay: simplify the code

928ffbc

dial delay: address review

a945f49

magik6k force-pushed the feat/dial-priority branch from 2dafe20 to a945f49 Compare June 8, 2018 18:26

magik6k commented Jun 8, 2018

View reviewed changes

whyrusleeping changed the title ~~[RFC] Delay dials to realy addresses~~ [RFC] Delay dials to relay addresses Jun 12, 2018

bigs added the status/blocked Unable to be worked further until needs are met label Sep 4, 2018

bigs added status/deferred Conscious decision to pause or backlog and removed status/in-progress In progress labels Sep 4, 2018

Stebalien mentioned this pull request Sep 20, 2018

Address book ordering, and private addresses libp2p/go-libp2p-peerstore#11

Closed

vyzo added P1 High: Likely tackled by core team if no one steps up and removed status/deferred Conscious decision to pause or backlog labels Sep 27, 2018

magik6k removed the status/blocked Unable to be worked further until needs are met label Sep 27, 2018

vyzo reviewed Sep 27, 2018

View reviewed changes

dial delay: safer relay address filter

91fe548

Just the presence of /p2p-circuit in the addr is enough to warrant delayed dialing. This accepts both explicit relay addresses and is also future-proof for changes in go-libp2p-circuit#48 and specs#72.

ghost assigned vyzo Sep 28, 2018

ghost added the status/in-progress In progress label Sep 28, 2018

vyzo added 2 commits September 28, 2018 19:18

dial delay: reify delay logic into top-level function

835519e

dial delay: fix failing test

4be8cf4


		var TierDelay = 1 * time.Second

		var relay = mafmt.Or(mafmt.And(mafmt.Base(p_circuit), mafmt.Base(ma.P_IPFS)), mafmt.And(mafmt.Base(ma.P_IPFS), mafmt.Base(p_circuit), mafmt.Base(ma.P_IPFS)))

[RFC] Delay dials to relay addresses #57

Are you sure you want to change the base?

[RFC] Delay dials to relay addresses #57

Conversation

magik6k commented Feb 15, 2018

vyzo left a comment

Choose a reason for hiding this comment

Stebalien commented Feb 15, 2018

whyrusleeping commented Feb 15, 2018

magik6k commented Feb 15, 2018

Stebalien commented Feb 15, 2018

magik6k commented Feb 15, 2018

Stebalien left a comment

Choose a reason for hiding this comment

whyrusleeping commented Feb 16, 2018

magik6k commented Feb 16, 2018

vyzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Feb 16, 2018

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

magik6k commented Feb 19, 2018 • edited Loading

whyrusleeping commented Feb 22, 2018

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

bigs commented Sep 4, 2018 • edited Loading

Stebalien commented Sep 4, 2018

vyzo commented Sep 27, 2018

vyzo commented Sep 27, 2018

magik6k commented Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Sep 27, 2018

vyzo commented Sep 28, 2018

magik6k commented Feb 19, 2018 •

edited

Loading

bigs commented Sep 4, 2018 •

edited

Loading

magik6k commented Sep 27, 2018 •

edited

Loading