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

Replace protobuf messages with IPLD #88

Open
chafey opened this issue Aug 25, 2020 · 5 comments
Open

Replace protobuf messages with IPLD #88

chafey opened this issue Aug 25, 2020 · 5 comments
Assignees
Labels
effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important P2 Medium: Good to have, but can wait until someone steps up

Comments

@chafey
Copy link

chafey commented Aug 25, 2020

go-graphsync currently uses google protobuf for messaging. Since graphsync depends on IPLD and IPLD can be used in place of GPB, we can reduce dependencies, tooling and simplify the design by replacing GPB with IPLD. IPLD schema can be used to define the messages and dag-cbor could be used to encode them. @warpfork said this was previously discussed but cut due to time constraints - adding this issue for tracking

@chafey chafey added the need/triage Needs initial labeling and prioritization label Aug 25, 2020
@welcome
Copy link

welcome bot commented Aug 25, 2020

Thank you for submitting your first issue to this repository! A maintainer will be here shortly to triage and review.
In the meantime, please double-check that you have provided all the necessary information to make this process easy! Any information that can help save additional round trips is useful! We currently aim to give initial feedback within two business days. If this does not happen, feel free to leave a comment.
Please keep an eye on how this issue will be labeled, as labels give an overview of priorities, assignments and additional actions requested by the maintainers:

  • "Priority" labels will show how urgent this is for the team.
  • "Status" labels will show if this is ready to be worked on, blocked, or in progress.
  • "Need" labels will indicate if additional input or analysis is required.

Finally, remember to use https://discuss.ipfs.io if you just need general support.

@chafey chafey changed the title Replace protobuf messages with dag-cbor Replace protobuf messages with IPLD Aug 25, 2020
@hannahhoward
Copy link
Collaborator

I am fully on board with this but would like it to be done in a backwards compatible way, as Filecoin mainnet is likely to ship with protobuf graphsync

In other words, I'd like to keep the protobuf protocol and define a new versioned protocol for the IPLD version, and gradually phase out the old one.

Also, serialization/deserialization of messages is very speed and memory critical, so we may need a code generated node or other specialized IPLD type to make it work.

I started down this before, and hit some challenges, that I want to lay out here -- not to discourage, but basically to cover all the dragons ahead of time -- I think getting these messages encoded/decoded with ipld-prime is a GREAT use case for proving IPLD primes performance abilities and flexibility.


I hit a bit of a barrier last time around the extensions. In the current protobuff work they are:

map[string][]byte

But actually, the []byte is just CBOR-encoded IPLD data. Once the whole thing is a CBOR IPLD struct, by IPLD schema is effectively:

type ExtensionName string
type Extensions {ExtensionName:Any}
type Request {
   ...
   Extensions Extensions
   ...
}

Except last I had checked there was still contention about whether IPLD schemas included an Any type.

Moreover, because there's no obvious way to fast deserialize an "Any" type of IPLD data, I had planned to defer deserialziation (note, this is different than a simply []byte type -- deferring deserialization means that the overall encoded data structure is still a correct CBOR encoding of the nested data).

Side note: After hitting this barrier, I had planned for my first quick and dirty version to not try to use ipld-prime at all and rather just use https://github.com/whyrusleeping/cbor-gen along with the Deferred type it uses for holding of on deserialization of certain types

One other challenge: Selectors -- in the protobuf we write them as []byte, but obviously they are just a node embedded in the larger structure. We don't currently have a custom node type for selectors. A Selector is an IPLD Node, but since we don't have a custom node type, we read them in as generic nodes and run parsing code to convert them into an executable Selector. I believe @warpfork's intent was to eventually just have a custom type, which makes sense to me. Then you can properly serialize/deserialize into the message and make it executable in a single step.

Anyway, these are the things that made me go ... "oh better go jump on some filecoin challenges"

@chafey
Copy link
Author

chafey commented Sep 3, 2020

Thanks for this information, it is really helpful! I will maintain backwards compatibility with the existing probobuf based version.

@hannahhoward hannahhoward added effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important P2 Medium: Good to have, but can wait until someone steps up and removed need/triage Needs initial labeling and prioritization labels Sep 28, 2021
@hannahhoward
Copy link
Collaborator

see for Proposal on format to use

@mvdan
Copy link
Contributor

mvdan commented Feb 4, 2022

Now being done by @rvagg in #332 :) I already did the IPLD part a few weeks ago, so I'm going to unassign myself for now. I continue to support Rod as he needs me to.

@rvagg rvagg self-assigned this Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

No branches or pull requests

4 participants