interledgerjs/ilp-protocol-stream

Moved to monorepo in interledgerjs/interledgerjs

ilp-protocol-stream's Issues

An in-range update of ilp-logger is breaking the build 🚨

The dependency ilp-logger was updated from 1.0.2 to 1.1.0.

🚨 View failing branch.

This version is covered by your current version range and after updating it in your project the build failed.

ilp-logger is a direct dependency of this project, and it is very likely causing it to break. If other packages depend on yours, this update is probably also breaking those in turn.

Status Details

• ❌ ci/circleci: build: Your tests failed on CircleCI (Details).
• ❌ ci/circleci: build_node_v8: Your tests failed on CircleCI (Details).

---

Commits

The new version differs by 5 commits.

• 5253753 1.1.0
• c2d9bb0 feat: upgrade to TypeScript (#8)
• 65d0941 Merge pull request #5 from interledgerjs/greenkeeper/initial
• fdd0711 chore: add greenkeeper-lockfile to CI
• d27e32f docs(readme): add Greenkeeper badge

See the full diff

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.

---

Your Greenkeeper Bot 🌴

Exponential Backoff hits 32-bit signed int boundary

If there are connection problems and temporary errors, the exponential backoff continues doubling (here:

ilp-protocol-stream/src/connection.ts

Line 1191 in c736e92

this.retryDelay = this.retryDelay * 2

) until it hits the 32-bit signed int boundary and explodes with this error:

TimeoutOverflowWarning: 1.915619426082361e+55 does not fit into a 32-bit signed integer.
Timeout duration was set to 1.

And this continues until the 64-bit double is overflowed and the connection logic likely stops.

In this situation we should either throw an error or simply cap the timeout length

Proper data handling

• Only send data in Prepare packets
• Resend data
• Make sure data received out of order is properly handled
• Resend control frames

Try compiling using Assemblyscript?

Curious if this would provide any performance benefits: https://github.com/AssemblyScript/assemblyscript

Update docs

• Auto-publish typedoc
• Remove todo list from readme
• Include how to use in readme
• Link to typedoc from readme
• Include badges from circleci, npm, codecov, standard

Default flow control values

Like QUIC, STREAM is going to have connection- and stream-level flow control (so both sides will be able to control both the amount of data they're getting for each stream and for the connection as a whole).

1. What should the default buffer size be for each stream? Node streams default to 16kb. However, STREAM packets can carry up to ~32kb so using the Node default would make the receiver reject a full STREAM packet.
2. What should the default maximum number of open streams be? 10 per side?
3. What should the default buffer size be for an entire connection (the total of the buffered data in all of the open streams)?

Reduce packet amount on temporary errors

The proper behavior for determining the packet amount is to have stream figure out how much money it can have in flight at a given time. This is a little complicated to implement so for the time being we should just make the default behavior for temporary errors be to reduce the packet amount (especially on T04 errors).

Related to interledgerjs/ilp-plugin-xrp-asym-server#39

Send money before determining the exchange rate?

If the user sets the sendMax amount before knowing the exchange rate, should the packets sent to determine the exchange rate carry money (and be fulfillable)? This would potentially reduce the latency for the first bit of money to get through, because it wouldn't be waiting for multiple round-trips with test packets before sending real money.

On the one hand, this opens things up for connectors to take a larger share of that money, because the sender will set a minimum destination amount of 0. On the other hand, though, connectors that want to do that could just take the same amount of the test packet to influence the minimum exchange rate the sender will set, so it might not make much of a difference.

If a user doesn't want to send money with a minimum acceptable amount of 0, they could just wait until the exchange rate is known before setting the sendMax.

Encode packet's array of frames with length prefix first

So that's it uses proper OER

Unable to establish connection, no packets meeting the minimum exchange precision of 3 digits made it through the path.

When I split example.js into server and client I get the error:
Error: Error connecting: Unable to establish connection, no packets meeting the minimum exchange precision of 3 digits made it through the path.

I run the server the server and wait for connections and from a different client.js file I put the part to connect and stream. The error is generated here:

const clientConn = await IlpStream.createConnection({
    plugin: clientPlugin,
    destinationAccount,
    sharedSecret
  })

Proper exchange rate handling

Right now the test section titled "Exchange Rate Handling" is empty and it would be good to add some tests to make sure it is behaving properly.

The minimum acceptable rate is determined when the stream connects by using a test (unfulfillable) packet (see here). When each subsequent packet is sent, the sender indicates the minimum amount the receiver should accept (which they check here) in the STREAM packet.

A couple of things that would be worth testing:

• Packets should be rejected if the exchange rate gets worse than the minimum acceptable amount
• The user should be able to configure the amount of slippage they'll accept (used here)
• The connection object should provide methods to query both the current exchange rate (total received / total sent) as well as the minimum acceptable rate (this needs to be implemented and tested)

Connection Timeout

• Add setInactivityTimeout function to the stream connection.
• If enabled then the connection times out after that set period if no data is being transferred on the connection.

Dedup dependencies

There are a lot of different versions of bignumber.js and oer-utils floating around our modules. It would be good to clean those up and get them all using the same versions so we don't need to install all of them.

ilp-protocol-stream git:es-docs ❯ npm ls --production
[email protected] .../ilp-protocol-stream
├── @types/[email protected]
├── [email protected]
├─┬ [email protected]
│ └── [email protected]
├─┬ [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ ├── [email protected]
│ └── [email protected]
├─┬ [email protected]
│ └─┬ [email protected]
│   ├── [email protected]
│   ├─┬ [email protected]
│   │ ├── [email protected] deduped
│   │ ├── [email protected]
│   │ ├── [email protected]
│   │ ├── [email protected]
│   │ └── [email protected]
│   ├── [email protected] deduped
│   ├── [email protected]
│   └─┬ [email protected]
│     ├── [email protected]
│     ├── [email protected]
│     └── [email protected]
├─┬ [email protected]
│ ├── [email protected] deduped
│ └── [email protected]
├─┬ [email protected]
│ └── [email protected] deduped
└─┬ [email protected]
  └── [email protected]

Connection-level inactivity timeout

• Close the connection if nothing has happened for a while
• Time out packets if they keep getting rejected for a long time. ex. StreamCloseFrame

An in-range update of @types/sinon is breaking the build 🚨

The devDependency @types/sinon was updated from 5.0.5 to 5.0.6.

🚨 View failing branch.

This version is covered by your current version range and after updating it in your project the build failed.

@types/sinon is a devDependency of this project. It might not break your production code or affect downstream projects, but probably breaks your build or test tools, which may prevent deploying or publishing.

Status Details

• ❌ ci/circleci: build: Your tests failed on CircleCI (Details).
• ❌ ci/circleci: build_node_v8: Your tests failed on CircleCI (Details).

---

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.

---

Your Greenkeeper Bot 🌴

Handle plugin disconnecting

The client and server should either try reconnecting or close if the underlying plugin disconnects

Drop connection when it has reached a certain number of packets

As @dappelt pointed out in interledger/rfcs#417 (comment), AES-GCM can only be used to encrypt 2^32 messages with a single key. Connections should automatically be dropped when either side has sent ~2^31 messages, or ideally some time before that.

Consistent Error Handling

On the connection and the stream we should match how node works with write() and only throw errors if there is not an error emitter present. Right now we throw errors regardless of the emitter being present and we do not emit errors if one is present.

if(this._events.error){
    this.safeEmit('error', streamClosedError)
}

Could be used to check if the error listener is present.

receiveTotal resolves before fulfillment is sent

I was testing a pull payment script that had:

await stream.receiveTotal(amount)
//exit

This resulted in receiveTotal resolving but the outgoing_money event never happening on the sender's side. According to the logs, the fulfillment (nor a rejection) was never sent by the receiver.
I think this is due to the fact that the money event is emitted (inside _addToIncoming) before doing the fulfillment, leaving a window of opportunity to fail to fulfill the payment despite having indicated that the amount was received.
https://github.com/interledgerjs/ilp-protocol-stream/blob/master/src/connection.ts#L556-L595

Determining the exchange rate precision

As of right now we are sending out packet amounts of 1, 1000, 1000000, and 1000000000 in an attempt to determine an exchange rate with an amount of precision as quickly as possible.

At the high level it seems that if we send the following amounts that we have a precision to at least 3 digits and a determined exchange rate:

Amount Sent	Prepare Amount	Exchange Rate
1000	1000	1
10000	1100	.11

However this does not take into account what is actually happening at the connector level. There could be multiple connectors which are changing the rate but since we only know the amount sent and the prepare amount that comes back we cannot see that they have a very different exchange rate so the 1 and .1 are not valid.

Suppose we have the connectors, where their exchange rate is represented in the ():

Conn A <---> (.001) Conn B <---> (1.6) Conn C <---> (1000) Conn D
1000 ----------------> 1 --------------> 1 ---------------> 1000 == 1 

In reality the exchange should have been 1000 * .001 * 1.6 * 1000 = 1600 with a rate of 1.6 but on this path the rate calculated was 1. A similar issue could occur with larger amounts as well.

Conn A <---> (.001) Conn B <---> (1.14) Conn C <---> (1000) Conn D
10000 ----------------> 10 --------------> 11 -------------> 1100 == .11

This should have been 10000 * .001 * 1.14 * 1000 = 11400 with a rate of 1.14 but was calculated as 1.11.

The issue is that connector C is using a 1.6/1.14 exchange rate but connector B is using a very large exchange rate which effectively takes the value down to 1 and connector D is ramping that back up.

How can stream determine without having knowledge about the Conn C exchange rate that in these scenarios the calculated exchange rate is incorrect and needs further clarification?

clarify relationship to other similar protocols

How does this relate to https://github.com/emschwartz/ilp-protocol-paystream and https://github.com/interledgerjs/ilp-protocol-psk2?

Don't serialize JSON for logging unless debug is on

All the debug statements in the stream library use string interpolation with backticks. This means that the objects are serialized for logging before they're passed to debug, and may not be printed if debug is turned off. We should make sure that these unnecessary string operations don't happen unless debug is actually enabled.

Multiple packets in flight

Currently, this module waits for the response to each Prepare packet before sending the next one. If the path can support sending multiple packets at the same time, the module should fire off as many as possible so that the money and data is sent as quickly as possible.

To do this, STREAM would need to keep track of:

• Total amount of money in flight and the path limit
• Total number of bytes in flight and the path limit
• Total number of packets in flight (and the path limit, if there is one)

Duplex.readableHighWaterMark unsupported before Node v8.10.0

Stateless STREAM Server

If ilp-protocol-stream could keep connection state in a data store like redis, we could run sharded STREAM servers. This would make it operationally simpler to scale STREAM servers, allowing us to do away with stateful sets and the two-layered architecture for scaling STREAM servers (a statefulset of connectors + STREAM servers behind a deployment of connectors).

An in-range update of @types/chai-as-promised is breaking the build 🚨

The devDependency @types/chai-as-promised was updated from 7.1.1 to 7.1.2.

🚨 View failing branch.

This version is covered by your current version range and after updating it in your project the build failed.

@types/chai-as-promised is a devDependency of this project. It might not break your production code or affect downstream projects, but probably breaks your build or test tools, which may prevent deploying or publishing.

Status Details

• ❌ ci/circleci: build: Your tests failed on CircleCI (Details).
• ❌ ci/circleci: build_node_v8: Your tests failed on CircleCI (Details).

---

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.

---

Your Greenkeeper Bot 🌴

What should ending a stream do if it still has money to send?

If you call end on a Node TCP socket, it will:

1. Keep sending data until all the data written to it has been sent
2. Emit 'finish' when it has finished sending all of the data
3. Emit 'end' when the other side has indicated it is finished sending data
4. Emit 'close' when the underlying socket is closed

If the other side isn't reading data from the socket and the reading side's buffer is full, the sending side will stay open until the consumer is finished. (See this gist if you want to play around with different possibilities)

The question is what the behavior should be for a DataAndMoneyStream if you call end or destroy when it's still trying to send money? Should it close the stream immediately, wait until the money is sent, or close the stream only if the other side is currently blocking it from sending more money?

Support paths with large exchange rates (e.g. big differences in currency scales)

Right now STREAM sends a test packet of 1000 source units to determine the exchange rate. If the amount that arrives is 0, the sender will assume that the path exchange rate is 0 and won't try to send anything through it. We should probably support paths in which the sender and receiver have very different scales.

I think the way to handle this would be:

1. Start with a test packet of 1000
2. If that gets to the receiver, increase the test packet size
3. If the sender gets an F08: Amount Too Large error before any money gets to the receiver, then the sender can conclude that no money will be able to be sent through the path

Add a timeout to sendTotal and receiveTotal

We should have a configurable timeout on both of those functions and a default of something like 60 seconds so that the promise doesn't hang forever

Stream Specific Errors

• Add Error Codes for specific stream errors
• Pin point who is at fault by matching ilp address by prefixes

An in-range update of oer-utils is breaking the build 🚨

The dependency oer-utils was updated from 3.0.1 to 3.1.0.

🚨 View failing branch.

This version is covered by your current version range and after updating it in your project the build failed.

oer-utils is a direct dependency of this project, and it is very likely causing it to break. If other packages depend on yours, this update is probably also breaking those in turn.

Status Details

• ❌ ci/circleci: build_node_v8: Your tests failed on CircleCI (Details).
• ❌ ci/circleci: build: Your tests failed on CircleCI (Details).

---

Commits

The new version differs by 5 commits.

• 4a0fd30 3.1.0
• e0813c4 fix: reject variable integers of length zero
• 3cdb8ab chore: re-enable code coverage reporting
• c072342 chore: update dependencies
• f3dc629 feat: allow using JS numbers instead of BigNumbers

See the full diff

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.

---

Your Greenkeeper Bot 🌴

An in-range update of @types/node is breaking the build 🚨

The dependency @types/node was updated from 10.11.0 to 10.11.1.

🚨 View failing branch.

This version is covered by your current version range and after updating it in your project the build failed.

@types/node is a direct dependency of this project, and it is very likely causing it to break. If other packages depend on yours, this update is probably also breaking those in turn.

Status Details

• ❌ ci/circleci: build: Your tests failed on CircleCI (Details).
• ❌ ci/circleci: build_node_v8: Your tests failed on CircleCI (Details).

---

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.

---

Your Greenkeeper Bot 🌴

Inactivity Timeout

• Add an inactivityTimeout option to the stream connection.
• If money is being transferred the timeout is reset.
• If no money is being transferred then the connection times out.
• Leave timeout option as it.

verifyReceipt function signature

https://github.com/coilhq/receipt-verifier/blob/eeccf81420c10dda77c50721eda67a1c9645025e/src/services/Balances.ts#L37-L38

      let receipt: Receipt
      try {
        const receiptBytes = Buffer.from(body.toString(), 'base64')
        receipt = decodeReceipt(receiptBytes)
        verifyReceipt(receiptBytes, generateReceiptSecret(this.config.receiptSeed, receipt.nonce))
      } catch (error) {
        ctx.throw(400, error.message)
      }

verifyReceipt takes a secret to verify the hmac, but in practice that secret is generated from the receipt nonce

as can be seen above, that can lead to inefficient double decodes

could the verifyReceipt function optionally take a function with the decoded receipt as an argument and return the secret to be used ?

Server connection can't send money on 'connection' event

Sending money from the server doesn't work well right now. The connection event is emitted before connection.handlePrepare is called, so the connection hasn't processed the request frames (that include the other party's ILP address) yet. As a result, if you call setSendMax synchronously, it will stop because it doesn't know the destination address.

I think we could address the sending money issue by making sure to restart the send loop once the connect event is emitted. However, it's still strange that the connection.destinationAccount is undefined on the server side, so it feels like we should solve this in a better way.

I'm not sure about the best way to do this right now so open to suggestions.

Unique id per money event

A question was raised by an implementer

Is there any way native to the Stream protocol or ILP to allow us to individually identify a micropayment?

This can be achieved by raising a unique id within each money event. @justmoon pointed out this id could be the SHA256 hash of the ILP packet. It will be a breaking change on the money event though as currently the value returned is an amount value as a string. The newer structure would be along the lines of

{
 "id": "a6f7b874ea69329372ad75353314d7bcacd8c0be365023dab195bcac015d6009",
 "amount": "123"
}

Another question is should we amend the spec to mandate this type of functionality for STREAM implementations?

CC @adrianhopebailie @sharafian @sentientwaffle @wilsonianb @justmoon @kincaidoneil

Update connection.totalDelivered before outgoing_money

See:
interledger/web-monetization-projects#115 (comment)

One-sided money connections

STREAM should work even if only one side is capable of sending money. Right now the connection will error out, however, if it is unable to send the first test packet. We should make it work under these conditions by going into some kind of "receive-only" mode.

Update RFC

The spec should be updated to reflect the latest protocol

Should there be ACKs for data or should data only be sent in Prepare packets?

To figure out if the other side got a data frame, we can either:

• Send data frames only in Prepare packets and use the Fulfill/Reject as an acknowledgement (if the data is authenticated and we know it came from the receiver)
  • If we do this, should we assume that a Reject is a NACK for the data, or should we assume they've gotten and processed the data even if the packet is rejected?
• Have ACK frames for data so that data can be sent in any type of ILP packet

Note that QUIC uses ACK frames to acknowledge receipt of whole packets rather than specific frames or blocks of data.

Send-only mode

In some use cases on top of STREAM, bidirectional communication isn't necessary. In Web Monetization, for instance, the sender repeatedly sends money to the receiver but does not expect data or money in return.

If we were able to turn on a feature in the STREAM client that stops the sender from sending their ILP address to the other side and also enable a feature that would allow the server to function in a half-open mode like this, we could simplify the infrastructure required for WM sending.

Instead of using a bidirectional communication like BTP, we could use HTTP/HTTP2, simplifying WM sending infrastructure and the WM sending code.

CC @kincaidoneil @wilsonianb @sentientwaffle if one of you is interested in looking into this, feel free to assign yourself on this issue.

@kincaidoneil I noticed you worked on the code for receive-only mode on the server, are there any considerations with this feature that I missed?

Probing/exchange rates are not a STREAM concern

Proposal: Allow logic enforcing minimum exchange rates to be completely disabled, including:

1. Sending test packets before the stream is established
2. Requiring packets to meet the minimum exchange rate that STREAM calculates

---

1. Supporting receive-only mode.
   • Case 1: credit with connector is 0, which currently causes all test packets to fail and stream to be closed, even if the endpoint won't be sending money.
   • Case 2: there exists credit with the connector, but the probing temporarily exhausts it, which is unnecessary and can cause disruptions for other applications.
2. Insecure. Exchange rates from probing can trivially be gamed unless there was a TON of liquidity and many connectors, so for the foreseeable future, most clients necessitate their own exchange rate oracle to ensure the rate is reasonable.
   • In practice, the current behavior only really ensures the exchange rate stays consistent/within the slippage margin throughout the stream, which isn't desirable since it should respond to exchange rate fluctuations. (By contrast, the price oracle approach can respond to fluctuations, if necessary).
   • Instead, a callback function could be passed to STREAM clients & servers, that given a source amount and a destination amount, could return true/false if that particular packet should be fulfilled. (If it returned false, STREAM would reject that packet). This would allow consumers of the module to integrate with their preferred price oracle or use their own method to enforce a minimum exchange rate on a per-packet basis.
3. Longer connection establishment. Probing wastes time while establishing the connection (#44), when alternatively it could be used to start sending money immediately, since the test packets can trivially be identified and gamed.
4. No support for large asset scale differences. Probing with no knowledge of the relative exchange rate can introduce problems with vastly different exchange rate scales. (And requires sending massive test packets, which as noted earlier, could temporarily exhaust bandwidth that other applications are using, if they're forwarded).

To determine the initial packet size to send, STREAM could send a single very large packet (e.g., maybe the maximum uint64 an ILP packet allows the total amount remaining to send). F08s (which connectors should be configured to trigger before T04s) should quickly reduce it to a reasonable amount. (Alternatively, STREAM could be more aware of settlement/how much credit exists with the upstream peer, but I'm guessing that'd be more contentious).

An in-range update of @types/webpack is breaking the build 🚨

---

☝️ Important announcement: Greenkeeper will be saying goodbye 👋 and passing the torch to Snyk on June 3rd, 2020! Find out how to migrate to Snyk and more at greenkeeper.io

---

The devDependency @types/webpack was updated from 4.41.10 to 4.41.11.

🚨 View failing branch.

This version is covered by your current version range and after updating it in your project the build failed.

@types/webpack is a devDependency of this project. It might not break your production code or affect downstream projects, but probably breaks your build or test tools, which may prevent deploying or publishing.

Status Details

• ✅ ci/circleci: build_node_v10: Your tests passed on CircleCI! (Details).
• ❌ ci/circleci: build_node_v12: Your tests failed on CircleCI (Details).

---

FAQ and help

There is a collection of frequently asked questions. If those don’t help, you can always ask the humans behind Greenkeeper.

---

Your Greenkeeper Bot 🌴

Make it harder to identify STREAM packets on the wire

Ideas for obscuring which transport protocol is being used:

• Pad packets to the maximum data size by default
• Randomize the expiry time
• Use a different connection tag for every packet
• Use amount tranches (send only increments of 1, 10, 100, etc instead of whatever the connector says the maximum packet amount is)

It's an interesting question whether any of this is worth doing, because connectors can probably figure out which transport protocols (or even which implementations of transport protocols) are being used just by looking at the timing of packets and behavior in response to certain kinds of errors.

Clean up closed streams

• Reject packets with money or data frames for closed streams
• Delete stream record to avoid memory leak
• Make sure stream numbers for closed streams aren't reused
• Delete everything when the connection is closed
• Make sure that end allows all the data & money to be sent from side that called end
• Make sure that destroy closed down the connection immediately from both ends and does not allow more data to be sent or received
• Calling destroy on a stream does not close the connection
• the server doesn’t stop accepting packets for a given connection when the connection closes. On the close event it should remove it from the connection array and also keep track of connections that were previously open to avoid them being opened again

Connection Timeout

• Add connection timeout which kills the connection if nothing is happening for a long time. Sorta like the http timeout.