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

Support bitswap 1.1.0 and bitswap 1.0.0 using CID #76

Closed
wants to merge 23 commits into from

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Dec 10, 2016

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:

image

  • 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

  • wantlist cid support
  • BitSwapMessage cid support
    • .serializeToBitswap1.0.0
    • .serializeToBitswap1.1.0
  • MessageQueue cid support
  • WantManager cid support
  • Create pretty graph
  • Update test folder structure to reflect moving around in src
  • DecisionEngine cid support
    • PriorityQueue
    • PeerRequestQueue
    • ledger
    • engine
  • Create network transform that serializes bitswap to 1.0.0 instead of 1.1.0
  • Default to dial on the new protocol version, try again on 1.0.0 if it fails

@daviddias daviddias added the status/in-progress In progress label Dec 10, 2016
@dignifiedquire
Copy link
Member

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

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

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 :)

this.key = key
this.priority = isUndefined(priority) ? 1 : priority
this.cid = cid
this.priority = priority || 1
Copy link
Member

Choose a reason for hiding this comment

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

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

return this.set.get(mh.toB58String(key))
contains (cid) {
const cidStr = cid.toBaseEncodedString()
return !!this.set.get(cidStr)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this was a good change 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

ew :P

Copy link
Member Author

Choose a reason for hiding this comment

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

This changed to cid :)

Copy link
Member

Choose a reason for hiding this comment

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

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

@dignifiedquire
Copy link
Member

@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
Copy link
Member

@diasdavid 😍 thank you for writing that diagram

@dignifiedquire
Copy link
Member

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

@daviddias
Copy link
Member Author

@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

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

pull(
this.blockstore.getStream(nextTask.entry.cid),
this.blockstore.getStream(nextTask.entry.cid.toV0()),
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

done

@@ -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) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

and also here

Copy link
Member

Choose a reason for hiding this comment

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

done

@daviddias daviddias self-assigned this Dec 12, 2016
@daviddias daviddias added status/in-progress In progress and removed status/ready Ready to be worked labels Dec 18, 2016
@daviddias daviddias force-pushed the feat/bitswap-1.1.0 branch 3 times, most recently from ac75d24 to f4ca6bd Compare December 18, 2016 22:07
@daviddias
Copy link
Member Author

daviddias commented Dec 18, 2016

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 daviddias closed this Dec 21, 2016
@daviddias daviddias deleted the feat/bitswap-1.1.0 branch December 21, 2016 07:55
@daviddias daviddias removed the status/in-progress In progress label Dec 21, 2016
@daviddias 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 daviddias mentioned this pull request Jan 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants