dht mode toggling (modulo dynamic switching) #350

whyrusleeping · 2019-06-13T16:54:53Z

closes #349

First stab at this, trying to make it as minimally invasive as possible.

Please suggest tests to write (or please help me test).

I think we can avoid the extra message type i mentioned in the issue, and rely on protocol negotiation to handle getting peers to drop us from their routing tables. Old peers will be a little messed up by this, but they already have such a hard time dialing and connecting anyways, i don't think anyone will notice a difference

vyzo

this looks good for the most part.

dht.go

raulk

Shaping up well.

raulk · 2019-06-13T17:46:06Z

dht.go

@@ -41,6 +41,11 @@ var logger = logging.Logger("dht")
 // collect members of the routing table.
 const NumBootstrapQueries = 5

+const (
+	ModeServer = 1


Can use a custom type definition and iota here.

dht.go

raulk · 2019-06-13T17:53:32Z

dht.go

+
+	// hacky... also closes both inbound and outbound streams
+	for _, c := range dht.host.Network().Conns() {
+		for _, s := range c.GetStreams() {


Re: backwards-compatibility. This is where the tricky part comes in. This drops streams for peers in our routing table. It will not invite the peer to drop us from theirs. So they’ll keep querying us, and we’ll “na” all their negotiations. We’ll basically become an unhelpful peer taking up a slot in their table, unless we disconnect to trigger the removal.

On a related note, there seems to be a race between identify and the DHT notifee. Even if we disconnect and reconnect, if the DHT notifee runs before identify finishes, we might be deciding on stale protocols: https://github.com/libp2p/go-libp2p-kad-dht/blob/master/notif.go#L27

yeah... the backwards compatibility bit kinda sucks. I don't know that a nice backwards compatible solution exists aside from hard disconnecting from those peers.

I'd much rather track streams manually.

dht.go

whyrusleeping · 2019-06-14T18:52:36Z

Alright, I'm only closing inbound dht streams now, and i'm resetting them instead of just closing. Anyone have ideas for good tests to write to test this out?

bigs · 2019-06-14T22:34:23Z

@whyrusleeping maybe just use mocknet to make two hosts and check the behavior of IdentifyService before and after the toggle for the most basic tests.

raulk · 2019-06-17T19:05:54Z

@bigs feel like taking a stab at writing those tests?

@whyrusleeping:

they already have such a hard time dialing and connecting anyways

With the relay infrastructure, the network is, in theory, very well connected.

We need to implement the logic that's going to dynamically switch from one to another. Without it, this PR is equivalent to the existing DHTClientOption option.

whyrusleeping · 2019-06-17T19:25:45Z

@raulk exactly. The point of this is just giving some other code the ability to switch between the two.

The next step is to start all ipfs nodes in client mode, and only switch to server mode if autonat detects a publicly dialable peer

raulk · 2019-06-17T19:34:40Z

@whyrusleeping do you want to implement the controller in IPFS (since I believe it has access to autonat), or should we do it in the DHT via one of the options here: #349 (comment), and then switch to the event bus when it’s ready?

whyrusleeping · 2019-06-17T23:55:16Z

I think doing it in ipfs will likely be the simplest solution. Unless you disagree, let's go with that.

…

On Mon, Jun 17, 2019, 2:34 PM Raúl Kripalani ***@***.***> wrote: @whyrusleeping <https://github.com/whyrusleeping> do you want to implement the controller in IPFS (since I believe it has access to autonat), or should we do it in the DHT via one of the options here: #349 (comment) <#349 (comment)>, and then switch to the event bus when it’s ready? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#350?email_source=notifications&email_token=AAJPQHDGY2XACBOZ7R3XOOTP27RNHA5CNFSM4HX36MEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX4G6ZA#issuecomment-502820708>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJPQHCW2T7PV56RDZBE5VDP27RNHANCNFSM4HX36MEA> .

Stebalien

We still need to wire this through to identify:

We need to know when the peer starts speaking the DHT protocol.
We need to know when the peer stops speaking the DHT protocol.

(mostly 1).

Stebalien · 2019-06-18T15:51:10Z

dht.go

+
+	// hacky... also closes both inbound and outbound streams
+	for _, c := range dht.host.Network().Conns() {
+		for _, s := range c.GetStreams() {


I'd much rather track streams manually.

Stebalien · 2019-06-18T15:56:40Z

dht.go

+	return nil
+}
+
+func (dht *IpfsDHT) moveToClientMode() error {


This should be fine but I'd feel safer if we checked if we were in client mode in the stream handler, just in case.

whyrusleeping · 2019-06-19T09:28:20Z

Alright, current plan of attack:

Give DHT ability to listen to changes in identify push
- When we hear about our peers adding DHT protocol handlers, put them into our routing table
- When we head about our peers removing DHT protocol handlers, remove them from our routing table

Old peers will be slow to add new peers to their routing tables, as we currently rely on the protocol probe up front for detecting if peers are in client mode. If older peers are informed about new peers in server mode who they previously rejected as a client, they will attempt to open a new stream to that peer, so eventually this will propagate around.

New peers will include old peers in their routing table if they start in server mode.

We thought about adding an extra DHT message to signal whether or not we are in server mode or client mode, but the problem there is that it requires you to keep the listeners on. I'm not actually sure this is that bad of a problem.

bigs · 2019-06-19T20:41:09Z

added an extremely basic test demonstrating the change in behavior from client -> server -> client

whyrusleeping · 2019-06-20T13:44:15Z

Thanks @bigs !

I pushed changes that add handling for identify push protocol changes from other peers.

The next (And hopefully final) step is to decide if we want to version bump the dht. it will be pretty painful, but should make the resulting DHT muuuuch better.

whyrusleeping · 2019-06-20T13:50:56Z

How this could pan out:

we add a dht/1.1.0 protocol version
we continue listening on dht/1.0.0
new peers only add peers supporting 1.1.0 to their routing tables
old peers will still add new peers to their routing table
queries by new peers will be limited to the 'new' subset of the DHT, and at first will likely fail to resolve many requests
- however, they should fail much faster than on the old dht
old nodes should not notice much degredation, as the dht they are part of consists of all nodes (though new nodes will only route new nodes, so if the 'multipath kademlia routing' is implemented correctly, all should be fine).

I think this is a good idea, and can implement it very easily. thoughts? @raulk @vyzo @Stebalien @Kubuxu

Stebalien · 2019-06-18T16:45:02Z

dht.go


 	if !cfg.Client {
+		dht.mode = ModeServer


Just call modeToServer?

Stebalien · 2019-06-20T15:20:04Z

Do it. Peer routing should still work as long as the old node is connected to at least one new node.

Note: We don't want to add support for the new protocol and use both. If we do that, the dht's will "join" and turning off the old protocol won't break them (until everyone upgrades).

raulk · 2019-06-20T16:13:02Z

dht.go

+	ch := make(chan event.EvtPeerProtocolsUpdated, 8)
+	cancel, err := dht.host.EventBus().Subscribe(ch)
+	if err != nil {
+		return nil, err


Does this compile?

raulk · 2019-06-20T16:14:36Z

dht.go

+
+			if add && drop {
+				// TODO: discuss how to handle this case
+				log.Warning("peer adding and dropping dht protocols? odd")


the way this is implemented, in practice it's very, very, very unlikely that the event will contain (a) multiple protocols, and/or (b) multiple operations.

right, but its possible, so what do we do when it does?

this is tricky. the data model allows it to happen, but the implementation doesn't allow it to happen. maybe we should revisit these event structs to eliminate the * cardinality.

dht_net.go

This is WIP and there are still items being discussed. Removing this review to avoid confusion.

raulk · 2019-06-20T16:30:53Z

@whyrusleeping that sounds fair, and I arrive to the same conclusion re: the tradeoff. But I'd like us to evaluate these alternate scenarios:

we preserve the protocol, therefore peers have no way to signal they support dynamic switching.
we preserve the protocol, and peers signal to us if they support dynamic switching via a bitflag on the DHT protobufs.
we change the protocol but add legacy peers to our routing table unless our connection to them is via a relay.

Co-Authored-By: Raúl Kripalani <raul.kripalani@gmail.com>

whyrusleeping · 2019-06-21T11:55:44Z

@raulk i moved the routing table segregating code to a different PR #356 I think all this here is ready to go once the deps are right

whyrusleeping · 2019-06-21T12:25:37Z

dht.go

+
+			if add && drop {
+				// TODO: discuss how to handle this case
+				logger.Warning("peer adding and dropping dht protocols? odd")


Still need to figure out what to do here, @Kubuxu and @raulk any thoughts?

Proposal: "Peer is bad, we don't want them in our routing table"

We can either noop (since both operations cancel each other out), or discard the peer altogether.

whyrusleeping · 2019-06-24T15:23:26Z

Alright, ready for some review. @raulk @Kubuxu

WIP: dht mode toggling

6599d91

whyrusleeping requested review from vyzo, Stebalien and raulk June 13, 2019 16:54

vyzo reviewed Jun 13, 2019

View reviewed changes

dht.go Outdated Show resolved Hide resolved

raulk reviewed Jun 13, 2019

View reviewed changes

bigs previously approved these changes Jun 13, 2019

View reviewed changes

dht.go Outdated Show resolved Hide resolved

reset inbound streams

b9468a8

Stebalien reviewed Jun 18, 2019

View reviewed changes

drop out of handlers if we are in client mode

03103d0

Add basic mode switching test

fe71ae8

add handlers for protocol changes

70c25e0

Stebalien reviewed Jun 20, 2019

View reviewed changes

dht.go Outdated

if !cfg.Client {

dht.mode = ModeServer

Copy link

Member

Stebalien Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call modeToServer?

whyrusleeping reacted with thumbs up emoji

raulk reviewed Jun 20, 2019

View reviewed changes

whyrusleeping and others added 2 commits June 21, 2019 13:51

Update dht_net.go

d99d2e2

Co-Authored-By: Raúl Kripalani <raul.kripalani@gmail.com>

fix build

5f434a8

whyrusleeping force-pushed the feat/mode-toggle branch from 210d9f1 to 5f434a8 Compare June 21, 2019 11:53

whyrusleeping commented Jun 21, 2019

View reviewed changes

whyrusleeping added 2 commits June 24, 2019 16:52

clean up constructor

37010e3

some fixes and gomod

31f06f4

raulk changed the base branch from master to stabilize June 26, 2019 16:45

raulk changed the title ~~WIP: dht mode toggling~~ dht mode toggling (modulo dynamic switching) Jun 26, 2019

Merge branch 'stabilize' into feat/mode-toggle

4abe0c6

raulk merged commit 4b31e56 into stabilize Jun 26, 2019

aschmahmann mentioned this pull request Oct 30, 2019

IPNS defaults will break in LANs with proposed NAT-detecting DHT ipfs/kubo#6737

Closed

aschmahmann pushed a commit that referenced this pull request Feb 28, 2020

dht mode toggling (modulo dynamic switching) (#350)

b6af715

Stebalien pushed a commit that referenced this pull request Mar 4, 2020

dht mode toggling (modulo dynamic switching) (#350)

fc3b762

aschmahmann pushed a commit that referenced this pull request Mar 5, 2020

dht mode toggling (modulo dynamic switching) (#350)

28380f4

aschmahmann pushed a commit that referenced this pull request Mar 5, 2020

dht mode toggling (modulo dynamic switching) (#350)

d570496

aschmahmann pushed a commit that referenced this pull request Mar 6, 2020

dht mode toggling (modulo dynamic switching) (#350)

08bf89b

aschmahmann pushed a commit that referenced this pull request Mar 6, 2020

dht mode toggling (modulo dynamic switching) (#350)

02f4a9b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dht mode toggling (modulo dynamic switching) #350

dht mode toggling (modulo dynamic switching) #350

whyrusleeping commented Jun 13, 2019

vyzo left a comment

raulk left a comment

raulk Jun 13, 2019

raulk Jun 13, 2019

whyrusleeping Jun 14, 2019

Stebalien Jun 18, 2019

whyrusleeping commented Jun 14, 2019

bigs commented Jun 14, 2019

raulk commented Jun 17, 2019

whyrusleeping commented Jun 17, 2019

raulk commented Jun 17, 2019

whyrusleeping commented Jun 17, 2019 via email

Stebalien left a comment

Stebalien Jun 18, 2019

Stebalien Jun 18, 2019

whyrusleeping commented Jun 19, 2019

bigs commented Jun 19, 2019

whyrusleeping commented Jun 20, 2019

whyrusleeping commented Jun 20, 2019

Stebalien Jun 18, 2019

Stebalien commented Jun 20, 2019 •

edited

Loading

raulk Jun 20, 2019

raulk Jun 20, 2019

whyrusleeping Jun 20, 2019

raulk Jun 20, 2019

raulk commented Jun 20, 2019

whyrusleeping commented Jun 21, 2019

whyrusleeping Jun 21, 2019

whyrusleeping Jun 21, 2019

raulk Jun 25, 2019

whyrusleeping commented Jun 24, 2019

dht mode toggling (modulo dynamic switching) #350

dht mode toggling (modulo dynamic switching) #350

Conversation

whyrusleeping commented Jun 13, 2019

vyzo left a comment

Choose a reason for hiding this comment

raulk left a 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

whyrusleeping commented Jun 14, 2019

bigs commented Jun 14, 2019

raulk commented Jun 17, 2019

whyrusleeping commented Jun 17, 2019

raulk commented Jun 17, 2019

whyrusleeping commented Jun 17, 2019 via email

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Jun 19, 2019

bigs commented Jun 19, 2019

whyrusleeping commented Jun 20, 2019

whyrusleeping commented Jun 20, 2019

Choose a reason for hiding this comment

Stebalien commented Jun 20, 2019 • edited Loading

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

raulk commented Jun 20, 2019

whyrusleeping commented Jun 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Jun 24, 2019

Stebalien commented Jun 20, 2019 •

edited

Loading