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

Document bitswap #18

Open
jbenet opened this issue May 30, 2016 · 5 comments
Open

Document bitswap #18

jbenet opened this issue May 30, 2016 · 5 comments
Labels
exp/wizard Extensive knowledge (implications, ramifications) required help wanted Seeking public contribution on this issue P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked topic/docs Documentation

Comments

@jbenet
Copy link
Member

jbenet commented May 30, 2016

Hey @dignifiedquire i have some questions re impl. Forgive silliness, first time i look at it.

  1. why is bitswap block key going to hex in the proto msg? it should be straight binary -- https://github.com/ipfs/js-ipfs-bitswap/blob/master/src/message/index.js#L48
  2. why bitswap.stop has this.network.start() https://github.com/ipfs/js-ipfs-bitswap/blob/master/src/index.js#L276
  3. why bitswap.network uses bl? isnt this wasteful? is it caching all buffers? (i've only glanced at bl, havent used it) https://github.com/ipfs/js-ipfs-bitswap/blob/master/src/network/index.js#L40
  4. provierRequestTimeout spelling - https://github.com/ipfs/js-ipfs-bitswap/blob/master/src/constants.js#L7
@dignifiedquire
Copy link
Member

Nothing to forgive,

  1. Actually it is not, as far as I understand go-bitswap casts the key to a string on the conversion to protobuf, which results in a conversion to base58
  2. cough typo *cough
  3. it's buffering up a single message, which we have to because the protocol-buffer parser we are using can only parse a complete message
  4. I like that spelling :)

@daviddias
Copy link
Member

Actually it is not, as far as I understand go-bitswap casts the key to a string on the conversion to protobuf, which results in a conversion to base58

Still, not hex. Still you catched a bunch of these in your PR, but miss this one -> https://github.com/ipfs/js-ipfs-bitswap/pull/17/files#diff-e4c2bb9741805e47edd01ae04b0894b5R52

@dignifiedquire
Copy link
Member

still, not hex. Still you catched a bunch of these in your PR, but miss this one

yes correct

@jbenet
Copy link
Member Author

jbenet commented May 30, 2016

Actually it is not, as far as I understand go-bitswap casts the key to a string on the conversion to protobuf, which results in a conversion to base58

I don't think this https://github.com/ipfs/go-ipfs/blob/master/exchange/bitswap/message/message.go#L156 calls https://github.com/ipfs/go-ipfs/blob/master/blocks/key/key.go#L16-L18 -- i think the keys are sent as straight up binary. calling string(buf) only casts to string, does not call .String(). cc @whyrusleeping

it's buffering up a single message, which we have to because the protocol-buffer parser we are using can only parse a complete message

ok so only one message? it releases the previous messages?

@daviddias daviddias self-assigned this Oct 31, 2016
@daviddias daviddias added status/in-progress In progress and removed js-ipfs-ready labels Oct 31, 2016
@daviddias daviddias added status/ready Ready to be worked and removed status/in-progress In progress labels Dec 5, 2016
@daviddias daviddias removed their assignment Dec 7, 2016
@daviddias daviddias changed the title Questions about Impl Document bitswap Dec 7, 2016
@daviddias
Copy link
Member

There is now a bit more documentation of Bitswap - https://github.com/ipfs/js-ipfs-bitswap#development

A lot came from this PR - #76 pointing it here for context

@daviddias daviddias added exp/wizard Extensive knowledge (implications, ramifications) required help wanted Seeking public contribution on this issue status/deferred Conscious decision to pause or backlog and removed status/ready Ready to be worked labels Jan 29, 2017
@daviddias daviddias added status/ready Ready to be worked P1 High: Likely tackled by core team if no one steps up and removed status/deferred Conscious decision to pause or backlog labels Oct 17, 2017
@daviddias daviddias added the topic/docs Documentation label Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/wizard Extensive knowledge (implications, ramifications) required help wanted Seeking public contribution on this issue P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked topic/docs Documentation
Projects
None yet
Development

No branches or pull requests

3 participants