Re: [ppsp] peer protocol in WG last call

[Arno Bakker <[e-mail]>]

Hi Riccardo

thanks for the in-depth review! Please see inline.

On 19/11/2012 11:45, Riccardo Bernardini wrote:
>
> First, the easy stuff.  Some typos:
>

Will be corrected.


> Now to the "consistent" stuff:
>
> == Page 9, Section 2.3 ==
>      "If A crashes ungracefully, peers should remove A from their peer
> list when they detect it no longer send messages."
>
> How much is "too much"?  Should we leave the peer free
> to decide or should we give at least some guidelines?
>

Your proposal makes sense, I'm just not sure what is customary in RFCs.
Should this go in this draft or in the planned usage guide?
Chairs?


> Also, there is the problem that when A crashes it remains registered
> with the tracker, although maybe this is not a problem directly
> related with PPSPP.
>

This is addressed in the current base tracker proposal. A peer needs to 
periodically refresh its registration to stay tracked.


> == Page 10, Section 3 ==
>
>     It is said that "absence of any response indicates and error" and
> that "Invalid messages are discarded, and further communication with
> the peer SHOULD be stopped."
>
> Isn't this criterion a bit too strong?  If a request is lost (e.g.,
> because of congestion), the sender will interpret the absence of
> response as an error or a bad peer.  Also, how long should a peer wait
> for declaring absence of response?  Should we give guidelines?
>

We could give guidelines, but again, I'm not sure that is customary. I 
assume implementers apply common sense and, for example, not declare a 
peer dead after 1 packet loss over an unreliable transport. Invalid 
messages mean that there is really something wrong and that sender and 
receiver don't understand eachother, so then the receiver really should 
stop the interaction.


> == Page 12, Section 3.9. ==
>
> It is said "The CHOKE and UNCHOKE messages are informational as a peer
> is not required to respond to REQUESTs."  However, if I understand
> correctly, if a peer does not respond to REQUESTs, the peer will be
> banned, according to the criterion in Section 3.  Am I missing
> something?
>

No, I don't think you are missing something. The idea is that a peer P 
requesting chunks from peer Q will look for another peer R when Q 
doesn't send chunks, and Q didn't send a CHOKE to announce the fact it 
wouldn't be responding. P should take into account packet loss and swarm 
size (if there is just 1 other peer in the swarm, don't disconnect), of 
course. If P is not getting chunks from Q it moves on.


> == Page 13, Section 3.10.2 ==
>
> I have a couple of doubts about this
>
>     * This supposes that A can send messages to B.
>
>       so this should not be a problem. Would it be worth adding a
> sentence to emphasize this point? (e.g. "Note that A can send a
> message to B since by 3.10.1 A must have exchanged messages with C..."

Sure.


>    * With this model of hole punching, the peer can only connect
> initially to peers with public IP; in particular, the addresses
> provided by the tracker must be public IPs.  Am I right?
>

Yes.


> == Page 16, last paragraph of Section 4.3.1 ==
>
>      Implementation note:
>          To record which chunks ... discussed in detail in [BINMAP]
>

You are right.


> == Page 16, Section 4.3.2 ==
>
> Another stylistic detail.  It is said that "...an implementation MAY
> choose to use ACK messages..." than later "When a receiving peer has
> successfully checked ... it MUST send an ACK message...".

Will be corrected.


> == Top of Page 17, Section 4.4 ==
>
>     "When a range translates to multiple bins, the byte-range peer the
> byte-range peer should send multiple e.g. HAVE message."
>
> This sentence looks a bit garbled, at the very least "the byte-range
> peer" is repeated twice.
>

Will be corrected.


> == Page 20, Section 5.3 ==
>
>     "The hashes necessary to verify a chunk are in principle its
> sibling's hash and all its uncle hashes ... can be optimized "
>
> If I understand correctly, this optimization requires that a peer MUST
> save and preserve all the received hashes.

Yes. The requirement that a sender must be able to provide these hashes 
implies that you should record them. I'll explicitly add the conclusion 
that this means you MUST record them, *if* you plan to announce you have 
them to others.

In other words, limited devices can circument this requirement by not 
sending HAVEs to others after they received a chunk. Hence they are not 
announcing the availability and are not required to provide them. Not 
sending HAVEs is allowed by the protocol. Note, however, that this does 
require that the implementation uses a reciprocity policy that allows 
this behaviour, as the limited devices are actually freeriding.


> == Page 29, Section 8.2. "Version option" ==
>
> Maybe this is thinking a bit in advance, but what should a peer set as
> version value when the peer is able to "speak" more than one protocol
> version?

Do you have pointers to protocols where this mechanism is used? This 
could save a few roundtrips of HANDSHAKE messages, so good for 
time-till-playback if there are indeed several incompatible versions, 
which I hope there will not be ;o)


> == Page 34, Section 9.3. Channels ==
>
> It is said that
>
>     "... PPSPP-over-UDP uses a multiplexing scheme, called "channels"..."
>
> Later it is said that
>
>     "When channels are used..."
>
> My doubt is: is the use of channels mandatory with UDP?  If yes, the
> hypothesis "when channels are used..." is redundant;

Yes it is mandatory. Pardon me if I'm wrong, but your point may just be 
grammatical, "When X" in English implies that X will be the case at some 
point. So you say "When I die" not "If I die", but I'm not a native 
speaker myself. I'll make it clearer.


> == Page 42, Section 13.2.2. ==
>
> This section is about using the tracker as certification authority.
> It is said
>
>     "Upon receipt, the tracker creates a membership certificate from
> the request with swarm ID S, a timestamp T and the external IP and
> port it received the message from..."
>
> Am I wrong, or this scheme could fail if the peer is behind a
> symmetric NAT?  Would it be worth to add at least a note about this?
>

You are right, but that problem applies for any tracker registration.
I'll add a note.

CU,
     Arno