-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add STREAM receipts #570
Conversation
Signed-off-by: Brandon Wilson <brandon@coil.com>
0029-stream/0029-stream.md
Outdated
| 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. | |
There was a problem hiding this comment.
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.
0029-stream/0029-stream.md
Outdated
|
||
| Field | Type | Description | | ||
|---|---|---| | ||
| HMAC | UInt256 | HMAC-SHA256 over all other fields using a secret pre-shared between the verifying party and the receiver. | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
0029-stream/0029-stream.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
0029-stream/0029-stream.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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
0029-stream/0029-stream.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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
There was a problem hiding this 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?
This suggests that leap second support isn't really a valid reason to use InterledgerTime, making me lean towards just a Unix timestamp. |
0009-simple-payment-setup-protocol/0009-simple-payment-setup-protocol.md
Outdated
Show resolved
Hide resolved
0029-stream/0029-stream.md
Outdated
|
||
| 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. | |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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
0029-stream/0029-stream.md
Outdated
| 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 | |
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
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
0009-simple-payment-setup-protocol/0009-simple-payment-setup-protocol.md
Show resolved
Hide resolved
0029-stream/0029-stream.md
Outdated
|
||
| 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. | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 8df9304
Clarify HMAC message
0029-stream/0029-stream.md
Outdated
| 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 | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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))?
There was a problem hiding this 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
There was a problem hiding this comment.
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
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? |
https://github.com/wilsonianb/receipt-verifier/ 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? |
On second thought now that I'm trying to update the verifier for receipts without the timestamp, per-connection receipts would simplify things. |
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.
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 |
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: I don't have a strong opinion either way. |
0009-simple-payment-setup-protocol/0009-simple-payment-setup-protocol.md
Outdated
Show resolved
Hide resolved
0009-simple-payment-setup-protocol/0009-simple-payment-setup-protocol.md
Outdated
Show resolved
Hide resolved
0009-simple-payment-setup-protocol/0009-simple-payment-setup-protocol.md
Outdated
Show resolved
Hide resolved
0009-simple-payment-setup-protocol/0009-simple-payment-setup-protocol.md
Outdated
Show resolved
Hide resolved
0009-simple-payment-setup-protocol/0009-simple-payment-setup-protocol.md
Outdated
Show resolved
Hide resolved
change SPSP response field to "receipts_enabled" Co-Authored-By: David Fuelling <sappenin@gmail.com>
proposals/0000-stream-receipts.md
Outdated
|
||
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. |
There was a problem hiding this comment.
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
proposals/0000-stream-receipts.md
Outdated
|
||
### 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. |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
asn1/Stream.asn
Outdated
} | ||
|
||
StreamFrame ::= SEQUENCE { | ||
type FRAME.&typeId ({FrameSet}), | ||
data FRAME.&Type ({FrameSet}{@type}) | ||
} | ||
|
||
RECEIPT ::= CLASS { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
Co-Authored-By: David Fuelling <sappenin@gmail.com>
#568