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

Add STREAM receipts #570

Merged
merged 20 commits into from
Apr 22, 2020
Merged

Add STREAM receipts #570

merged 20 commits into from
Apr 22, 2020

Conversation

wilsonianb
Copy link
Contributor

| Receipt Nonce | UInt128 | A random nonce pre-shared between the verifying party and the receiver used to identify the STREAM connection. |
| Stream ID | UInt8 | Identifier of the stream this receipt refers to. |
| Total Received | UInt64 | Total amount, denominated in the units of the receiver, that the receiver has received on this stream thus far. |
| Stream Start Time | UInt64 | A UNIX timestamp referring to the time that the stream was established at the receiver. |

Choose a reason for hiding this comment

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

It's worth considering whether we should use an InterledgerTimestamp here. It still is fixed length, and although it is a little trickier to parse it solves things like leap seconds. That said a unix timestamp is gonna be easier to use in a lot of different environments.


| Field | Type | Description |
|---|---|---|
| HMAC | UInt256 | HMAC-SHA256 over all other fields using a secret pre-shared between the verifying party and the receiver. |

Choose a reason for hiding this comment

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

we should explicitly name this pre-shared secret the Receipt Secret and be explicit that it's an additional optional parameter to create a STREAM connection. I could imagine someone reading this and thinking we're referring to the shared secret here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bc04d01 adds more details on Receipt Secret and Receipt Nonce

@@ -192,6 +193,8 @@ Client streams MUST be odd-numbered starting with 1 and server-initiated streams

Money can be sent for a given stream by sending an ILP Prepare packet with a non-zero `amount` and a `StreamMoney` frame in the STREAM packet to indicate which stream the money is for. A single ILP Prepare can carry value destined for multiple streams and the `shares` field in each of the `StreamMoney` frames indicates what portion of the Prepare amount should go to each stream.

The receiver SHOULD include `StreamReceipt` frames in the ILP Fulfill packet indicating the total amount of money received in each stream, unless a secret key with which to generate receipts was not pre-shared with the receiver. Receipts can be verified, in order to confirm payment, using the pre-shared secret.

Choose a reason for hiding this comment

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

A Receipt Secret to generate receipts and a Receipt Nonce to include in those receipts. Also did we land on whether there's a required length for the Receipt Secret & Receipt Nonce? Might make sense to say they're both 32 bytes to prevent someone from DoSing an SPSP server with a massive Receipt Secret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, it's 16 byte nonce and 32 byte secret (https://github.com/interledger/rfcs/pull/570/files#diff-0bbc536b39826341170954e2675f765cR72-R73).
Is it worth making the nonce 32 bytes?

Choose a reason for hiding this comment

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

Well for the nonce we only need uniqueness, given it's public. So 16 is probably fine. 32 byte secret is good because it's often gonna be output of sha256. I think those values are good, but open to hear any other opinions

@sharafian sharafian self-requested a review March 16, 2020 17:45
@@ -192,6 +195,8 @@ Client streams MUST be odd-numbered starting with 1 and server-initiated streams

Money can be sent for a given stream by sending an ILP Prepare packet with a non-zero `amount` and a `StreamMoney` frame in the STREAM packet to indicate which stream the money is for. A single ILP Prepare can carry value destined for multiple streams and the `shares` field in each of the `StreamMoney` frames indicates what portion of the Prepare amount should go to each stream.

The receiver SHOULD include `StreamReceipt` frames in the ILP Fulfill packet indicating the total amount of money received in each stream, unless a Receipt Secret and Receipt Nonce were not pre-shared with the receiver. Receipts can be verified, in order to confirm payment, using the Receipt Secret.

Choose a reason for hiding this comment

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

I think it's worth explaining a little bit how this works because I think the first thing someone reading this will get tripped up on is that the verifying party (who they may imagine to be the sender) can sign new receipts.

Something like: "To use STREAM receipts, the Receipt Secret is pre-shared between the verifier and the receiver. The sender presents the STREAM receipts to the verifier, which the verifier can trust because only verifier and receiver know the Receipt Secret."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

included more details in 694f2d7

@@ -192,6 +193,8 @@ Client streams MUST be odd-numbered starting with 1 and server-initiated streams

