Support bitswap 1.1.0 and bitswap 1.0.0 using CID #76

daviddias · 2016-12-10T04:24:44Z

tl;dr; This PR adds support to CID and enables js-ipfs-bitswap to interop with both bitswap versions.

My initial goal was only to add the new support, however, I was having trouble to deal with the cognitive load that I was getting by having to sync in how ipfs-bitswap works and how this repo is organized each time I context switched. Since there was never a documentation/writeup, I started adding notes, later an image of the architecture and finally organized a bit the folder structure, so that the structure gives us a more intuitive way to understand what is going on. I hope this helps other people contribute and debug bitswap.

Thoughts and questions to answer

Also, during the process, I've realized that some things can be improved, that some others look like 'Go code being forced in JS land' and some that I'm simply not sure why it is the way it is. It doesn't have to be part of this PR, but I want to make sure to write them down so that we go through them later. First, here is the:

I don't see any code to coalesce messages in the messages queues, which means the more peers we have, the more buffering it will happen, to the point where the message to reset the wantlist might be added even before the previous having been sent and nothing is clearing the message queue.
The decision engine buffers the blocks in memory when it informs the message queue that there is one more block to send, this means that if thousands of transfers are being performed, every block will stay in memory till the message gets sent. A better approach would be to only read from disk when the message is going to be sent.
Why is there a refcounter for PeerHandler in the WantManager?
Is a MessageQueue per peer the best approach? Why not piggy back on want list updates and just check what the peer wants and attach the blocks necessary?
Why have two classes for WantListEntry? The BitSwapMessage Entry only has one more property cancel, can't we just extend the other and add that property?
stats (exchanged byteCount and so on) kind of don't have any self guards to overflowing

What's left

…g, reducing cognitive load

dignifiedquire · 2016-12-10T09:28:17Z

this is super hard to review with all the file moving and renaming. Can you please put that into a different PR?

dignifiedquire

Found some small issues but overall this looks already pretty solid. No need to move the renaming somewhere else, githubs review support for individual commits is pretty sweet at this point :)

dignifiedquire · 2016-12-10T09:56:13Z

src/wantlist/entry.js

-    this.key = key
-    this.priority = isUndefined(priority) ? 1 : priority
+    this.cid = cid
+    this.priority = priority || 1


This is not good, you shouldn't use || if the input is a number.

dignifiedquire · 2016-12-10T09:57:43Z

src/wantlist/index.js

