fix: refactor query logic #237

achingbrain · 2021-10-06T16:00:13Z

Queries are plain functions now
Queries are run by the QueryManager
Query functions yield events - dial, send request, receive response, value, error, etc
Timeouts have been replaced with AbortSignals
Halting in-progress queries is done by aborting an AbortSignal from the top level or breaking out of the for await..of loop
Internal components do not depend on the top-level DHT component any more
Some methods have been moved to other components to break circular dependencies
The top level KadDHT class now has a ts interface and a factory function
There are two DHTs, lan (for private addresses) and wan (for public addresses)
Client mode is now on by default for the wan DHT, off for the lan DHT
DHT queries for CIDs search for the multihash rather than the full CID

1. Queries are plain functions now 2. Queries are run by the QueryManager 3. Timeouts have been replaced with AbortSignals 4. All long-lived operations require AbortSignals 5. Halting in-progress queries is done by aborting an AbortSignal from the top level 6. Internal components do not depend on the top-level DHT component any more 7. Some methods have been moved to other components to break circular depenedencies 8. The top level KadDHT class now has a ts interface

kumavis

only got half way through the review before bedtime. generally looks good, nice improvements.

kumavis · 2021-10-12T09:38:08Z

src/content-fetching/index.js


-      await utils.mapParallel(dht.getClosestPeers(key, { shallow: true }), async (peer) => {
+    await drain(parallel(map(this._peerRouting.getClosestPeers(key, { signal: options.signal, shallow: true }), (peer) => {


a little dense here

kumavis · 2021-10-12T09:56:55Z

src/query-manager/disjoint-path.js

+        if (result && !result.done && result.closerPeers) {
+          for (const closerPeer of result.closerPeers) {
+            const closerPeerKadId = await convertPeerId(closerPeer)
+            const closerPeerXor = BigInt('0x' + toString(xor(closerPeerKadId, kadId), 'base16'))


worthy of a utility fn?

kumavis · 2021-10-12T09:57:47Z

src/query-manager/disjoint-path.js

+            // only continue query if closer peer is actually closer
+            if (closerPeerXor > peerXor) {
+              // log('skipping %p as they are not closer to %b than %p', closerPeer, key, peer)
+              // continue


why is this commented out?

It causes this test to pass but then this test to fail.

There's a bug in there somewhere, I wanted to experiment a bit more with go-ipfs and the expanded interop test suite before returning to this.

kumavis · 2021-10-12T10:02:06Z

src/query-manager/disjoint-path.js

+    // TODO: ensure we can break out of this loop early - this promise needs to
+    // be resolved manually and will leak memory otherwise
+    await deferred.promise
+    deferred = defer()


maybe can be replaced by a for-await ?

kumavis · 2021-10-12T10:03:34Z

src/query-manager/disjoint-path.js

+    deferred = defer()
+
+    // yield all available results
+    while (results.length) {


can you do a yield-star here?

jacobheun · 2021-11-16T12:05:40Z

Nice! Will try to carve out some time today to go through this. If you haven't already gone through and checked the implementation summary doc, https://github.com/libp2p/js-libp2p-kad-dht/blob/fix/refactor-query-logic/docs/IMPL_SUMMARY.MD, it would be helpful to make sure that's up to date, especially if we're doing anything different/in addition to the dht spec.

achingbrain · 2021-11-16T15:42:56Z

@jacobheun I don't think we're doing anything extra to the spec, at least, we shouldn't be.

That document is interesting, it seems to be mostly covered by https://docs.ipfs.io/concepts/dht/ - perhaps we could link to that instead?

jacobheun · 2021-11-16T16:07:34Z

If we're following along with the spec we can just point to it. The main value of that doc is that there wasn't a spec when it was written and that helped clarify what this implementation was doing. Some implementations of libp2p will extend the behavior of the protocol for certain components, so it's helpful to have a readme that clarifies what those are. An example of this is that to date the js dht was using dist paths for lookup, but Go never did.

achingbrain · 2021-11-16T16:14:39Z

Ha, well ok then in that case we are doing extra to the spec as we're still using disjoint paths. Probably a good thing though.

jacobheun

Some minor things, overall looks good, great improvements to the query logic. @achingbrain and I reviewed on a call to walk through the changes.

jacobheun · 2021-11-16T16:12:19Z

src/constants.js

@@ -33,3 +33,5 @@ exports.K = 20

 // Alpha is the concurrency for asynchronous requests
 exports.ALPHA = 3
+
+exports.QUERY_SELF_INTERVAL = Number(minute)


This seems too frequent, can this be configured? Looks like spec default is 5mins https://github.com/libp2p/specs/tree/master/kad-dht#bootstrap-process.

It's exposed as the querySelfInterval factory function option. Will increase the default to 5 mins

jacobheun · 2021-11-16T16:18:21Z

src/index.js

+ * A DHT implementation modelled after Kademlia with S/Kademlia modifications.
 * Original implementation in go: https://github.com/libp2p/go-libp2p-kad-dht.


We don't use S/Kademlia in the new spec.

Suggested change

* A DHT implementation modelled after Kademlia with S/Kademlia modifications.

* Original implementation in go: https://github.com/libp2p/go-libp2p-kad-dht.

* A Kademlia based DHT implementation compliant with the libp2p specification.

* DualKadDHT extends the existing specification by providing support for a public

* and private DHT, which is inline with the go-libp2p implementation.

* Spec at https://github.com/libp2p/specs/tree/master/kad-dht

jacobheun · 2021-11-16T16:23:00Z

src/index.js

+    this._wan = wan
+    this._lan = lan
+    this._libp2p = libp2p
+    this._datastore = libp2p.datastore || this._wan._datastore


Are there any potential conflicts with lan/wan using the same datastore?

We only store libp2p-records in the datastore and they are externally validated so I don't think so, no.

jacobheun · 2021-11-16T16:27:49Z

src/index.js

-    this._running = false
+      await Promise.all([
+        this._lan.onPeerConnect({ id: peerId, multiaddrs: [connection.remoteAddr] }),
+        this._wan.onPeerConnect({ id: peerId, multiaddrs: [connection.remoteAddr] })


Does this check for the DHT protocol? We don't want to add peers we connect to, we want to ensure they're running the DHT protocol. I believe go also has logic around usefulness. Will check the rest of the code and update here when I see this logic.

jacobheun · 2021-11-16T16:29:48Z

src/index.js

+   */
+  enableServerMode () {
+    log('enabling server mode')
+    this._wan.enableServerMode()


Not needed in the lan? I assume lan is created with everyone in server mode.

lan is server-mode only

jacobheun · 2021-11-18T10:18:44Z

src/kad-dht.js

+    this._queryManager = new QueryManager({
+      peerId: libp2p.peerId,
+      // Number of disjoint query paths to use - This is set to `kBucketSize/2` per the S/Kademlia paper
+      disjointPaths: Math.ceil(kBucketSize / 2),


We should get rid disjoint paths in a future PR. In conversations last year we netted out that there's not much value here and it can slow the queries down. Go get's around the potential malicious node problem with ASN distribution protection in the routing tables.

jacobheun · 2021-11-18T10:43:01Z

test/kad-dht.spec.js

-      tdht.teardown()
-    })
-
-    it('getClosestPeers', async function () {


Why'd we remove this one?

jacobheun · 2021-11-18T10:44:50Z

test/kad-dht.spec.js


      // TODO: after error switch
    })

-    it('get should handle correctly an unexpected error', async function () {


Why'd we get rid of record validation testing?

jacobheun · 2021-11-18T10:48:44Z

test/query.spec.js

-  it('stop after finding k closest peers', async () => {
-    // mock this so we can dial non existing peers
-    dht.dialer.dial = () => {}
+  it.skip('should end paths when they have no closer peers to those already queried', async () => {


jacobheun · 2021-11-18T10:58:32Z

src/query/events.js

+ */
+function sendingQueryEvent (fields) {
+  return {
+    ...fields,


Would be cool if fields also included the query id so you could trace the query at the libp2p layer instead of just logging.

achingbrain added 3 commits October 6, 2021 16:56

chore: remove libp2p from component types

8c99da8

chore: query manager tests passing

5f953d3

achingbrain linked an issue Oct 7, 2021 that may be closed by this pull request

Query logic overhaul #233

Closed

achingbrain mentioned this pull request Oct 7, 2021

Track: enabling the DHT by default ipfs/js-ipfs#3905

Closed

13 tasks

achingbrain added 15 commits October 8, 2021 19:59

chore: kad-dht tests passing

36aae55

fix: couple of tests left

d965111

chore: tests passing

f4fca0b

chore: swap travis for gh actions

05b7005

chore: test on node

27707e2

chore: ignore coverage dir

416e19d

chore: update parallel

12204ac

chore: make timeouts optional

a234676

chore: speed up tests

cc1e3d4

chore: allow sub-queries to time out without aborting the whole query

8e5fe2b

chore: restore prepare script, get datastore from libp2p

6e287c8

fix: bubbled up query errors should be non-fatal

546db47

chore: temp fix for fix node 14

09eb418

feat: query self occasionally

e351e02

chore: linting

0d63aee

kumavis reviewed Oct 12, 2021

View reviewed changes

achingbrain added 9 commits October 13, 2021 15:33

feat: add query event handlers

43be700

feat: events all the way down

1c8a125

chore: tests passing for events

287a77a

chore: tests passing again

a29959a

chore: linting

c61ca6c

chore: listen to connection events

ac53b24

chore: linting

93e2dd1

chore: do not expose dht messages

1d5f2f5

chore: fields will always be set

ac8698b

achingbrain added 2 commits November 9, 2021 08:58

chore: avoid leaking libp2p-record in types as it depends on crypto

4af423b

fix: split DHTs into lan and wan

fffed89

achingbrain marked this pull request as ready for review November 15, 2021 15:55

achingbrain added 4 commits November 15, 2021 17:22

chore: rename as events

2e96752

chore: let caller throw and only continue query if xor is closer

36e198e

chore: fix flaky test

6bdf4b4

chore: add missing dep

04e7aca

achingbrain requested review from vasco-santos and jacobheun November 16, 2021 10:34

This was referenced Nov 17, 2021

chore: update dht libp2p/js-libp2p-daemon#49

Merged

feat: update dht libp2p/js-libp2p#1009

Merged

achingbrain added 2 commits November 17, 2021 17:27

chore: better logging

e8f882f

chore: update implementation summary

dc47b5c

jacobheun approved these changes Nov 18, 2021

View reviewed changes

achingbrain added 2 commits November 18, 2021 14:37

chore: address review comments

1cfea97

chore: slow ci is slow

1ead080

achingbrain merged commit 1f8bc6a into master Nov 18, 2021

achingbrain deleted the fix/refactor-query-logic branch November 18, 2021 14:59

This was referenced Nov 18, 2021

Include query id in events #238

Open

Table refresh #232

Closed

achingbrain mentioned this pull request May 24, 2022

Switch to using multihashes for DHT queries #163

Closed

galargh mentioned this pull request Jun 2, 2022

chore: clean up inactive users from the org, teams and repos libp2p/github-mgmt#12

Merged

joeltg mentioned this pull request Oct 7, 2022

Inconsistency between message keys in ADD_PROVIDER and GET_PROVIDER (cid bytes vs multihash bytes) #381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refactor query logic #237

fix: refactor query logic #237

achingbrain commented Oct 6, 2021 •

edited

Loading

kumavis left a comment

kumavis Oct 12, 2021

kumavis Oct 12, 2021

kumavis Oct 12, 2021

achingbrain Oct 12, 2021

kumavis Oct 12, 2021

kumavis Oct 12, 2021

jacobheun commented Nov 16, 2021

achingbrain commented Nov 16, 2021

jacobheun commented Nov 16, 2021

achingbrain commented Nov 16, 2021 •

edited

Loading

jacobheun left a comment

jacobheun Nov 16, 2021

achingbrain Nov 18, 2021

jacobheun Nov 16, 2021

jacobheun Nov 16, 2021

achingbrain Nov 18, 2021

jacobheun Nov 16, 2021

jacobheun Nov 16, 2021

achingbrain Nov 18, 2021

jacobheun Nov 18, 2021

jacobheun Nov 18, 2021

jacobheun Nov 18, 2021

jacobheun Nov 18, 2021

jacobheun Nov 18, 2021


		await utils.mapParallel(dht.getClosestPeers(key, { shallow: true }), async (peer) => {
		await drain(parallel(map(this._peerRouting.getClosestPeers(key, { signal: options.signal, shallow: true }), (peer) => {

		* A DHT implementation modelled after Kademlia with S/Kademlia modifications.
		* Original implementation in go: https://github.com/libp2p/go-libp2p-kad-dht.

- * A DHT implementation modelled after Kademlia with S/Kademlia modifications.
- * Original implementation in go: https://github.com/libp2p/go-libp2p-kad-dht.
+ * A Kademlia based DHT implementation compliant with the libp2p specification.
+ * DualKadDHT extends the existing specification by providing support for a public
+ * and private DHT, which is inline with the go-libp2p implementation.
+ * Spec at https://github.com/libp2p/specs/tree/master/kad-dht

fix: refactor query logic #237

fix: refactor query logic #237

Conversation

achingbrain commented Oct 6, 2021 • edited Loading

kumavis 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Nov 16, 2021

achingbrain commented Nov 16, 2021

jacobheun commented Nov 16, 2021

achingbrain commented Nov 16, 2021 • edited Loading

jacobheun 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

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

achingbrain commented Oct 6, 2021 •

edited

Loading

achingbrain commented Nov 16, 2021 •

edited

Loading