Abandon API for pending Loop-in swaps #661

hieblmi · 2023-11-13T14:13:21Z

This PR introduces a AbandonSwap Api that currently supports only loop-in swaps. Any pending swap's status can be overridden with a new database state called StateFailAbandoned. This command relies on the users judgement to only abandon swaps that don't lead to loss of funds, e.g. if a swap is abandoned that had it's time out path not swept yet.

loopd/swapclient_server.go

bhandras

Did a quick pass, looking good so far! I think we'll need some test coverage on the client side too. Essentially we want to make sure that calling the API in an on-going swap will indeed abandon it.

looprpc/client.proto

client.go

loopd/swapclient_server.go

lightninglabs-deploy · 2023-11-21T09:35:47Z

@bhandras: review reminder

GeorgeTsagk

Very clean PR 🙌
I identified one issue with the usage of abandonChannel. Let me know if you agree and whether one of my suggestions make sense

GeorgeTsagk · 2023-11-21T12:43:19Z

looprpc/client.proto

+}
+
+message AbandonSwapResponse {
+}


why not include some basic info here?
Can't the "abandon" operation result in an error?

Don't we usually have a pattern where if there is only one success state with no information we return an empty response (is in some lnd apis). If there is an error, we return an error

GeorgeTsagk · 2023-11-21T12:43:47Z

looprpc/client.proto

+    risking loss of funds by abandoning a swap. This could happen if an
+    abandoned swap would wait on a timeout sweep by the client.
+     */
+    bool i_know_what_i_am_doing = 2;


good choice 👍

GeorgeTsagk · 2023-11-21T12:45:49Z

swap/type.go

 	TypeOut
 )

+func (t Type) IsOut() bool {


nit: could fixup this commit? bit tiny

GeorgeTsagk · 2023-11-21T13:00:20Z

client.go

+		return errors.New("no request provided")
+	}
+
+	s.abandonChan <- req.SwapHash


I think you need to change the core structure here. A single channel won't notify all swaps that are waiting to read from it.

Here's an example:
We have 3 loop-ins active, in1 in2 and in3
All of them are waiting to read from s.abandonChan in case the user requests for one of them to be abandoned

User wants to abandon in2 so they call AbandonSwap method and that method writes to s.abandonChan as shown in the line above.

A random swap will read that value once, and then the channel is going to be clear. Let's say that in the above example in1 is reading the value, it checks that in1 != in2 and continues with a noop. The intended swap never received the value over the channel because it was consumed only once by in1.

Here's 2 ways you can proceed with this:

Enhance the "abandonChan" into a structure that is able to hold multiple channels for each subscriber (i.e you could have a map[lntypes.Hash]chan Bool which means one dedicated channel for each swap). When user tries to AbandonSwap we will first find the correct channel by accessing the map, and then try to write to that channel.

The more hacky approach is for each loop-in to re-write the value of lntypes.Hash into the channel when they check it and realise it doesn't match theirs. In that case the value would be written only once on the channel from this speciifc line above, but then it would ping-pong between all active loop-ins until it would eventually reach the intended target. (hacky and dirty, but only 1 line of code)

Thank you for the thorough review and the nice catch. I prefer option 1.) to fix this.

GeorgeTsagk

Looks good! just 1 last comment about deleting abandon chans

GeorgeTsagk · 2023-11-22T12:21:50Z

loopd/swapclient_server.go

 }

+// AbandonSwap requests the server to abandon a swap with the given hash.
+func (s *swapClientServer) AbandonSwap(_ context.Context,


Here we are not actually making use of the request context, but the current implementation of AbandonSwap is writing to a channel that is not buffered. Why not pass down the request context to the methods?

GeorgeTsagk · 2023-11-22T12:35:37Z

client.go

+		return errors.New("no request provided")
+	}
+
+	s.abandonChans[req.SwapHash] <- struct{}{}


According to my previous comment we could make use of ctx here and do smth like this

Suggested change

s.abandonChans[req.SwapHash] <- struct{}{}

select {

case s.abandonChans[req.SwapHash] <- struct{}{}:

case ctx.Done():

return ctx.Err()

}

in order to avoid blocking indefinitely.

This also raises a new topic: How are channels erased from memory?
And I think an important parameter here is whether the channel is buffered or not (i.e whether you block while writing to it or not)

Buffered channel: That means you immediately return from the above line thus there is also no need for using ctx
Note: How/When could we delete entries from the map in this case?

Unbuffered channel: That means we block in the above line and when the execution resumes either
i) ctx is done
ii) swap actually read the signal
and since the channel is only used once we could safely then delete the channel from the map

Seems to me like sticking with 2 (unbuffered channels) is more simple, so I would suggest adding this line on top of the function

// When this function returns there is no use of the swap's abandon chan. defer delete(s.abandonChans, req.SwapHash)

given that the upper block of code with the select statement and ctx is used too

Or if there is another place in the code where client receives signals from swaps when they reach a final state we could delete the map entry there, as it is no longer of use

Great points @GeorgeTsagk, thank you!

I think the defer delete however should only be applied if the swap was successfully abandoned, which could still fail even if it was sent on abandon channel. So one might try later again to abandon?

Let me check out the places where we delete from the map.

Thanks again.

The deletion introduces another problem, namely concurrent access to the channel map. I solved this by introducing a Mutx to the executor that is locked when a loop-in is created and locked again when a swap finishes.

Would be interested in alternative solutions.

client.go

GeorgeTsagk

Great work, LGTM 🤌

executor.go

bhandras

Looks really good now! Just a question on my side whether we should allow abandoning a loopin swap if the client already published the htlc (because they won't sweep it back then and it'll just hang in limbo) and some smallish comments.

bhandras · 2023-11-22T18:05:21Z

looprpc/client.proto

+    ABANDON_IN_PROGRESS = 0;
+
+    /*
+    ABANDON_FAILURE indicates that the swap was not abandoned due to an unknown


nit: doc seems to be out of sync.

See other comment, if we go with a simple error then there's no need to fix these comments.

bhandras · 2023-11-22T18:05:38Z

looprpc/client.proto

+    FAILURE_REASON_MISSING_FLAG = 1;
+
+    /*
+    ABANDON_FAILURE indicates that the swap was not abandoned because it is


nit: seems out of sync.

loopdb/swapstate.go

client.go

cmd/loop/swaps.go

bhandras · 2023-11-22T18:28:38Z

cmd/loop/swaps.go

+	Usage: "abandon a swap with a given swap hash",
+	Description: "This command overrides the database and abandons a " +
+		"swap with a given swap hash.\n\n" +
+		"!!! This command might potentially lead to loss of funds if" +


nit: add space at the end.

bhandras · 2023-11-22T18:28:59Z

cmd/loop/swaps.go

+		"!!! This command might potentially lead to loss of funds if" +
+		"it is applied to swaps that are still waiting for pending " +
+		"user funds. Before executing this command make sure that " +
+		"no funds are locked by the swap",


nit: missing period.

bhandras · 2023-11-22T18:29:11Z

cmd/loop/swaps.go

+			Name: "i_know_what_i_am_doing",
+			Usage: "Specify this flag if you made sure that you " +
+				"read and understood the following " +
+				"consequence of applying this command",


nit: missing period.

sputn1ck · 2023-11-22T18:59:18Z

looprpc/client.proto

+    /*
+    The result of the abandon operation.
+    */
+    AbandonResult result = 1;


to me the result struct here does not really make sense. The enums are only ever used in conjunction with an error (besides the ABANDON_IN_PROGRESS enum), this means that on the loop-cli side we will always just print the written out error instead of the enum. IMO the swap response can just be empty if successful and throw an error otherwise.

I tend to agree with this.

sputn1ck · 2023-11-22T19:00:50Z

cmd/loop/swaps.go

+		},
+	)
+	if err != nil {
+		return err


see earlier comment, but we will always error out, except on empty response.

bhandras

LGTM, pending decision about the status enum vs just returning an error.

bhandras · 2023-11-23T17:30:55Z

looprpc/client.proto

+    /*
+    The result of the abandon operation.
+    */
+    AbandonResult result = 1;


I tend to agree with this.

bhandras · 2023-11-23T17:31:12Z

looprpc/client.proto

+    ABANDON_IN_PROGRESS = 0;
+
+    /*
+    ABANDON_FAILURE indicates that the swap was not abandoned due to an unknown


See other comment, if we go with a simple error then there's no need to fix these comments.

bhandras · 2023-11-23T17:48:21Z

loopin.go

+		// If the client requested the swap to be abandoned, we override
+		// the status in the database.
+		case <-s.abandonChan:
+			err = s.setStateAbandoned(ctx)


nit: can extract this block as a method and then reuse in waitForSwapToComplete too.

loopin.go

bhandras · 2023-11-23T18:12:23Z

loopin.go

+		s.log.Infof("Abandoning swap %v", s.hash)
+		s.setState(loopdb.StateFailAbandoned)
+
+		err := s.persistAndAnnounceState(ctx)


Still applies.

swaprpc: abandon swap api

bhandras

LGTM, pending the last few comments 🤓 Great work! 🎉

bhandras · 2023-11-24T13:53:07Z

executor.go

+				// since the swap finalized.
+				if swap, ok := newSwap.(*loopInSwap); ok {
+					s.Lock()
+					delete(


nit: it seems there's no need for the line breaks as the delete still fits in 80 cols.

bhandras · 2023-11-24T13:54:55Z

client.go

 // cancelling the context.
-func (s *Client) Run(ctx context.Context, statusChan chan<- SwapInfo) error {
+func (s *Client) Run(ctx context.Context, statusChan chan<- SwapInfo,
+	abandonChans map[lntypes.Hash]chan struct{}) error {


Instead of passing in this map, it could just be instantiated in NewClient.

bhandras · 2023-11-24T13:56:08Z

loopd/swapclient_server.go

 	swaps            map[lntypes.Hash]loop.SwapInfo
 	subscribers      map[int]chan<- interface{}
 	statusChan       chan loop.SwapInfo
+	abandonChans     map[lntypes.Hash]chan struct{}


Seems like this is not used?

bhandras · 2023-11-24T13:56:32Z

loopin.go

+
+		return err
+	}
+	err = s.lnd.Invoices.CancelInvoice(


nit: no need for line break

bhandras · 2023-11-24T13:56:38Z

loopin.go

+		ctx, s.hash,
+	)
+	if err != nil {
+


nit: extra nl

bhandras · 2023-11-24T13:56:47Z

loopin.go

+	)
+	if err != nil {
+
+		return fmt.Errorf("swap hash "+


not: no need for so many line breaks.

bhandras · 2023-11-24T13:58:01Z

loopin.go

+func (s *loopInSwap) abandonSwapCancelInvoice(ctx context.Context) error {
+	err := s.setStateAbandoned(ctx)
+	if err != nil {
+


nit: extra nl

bhandras · 2023-11-24T13:58:29Z

loopin.go

+				return err
+			}
+
+			return fmt.Errorf("swap hash "+


How about returning this simply from abandonSwapCancelInvoice()?

We rely on the error here, it might be that abandonSwapCancelInvoice() is not returning an error. Also the function might fail due db issues which we want to catch here.

bhandras · 2023-11-24T13:59:45Z

loopin.go

 }

+// setStateAbandoned stores the abandoned state and announces it.
+func (s *loopInSwap) setStateAbandoned(ctx context.Context) error {


Perhaps this function could return a bool and and error indicating whether canceling the invoice is necessary. Alternatively, since this function is only called from one place you can just inline it there.

Ah right, setStateAbandoned could just handle state and invoice cancellation. Less code 👍

bhandras · 2023-11-24T14:04:39Z

loopin.go

 	return nil
 }

+// abandonSwapCancelInvoice abandons the swap and cancels the swap invoice.


nit: either move in previous commit or don't move here to make the diff smaller and consistent with the commit intent.

sputn1ck

Great work! tACK!

LGTM pending @bhandras comments

sputn1ck · 2023-11-24T14:19:09Z

swap/type.go

 	TypeOut
 )

+func (t Type) IsOut() bool {


nitty nit: missing godoc

hieblmi force-pushed the abandon-swaps branch 2 times, most recently from ce8e097 to a96802c Compare November 13, 2023 16:10

hieblmi commented Nov 13, 2023

View reviewed changes

loopd/swapclient_server.go Outdated Show resolved Hide resolved

hieblmi requested review from GeorgeTsagk, bhandras and sputn1ck November 13, 2023 16:47

hieblmi changed the title ~~Abandon pending Loop-in swaps~~ Abandon API for pending Loop-in swaps Nov 13, 2023

bhandras reviewed Nov 13, 2023

View reviewed changes

hieblmi force-pushed the abandon-swaps branch 4 times, most recently from 3de6e81 to 9e0ea5e Compare November 14, 2023 18:17

hieblmi requested a review from bhandras November 14, 2023 18:21

hieblmi force-pushed the abandon-swaps branch 3 times, most recently from c8d5d0b to 303631e Compare November 15, 2023 09:14

levmi assigned hieblmi Nov 16, 2023

hieblmi force-pushed the abandon-swaps branch 2 times, most recently from ca863da to 0f5fa01 Compare November 20, 2023 19:12

hieblmi force-pushed the abandon-swaps branch from 0f5fa01 to 7be9351 Compare November 21, 2023 11:51

GeorgeTsagk requested changes Nov 21, 2023

View reviewed changes

hieblmi force-pushed the abandon-swaps branch 3 times, most recently from 322c807 to b255724 Compare November 22, 2023 09:31

hieblmi requested a review from GeorgeTsagk November 22, 2023 09:31

GeorgeTsagk reviewed Nov 22, 2023

View reviewed changes

hieblmi force-pushed the abandon-swaps branch from b255724 to 0f4cbb7 Compare November 22, 2023 17:10

hieblmi commented Nov 22, 2023

View reviewed changes

client.go Outdated Show resolved Hide resolved

hieblmi force-pushed the abandon-swaps branch from 0f4cbb7 to 4ffd3fd Compare November 22, 2023 17:28

hieblmi requested a review from GeorgeTsagk November 22, 2023 17:32

GeorgeTsagk approved these changes Nov 22, 2023

View reviewed changes

executor.go Show resolved Hide resolved

bhandras reviewed Nov 22, 2023

View reviewed changes

sputn1ck reviewed Nov 22, 2023

View reviewed changes

hieblmi mentioned this pull request Nov 23, 2023

Finalize swap if wallet holds insufficient confirmed funds #665

Merged

hieblmi requested a review from bhandras November 23, 2023 17:29

hieblmi force-pushed the abandon-swaps branch from 4ffd3fd to 84ead39 Compare November 23, 2023 17:35

bhandras reviewed Nov 23, 2023

View reviewed changes

hieblmi force-pushed the abandon-swaps branch from 84ead39 to 4036ff7 Compare November 24, 2023 09:22

hieblmi added 2 commits November 24, 2023 11:09

looprpc: abandon swap api

8612912

swaprpc: abandon swap api

loopdb: abandon swap state

0fbf253

hieblmi force-pushed the abandon-swaps branch 2 times, most recently from a4f2c12 to 06f3f35 Compare November 24, 2023 10:46

hieblmi requested review from bhandras and sputn1ck November 24, 2023 11:10

bhandras approved these changes Nov 24, 2023

View reviewed changes

sputn1ck approved these changes Nov 24, 2023

View reviewed changes

swap/type.go

TypeOut

)

func (t Type) IsOut() bool {

Copy link

Member

sputn1ck Nov 24, 2023

Choose a reason for hiding this comment

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

nitty nit: missing godoc

hieblmi reacted with thumbs up emoji

hieblmi added 4 commits November 27, 2023 11:14

loopd: abandon loop-ins

5e91c44

loopin: restore abandon chan on restart

c0b4193

cmd: abandon api support

378d817

unit: abandon swap

ce6f826

hieblmi force-pushed the abandon-swaps branch from 06f3f35 to ce6f826 Compare November 27, 2023 10:25

hieblmi requested a review from bhandras November 27, 2023 10:26

hieblmi merged commit 9c1b745 into lightninglabs:master Nov 27, 2023

hieblmi deleted the abandon-swaps branch August 22, 2025 14:42

-	s.abandonChans[req.SwapHash] <- struct{}{}
+	select {
+	case s.abandonChans[req.SwapHash] <- struct{}{}:
+	case ctx.Done():
+	    return ctx.Err()
+	}

Abandon API for pending Loop-in swaps #661

Abandon API for pending Loop-in swaps #661

Uh oh!

Conversation

hieblmi commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lightninglabs-deploy commented Nov 21, 2023

Uh oh!

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hieblmi Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GeorgeTsagk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hieblmi commented Nov 13, 2023 •

edited

Loading

hieblmi Nov 22, 2023 •

edited

Loading