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

feat: introduce best-effort payments (with no amount specified) #323

Merged
merged 1 commit into from
Dec 5, 2017

Conversation

michielbdejong
Copy link
Contributor

Counter-proposal against #312 (comment)

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Oct 26, 2017

This is not me voicing an intent to actually implement best effort payments, but I think it's good if we have a clear packet vocabulary so there is no ambiguity about what each packet type means (type 1 has a destination amount, type 9 doesn't).

@michielbdejong
Copy link
Contributor Author

@emschwartz from the other ticket:

I can see it both ways

That's not an argument against this proposal. Do you see any downsides in using two different call types? Are you afraid we'll use up our 256 possible call types too soon if we use type 9 for this? Even if that's the concern, I don't think multiplexing/overloading different behaviors onto the same call types would be the solution, we should then just make the call type an Int16 instead of an Int8.

@emschwartz
Copy link
Member

I'm not worried about using up call types. I'm having a hard time getting behind having two versions of Interledger before anyone is really using any version of it.

That said, I do think it's better not to have the amount in the packet as cleartext in basically all cases: delivering at least a certain fixed amount, chunked payments, streaming payments, end-to-end quoting, or building any other type of protocol on top of ILP.

@adrianhopebailie
Copy link
Collaborator

I'm having a hard time getting behind having two versions of Interledger before anyone is really using any version of it.

Agree. I think this change may happen but is premature.

