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

rpc: allow wumbo invoices #4075

Merged
merged 1 commit into from
Apr 10, 2020
Merged

Conversation

Roasbeef
Copy link
Member

In this commit, we remove the restriction surrounding the largest
invoices that we'll allow a user to create. After #3967 has landed,
users will be able to send in aggregate a payment larger than the
current max HTLC size limit in the network. As a result, we can just
treat that value as the system's MTU, and allow users to request
payments it multiples of that MTU value.

A follow up to this PR at a later time will also allow wumbo channels.
However, that requires us to tweak the way we scale CSV values, as post
wumbo, there is no true channel size limit, only the
local limit of a given node. We also need to implement a way for nodes
to signal to other nodes their accepted max channel size.

@Roasbeef Roasbeef added payments Related to invoices/payments v0.10 labels Mar 13, 2020
@Roasbeef Roasbeef added this to the 0.10.0 milestone Mar 13, 2020
@Roasbeef Roasbeef requested a review from joostjager March 17, 2020 00:13
Copy link
Contributor

@joostjager joostjager left a comment

Choose a reason for hiding this comment

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

Tested this with #3967 (mpp splitting). Can confirm it works after also removing the send payment limit.

Why don't we remove the send payment limit in this PR too? I don't get the rationale for the 4M MTU idea. Especially because we do forward larger htlcs already.

@@ -4104,7 +4104,6 @@ func (r *rpcServer) AddInvoice(ctx context.Context,
IsChannelActive: r.server.htlcSwitch.HasActiveLink,
ChainParams: activeNetParams.Params,
NodeSigner: r.server.nodeSigner,
MaxPaymentMSat: MaxPaymentMSat,
Copy link
Contributor

Choose a reason for hiding this comment

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

the MaxPaymentMSat constant shouldn't be in use anymore, can it be removed?

another alternative would be to simply change this constant to rule out non-sensical values, e.g. >21M BTC

Copy link
Member Author

Choose a reason for hiding this comment

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

We still use it at times to ensure that routes submitted don't have hops which attempt to send more than that value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we allow a simple payinvoice to send out an htlc of any size, and restrict what gets offered via SendToRoute?

Copy link
Member Author

@Roasbeef Roasbeef Apr 4, 2020

Choose a reason for hiding this comment

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

Assuming payInvoice does payment splitting then it would/should adhere to the max split size of 4.2 mil satoshis. On the other hand, nodes should/will reject an HTLC above that size, so all routes need to always be below that value.

@Roasbeef
Copy link
Member Author

Updated to remove the sending limits in the main RPC server and the router rpc. Once the main splitting PR is in, I'll circle back and add sanity checks for things like trying to send more coins than will ever exist.

In this commit, we remove the restriction surrounding the largest
invoices that we'll allow a user to create. After lightningnetwork#3967 has landed,
users will be able to send in _aggregate_ a payment larger than the
current max HTLC size limit in the network. As a result, we can just
treat that value as the system's MTU, and allow users to request
payments it multiples of that MTU value.

A follow up to this PR at a later time will also allow wumbo _channels_.
However, that requires us to tweak the way we scale CSV values, as post
wumbo, there is no true channel size limit, only the
_local_ limit of a given node. We also need to implement a way for nodes
to signal to other nodes their accepted max channel size.
@Roasbeef
Copy link
Member Author

Pushed up a slightly modified version that leaves the limit in place for the legacy RPC as that doesn't actually support payment splitting.

@Roasbeef Roasbeef merged commit b947ed5 into lightningnetwork:master Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
payments Related to invoices/payments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants