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

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

Open
emschwartz opened this issue Sep 5, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@emschwartz
Copy link
Member

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

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

@emschwartz

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

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

// 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

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.