As I said to @michielbdejong privately our decision process should go as follows:

  1. We have not defined an expected behaviour for a node that receives an ILP Payment Packet with an amount of 0.
  2. Now there are use cases where participants on the network want to send packets with an amount of 0 so we need to define the expected behaviour to make everyone interoperable.
  3. The best way to do this is look at existing implementations and standardize on what the majority of them are already doing (unless that's a glaring issue)

IMO these are our options:
"When a node receives an incoming transfer carrying an ILP Payment Packet that has an amount of 0 it MUST:"

  1. Reject the transfer with an error (TBD which, maybe a new one)
  2. Forward the packet on another transfer.

The ILP protocol doesn't specify how a node calculates the amount and expiry on the outgoing transfer so I think it's as simple as that.

I believe that the majority of implementations will behave according to 2 so we should standardize on that.

If we decide that we prefer to standardize on 1 THEN we should consider this proposed change.

@@ -9,6 +9,7 @@ IMPORTS
FROM GenericTypes

InterledgerProtocolPaymentData,
BestEffortPaymentData,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest this is called:

InterledgerProtocolData

Copy link
Contributor Author

@michielbdejong michielbdejong Oct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we deprecate packet types 1-7, then it would still make sense to have type 8 be called 'error' and have type 9 be called something like 'request', and not just 'protocol'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be called something like InterledgerProtocolV2Data. I don't think it's really a request/response protocol (though I might feel differently about that if we end up adding fulfillment data)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for InterledgerProtocolRequestData for symmetry with InterledgerProtocolErrorData

👎 for adding versioning, that already implicit in the packet type numbering. (Unless a new packet is an explicit replacement for an old one, in which case I think it's required). This packet, by design, could carry more than payments.

Copy link
Member

@emschwartz emschwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking about this more individually with @michielbdejong and @justmoon I was more convinced that it would make sense to make the no-amount packet a separate type. I'm now in favor of defining a separate type as the Interledger V2 Packet, with roughly the structure suggested here, and then at some point in the not too distant future deprecating packet type 1. @michielbdejong made a fair point that making a change to the meaning of packet 1 means you need to know whether you're in a network that uses the old behavior or the new one, so it's better to be more explicit and just use a new type. Even so it'll need to be a breaking change because we are going to expect everyone to upgrade to start supporting packet 9 at some point, and then I think we're going to start removing support for packet 1.

Whether or not we deprecate ILQP is a separate question. I would also be in favor of that but we should discuss that in a different issue.

account Address,
-- Information for recipient (transport layer information)
data OCTET STRING (SIZE (0..32767)),
-- Enable ASN.1 Extensibility
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justmoon mentioned the idea that this might be a good time to add a Time to Live value. I don't feel too strongly about it either way, but adding a new packet type is a decent "speak now (for new features/fields) or forever hold your peace" moment. So...thoughts on TTL (or other new fields)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if signed data is expected to come back from the receiver, the payment packet could contain the pubkey of the receiver. Even if the ledgers don't enforce a crypto-condition (just like there might be ledgers in the chain that don't enforce the hashlock), it would be correct to define the pubkey and the signature scheme as an expectation of the payment as a whole

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be correct to define the pubkey and the signature scheme as an expectation of the payment as a whole

That doesn't sound like a good idea to me. If some parties enforce it but not all, that would be easily exploitable by sending payments you know one party will accept that the other won't. I think whether or not we have fulfillment data is orthogonal to this change.

Copy link
Contributor Author

@michielbdejong michielbdejong Oct 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If some parties enforce it but not all

A ledger sits between two connectors. The sending connector should make sure they know if the next ledger enforces it or not. If they know it doesn't, then they need to trust the next connector for the in-flight amount (because the next connector could claim the in-flight amount without producing correctly signed fulfillment data). But that's just a basic HTLA trade-off. You cannot exploit a remote connector, you can only get into disputes with your direct neighbors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a jankier way of doing Crypto Conditions that would run into similar problems. Unless you can rely on the whole network supporting a feature (that needs the whole path to support it in order for it to work), it either can't really be used or it becomes a point of incompatibility between different parts of the network.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about what you're arguing for here and how this is related to this discussion (as opposed to #314). Are you proposing that a public key would go in the Interledger packet and that a signature by the corresponding private key would be an additional condition for executing transfers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, i agree it would be too much work to add such enforcement. i just feel that it should be clear, if Alice sends Bob a conditional transfer, and Bob produces the fulfillment but nothing else, will Bob have done his job? Or does his job also include delivering the attached data? Or is this something neither Alice nor Bob can know by just looking at the packet? If Alice includes the final receiver's pubkey in her ask to Bob, and Bob comes back with only a fulfillment, then it will be clear that Bob didn't do all the things Alice hoped he would do.

But none of this is necessary anyway if we decide against adding data along with the fulfillment as part of Interledger, and decide that that data will just have to travel out-of-band (even if it piggybacks over side-protocols)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emschwartz @justmoon what would be the purpose of the TTL and how would it be implemented by senders/connectors/receivers?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if Alice sends Bob a conditional transfer, and Bob produces the fulfillment but nothing else, will Bob have done his job?

I think that's the question under discussion in #314. I think it should either be an expectation of the ILP protocol that connectors send response data back, or not. I don't think there needs to be an indication on the forward path whether it's expected. The transport protocols will know when it's expected or not.

what would be the purpose of the TTL and how would it be implemented by senders/connectors/receivers?

There would be a field in the packet that the sender would set (to a number less than 256 and probably closer to 10). Each connector would decrease that value by 1 before passing on the packet. The purpose, like in IP, is to mitigate the damage caused by routing loops. When the value gets to 0, connectors would not forward the packet any more and would reject the incoming transfer with a specific error.

Previously we've thought that we didn't need a TTL or max number of hops because the routing loops would be implicitly handled by the amount and/or transfer timeout. For packets with no amount in them, it's less clear to connectors when there is not enough money to deliver the destination amount -- unless the transfer amount is 0. @justmoon made the point that relying on the amount or transfer timeout may take too long for packets to get dropped for them to mitigate the damage of routing loops.

I'm on the fence about whether it's worth adding a max number of hops. Leaning towards no right now but could be convinced otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. What if we add and make FF or something a flag to ignore so it can be optional.

@@ -9,6 +9,7 @@ IMPORTS
FROM GenericTypes

InterledgerProtocolPaymentData,
BestEffortPaymentData,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be called something like InterledgerProtocolV2Data. I don't think it's really a request/response protocol (though I might feel differently about that if we end up adding fulfillment data)

@michielbdejong
Copy link
Contributor Author

Postponing until after Evan's experiment, we can reopen this issue in Q1 2018.

I'll close it now to keep this repo's PRs list clean until then.

@michielbdejong
Copy link
Contributor Author

Reopening this as several (maybe even most?) community members have expressed interest in experimenting with this.

@michielbdejong michielbdejong reopened this Dec 1, 2017
@michielbdejong michielbdejong force-pushed the mj-best-effort-payments branch 2 times, most recently from d9bd5bd to d58cfb4 Compare December 1, 2017 14:32
@justmoon
Copy link
Member

justmoon commented Dec 3, 2017

Hmm, I was thinking we would experiment using the existing method for best-effort payments for a while and then decide what we want the new packet to look like.

Right now I'm not aware of any other changes we would make, so this looks good to me. But I think it's possible that we would come up with changes while experimenting with the new transport layer protocols.

@michielbdejong
Copy link
Contributor Author

ok, let's mark it as experimental, that gives us a free ticket to make breaking changes when we remove that label.

@emschwartz
Copy link
Member

@michielbdejong what's the benefit of doing that over continuing to use the zero-amount packets while we're experimenting?

@michielbdejong
Copy link
Contributor Author

michielbdejong commented Dec 4, 2017

@emschwartz as explained on Slack, overloading type 1 would be a dirty hack and a breaking change, and both those things are undesirable, and also avoidable thanks to the fact that the ilp packet has an envelope. That is why I implemented the type 10 forwarded payments in several PRs to the reference stack, sorry if that wasn't clear.

@adrianhopebailie
Copy link
Collaborator

I'm interested to hear what benefit anyone feels having these packet definitions is providing at this point?

It seems to me that having a standard packet format is only mildly useful in that it provides a standard binary encoding which you can adopt in a ledger protocol but that's it.

@michielbdejong
Copy link
Contributor Author

I'm interested to hear what benefit anyone feels having these packet definitions is providing

You could replace them with something else, but as long as we don't, they are the language in which we describe the interaction between connectors.

@@ -152,7 +152,12 @@ See below for the [ILP Error Format](#ilp-error-format) and [ILP Error Codes](#i

### ILP Payment Packet Format

Here is a summary of the fields in the ILP payment packet format:
There are two types of payment packets: quoted and forwarded. A quoted payment specifies the amount that should arrive at the destination, a forwarded payment doesn't.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is between whether it is forwarded or delivered, as per #77. Whether you get a quote before sending the payment is a separate issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, renamed 'quoted' to 'delivered'.

Copy link
Member

@emschwartz emschwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(don't forget to update the draft number on RFC 3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants