loop out: allow outbound channel set for multi-loops #205

joostjager · 2020-05-19T08:22:52Z

Currently loop out supports multi-loops (looping out multiple channels in one operation using MPP). But there is no control over which channels are used for the payment.

This issue adds the ability for the user to indicate a set of channels which are all allowed to be used in the swap.

Depends lightningnetwork/lnd#4257

joostjager · 2020-05-19T08:55:16Z

This is now implemented as an on-the-fly migration, but I am not sure whether it is the best choice. Especially the question of how to test this. Dragging in all the old serialization code mostly brings us back to the old-style migrations. And neither do I like the handling of legacy fields to be around forever.

carlaKC

Tangential, but do we have an official deprecation plan for loop?

Some small comments, nice clean PR overall.

loopdb/store.go

carlaKC · 2020-05-19T11:58:57Z

client.go

+	var channels string
+	for i, chanID := range request.OutgoingChanSet {
+		if i > 0 {
+			channels += ","


Could also just make []string channels here then use strings.Join for the log

+1 for the strings.Join, would also not add a , at the end.

This doesn't add , at the end

Created ChannelSet struct with String method

loopd/view.go

cmd/loop/loopout.go

joostjager · 2020-05-19T12:24:53Z

Tangential, but do we have an official deprecation plan for loop?

Just created this morning: #206

bhandras · 2020-05-19T13:29:13Z

loopdb/store.go

+	// each of the past updates to the swap itself.
+	stateBucket := swapBucket.Bucket(updatesBucketKey)
+	if stateBucket == nil {
+		return nil, errors.New("updates bucket not found")


It may be the right time to also create predefined errors instead of creating them inline (I know this is a common pattern in this file).

I am not convinced that we should replace every error with a constant. Some errors also have parameters and for those we'd need to create separate error structs to store the parameters. It is useful to assert on in unit tests, but that is not the case for this error.

loopdb/store.go

lndclient/router_client.go

loopdb/store_test.go

bhandras · 2020-05-19T13:42:15Z

client.go

+	var channels string
+	for i, chanID := range request.OutgoingChanSet {
+		if i > 0 {
+			channels += ","


+1 for the strings.Join, would also not add a , at the end.

loopdb/loopout.go

loopout.go

bhandras · 2020-05-19T13:51:38Z

cmd/loop/loopout.go

+	chanStrings := strings.Split(ctx.String("channel"), ",")
+	var outgoingChanSet []uint64
+	for _, chanString := range chanStrings {
+		chanID, err := strconv.ParseUint(chanString, 10, 64)


What if the the passed list/set contains duplicate items?

This is now handled in loopd

bhandras · 2020-05-19T18:09:09Z

Looks good. It would be nice if you could cover the added functionality with at least one test in the PR too.

joostjager · 2020-05-20T10:30:42Z

Added unit test and test for on-the-fly migration

joostjager · 2020-05-20T10:36:06Z

Before release, I want to follow up with an itest covering multi-loop: https://github.com/lightninglabs/nautilus/issues/250

bhandras

LGTM

bhandras · 2020-05-20T10:40:35Z

loopout_test.go

+	blockEpochChan := make(chan interface{})
+	statusChan := make(chan SwapInfo)
+
+	const maxParts = 5


how about extending this test just a little bit, such that max parts is a parameter and we test that with zero it fails, and then 1, 2, say 5?

We can't just test that at this level. There is no real lnd or pathfinding involved. The test just asserts passing through the parameter. This more suitable for an itest, although it partially redoes the lnd integration test. In loop we can assume the lnd part of mpp to be working.

Thanks for explaining, I missed that. Maybe if LND mock could be parametrized then we could still test our side a bit better. I agree it is much more convenient with itest though.

loopout_test.go

loopdb/store.go

cmd/loop/loopout.go

carlaKC · 2020-05-21T07:24:16Z

cmd/loop/loopout.go

+	// Parse outgoing channel set.
+	chanStrings := strings.Split(ctx.String("channel"), ",")
+	var outgoingChanSet []uint64
+	for _, chanString := range chanStrings {


Is there any need to check that the set of channels provided are unique? I doubt it, but wanted to check that it won't behave strangely with duplicates on the lnd side.

It is checked in loopd

I don't know how seriously we account for the alternate client case?

The alternate client would still connect to loopd?

Another step in the separation of loop in and loop out. This prepares for a new loop out-specific key to be added.

Preparation to prevent code duplication in future refactoring.

Split the fetch logic so that it is easier to add loop type-specific serialization.

Expose the channel set restriction that was introduced in LND 0.10.1 on the proxy object.

Upgrade the database schema to allow for multiple outgoing channels. This is implemented as an on-the-fly migration leaving the old key in place.

Expose the new channel set restriction on the loopd client rpc.

carlaKC

LGTM, awesome feature to support!

joostjager mentioned this pull request May 19, 2020

loop out: allow outbound channel set for multi-loops #195

Closed

joostjager added this to the 0.6.3 milestone May 19, 2020

joostjager force-pushed the channel-set branch 3 times, most recently from e69c89a to 97cc3b0 Compare May 19, 2020 08:41

joostjager marked this pull request as ready for review May 19, 2020 08:53

joostjager requested review from bhandras and carlaKC May 19, 2020 08:53

joostjager added client rpc enhancement New feature or request loop out labels May 19, 2020

carlaKC reviewed May 19, 2020

View reviewed changes

bhandras reviewed May 19, 2020

View reviewed changes

joostjager force-pushed the channel-set branch from 97cc3b0 to a080793 Compare May 19, 2020 16:27

joostjager requested review from bhandras and carlaKC May 19, 2020 16:28

joostjager force-pushed the channel-set branch from a080793 to f4d2a16 Compare May 19, 2020 17:17

joostjager force-pushed the channel-set branch 2 times, most recently from 5082260 to 74301e0 Compare May 20, 2020 10:29

joostjager force-pushed the channel-set branch from 74301e0 to 0cd89ad Compare May 20, 2020 10:35

bhandras approved these changes May 20, 2020

View reviewed changes

carlaKC reviewed May 21, 2020

View reviewed changes

joostjager added 3 commits May 21, 2020 11:13

loopout/test: test payment parameters

c6697da

loopdb: unroll shared creation logic

ba55777

Another step in the separation of loop in and loop out. This prepares for a new loop out-specific key to be added.

loopdb: extract update deserialization

9927139

Preparation to prevent code duplication in future refactoring.

joostjager added 7 commits May 21, 2020 11:21

loopdb: unroll shared fetch logic

503c83c

Split the fetch logic so that it is easier to add loop type-specific serialization.

lndclient: add outgoing channel set restriction

c62acd5

Expose the channel set restriction that was introduced in LND 0.10.1 on the proxy object.

loopdb/test: parameterize loop out test

bd6e3f4

loopdb/test: add database dump and restore tools

044c1c1

loopdb: store outgoing channel set

8c544bf

Upgrade the database schema to allow for multiple outgoing channels. This is implemented as an on-the-fly migration leaving the old key in place.

looprpc: add outgoing channel set restriction

ccec719

Expose the new channel set restriction on the loopd client rpc.

lndclient: document SendPaymentRequest fields

23a1e33

joostjager force-pushed the channel-set branch from 0cd89ad to 23a1e33 Compare May 21, 2020 09:22

joostjager requested a review from carlaKC May 21, 2020 09:23

carlaKC approved these changes May 21, 2020

View reviewed changes

joostjager merged commit 3316c4b into lightninglabs:master May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loop out: allow outbound channel set for multi-loops #205

loop out: allow outbound channel set for multi-loops #205

joostjager commented May 19, 2020

joostjager commented May 19, 2020

carlaKC left a comment

carlaKC May 19, 2020

bhandras May 19, 2020

joostjager May 19, 2020

joostjager May 19, 2020

joostjager commented May 19, 2020

bhandras May 19, 2020

joostjager May 19, 2020

bhandras May 19, 2020

bhandras May 19, 2020

joostjager May 19, 2020

bhandras commented May 19, 2020

joostjager commented May 20, 2020

joostjager commented May 20, 2020

bhandras left a comment

bhandras May 20, 2020

joostjager May 20, 2020

bhandras May 20, 2020

carlaKC May 21, 2020

joostjager May 21, 2020

carlaKC May 21, 2020

joostjager May 21, 2020

carlaKC left a comment

loop out: allow outbound channel set for multi-loops #205

loop out: allow outbound channel set for multi-loops #205

Conversation

joostjager commented May 19, 2020

joostjager commented May 19, 2020

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented May 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhandras commented May 19, 2020

joostjager commented May 20, 2020

joostjager commented May 20, 2020

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment