network spec feedback

brief feedback as requested, written stream of consciousness not edited for politeness/understandability/clarity, please to forgive.

- biggest piece of general feedback: orient the spec more around me as an implementer, give me a skeleton to implement, sketch what goes inside of it, and tell me about practicalities. def also precisely define wire formats etc but that should be the easy part. with my implementer hat on when i open a protocol doc like this i want to see:
- sequence diagram like [this](https://user-images.githubusercontent.com/884507/41811847-2d6fdf92-76cc-11e8-95d0-0c3fa7412d08.png) or [this](https://user-images.githubusercontent.com/884507/41666941-8e49c6fc-7460-11e8-916a-cca93d832ee3.png). does not have to be as detailed at first but something visual like this
- wire formats which i see great yay
- pseudo-code for functions that i need to implement eg Client.ProposeDeal(ask, file) has pseudocode steps like "preprocess piece", "ingest into blockservice if using default transport", "compute cid or get from call above", "use lookup service to locate miner address and open stream to them", etc. Then same on the miner side.
- a v short list of practicalities: what data do i need to persist on the miner side, on the client side, how to signal errors, should i time deals out, note in progress deals is a buffer that should be limited in length, etc

I don't know if the above is in conflict with an idea that the spec only covers interactions between entities in the protocol. It would be really helpful tho.

specific feedback on https://github.com/filecoin-project/specs/blob/master/network-protocols.md:
- cbor rpc: re "varint length delimited series of cbor serialized objects": there is a lot room for (mis)-interpretation here and i cannot find a spec for this encoding.
- "Length delimited series of serialized objects" seems to imply that the outer thing is a series of serialized objects instead of one serialized object that has embedded fields? I think this means that the top-level fields in the object message are each separately cbor-encoded, and then concatenated in a length-delimited fashion. An alternative, simpler explanation is "serialize X and prefix its with leb128-encoded length" which is what it later says, but the "objects" in the initial description throws me.
- I think there's also ambiguity in how data are cbor encoded. I haven't looked at the cbor spec in a while but I recall that encoding is not unambiguous (eg are a struct's field keyed by name? do they appear sequentially?).
- What is the model for change for these messages?
- What do implementations do with fields they don't recognize?
- Are there limits on message size?
- Do clients ever signal timeouts?
- Is there a standard way of signaling errors?
- Hello
- json whaaaaa? Why not just drop the "json" from the spec and file an issue on go-filecoin?
- ok so the exchange of hello is not symmetrical, correct? node A connects to B and then B initiates a new connection to A. Seems easier to implement if the exchange is symmetrical: A connects to B and sends hello and waits for hello in return; no book-keeping on A's side. (If you are wondering why I think it is not symmetrical the language is "writing it over the stream, and finally, closing the stream.")
- if it is in fact not symmetrical that is a little weird: it requires A be callable by B. Why can't A just connect to B and they exchange hellos over that stream?
- GetHelloMessage could imply receiving the message. Maybe better: NewHelloMessage().
- "should probably be disconnected from" -- no you definitely should in general. Dont leave it ambiguous.
- when I receive a hello from someone i'm compatible with is the ONLY thing i do pass the tipset/height/peerid to the syncer? Also is there no additional thing I should do that is maybe add this peer to the set of known peers or will that happen automatically? (That should probably not be chain syncer responsibility -- chain syncer should sync chain, not maintain peer connections).
- i think we also will need something here in hello for filecoin protocol version eg to prevent something from before a hard fork from talking to something after
- storage deal
- drop reference to broker
- what does prepared into a piece mean? important to note that i think you should already have added it to your block service, or at least chunked, ipld-ified, maybe encrypted etc
- assumption going into this protocol: that the client identified a specific ask in the on-chain storage market and they have found their address via the lookup service
- i would love it if going forward we called things that were cids foocid. when we call a foocid a fooref i have to go think about what a ref is. turns out it is a cid. two names, one cup. i am not saying change this protocol, i'm saying next time.
- i have no idea how to compute translatedref
- clarify that piece referenced by cid is ipld-encoded
- clarify that size is sum of all pieces transitively reachable by pieceref
- i've always been unclear on price units -- i think this is the price to store the piece for numblocks which i guess we have to compute from the ask
- payment vs paymentref in comment
- paychactor -- i think there is exactly one payment channel actor? maybe for now say this is set to address of "the" payment channel actor since we only have one?
- we should say specifically what kind of validation the miner does on the storagedealproposal
- doesn't paymentvoucher include channelid in it? if so why do we need both in paymentinfo?
- how as a client do i know how many vouchers to include paymentinfo and in what amounts?
- deal state value
- lead with 1 sentence about what deal state is
- indentation issues in code section
- "negotiation" in the comment of Unknown is a non-sequitor, what is a negotiation? it is not mentioned in this doc
- staged and complete should be in the opposite order so ocd people like me dont lose their marbles?
- re "possibly include" this kind of note i think will confuse readers. i think the spec should be the spec and not include maybe we shoulds.
- "After the first response, " this sentence occurs before the definition of the response.
- for the future I think we should always, always call things FooRequest and FooResponse, makes it so much more clear. dont change this one, next time.
- should note in the request section that the client needs to keep track of its deal requests by cid because this is how storagedealresponses are queries are keyed
- piececonfirmation isn't something i can get from the chain?
ok i ran out of time :(

Start with network-protocols.md, especially the CBOR RPC, Hello, storage deal, and deal state sections named in the feedback. Establish the intended protocol behavior and then update the specification with implementer-oriented diagrams, pseudocode, wire-format definitions, and practical guidance. Done means the listed ambiguities and terminology questions have clear, agreed answers in the document.

Written by the indexing model from the issue text.