Money can be sent for a given stream by sending an ILP Prepare packet with a non-zero `amount` and a `StreamMoney` frame in the STREAM packet to indicate which stream the money is for. A single ILP Prepare can carry value destined for multiple streams and the `shares` field in each of the `StreamMoney` frames indicates what portion of the Prepare amount should go to each stream.

The receiver SHOULD include `StreamReceipt` frames in the ILP Fulfill packet indicating the total amount of money received in each stream, unless a secret key with which to generate receipts was not pre-shared with the receiver. Receipts can be verified, in order to confirm payment, using the pre-shared secret.

Choose a reason for hiding this comment

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

Well for the nonce we only need uniqueness, given it's public. So 16 is probably fine. 32 byte secret is good because it's often gonna be output of sha256. I think those values are good, but open to hear any other opinions

Copy link

@sharafian sharafian left a comment

Choose a reason for hiding this comment

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

My last concern is the timestamp format. Do you think we should use unix timestamp or the Interledger timestamp? Both are fixed-length which is good, Interledger timestamp has some advantages of a string date-time (like handling leap-seconds). Although that might not be an issue, with many systems 'smearing' leap-seconds.

Interledger timestamps are a little harder to parse (https://github.com/interledgerjs/interledgerjs/blob/master/packages/ilp-packet/src/utils/date.ts#L27-L43) but it'd feel a little odd to use InterledgerTime in the prepare packets and Unix timestamps in the receipt.

Anyone have thoughts here?

@sharafian
Copy link

#481

This suggests that leap second support isn't really a valid reason to use InterledgerTime, making me lean towards just a Unix timestamp.

0029-stream/0029-stream.md Outdated Show resolved Hide resolved

| Field | Type | Description |
|---|---|---|
| HMAC | UInt256 | HMAC-SHA256 over all other fields using the 32 byte Receipt Secret, which is pre-shared between the verifying party and the receiver. |
Copy link
Member

Choose a reason for hiding this comment

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

Could we explain the inputs to the MAC more precisely, e.g., "the message is the concatenation of each of the following fields, in this order"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the description in 8df9304

| Field | Type | Description |
|---|---|---|
| Stream ID | VarUInt | Identifier of the stream this frame refers to. |
| Receipt | 65-Byte OctetString | Proof provided by the receiver of the total amount received on this stream |
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't the frame individually enumerate the fields of the receipt? Are they not intended to be exposed at the STREAM layer...?

Choose a reason for hiding this comment

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

The receipt is kinda treated as a whole object that's passed around, for instance the receipt would be emitted in WM. So I think "grouping" like this can be helpful


| Field | Type | Description |
|---|---|---|
| HMAC | UInt256 | HMAC-SHA256 over all other fields using the 32 byte Receipt Secret, which is pre-shared between the verifying party and the receiver. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if the HMAC was the last field instead of the first, it would be easy to implement serialization with one fewer allocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I put the hmac first with verification in mind, but it makes sense to me to optimize the serialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 8df9304

| Field | Type | Description |
|---|---|---|
| Stream ID | VarUInt | Identifier of the stream this frame refers to. |
| Receipt | 65-Byte OctetString | Proof provided by the receiver of the total amount received on this stream |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why mandate the exact size of the receipt instead of leaving it open-ended for possible extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍
Should we also consider including a receipt version number?
Or could the receipt size be used to check compatibility, such as with receipt support discovery (#570 (comment))?

Choose a reason for hiding this comment

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

Perhaps the receipt itself could have a version byte? Size might not be good enough if a future version is variable length. And if the receipt contains its own version info then STREAM doesn't need to emit extra information about receipt version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed receipt to VarOctetString and added version number: 1633e86

add receipt version number
make receipt variable length
indicate receipt support in SPSP response
remove stream start time from receipt
update fixtures and asn
@kincaidoneil
Copy link
Member

Since money can be sent over multiple streams, the verifier's accounting might be more complicated since they have to support tracking and totaling the amount received over each stream.

What if receipts attested the amount received over the whole connection instead? What are the arguments for/against a per-connection receipt vs a per-stream receipt?

@wilsonianb
Copy link
Contributor Author

https://github.com/wilsonianb/receipt-verifier/
👆 The current verifier implementation aggregates receipt amounts by a specified id (not necessarily the receipt nonce), so per-connection receipts wouldn't save it that much complexity.

I think it's still worth considering though. Would one type of accounting be easier to add to the Rust and Java STREAM implementations than the other?

@wilsonianb
Copy link
Contributor Author

On second thought now that I'm trying to update the verifier for receipts without the timestamp, per-connection receipts would simplify things.
The verifier can create a cache entry with an expiry for the receipt nonce when the SPSP query happens. This entry can be used to store the latest receipt amount as well as determining when a submitted receipt is too old (the entry will have expired).

@sharafian
Copy link

Since money can be sent over multiple streams, the verifier's accounting might be more complicated since they have to support tracking and totaling the amount received over each stream.

I was imagining that the key in the verifier's store would be the receipt nonce concatenated to stream ID. I'm not seeing right now how it's significantly simpler to just key with the receipt nonce.

The verifier can create a cache entry with an expiry for the receipt nonce when the SPSP query happens. This entry can be used to store the latest receipt amount as well as determining when a submitted receipt is too old (the entry will have expired).

Gotcha, so it saves a lookup because we don't need a separate key tracking the expiration of the connection as a whole?

In general I'm not hugely opposed to dropping the STREAM id but I want to make sure it doesn't mess anything else up in the future. STREAM was designed with multiplexing as a 'first-class' feature, where the idea is everything is done on streams and you might have long-lived connections on which you do many streams (like a long-lived QUIC connection with several HTTP calls over it).

We always have the option of making a new version of the receipt format but I think it could save us from future work if the receipts conceptually make sense with the rest of STREAM by working with the Stream abstraction

@wilsonianb
Copy link
Contributor Author

so it saves a lookup because we don't need a separate key tracking the expiration of the connection as a whole?

More or less yeah. This gives an idea of how the verifier might change from using a redis hash to a single value for per-connection receipts:
interledger/receipt-verifier@13f77b9

I don't have a strong opinion either way.

0029-stream/0029-stream.md Outdated Show resolved Hide resolved
0029-stream/0029-stream.md Outdated Show resolved Hide resolved
asn1/Stream.asn Outdated Show resolved Hide resolved
asn1/Stream.asn Show resolved Hide resolved
proposals/0000-stream-receipts.md Outdated Show resolved Hide resolved

4. Sender MUST give the Receipt directly or indirectly to the Verifier.

5. Verifier MUST verify the Receipt before crediting the Sender with the Receipt amount.

Choose a reason for hiding this comment

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

crediting makes me think of wallet balances where the Sender will get that money back. Can we clarify, something like accepting the Receipt amount as payment for services


### Stateless Receiver

Before a STREAM connection is established, the Receiver MAY use the Receipt Nonce and Receipt Secret as parameters to generate connection details (ILP Address and shared secret). The Receiver MAY encode the Receipt Nonce and Receipt Secret in the STREAM server ILP Address to allow the STREAM receiver to avoid persisting state. If doing so, the Receipt Secret MUST be encrypted. The Receipt Nonce MUST be unique each time STREAM parameters are generated, since it MUST be unique per STREAM connection.

Choose a reason for hiding this comment

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

since it MUST be unique per STREAM connection the receipt nonce must be unique globally

nonce is random -> unique
@wilsonianb wilsonianb requested a review from sappenin April 8, 2020 18:08
Copy link
Contributor

@sappenin sappenin left a comment

Choose a reason for hiding this comment

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

Just a few more questions, but 99% there from my end.

0029-stream/0029-stream.md Outdated Show resolved Hide resolved
asn1/Stream.asn Outdated
}

StreamFrame ::= SEQUENCE {
type FRAME.&typeId ({FrameSet}),
data FRAME.&Type ({FrameSet}{@type})
}

RECEIPT ::= CLASS {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a minor nit, but can we move these definitions to the bottom of this file? As they are now, they sort of break the flow of the STREAM and frame definitions themselves, which makes the ASN harder to read.

We could even put these directly above the StreamReceipt frame definition on line 209? Or pull the receipt definitions themselves into a separate file? (I can't think of a use-case, but I suppose lines 80-109 can technically stand on their own independent of STREAM and be imported into this file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a separate file 7dd4111
(force pushed because the commit wasn't originally appearing in the pull request, likely due to earlier github incident)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants