Proposal 340 clarifications

2 files changed, 81 insertions, 35 deletions

diff --git a/proposals/340-packed-and-fragmented.md b/proposals/340-packed-and-fragmented.md
index da0cbdb..2407f99 100644
--- a/proposals/340-packed-and-fragmented.md
+++ b/proposals/340-packed-and-fragmented.md
@@ -22,6 +22,9 @@ between messages and cells, for two reasons:
    (notably `SENDME`, `XON`, `XOFF`, and several types from
    [proposal 329](./329-traffic-splitting.txt)) are much smaller than
    the relay cell size, and could be sent comparatively often.
+   We also want to be able to *hide* the transmission of small control messages
+   by packing them into what would have been the padding of other messages,
+   making them effectively invisible to a network traffic observer.
 
 In this proposal, we describe a way to decouple relay cells from relay
 messages.  Relay messages can now be packed into multiple cells or split
@@ -65,7 +68,7 @@ If we are lucky, we won't have to build this encryption at all, and we
 can just move to some version of GCM-UIV or other RPRP that reserves 16
 bytes for an authentication tag or similar cryptographic object.
 
-The `body` MUST contain exactly 493 bytes as cells have a fixed size.
+The `body` MUST contain exactly 493 bytes as relay cells have a fixed size.
 
 ## New relay message packing
 
@@ -94,14 +97,16 @@ The following message types take required stream IDs: `BEGIN`, `DATA`, `END`,
 The following message types from proposal 339 (UDP) take required stream IDs:
 `CONNECT_UDP`, `CONNECTED_UDP` and `DATAGRAM`.
 
-No other message types take stream IDs. The `stream_id` field, when present,
-MUST NOT be zero.
+No other current message types take stream IDs. The `stream_id` field, when
+present, MUST NOT be zero.
 
 Messages can be split across relay cells; multiple messages can occur in
 a single relay cell.  We enforce the following rules:
 
   * Headers may not be split across cells.
   * If a 0 byte follows a message body, there are no more messages.
+  * A message body is permitted to end at exactly the end of a relay cell,
+    without a 0 byte afterwards.
   * A relay cell may not be "empty": it must have at least some part of
     some message.
 
@@ -116,40 +121,41 @@ Receivers MUST validate that the cell `header` and the `message header` are
 well-formed and have valid lengths while handling the cell in which the header
 is encoded. If any of them is invalid, the circuit MUST be destroyed.
 
-An unrecognized `command` is considered invalid and thus MUST result in the
-circuit being immediately destroyed.
+A message header with an unrecognized `command` is considered invalid and thus
+MUST result in the circuit being immediately destroyed (without waiting for the
+rest of the relay message to arrive, in the case of a fragmented message).
 
-## Negotiation (OBSOLETE)
+## New subprotocol `RelayCell`
 
-> We do not want to do it this way. Instead, we want to use the system
-> described in the forthcoming proposal, "xxx-protovers-again.md".
+We introduce a new subprotocol `RelayCell` to specify the relay cell ABI. The
+new format specified in this proposal, supporting packing and fragmentation,
+corresponds to `RelayCell` version 1. The ABI prior to this proposal is
+`RelayCell` version 0.
 
-This message format requires a new `Relay` subprotocol version to indicate
-support. If a client wants to indicate support for this format, it sends the
-following extension as part of its `ntor3` handshake:
+All clients and relays implicitly support `RelayCell` version 0.
 
-  EXT_FIELD_TYPE:
+> XXX: Do we want to consider some migration path for eventually removing
+> support for `RelayCell` version 0? e.g. maybe this should be something like
+> "Support for any of `Relay` versions 1-5 imply support for `RelayCell`
+> version 0"?
 
-    [03] -- Packed and Fragmented Cell Request
+We reserve the protocol ID 13 for binary encoding of this subprotocol with
+respect to [proposal 346][prop346] and [proposal 323][prop323].
 
-This field is zero payload length. Its presence signifies that the client
-wants to use packed and fragmented cells on the circuit.
+To use `RelayCell` version 1 or greater with a given relay on a given circuit,
+the client negotiates it using an `ntor_v3` extension, as per [proposal
+346][prop346]. This implies that the relay must advertise support for `Relay`
+version 5 (`ntor_v3` circuit extensions) as well as the target `RelayCell`
+version (1 for the format introduced in this proposal).
 
-The Exit side ntorv3 response payload is encoded as:
-
-  EXT_FIELD_TYPE:
-
-    [04] -- Packed and Fragmented Cell Response
-
-Again, the extension presence indicates to the client that the Exit has
-acknowledged the feature and is ready to use it. If the extension is not
-present, the client MUST not use the packed and fragmented feature even though
-the Exit has advertised the correct protover.
-
-The client MUST reject the handshake and thus close the circuit if:
-
-  - The response extension is seen for a non-ntorv3 handshake.
-  - The response extension is seen but no request was made initially.
+Circuits using mixed `RelayCell` versions are permitted. e.g. we anticipate
+some of the use-cases for packing and fragmentation to only need the exit-relay
+to support it. Not requiring `RelayCell=1` for other relays in the circuit
+provides a larger pool of candidate relays. While an intermediate relay using a
+different `RelayCell` version than the destination relay of a given relay cell
+will look at the wrong bytes for the `recognized` and `digest` fields, they
+will reach the correct conclusion that the cell is not intended for them and
+pass it to the next hop in the circuit.
 
 ## Migration
 
@@ -181,6 +187,23 @@ there is, then it adds a DATA message to the end of the current cell,
 with as much data as possible.  Otherwise, the client sends the cell
 with no packed data.
 
+> XXX: This isn't quite right. What was actually implemented in tor, and
+> what we want in arti, is to defer sending some "control" messages like
+> confluence switch and (non-first) xon, until they can be invisibly packed
+> into a cell for a DATA message.
+>
+> dgoulet: Could you update this section with the concrete details, and exactly
+> what property we're trying to achieve? e.g.:
+>
+> If we have data to send, but the corresponding DATA messages don't leave
+> enough room to pack in the deferred control message(s), what do we do? If we
+> continue deferring could we end up deferring forever if the application
+> always writes in chunks that happen to align this way?
+>
+> Since cells containing any part of a DATA message is subject to congestion
+> windows, does that mean if our congestion window is empty we can't send these
+> control messages either (until the window becomes non-empty)?
+
 ## Onion services
 
 Negotiating this for onion services will happen in a separate proposal;
@@ -259,7 +282,7 @@ follows:
      accommodates most reasonable MTU choices)
 
 Any increase in maximum length for any other message type requires a new
-Relay subprotocol version.  (For example, if we later want to allow
+`RelayCell` subprotocol version.  (For example, if we later want to allow
 EXTEND2 messages to be 2000 bytes long, we need to add a new proposal
 saying so, and reserving a new subprotocol version.)
 
@@ -288,7 +311,27 @@ Here is an example of the simplest case: one message, sent in one relay cell:
       random                            [464 bytes]
 ```
 
-Total of 514 bytes which is the absolute maximum cell size.
+Total of 514 bytes which is the absolute maximum relay cell size.
+
+A message whose body ends at exactly the end of a relay cell has no
+corresponding end-of-messages marker.
+
+```
+  Cell 1:
+    header:
+       circid         ..                [4 bytes]
+       command        RELAY             [1 byte]
+    relay cell header:
+       recognized     0                 [2 bytes]
+       digest         (...)             [14 bytes]
+    message header:
+       command        DATA              [1 byte]
+       length         488               [2 bytes]
+    message routing header:
+       stream_id      42                [2 bytes]
+    message body:
+       (data)                           [488 bytes]
+```
 
 Here's an example with fragmentation only: a large EXTEND2 message split
 across two relay cells.
@@ -320,7 +363,6 @@ across two relay cells.
       0                                [1 byte]
     padding up to end of cell:
       random                           [182 bytes]
-
 ```
 
 Each cells are 514 bytes for a message body totalling 800 bytes.
@@ -428,3 +470,6 @@ what parties need to accept.)
     padding up to end of cell:
       random                           [241 bytes]
 ```
+
+[prop323]: ./323-walking-onions-full.md "Specification for Walking Onions"
+[prop346]: ./346-protovers-again.md "Clarifying and extending the use of protocol versioning"
diff --git a/proposals/346-protovers-again.md b/proposals/346-protovers-again.md
index a18e238..a5c7e24 100644
--- a/proposals/346-protovers-again.md
+++ b/proposals/346-protovers-again.md
@@ -93,7 +93,8 @@ ordered in the same way as in tor-spec.
 | Padding  | 10 |
 | FlowCtrl | 11 |
 | Conflux  | 12 |
-| Datagram | 13 |
+| RelayCell| 13 |
+| Datagram | TBD|
 
 > Note: This is the same encoding used in [walking onions proposal][prop323].
 > It takes its order from the ordering of protocol versions in
@@ -172,7 +173,7 @@ Currently specified subprotocol versions which can be negotiated using
 this extension are:
 
    * FlowCtrl=2 (congestion control)
-   * Packed-and-fragmented support (proposal 340)
+   * RelayCell=1 (proposal 340)
 
 The availability of the `subproto_request` extension itself
 will be indicated by a new Relay=X flag.  When used, it will supplant