-    return this.set.get(mh.toB58String(key))
+  contains (cid) {
+    const cidStr = cid.toBaseEncodedString()
+    return !!this.set.get(cidStr)


this will break behaviour in other places, I've used this as a shortcut to avoid double lookups.

Oh, you want it to continue returning the value? ok

I'm okay with refactoring it so that it's not needed anymore, but there are places where changing this breaks things, that's all I wanted to point out.

dignifiedquire · 2016-12-10T10:02:48Z

test/wantmanager/index.spec.js

      const network = mockNetwork(6, (calls) => {
        expect(calls.connects).to.have.length(6)
        const m1 = new Message(true)
        m1.addEntry(new Buffer('hello'), cs.kMaxPriority)
-        m1.addEntry(new Buffer('world'), cs.kMaxPriority - 1)
+        m1.addEntry(ew Buffer('world'), cs.kMaxPriority - 1)


I don't think this was a good change 😛

This changed to cid :)

good, because I think we are all out of ew buffers

dignifiedquire · 2016-12-10T10:08:44Z

@diasdavid can we please move the question and answer section into individual issues, I think those all warrant discussion/thought/individual PRs and should be handled individually.

dignifiedquire · 2016-12-10T10:09:11Z

@diasdavid 😍 thank you for writing that diagram

dignifiedquire · 2016-12-10T18:38:05Z

Can you please update the test folder structure so it reflects the one on src?

daviddias · 2016-12-10T18:40:15Z

@diasdavid can we please move the question and answer section into individual issues, I think those all warrant discussion/thought/individual PRs and should be handled individually.

Absolutely, that is my plan once this gets merged. For now, it is just a place for me to collect everything.

@diasdavid 😍 thank you for writing that diagram

Glad you like it :D

Can you please update the test folder structure so it reflects the one on src?

On it, added to the tasks

daviddias · 2016-12-11T19:58:00Z

src/components/decision-engine/index.js

@@ -70,8 +69,9 @@ class DecisionEngine {
        log('got task')

        pull(
-          this.blockstore.getStream(nextTask.entry.cid),
+          this.blockstore.getStream(nextTask.entry.cid.toV0()),


I'll change this one to multihash, as it might look confusing.

daviddias · 2016-12-11T19:58:08Z

src/components/decision-engine/index.js

@@ -168,9 +167,8 @@ class DecisionEngine {
      ledger.wants(entry.cid, entry.priority)

      // If we already have the block, serve it
-      this.blockstore.has(entry.cid, (err, exists) => {
+      this.blockstore.has(entry.cid.toV0(), (err, exists) => {


and also here

start the work on network support

….1.0 messages

daviddias · 2016-12-18T22:22:03Z

Update

This PR is pretty much in a done state. The only two tests missing are the deserialization and serialization of BitswapMessages that came from go, but that is (I believe) cause they are mangled, as we can use the Protobuf to serialize and deserialize fine.

Some remarks, Bitswap, as a thing, is really complex and it understandable that it requires a deep dive to understand, however, the code isn't really helping, I hope with this PR it welcomes more contributors as I tries to structure and refactor to minimize the cognitive overhead that one has to take in order to tinker with the code. Other than the things listed to improve in the first comment, here are some more vectors where we can improve:

Less pull-streams overload, there were (and still are some cases) of pull-streams within pull-streams without any comments of what is intended, it takes some seconds to dial everything in.
The fact that we use native Maps, means that we get a log of .entries(), which in Bitswap message, entry has a meaning. I wonder how we can make this more explicit, to avoid data type head 💥

@dignifiedquire thank you for adding a lot of tests to this module, it made the migration so much easier and I was very confident as I walked through it ❤️

Tomorrow I will update the Block Service to use this new bitswap and then DAG API :)

daviddias · 2016-12-21T07:46:15Z

Now the code for this PR is present on: #84

I've opened issues to handle all the concerns presented in this PR:

daviddias added the status/in-progress In progress label Dec 10, 2016

daviddias added 12 commits December 9, 2016 20:25

add new protobuf

4384208

docs: move API docs to readme

b15fb89

fix: string is not the same as bytes in JS

effdbe2

feat: update wantlist to support cid

ae95bbe

wip: bitswap message

84abea9

feat: cid support in Bitswap message

99d64dd

docs: add some notes to the readme about what is the code structure

cebd4b2

test: cidV1 test in wantlist

3189db6

feat: update wantmanager msg-queue to use cid

8758da6

feat: wantmanager uses cid

fd60a9c

chore: refactor structure to make it more explicit what is which thin…

e7835cb

…g, reducing cognitive load

docs: add architecture graph

de99976

daviddias force-pushed the feat/bitswap-1.1.0 branch from ab1e902 to de99976 Compare December 10, 2016 04:26

dignifiedquire suggested changes Dec 10, 2016

View reviewed changes

daviddias added 5 commits December 10, 2016 12:59

cr: apply CR

8e3b7a7

feat: priority-queue done

d15910d

feat: PeerRequestQueue with CID support

c17f410

feat: upgrade ledger to use CID

89d922e

feat: decision engine migration to CID (just missing one test)

65dbd28

daviddias force-pushed the feat/bitswap-1.1.0 branch from fdac00f to 65dbd28 Compare December 11, 2016 17:48

fix engine tests

a8da40a

daviddias commented Dec 11, 2016

View reviewed changes

daviddias self-assigned this Dec 12, 2016

dignifiedquire and others added 2 commits December 12, 2016 17:22

use .multihash intead of toV0

0dc3b60

feat: update network to understand CID

32e8997

daviddias force-pushed the feat/bitswap-1.1.0 branch from ffc4d17 to 105456b Compare December 18, 2016 05:16

upgrade message to support bitswap 1.0.0 and 1.1.0 simultaneously, also

beff8d1

start the work on network support

daviddias force-pushed the feat/bitswap-1.1.0 branch from 105456b to beff8d1 Compare December 18, 2016 06:41

daviddias added status/in-progress In progress and removed status/ready Ready to be worked labels Dec 18, 2016

feat: upgrade network component to support bitswap 1.0.0 and bitwap 1…

3dc3493

….1.0 messages

daviddias force-pushed the feat/bitswap-1.1.0 branch 3 times, most recently from ac75d24 to f4ca6bd Compare December 18, 2016 22:07

feat: update bitswap own API

e892863

daviddias force-pushed the feat/bitswap-1.1.0 branch from f4ca6bd to e892863 Compare December 18, 2016 22:16

daviddias mentioned this pull request Dec 19, 2016

feat/use new bitswap ipfs/js-ipfs-block-service#51

Merged

daviddias requested a review from dignifiedquire December 19, 2016 09:17

dignifiedquire mentioned this pull request Dec 20, 2016

Now all together: Perf and CIDs #84

Merged

daviddias closed this Dec 21, 2016

daviddias deleted the feat/bitswap-1.1.0 branch December 21, 2016 07:55

daviddias removed the status/in-progress In progress label Dec 21, 2016

daviddias changed the title ~~WIP: Support bitswap 1.1.0 and bitswap 1.0.0 using CID~~ Support bitswap 1.1.0 and bitswap 1.0.0 using CID Dec 23, 2016

daviddias mentioned this pull request Jan 29, 2017

Document bitswap #18

Open

daviddias mentioned this pull request Sep 2, 2017

Create better documentation for bitswap + webpage for it #21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bitswap 1.1.0 and bitswap 1.0.0 using CID #76

Support bitswap 1.1.0 and bitswap 1.0.0 using CID #76

daviddias commented Dec 10, 2016 •

edited

dignifiedquire commented Dec 10, 2016

dignifiedquire left a comment

dignifiedquire Dec 10, 2016

dignifiedquire Dec 10, 2016

daviddias Dec 10, 2016

dignifiedquire Dec 10, 2016

dignifiedquire Dec 10, 2016

daviddias Dec 10, 2016

daviddias Dec 10, 2016

dignifiedquire Dec 10, 2016

dignifiedquire commented Dec 10, 2016

dignifiedquire commented Dec 10, 2016

dignifiedquire commented Dec 10, 2016

daviddias commented Dec 10, 2016

daviddias Dec 11, 2016

dignifiedquire Dec 12, 2016

daviddias Dec 11, 2016

dignifiedquire Dec 12, 2016

daviddias commented Dec 18, 2016 •

edited

daviddias commented Dec 21, 2016

Support bitswap 1.1.0 and bitswap 1.0.0 using CID #76

Support bitswap 1.1.0 and bitswap 1.0.0 using CID #76

Conversation

daviddias commented Dec 10, 2016 • edited

tl;dr; This PR adds support to CID and enables js-ipfs-bitswap to interop with both bitswap versions.

Thoughts and questions to answer

What's left

dignifiedquire commented Dec 10, 2016

dignifiedquire 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

dignifiedquire commented Dec 10, 2016

dignifiedquire commented Dec 10, 2016

dignifiedquire commented Dec 10, 2016

daviddias commented Dec 10, 2016

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

daviddias commented Dec 18, 2016 • edited

Update

daviddias commented Dec 21, 2016

daviddias commented Dec 10, 2016 •

edited

daviddias commented Dec 18, 2016 •

edited