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

bolt 11 payment addr + single shot mpp for SendPayment #3679

Merged
merged 19 commits into from Dec 19, 2019

Conversation

cfromknecht
Copy link
Collaborator

@cfromknecht cfromknecht commented Nov 6, 2019

In this PR, we make single-shot MPP payments the default payment type in lnd, and marks a major milestone in our long series of PRs preparing us for multi-path payments.

Notable changes

  • Invoices generated by lnd will:
    • Advertise optional support for TLV onions (var_onion_optin) and payment address (option_payment_addr).
    • Include a random 32-byte payment address.
  • Paying such an invoice via Lightning.SendPayment or RouterRPC.SendPayment will:
    • allow the router to determine support for MPP records using threaded feature vector state.
    • include the new MPP record in the final hop's payload if both the above options are supported, which includes the payment address from the invoice.
  • Fixes a bug that would prevent us from not being able to send custom records to unadvertised nodes

By upgrading to this payment type, lnd senders and receivers are offered increased protection from payment sniping and against probing attacks on the receiver. In the future both features will be made mandatory, entirely disabling support for the legacy payment style.

Depends on:

@cfromknecht cfromknecht added amp onion routing payments labels Nov 6, 2019
@cfromknecht cfromknecht force-pushed the bolt-11-payment-addr branch 3 times, most recently from b0ac5ac to 52371dc Compare Nov 6, 2019
@joostjager joostjager self-requested a review Nov 6, 2019
Copy link
Collaborator

@joostjager joostjager left a comment

Another very clean PR. Commit structure matters 👍

zpay32/invoice.go Outdated Show resolved Hide resolved
zpay32/invoice.go Outdated Show resolved Hide resolved
zpay32/invoice.go Outdated Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
routing/pathfind.go Outdated Show resolved Hide resolved
rpcserver.go Outdated Show resolved Hide resolved
lnrpc/invoicesrpc/addinvoice.go Outdated Show resolved Hide resolved
@joostjager joostjager added this to WIP in v0.9.0-beta Nov 9, 2019
@cfromknecht cfromknecht force-pushed the bolt-11-payment-addr branch 2 times, most recently from eb2df71 to 92a488b Compare Nov 26, 2019
@cfromknecht cfromknecht marked this pull request as ready for review Nov 26, 2019
@cfromknecht cfromknecht moved this from WIP to Needs Review in v0.9.0-beta Dec 3, 2019
@joostjager joostjager added this to the 0.9.0 milestone Dec 5, 2019
Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🎊

I think this is ready to land after the dependent PR has (and this PR has been rebased ofc).

lnrpc/invoicesrpc/addinvoice.go Outdated Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
halseth
halseth approved these changes Dec 9, 2019
Copy link
Collaborator

@halseth halseth left a comment

Excellent PR! LGTM pending rebase 🕺

rpcserver.go Outdated Show resolved Hide resolved
v0.9.0-beta automation moved this from Needs Review to Approved Dec 9, 2019
@cfromknecht cfromknecht requested review from Roasbeef and halseth Dec 10, 2019
v0.9.0-beta automation moved this from Needs Review to Approved Dec 17, 2019
@cfromknecht cfromknecht force-pushed the bolt-11-payment-addr branch 4 times, most recently from 7d03bd3 to 111062b Compare Dec 18, 2019
routing/pathfind_test.go Outdated Show resolved Hide resolved
@@ -292,6 +292,11 @@ type RestrictParams struct {
// DestCustomRecords contains the custom records to drop off at the
// final hop, if any.
DestCustomRecords record.CustomSet

// DestFeatures is a feature vector describing what the final hop
// supports. If none are provided, pathfinding will try to inspect any
Copy link
Collaborator

@halseth halseth Dec 18, 2019

Choose a reason for hiding this comment

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

could an alternative be to take the concatenation of the provided features and those found in the graph? Or would there be a use for "disabling" features found in the graph.

Copy link
Collaborator Author

@cfromknecht cfromknecht Dec 19, 2019

Choose a reason for hiding this comment

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

we could do that, for now we just override on the assumption that the invoice is more critical than the node announcement (since it may advertise a distinct combination from the graph). we could consider changing this in the future tho

routing/pathfind_test.go Outdated Show resolved Hide resolved
routing/pathfind_test.go Outdated Show resolved Hide resolved
// PaymentAddr is a random 32-byte value generated by the receiver to
// mitigate probing vectors and payment sniping attacks on overpaid
// invoices.
PaymentAddr *[32]byte
Copy link
Collaborator

@halseth halseth Dec 18, 2019

Choose a reason for hiding this comment

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

since the value is not used, maybe better to just make this a bool, or another feature vector encoding the features we require the destination to have.

EDIT: I see it is used in later commits. Though it is a bit werid that pathfinding is responsible for setting the addr

// transitive feature dependencies for this node, and skip the
// channel if they are invalid.
default:
err := feature.ValidateDeps(edge.Node.Features)
Copy link
Collaborator

@halseth halseth Dec 18, 2019

Choose a reason for hiding this comment

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

I'm not super familiar with the TLV blobs passed to intermediate nodes, but do we really need to care whether the features are valid?

Copy link
Collaborator Author

@cfromknecht cfromknecht Dec 19, 2019

Choose a reason for hiding this comment

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

for now actually, we only care about TLV support which has no dependencies. in general tho i think it is better to be strict and ignore nodes that don't generate well formed announcemements

routing/pathfind.go Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
routing/pathfind.go Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
@halseth
Copy link
Collaborator

@halseth halseth commented Dec 18, 2019

Great commit structure, makes even a massive PR like this manageable to review 👍

cfromknecht added 19 commits Dec 19, 2019
Previously we would return nil features when we didn't have a node
announcement or a given node. With this change, we can always assume the
feature vector is populated during pathfinding.
Later this will be used to populate distinct feature vectors for either
end of the channel.
This commit allows custom node features to be populated in specific test
instances. For consistency, we auto-populate an empty feature vector for
nodes that have nil feature vectors before writing them to the database.
In this commit, we fix a bug that prevents us from sending custom
records to nodes that aren't in the graph. Previously we would simply
fail if we were unable to retrieve the node's features.

To remedy, we add the option of supplying the destination's feature bits
into path finding. If present, we will use them directly without
consulting the graph, resolving the original issue. Instead, we will
only consult the graph as a fallback, which will still fail if the node
doesn't exist since the TLV features won't be populated in the empty
feature vector.

Furthermore, this also permits us to provide "virtual features" into the
pathfinding logic, where we make assumptions about what the receiver
supports even if the feature vector isn't actually taken from an
invoice. This can useful in cases like keysend, where we don't have an
invoice, but we can still attempt the payment if we assume the receiver
supports TLV.
This commit adds an optional PaymentAddr field to the RestrictParams, so
that we can verify the final hop can support it before doing an
expensive round of pathfindig.
In this commit, we overwrite the final hop's features with either the
destination features or those loaded from the graph fallback. This
ensures that the same features used in pathfinding will be provided to
route construction.

In an earlier commit, we validated the final hop's transitive feature
dependencies, so we also add validation to non-final nodes.
This commit creates a wrapper struct, grouping all parameters that
influence the final hop during route construction. This is a preliminary
step for passing in the receiver's invoice feature bits, which will be
used to select an appropriate payment or payload type.
We move up the check for TLV support, since we will later use it to
determine if we can use dependent features, e.g. TLV records and payment
addresses.
This commit constructs a helper closure assertAmountSent that can be
reused by other functions. The closure returns an error so that it can
be used with wait.NoError or the new wait.InvariantNoError. The latter
is added since the predicate could otherwise pass immediately for the
sphinx_replay_persistence tests, but change shortly after. It also
rounds out the wait package so that we offer all combinations of
predicate and no-error style waits.
@cfromknecht cfromknecht merged commit 8ce9e15 into lightningnetwork:master Dec 19, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Dec 19, 2019
@cfromknecht cfromknecht deleted the bolt-11-payment-addr branch Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
amp onion routing payments v0.9.0
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants