features+invoices: force MPP payload inclusion for non-keysend payments #4752
Conversation
invoices/update.go
Outdated
| // payment, then we'll return an error that the payment addr was wrong, | ||
| // since we require it for improved e2e payment security. | ||
| if ctx.mpp == nil { | ||
| return nil, ctx.failRes(ResultAddressMismatch), nil |
There was a problem hiding this comment.
i'm not sure this is the right error to return. in this case the sender sent a legacy payment, but they are getting errors about a feature they don't have implemented (otherwise they would have used mpp). presumably if they haven't implemented mpp they haven't implemented mpp errors, so this will be interpreted as unknown. perhaps incorrect-payment-details is more appropriate? should probably coordinate with the other implementations to ensure we all agree on how to handle it
There was a problem hiding this comment.
These errors are mainly just for proper internal logging and tests. The only errors we ever return that're soured from the invoice registry are: lnwire.FailMPPTimeout and IncorrectPaymentDetails: https://github.com/lightningnetwork/lnd/blob/master/htlcswitch/link.go#L1257
25164fe to
4ed90f7
Compare
|
PTAL @cfromknecht |
|
Looking into the new itest failure. |
invoices/update.go
Outdated
| // we'll permit it to pass. | ||
| _, isKeySend := ctx.customRecords[record.KeySendType] | ||
| invoiceFeatures := inv.Terms.Features | ||
| paymentAddrRequired := invoiceFeatures.HasFeature( |
There was a problem hiding this comment.
i think we need a new method here: RequiresFeature. HasFeature returns true if either are set.
There was a problem hiding this comment.
Added in the latest version! Also added a test for both the new method, as well as the positive path (for legacy invoices) in this area as well.
|
also needs squash! |
c571ce8 to
5a8cd7e
Compare
|
Pushed up new version that should pass that itests, and which also addresses Conner's latest comments. |
| feature ^= 1 | ||
| } | ||
|
|
||
| return fv.IsSet(feature) |
There was a problem hiding this comment.
alternatively could be fv.IsSet(feature&0xfe) which clears the lowest bit
|
|
||
| // mpp sets the MPP fields on the request if true, otherwise submits a | ||
| // regular payment. | ||
| mpp bool |
There was a problem hiding this comment.
doesn't this mean we are no longer testing legacy payments? or do we need to do this because our invoices now require them?
In this commit, we move to start requiring the payment addr feature bit in the invoices we produce. With this change, if a user attempts to pay one of our invoices (assuming they're also an lnd node), then they'll receive an error when they attempt to pay. At this point, *most* lnd nodes should be on v0.11 at this point, and this change will notify any lagging wallet authors to update, as these payments are generally more secure.
In this commit, we add a new RequiresFeature method to the feature vector struct. This method allows us to check if the set of features we're examining *require* that the even portion of a bit pair be set. This can be used to check if new behavior should be allowed (after we flip new bits to be required) for existing contexts.
In this commit, we move to start rejecting any normal payments that aren't keysend, if they don't also include the MPP invoice payload. With this change, we require that some sort of e2e secret (either the payment addr or the keysend pre-image) is present in a payload before we'll accept the payment. The second portion of the commit also updates all current tests in the package. We kept the base `TestSettleInvoice` test in-tact as it still exercises some useful behavior. However, we've removed all cases that allow an overpayment, as the new MPP logic doesn't allow overpayment for various reasons. In addition to this, some of the returned errors are slightly different, tho the actual behavior is equivalent.
In this commit, we extend the `BuildRoute` method and RPC on the router sub-server to accept a raw payment address which will be included as part of an MPP payload for the finla hop. This change actually also allows users to craft their own MPP paths using BuildRoute+SendToRoute. Our primary goal however, was to fix some broken itests since we now require the payAddr to be present for ALL payments other than key send payments.
With this change ListInvoices will return the payment addr, and the response to AddInvoice will as well.
It needs to include the MPP payload now.
d74ad5e to
4e079d1
Compare
In this PR, we move to start rejecting any normal payments that aren't keysend, if they don't also include the MPP invoice payload. With this change, we require that some sort of e2e secret (either the payment addr or the keysend pre-image) is present in a payload before we'll accept the payment.
In other words, we move to start requiring the payment addr feature bit in the invoices we produce. With this change, if a user attempts to pay one of our invoices (assuming they're also an lnd node), then they'll receive an error when they attempt to pay. At this point, most lnd nodes should be on v0.11 at this point, and this change will notify any lagging wallet authors to update, as these payments are generally more secure.
Fixes #4661 ?