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

multi: update the switch+router to be aware of the new TLV EOB format #3362

Merged
merged 12 commits into from Aug 23, 2019

Conversation

@Roasbeef
Copy link
Member

commented Jul 31, 2019

This is a preparatory PR meant to lay the remaining ground work for our AMP implementation in lnd. This PR builds on top of #3061 and lightningnetwork/lightning-onion#38.

routing+channeldb Extensions

After this PR the router is able to preferentially provide a hop with the new hop payload format based on its set of known feature bits. Clients to the router are now also able to provide an arbitrary set of KV pairs for the final destination, with path finding failing early if it's known that the destination doesn't support this new update.

The Hop struct now carries TLV records that may have been assigned during the path finding phase, with features in mind such as rendezvous routing. Along the way we also update the lnrpc protos to be able to accept these new TLV fields in order to allow the callers of the SendToRoute call to provide arbitrary TLV pairs.

We also update the payment serialization for the Hop structs to be aware of the new fields. In this PR we opted to append the fields to the end of the existing Hop serialization. However, given that this PR depends on the new TLV construct, we very well could add the new set of fields as composite TLV records within the database. However, it would seem preferable to implement a unified TLV-based optional field abstraction within the channeldb package rather than introducing one here to ensure we're all comfortable with the design.

invoices + htlcswitch Extensions

When parsing the forwarding information from a raw onion packet, we're now able to pull the new TLV encoded information as well as the legacy packed info within a unified call. As a result, the caller doesn't need to know what type of onion was provided, since in the near future both formats will be in flight.

The primary InvoiceRegistry struct has been updated to accept an optional eob parameter. This will include the entire TLV payload, which allows the registry to extract any additional information it needs to settle a payment such as payment shards for AMP. We'll likely also want to expose this EOB information over the RPC invoice acceptance calls as well, since they give developers an additional level of flexibility to roll out their own application level protocols.

====

TODO

  • Include the new EOB (if present) in the invoice acceptance hooks for the RPC interface

  • tests

  • allow destination TLV pairs to be passed over the RPC payment API

Copy link
Collaborator

left a comment

Did an initial pass.

My main comment is about the reach of the tlv types. The way I look at them is as a protocol level data structure. In this PR tlv records are exposed all the way to the rpc interface. I have a feeling that we may run into undesirable situations in the future with that.

Are there real downsides to decoding the eob payload in a regular go struct directly after we receive it? If there is a performance benefit, I wouldn't expect it to be significant.

lnrpc/rpc.proto Outdated Show resolved Hide resolved
htlcswitch/interfaces.go Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
routing/pathfind.go Outdated Show resolved Hide resolved
contractcourt/interfaces.go Show resolved Hide resolved
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

The way I look at them is as a protocol level data structure. In this PR tlv records are exposed all the way to the rpc interface

The same can be send for calls like SendToRoute which expose protocol level details (the hop payload, etc) over the RPC level interface. Exposing them over the RPC gives developers more control, as they can attach an special application specific payload, and have that be read out on the other side. The TLV namespace won't only be used for forwarding or receiving critical information as in the case of AMP and the like.

Are there real downsides to decoding the eob payload in a regular go struct directly after we receive it?

Passing records or blobs around leaves the decoding to the furthest endpoint, that being the caller. On the RPC level we don't really care what it is, only once it reaches say the invoice registry does it actually need manifest that into a concrete struct type (like a payment shard).

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

Passing records or blobs around leaves the decoding to the furthest endpoint, that being the caller.

Yes, that is clarified now. I didn't realize it could be used to pass completely custom data to the recipient.

lnrpc/rpc.proto Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Include the new EOB (if present) in the invoice acceptance hooks for the RPC interface

I checked off this box as I think we can leave it to anther PR, as in order for us to be consistent (including this information for all accept calls), we need to persist this information somewhere.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Also noting that for the onion section, we need to use a different integer encoding that leaves off the length of the total integer. The tlv package is to be updated to expose this new serialization, after which, I'll update that section to use the proper type.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Pushed up a new version with some bug fixes as fixup commits. I've also modified the integration tests to be able to test the old+new nodes. To do this, we've now gained a new LegacyProtocol sub-config to force nodes to use the old format, but allow us to default to the new format. I thought of the idea when writing the PR for the safu commitments, but we can roll it out here first, since this PR is likely to land before that.

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from f3d6e4b to 5c595cc Aug 7, 2019
@Roasbeef Roasbeef changed the title [WIP] multi: update the switch+router to be aware of the new TLV EOB format multi: update the switch+router to be aware of the new TLV EOB format Aug 8, 2019
@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 5c595cc to 9899fe3 Aug 8, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Pushed up a new version that uses the proper truncated integers now that the TLV PR has been merged into master. During testing I found a bug in the tlv package which is being fixed right now (we know the solution), so the current itests will fail.

When updating the tests in the routing package, I realized that we may actually need to perform a migration due to the change in the serialization of *route.Hop. IIRC, @joostjager has a migration in that area planned, so perhaps we can do a single migration.

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 9899fe3 to f77f56e Aug 8, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Pushed up another version rebasing of the bug fix in the tlv package. One more rebase left for this: after the onion PR is merged.

const (
// AmtOnionType is the type used in the onion to refrence the amount to
// send to the next hop.
AmtOnionType Type = 2

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Aug 8, 2019

Collaborator

perhaps define these in invoices, since there's no global tlv namespace?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 9, 2019

Author Member

Yeah I had this in a few other places, then settled on this since the package doesn't import any other internal packages other than for benchmarks. I ran into an import cycle earlier on when it was in the switch as an example.

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Aug 14, 2019

Collaborator

might be worth making a subpackage of tlv, e.g. tlv/onion just for defining these constants. just a thought

channeldb/payments.go Show resolved Hide resolved
channeldb/payments.go Outdated Show resolved Hide resolved
lncfg/protocol_legacy_on.go Outdated Show resolved Hide resolved
dave, err := net.NewNode("Dave", nil)
// First, we'll create Dave and establish a channel to Alice. Dave will
// be running an older node that requires the legacy onion payload.
daveArgs := []string{"--legacyprotocol.legacyonion"}

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Aug 8, 2019

Collaborator

should we run this test with both legacy and non-legacy payloads? possibly even using a mixed route?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 9, 2019

Author Member

The route is already mixed as Dave it the only one using the legacy protocol. Or do you mean an entire all legacy or modern route?

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from f77f56e to ba1788b Aug 9, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

New version pushed up that includes a migration for existing payment attempts, and fixes a bug in the old serialization. I think this is ready for final review now!

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from ba1788b to 5265dae Aug 9, 2019
@cfromknecht

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

--- FAIL: TestHoldInvoice (0.01s)
    invoiceregistry_test.go:402: expected state ContractAccepted, but got Open

--- FAIL: TestLightningNetworkDaemon/channel_backup_restore (52.60s)
    --- FAIL: TestLightningNetworkDaemon/channel_backup_restore/restore_from_RPC_backup (52.60s)
        lnd_test.go:102: Error outside of test: *errors.errorString unable to find Carol's force close tx in mempool: wanted 1, found 0 txs in mempool: []

contractcourt/htlc_incoming_resolver_test.go:197:4: cannot use registry (type *mockRegistry) as type Registry in field value:
	*mockRegistry does not implement Registry (wrong type for NotifyExitHopHtlc method)
		have NotifyExitHopHtlc(lntypes.Hash, lnwire.MilliSatoshi, uint32, int32, chan<- interface {}) (*invoices.HodlEvent, error)
		want NotifyExitHopHtlc(lntypes.Hash, lnwire.MilliSatoshi, uint32, int32, chan<- interface {}, []byte) (*invoices.HodlEvent, error)
@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 5265dae to 1bac725 Aug 9, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

The TestHoldInvoice is a known flake I think? Unable to repro that locally. Also unable to repro the SCB flake locally, as that's also a known flake that should be resolved with #3016. Pushed a fix for that test compile error.

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

Found the cause of the TestHoldInvoice flake, working on a fix

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 4593820 to 70a1ec5 Aug 21, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

I haven't been provided a compelling reason to remove the RPC functionality from the PR within the correspondence of this PR so far. As for the resolver functionality, there's no known code path that will result in us needing information there. When that code path is added (mmp, amp), then we can resolve that as I mentioned above by supplementing the resolver as we do when we need to obtain additional context from the channel/commitment.

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

A compelling reason for me is that we shouldn't merge functionality that has no practical use. Without the invoice registry handling the custom records, it has no practical use. Afaik there is no time line for the follow up that makes this functionality complete, so it may remain incomplete for a while.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

The functionality has near term practical use. The timeline for following up is this coming release. It's possible on this very day to create a branch dependent on this PR this beings to utilize this functionality.

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 70a1ec5 to 6b5b887 Aug 21, 2019
@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

I think I have expressed my opinion about pr scope clear enough. I still see no reason why we should merge a partial implementation now and not as part of that near term follow up.

@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

Merging this partial implementation, which makes no irreversible decisions on potential design points that may arise during the follows won't impede them at all. We have no golden rule against proceeding with partial versions of a solution when the follow ups are direct and imminent. Implementing all the planned follow ups in this PR would result in it being massive, as the planned follow ups span multiple sub-systems and even implement a new payment type within the system.

@joostjager

This comment has been minimized.

Copy link
Collaborator

commented Aug 21, 2019

There is no golden rule, but a well-scoped pr is better than a pr with a boundary that isn't as clearly defined. It is not a blocker, but imo still something to acknowledge and keep in mind for future prs. I am not advocating for fixing everything in this pr which is already large, but to do the opposite and slim it down.

In the tlv payload domain, there were multiple scopes possible for this pr:

  1. Send and receive the new tlv payload but limit the implementation to the existing fields. No additional records.
  2. Sending of custom records passed in via rpc (depends on 1)
  3. Deliver the tlv payload to the invoice registry by both link and resolver (depends on 1)
  4. Extracting custom records from tlv payload in invoice registry and expose over rpc (depends on 2)

The pr currently implements 1, 2 and a part of 3.

For multi-path payments, we need 1 and 3.

channeldb/migration_10_route_tlv_records.go Outdated Show resolved Hide resolved
channeldb/migration_10_route_tlv_records.go Outdated Show resolved Hide resolved
tlv/record_test.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
channeldb/payments.go Outdated Show resolved Hide resolved
htlcswitch/link.go Outdated Show resolved Hide resolved
htlcswitch/link.go Show resolved Hide resolved
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 6b5b887 to 9a999be Aug 21, 2019
Copy link
Collaborator

left a comment

LGTM 🍾

Copy link
Collaborator

left a comment

There are still a few open comments buried in the long discussion of this pr. I think there is some value left in those, but nothing that cannot be addressed later.

LGTM! 🚀

@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 9a999be to 6f1b250 Aug 22, 2019
Roasbeef added 12 commits Jan 11, 2019
In this commit, we add two new method so the `Record` struct: Type() and
Encode(). These are useful when a caller is handling a record and may
not know its underlying type and may need to encode a record in
isolation.
…message
In this commit, we extend the Hop struct to carry an arbitrary set of
TLV values, and add a new field that allows us to distinguish between
the modern and legacy TLV payload.

We add a new `PackPayload` method that will be used to encode the
combined required routing TLV fields along any set of TLV fields that
were specified as part of path finding.

Finally, the `ToSphinxPath` has been extended to be able to recognize if
a hop needs the modern, or legacy payload.
In this commit, we extend the path finding to be able to recognize when
a node needs the new TLV format, or the legacy format based on the
feature bits they expose. We also extend the `LightningPayment` struct
to allow the caller to specify an arbitrary set of TLV records which can
be used for a number of use-cases including various variants of
spontaneous payments.
In this commit, we add a new field to the Hop proto to allow callers to
be able to specify TLV records for the SendToRoute call, and also to be
able to display TLV records that were used during regular path finding.

We also update SendPayment to support dest TLV records.
…fields

We also include a migration for the existing routes stored on disk.
… registry

In this commit, we update the `HopIterator` to gain awareness of the new
TLV hop payload. The default `HopIterator` will now hide the details of
the TLV from the caller, and return the same `ForwardingInfo` struct in
a uniform manner. We also add a new method: `ExtraOnionBlob` to allow
the caller to obtain the raw EOB (the serialized TLV stream) to pass
around.

Within the link, we'll now pass the EOB information into the invoice
registry. This allows the registry to parse out any additional
information from the EOB that it needs to settle the payment, such as a
preimage shard in the AMP case.
…gacy onion

In this commit, we add a new build tag protected sub-config for legacy
protocol features. The goal of this addition is to be able to default to
new feature within lnd, but expose hooks at the config level to allow
integration tests to force the old behavior to ensure that we're able to
support both the old+new versions.
… pay test

In this commit, we force Dave to use the legacy onion payload for the
multi-hop test to ensure that we're able to properly mix the old and new
formats, and have all nodes properly decode+forward the HTLC.
@Roasbeef Roasbeef force-pushed the Roasbeef:tlv-onion-payload branch from 6f1b250 to b1aea41 Aug 23, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

Fixed a bug in the migration, needed to check for the existence of the mission control bucket (as 0.7.1 didn't have that present for example). Confirmed the migration is able to be applied without issue now!

@Roasbeef Roasbeef merged commit 64c7a0f into lightningnetwork:master Aug 23, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.