Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed address records in the DHT #516

Closed
wants to merge 40 commits into from

Conversation

aarshkshah1992
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 commented Mar 27, 2020

Corresponding Identify/Host PR:
libp2p/go-libp2p#747.

@aschmahmann @Stebalien

This is a first working draft and I might have made some assumptions which might be wrong. Please let me know what you think & I'll fix it quickly.

@aschmahmann I'll do this in the next commit

TODO

  • Have a findNode(kadID) for GetClosestPeers to call and not use FindPeer for it.

  • If FindPeer returns non DHT peers, mark them as such.

aschmahmann and others added 30 commits March 4, 2020 22:11
…we have heard about (in a given path) has been found
…ally. Concurrency option now sets alpha. DisjointPaths option now sets d. Default number of disjoint paths is now bucketSize/2.
* feat: consume identify events to evaluate routing table addition
* fix: routing table no longer gets an update just because new messages have arrived or been sent
* fix: add already connected peers into the routing table before listening to events

Co-authored-by: Raúl Kripalani <raul.kripalani@gmail.com>
Co-authored-by: Aarsh Shah <aarshkshah1992@gmail.com>
1. Continue to best-effort provide, but still return an error when we fail to
send provider records to the _best_ peers.
2. Continue returning the best peer's we've found in GetClosestPeers, but also
return an error to indicate that we didn't find the closest ones.

And fix the hang test.
* created Mode(ModeOpt) option for choosing between auto/client/server modes
* Auto mode internally switches the DHT between client and server modes based on the EvtLocalReachabilityChanged event emitted on the event bus (e.g. by AutoNAT)
* routing table management of peers that switch between client and server mode while we are connected to them (i.e. are in auto mode)
* removed Client(bool) option, becoming a DHT client is specified using Mode(ModeClient) instead
* feat: move options to main package and make internals private

Rationale:

1. This allows us to make private options for testing.
2. This removes an import for DHT users.
3. This makes options much easier to discover.
4. This makes it possible to make the config/options internals private.

We originally put them in a sub-package to avoid poluting the root namespace,
but that isn't really necessary.

This keeps the old package (for now) to avoid breaking too much.
* upgraded the protocol id to version 2 (i.e. /kad/2.0.0) and made it so v2 peers running in server mode respond to queries from v1 peers. Note: v2 peers will only send queries using the v2 protocol, will only add v2 peers to their routing tables, and will only tell v1 peers about v2 peers.
* to run a forked network we now use network specific protocol prefixes instead of manually setting protocol IDs. Use the ProtocolPrefix option instead of the Protocols option.
* emit errors during initialization if the user misuses the default protocol prefix by setting parameters inconsistent with the default protocol's network specification
* since the Client option has been deprecated it's been removed from the dht's options. While deprecated it is still available in the dht options package. Setting `Client(false)` now puts the node into ModeAuto.
* kpeerset refactoring
* query code cleanup
Fix test logging & document timing issue on Windows
aschmahmann and others added 10 commits March 23, 2020 01:59
RT peer validation should also include support for the DHT protocol
* feat(query): fully async implementation of Kademlia lookup. peers returned from the lookup are not guaranteed to be alive (i.e. we're only guaranteed to have dialed the closest beta peers to the target), but given stable and correct routing tables the expectation that most of the peers returned are alive is high.
* feat(query): add wrapper lookup followup function to followup after the lookup is completed and ensure that the closest k returned peers from a lookup have been queried even for beta < k
* refactor(query) modified the structure returned from lookups to be a useful subset of the full query state instead of the entire query state
* feat(options): beta parameter exposed as the Resiliency parameter
* feat(routing): do not mark the routing table as updated after a FindPeer query
* feat(routing): FindPeer can return addresses even if not Connected as long as it was either recently connected (CanConnect) or was discovered during the lookup
* feat(bootstrap): bootstrap logic now uses GetClosestPeers instead of FindPeer
* refactor(dht): stopFn no longer takes any state
* fix(test): changed GetClosestPeers test to only assume beta instead of k peers since that is now more appropriate given the query logic changes and that the routing tables in that test are bad, i.e. a ring network with arbitrary peerIDs

Co-authored-by: Petar Maymounkov <petarm@gmail.com>
Co-authored-by: Aarsh Shah <aarshkshah1992@gmail.com>
@@ -71,6 +71,7 @@ func TestHungRequest(t *testing.T) {
}

func TestGetFailures(t *testing.T) {
t.Skip("always fails")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue created at #517.

@aarshkshah1992 aarshkshah1992 requested review from Stebalien and aschmahmann and removed request for Stebalien March 28, 2020 10:43
@Stebalien Stebalien added the status/blocked Unable to be worked further until needs are met label Apr 1, 2020
@@ -4,7 +4,7 @@ GO = $(PB:.proto=.pb.go)
all: $(GO)

%.pb.go: %.proto
protoc --proto_path=$(GOPATH)/src:. --gogofast_out=. $<
protoc --proto_path=$(GOPATH)/src:. --gofast_out=. $<
Copy link
Member

Choose a reason for hiding this comment

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

Why are we switching this?

// Used to return peers closer to a peer we are looking for
// Used ONLY for FIND_NODE
repeated SignedPeerRecord signedCloserPeers = 11;
}
Copy link
Member

Choose a reason for hiding this comment

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

newline

}

// Used to return peers closer to a peer we are looking for
// Used ONLY for FIND_NODE
Copy link
Member

Choose a reason for hiding this comment

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

Why only for FIND_NODE?

@Stebalien
Copy link
Member

Blocking until cypress is live.

@aarshkshah1992
Copy link
Contributor Author

Closing in favour of #630 .

@aarshkshah1992 aarshkshah1992 deleted the feat/signed-addr-records branch May 6, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants