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

[emschwartz]

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.

[sentientwaffle]

What if 'connection' just didn't emit until the rate/destinationAccount are both ready?

[emschwartz]

The main issue I see is that we do kind of want the receiveMax to be set before the connection responds to the first packet so that we can get to sending the actual money ASAP. I'm not sure we want to wait for the full handshake before starting with real packets.

[sentientwaffle]

I see what you mean (and this may be a separate issue), but I don't really like the asymmetry in the API of: the client doesn't get the use a connection until the rate is available, whereas the server gets it immediately.

Is the receiveMax usually getting to the same value for every stream? If so, there could be a default value in ConnectionOpts.

[emschwartz]

We could set the receiveMax to Infinity by default, but it could be a bit strange if money just starts showing up. We could also do that plus add a field to the connection/server options that allows you to set the default receiveMax for all connections, so you can ensure you don't get money out of the blue if you don't want it.

[sentientwaffle]

That sounds reasonable. Its also has the benefit of being extremely similar to how data offsets are handled.

If a connection/stream is initially configured with receiveMax=Infinity would it be allowed to decrease it later?

[emschwartz]

Ah, good point -- that was another reason we didn't set it higher later. If we follow the QUIC model, it isn't possible to lower the limits. Because you don't know what order packets were sent in, they assume that you only ever raise the limits and can discard anything that's lower.

One option might be to:
a) start at Infinity, but don't send a frame over the wire
b) assume the other side is at Infinity until you're told otherwise
c) once you set a limit, ignore frames that lower it

[sentientwaffle]

Right now the connection allows the data offset to be decreased in some cases -- is that a bug? (

ilp-protocol-stream/src/connection.ts

Lines 634 to 635 in afd51ba

	// We assumed their size was 64kb but it turned out to be less
	this.remoteMaxOffset = frame.maxOffset.toNumber()

) .. or is it different since one side assumed that its peer had a higher limit, even if that limit was never explicitly set?

I can't find any issues with your proposed solution -- if some race occurs and one side does oversend, they'd just get a ILP reject packet with a StreamMaxMoney frame which seems like the right behavior.

[emschwartz]

or is it different since one side assumed that its peer had a higher limit, even if that limit was never explicitly set?

Yup, exactly.

If that solution sounds good I'll try to implement it some time soon.

[sentientwaffle]

That solution sounds good.

---

Re: the data offsets, it is a bit different from money -- if one side sends too much data it's a flow control error and the connection is terminated, as opposed to money which just rejects the packet.

[sentientwaffle]

@emschwartz thinking about this some more, won't this be a breaking change? The current implementation of stream is expecting a MaxMoneyFrame, and if it doesn't get one from the new version of stream it will assume a RemoteReceiveMax of 0, which is blocked.

[emschwartz]

Good point. Sounds like we can only do two of these three things:

1. Change the default receiveMax from 0 to Infinity
2. Disallow decreasing the receiveMax
3. Be compatible with the existing implementation

Need to think about this a bit more, but this is definitely the kind of assumption that should be made explicit in the spec.