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

Clarify STREAM Connection Details Functionality #573

Merged
merged 22 commits into from
Jul 29, 2020

Conversation

sappenin
Copy link
Contributor

@sappenin sappenin commented May 2, 2020

This PR clarifies STREAM functionality by tightening the language related to ConnectionAssetDetails and ConnectionNewAddress frames.

Specifically:

  1. ConnectionNewAddress is useful for a receiver that wants to change its ILP address or for a client that wants to become a receiver. Therefore:
    1. ConnectionNewAddress Frame is optional, and can be sent at any time (e.g., a non-routable sender shouldn't send it unless it has other reasons to send it -- see below).
    2. A client only emits a ConnectionNewAddress once it wants to enable the receiver to start sending to it (this generally happens near the beginning of a Connection, but doesn't have to).
    3. A receiver (i.e., a client or server playing that role) emits a ConnectionNewAddress frame if it needs to migrate the connection.
  2. Info in ConnectionAssetDetails is typically only used by senders, and only when the sender needs to verify the amount received on a path. Therefore, a sender doesn’t generally emit this frame (the receiver does).

Signed-off-by: sappenin sappenin@gmail.com

adrianhopebailie and others added 3 commits April 23, 2020 15:26
This PR clarifies STREAM functionality by tightening the language related to `ConnectionAssetDetails` and `ConnectionNewAddress` frames.

Specifically:

1. `ConnectionNewAddress` is useful for a receiver that wants to change its ILP address or for a client that wants to become a receiver. Therefore:
    1. A _client_ only emits a `ConnectionNewAddress` near the beginning of a Connection if it wants to receive.
    1. A _receiver_ (i.e., a client or server playing that role) emits a `ConnectionNewAddress` frame if it needs to migrate the connection.
1. Info in `ConnectionAssetDetails` is typically only used by senders, and only when the sender needs to verify the amount received on a path. A _sender_ dooesn’t generally to emit this frame; only the reciever does.
1. A client should send a `ConnectionAssetDetails` whenever the sequence identifier is 0, and ignore duplicate frames on other sequences. If a receiver misses this frame, then the connection should be closed and restarted.

Signed-off-by: sappenin <sappenin@gmail.com>
…rame-clarifications

Signed-off-by: sappenin <sappenin@gmail.com>

# Conflicts:
#	0029-stream/0029-stream.md
Signed-off-by: sappenin <sappenin@gmail.com>
Signed-off-by: sappenin <sappenin@gmail.com>
* Bump draft numbers for SPSP and STREAM
* Add appropriate `type` designators

Signed-off-by: sappenin <sappenin@gmail.com>
…/stream-frame-clarifications

Signed-off-by: sappenin <sappenin@gmail.com>

# Conflicts:
#	0029-stream/0029-stream.md
@sappenin sappenin changed the base branch from master to stream-receipts May 2, 2020 17:39
@sappenin sappenin changed the title Clarify STREAM Connection Details Functionality WIP: Clarify STREAM Connection Details Functionality May 2, 2020
@sappenin sappenin changed the title WIP: Clarify STREAM Connection Details Functionality Clarify STREAM Connection Details Functionality May 2, 2020
Signed-off-by: sappenin <sappenin@gmail.com>
@sappenin sappenin changed the base branch from stream-receipts to master May 5, 2020 19:17
…rame-clarifications

Signed-off-by: sappenin <sappenin@gmail.com>

# Conflicts:
#	0029-stream/0029-stream.md
Signed-off-by: sappenin <sappenin@gmail.com>
@sappenin
Copy link
Contributor Author

sappenin commented May 5, 2020

I met with @kincaidoneil and @nhartner and we reached a rough consensus around how to clarify the verbiage in this RFC (i.e., what should be in this PR). Those PR changes have been made and this PR is ready to be reviewed.

We also discovered that there is lack-of-consensus around one commonly-deployed STREAM topology. That is, in the case where the client is the sender, and is non-routable (i.e., "send only") how should the client expect to get a ConnectionAssetDetails from the server/receiver (see below for more details)?

For now, here's what we agreed to:

  1. Clarification to RFC-29
    1. See PR description above.
  2. JS Sender/Receiver Changes
    1. Receiver: Don’t close stream when receiver’s rate probe fails.
    2. Receiver: Don’t close stream when data-reporting back to the client fails
    3. Backward Compatibility: JS client send a CNA with empty ILP address.
  3. Java Sender/Receiver Changes
    1. StreamCore: Make ConnectionNewAddress frame allow an empty address.
    2. StatelessReceiver: If you get a ConnectionNewAddress frame, even with an empty address, always respond with a ConnectionAssetDetails (and sometimes CNA will have empty address).

**Question: How does a client-sender tell the receiver server to send it a ConnectionAssetDetails frame?

No consensus. Some people think, “get it from a higher layer protocol” and some people think “get it from the receiver somehow by allowing the client to 'trigger' the frame somehow" -- implementations support both, and triggering a CAD is done by sending a CNA frame until you get back a CAD.

Nobody really likes the current state of affairs, because the spec is silent which makes for ambiguity. We identified that if the future state of the world is, "STREAM should allow a client-sender to "request" a ConnectionAssetDetails", then we should change the RFC. However, if the future state of the world is that ConnectionAssetDetails are only obtained from higher-layer protocols, then the RFC should be silent here because nobody will get ConnectionAssetDetails in this manner.

This probably deserves more community weigh-in, but for now all implementations use the CNA/CAD dance, which is OK in the short term.

0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Show resolved Hide resolved
sappenin and others added 3 commits May 7, 2020 14:01
Co-authored-by: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>
Co-authored-by: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>
Co-authored-by: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>
sappenin and others added 5 commits May 7, 2020 15:06
Co-authored-by: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>
Signed-off-by: sappenin <sappenin@gmail.com>
Signed-off-by: sappenin <sappenin@gmail.com>
Signed-off-by: sappenin <sappenin@gmail.com>
Signed-off-by: sappenin <sappenin@gmail.com>
Copy link
Member

@kincaidoneil kincaidoneil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- just a few minor edits.

0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
Co-authored-by: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>
@stale stale bot added the wontfix label Jul 3, 2020
@interledger interledger deleted a comment from stale bot Jul 10, 2020
@stale stale bot removed the wontfix label Jul 10, 2020
sappenin and others added 2 commits July 15, 2020 17:29
Co-authored-by: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>
Co-authored-by: Kincaid O'Neil <kincaidoneil@users.noreply.github.com>
@kincaidoneil kincaidoneil self-requested a review July 16, 2020 14:00
@sappenin sappenin merged commit 2861e14 into master Jul 29, 2020
@sappenin sappenin deleted the df/stream-frame-clarifications branch July 29, 2020 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants