interledger / interledger-rs

An easy-to-use, high-performance Interledger implementation written in Rust
http://interledger.rs
Other
201 stars 70 forks source link

Ensure that we enforce a maximum timeout for transactions #163

Closed bstrie closed 5 years ago

bstrie commented 5 years ago

This came up in a meeting today regarding attacks against distributed systems and @emschwartz wanted to make sure that we enforce such timeouts.

emschwartz commented 5 years ago

The maximum expiry duration should be configurable and we should set a sane default

emschwartz commented 5 years ago

I implemented a version of this in https://github.com/emschwartz/interledger-rs/commit/5fa9d4e656a787a4f0b2b806c2712c915599b8e9 but now I'm not sure that that's the right approach.

Open questions:

As a side note, I considered whether we actually need to enforce a maximum timeout at all. The answer is probably yes but because of the exchange rates rather than a concern over locking up too much of the connector's liquidity. Originally, we had thought about limiting the amount of money a sender could tie up. However, this is better enforced by limiting the payment bandwidth of each account. If you do that, it doesn't matter whether the account holder uses the bandwidth to simply tie up liquidity or actually send payments. However, the maximum expiry does matter when you're doing cross-currency transactions, because the connector is locking in an exchange rate for the duration of that packet.

What do you think? cc @bstrie @dora-gt @gakonst

dora-gt commented 5 years ago

Please read the following as an opinion from a person of limited knowledge.

AFAIK one of the biggest change from ILPv1 to ILPv4 was that we changed our philosophy from "one big" to "small many". I feel like it reflects the concept of "smaller and faster is better". I think we should implement following the concept, not to have contradictions. I mean... expiry should be as small as possible. Is there any reason to set this larger with the current specs and environment?

I feel 30 or 60 secs are too large to transfer 10K or something larger because we don't need Layer-1 transaction to pay forward as long as the bandwidth allows.

I would like to know what level of speed you suppose for the production environment at. My image is like "Paying $10 in a second" or "Paying $100 in 2 seconds". Idk what you imagine but this is my goal image and it would be possible now that we don't need on-ledger settlements. Wouldn't it?

If paying just $10 takes 10secs or so, would people use ILP? I don't think so.

I'm sorry the above may not be answering your questions directly.

  1. I think applying max duration to incoming packets doesn't make sense because we could shorten it. I think applying to outgoing packets makes sense.
  2. I think it should be shorter. The reason is above.
  3. I'm not sure but there might be some critical paths where the system works so slowly. It would be helpful if we could set special values for those.

I'm sorry if I'm missing something.

emschwartz commented 5 years ago

While the packet Round Trip Time (RTT) should be as short as possible in the normal case, we should arguably give a little more buffer for worst case scenarios. Each intermediary node should deduct some amount of time from the expiry such that they still have time to pass on the fulfillment for a packet that was fulfilled as the last instant before it expired. Each node currently deducts 1-2 seconds from the expiry by default. If we make the maximum expiry very very short, we'll severely limit the length of paths that are possible.

The time to complete a payment will be more a function of your payment bandwidth (amount you can send per time) than the maximum packet expiry. I think payments of $10-200 should take less than 1 second to complete if we want to support retail payments.

I think applying max duration to incoming packets doesn't make sense because we could shorten it. I think applying to outgoing packets makes sense.

:+1:

sappenin commented 5 years ago

default maximum 60 seconds, set the peer protocol expiry duration to 30 seconds,

Can you provide more context around why the peer protocols might want a shorter expiry than normal packets?

sappenin commented 5 years ago

Is there a reason you'd want to allow certain accounts to use a longer expiry duration?

Not entirely sure, but you might have accounts that you know will connect to potentially longer payment paths, in which case you might want to special case a longer timeout (but in general use shorter maximums for most accounts).

sappenin commented 5 years ago

Should the maximum expiry duration be applied to incoming packets, outgoing packets, or should it simply cap the expiry duration of the outgoing packet

Seems like the consensus (reading above) is to apply the maximum to the outgoing packet, which makes sense to me.

Should the maximum expiry duration be applied to outgoing packets, or should it simply cap the expiry duration of the outgoing packet

Capping the expiry is how I was thinking of implementing it (and seems to be your choice too). This has the added benefit of helping downstream Connectors know that an upstream Connector has this kind of limit because it will manifest in the packet expiry (though, am I misunderstanding the difference between "applying to the packet" vs "capping the expiry"?)

emschwartz commented 5 years ago

Can you provide more context around why the peer protocols might want a shorter expiry than normal packets?

Currently the peer protocols use 60 seconds. The question was whether that should be shortened to 30 seconds.

Not entirely sure, but you might have accounts that you know will connect to potentially longer payment paths, in which case you might want to special case a longer timeout (but in general use shorter maximums for most accounts).

Maybe something to implement later but I don't see a strong reason to add this to the account configuration now. In general, it's unlikely that packets with longer than 30 seconds expiry will make it through the network with a longer expiry, because other nodes will be configured to shorten the expiry down to the default.

Seems like the consensus (reading above) is to apply the maximum to the outgoing packet, which makes sense to me.

Yes

Capping the expiry is how I was thinking of implementing it (and seems to be your choice too). This has the added benefit of helping downstream Connectors know that an upstream Connector has this kind of limit because it will manifest in the packet expiry (though, am I misunderstanding the difference between "applying to the packet" vs "capping the expiry"?)

There's no difference between applying it to the packet or capping the expiry. The main question was whether to reject packets whose expiries were too long, or just to shorten it. We've opted to shorten the expiry.