[7/?] StaticAddr: Loop-In #786

hieblmi · 2024-06-28T12:59:12Z

This PR introduces loop-ins of static address deposits.
The client/server interaction is depicted below:

Open tasks that need resolution until this PR can leave draft mode:

Refactorings, unify code with reservations/instantout
DB storage & fsm recovery
Manual quote acceptance
FSM transition consistency checks
unit + integration tests
signing multiple fee-version htlcs, potential anchor output

Previously merged PRs:

#642
#650
#677
#719
#721
#750

master...static-addr-staging

starius

Reviewed only the first commit with protobuf types "looprpc: static address loop-ins".

looprpc/client.proto

starius · 2024-08-01T02:45:17Z

looprpc/client.proto


    /*
    EXPIRED indicates that the deposit has expired and the sweep transaction
    has been sufficiently confirmed.
    */
-    EXPIRED = 6;
+    EXPIRED = 10;


Maybe rename this state to EXPIRY_SWEEP_CONFIRMED?

These states were introduced in a different PR, so I'd add create a separate refactor PR to fix these up.

looprpc/client.proto

swapserverrpc/server.proto

starius

Reviewed another 3 commits:

sqlc: loop-in tables and queries
log: static address loop-in logging
staticaddr: deposit changes to be squashed

loopdb/sqlc/migrations/000011_static_address_loop_in.up.sql

loopdb/sqlc/queries/static_address_loopin.sql

loopdb/sqlc/migrations/000011_static_address_loop_in.up.sql

loopdb/sqlc/queries/static_address_loopin.sql

staticaddr/deposit/fsm.go

bhandras

Really awesome and huge work! 🥇 I only have a few questions and nit comments left.
tACK, LGTM 🌊

bhandras · 2024-10-29T07:13:18Z

loopdb/sqlc/queries/static_address_loopin.sql

+            WHERE swap_hash = u.swap_hash
+        )
+WHERE
+    ',' || $1 || ',' LIKE '%,' || u.update_state || ',%'


This is a nice trick! I'd say sqlc.slice is slightly more readable though, I had to test this filter condition a bit to make sure it works as expected.

bhandras · 2024-10-29T10:53:46Z

staticaddr/deposit/manager.go

@@ -34,7 +34,7 @@ const (

 	// DefaultTransitionTimeout is the default timeout for transitions in
 	// the deposit state machine.
-	DefaultTransitionTimeout = 1 * time.Minute
+	DefaultTransitionTimeout = 5 * time.Second


bhandras · 2024-10-29T14:17:20Z

staticaddr/deposit/manager.go

 }

 // AllOutpointsActiveDeposits checks if all deposits referenced by the outpoints
-// are active and in the specified state.
+// are in out in-mem active deposits map and in the specified state. If


nit: in out => in our

bhandras · 2024-10-29T14:25:24Z

staticaddr/deposit/manager.go


-	m.Lock()
-	defer m.Unlock()
+	_, deposits := m.toActiveDeposits(&outpoints)


This may not cause obvious problems, but iiuc AllOutpointsActiveDeposits (and other functions using toActiveDeposits(...) may suffer from non-atomicity. Instead of separately fetching active deposits, then locking them if they are in the target state we should be atomically filter and lock them.

You are right, there's a gap in coverage. It could be that a deposit is removed from the active map after the toActiveDeposits call.
Instead, we can m.mu.Lock() throughout the AllOutpointsActiveDeposits call and in other calls sites, so the in-mem deposit map is locked during filtering.

Left a comment that the caller of toActiveDeposits should call mu.Lock on the manager.

bhandras · 2024-10-29T14:33:40Z

loopd/swapclient_server.go

@@ -1429,12 +1429,17 @@ func (s *swapClientServer) WithdrawDeposits(ctx context.Context,
 		}
 	}

-	err = s.withdrawalManager.WithdrawDeposits(ctx, outpoints)
+	txhash, pkScript, err := s.withdrawalManager.DeliverWithdrawalRequest(


nit: time to squash these.

I was thinking to squash them on the staging branch once this PR is merged to it.

bhandras · 2024-10-29T16:04:27Z

staticaddr/loopin/sql_store.go

+}
+
+// toStaticAddressLoopIn converts sql rows to an instant out struct.
+func toStaticAddressLoopIn(_ context.Context, network *chaincfg.Params,


+1, perhaps again this could come as a follow up PR to reduce scope creep.

bhandras · 2024-10-29T16:23:40Z

staticaddr/loopin/actions.go

+	fee := feeRate.FeeForWeight(f.loopIn.htlcWeight())
+	if fee > maxHtlcTxFee {
+		// Abort the swap by pushing empty sigs to the server.
+		pushEmptySigs()


nit: Why not push empty sigs for the above errors?

Great point, if added it in all error cases after the call ServerStaticAddressLoopIn.

bhandras · 2024-10-29T16:24:12Z

staticaddr/loopin/actions.go

+	if err != nil {
+		err = fmt.Errorf("unable to derive htlc timeout sweep "+
+			"address: %w", err)
+


nit: Why not push empty sigs here?

bhandras · 2024-10-29T18:24:45Z

staticaddr/loopin/actions.go

+				deposit.SweepHtlcTimeout,
+			)
+			if err != nil {
+				log.Errorf("unable to transition "+


In this case manual intervention would be needed iiuc?

The timeout transaction would be published and funds would be safe, but the client would show the deposits in state Deposited(cause the payment deadline hit and reset the deposits). So for sanity the state would have to be changed manually, yes.

bhandras · 2024-10-29T18:34:57Z

staticaddr/loopin/actions.go

+			log.Errorf("%v: %w", txLabel, err)
+			f.LastActionError = err
+			return err
+		}


Ideally we also want to keep on republishing the timeout sweep (and therefore ignore errors handled here). We don't currently republish unless we recovered and we have one in the mempool already but what happens if it is priced out?

sputn1ck · 2024-10-30T09:55:26Z

swapserverrpc/server.proto

+    // ServerStaticAddressLoopIn initiates a static address loop-in swap. The
+    // server will respond with htlc details that the client can use to
+    // construct and sign the htlc tx.
+    rpc ServerStaticAddressLoopIn (ServerStaticAddressLoopInRequest)


wdyt about moving this to a seperate file and service?

I think it makes sense given the number of endpoints and also given the pattern we have with reservation/instantout. I think that's a candidate for a follow-up PR.

For awareness: a new service would be a breaking change

sputn1ck · 2024-10-30T09:55:55Z

swapserverrpc/server.proto

+
+    // The info the server used to generate the standard fee partial htlc tx
+    // sigs.
+    ServerHtlcSigningInfo standard_htlc_info = 3;


this could be a map as well https://protobuf.dev/programming-guides/proto3/#maps

That'd be much cleaner, although these maps don't allow enum keys, just arbitrary <string,string> etc. mappings, so we'd have to setup keys for this map in code rather than in the protobuf definition, which makes it a little less clear what's transported imo.
Can change it ofc if there's strong opinions.

hieblmi · 2024-10-30T12:33:02Z

staticaddr/loopin/actions.go

+	// Once the htlc tx is initiated, we store the loop-in in the database.
+	err = f.cfg.Store.CreateLoopIn(ctx, f.loopIn)
+	if err != nil {
+		err = fmt.Errorf("unable to store loop-in in db: %w", err)


here as well called pushEmptySigs().

sputn1ck

LGTM tACK! 🥳 🥳 🥳 🥳

starius

LGTM 🏆

Few notes

starius · 2024-11-04T19:38:21Z

cmd/loop/staticaddr.go

+	Description: `
+	Requests a loop-in swap based on static address deposits. After the
+	creation of a static address funds can be send to it. Once the funds are
+	confirmed on-chain they can be swapped instantaneously. If deposited


I propose to add the number of confirmations needed here.

starius · 2024-11-04T19:59:49Z

staticaddr/loopin/fsm.go

+// Debugf logs a debug message with the loop-in swap hash.
+func (f *FSM) Debugf(format string, args ...interface{}) {
+	if f.loopIn == nil {
+		log.Infof(format, args...)


Should be "log.Infof" I think

good catch, should be log.debugf.

starius · 2024-11-04T20:04:01Z

staticaddr/loopin/actions.go

+)
+
+const (
+	defaultConfTarget = 3


I propose to change to 6

starius · 2024-11-04T20:07:58Z

staticaddr/loopin/loopin.go

+// initiation time of the swap. If more than the swaps configured payment
+// timeout has passed, the remaining time will be negative.
+func (l *StaticAddressLoopIn) RemainingPaymentTimeSeconds() int64 {
+	elapsedSinceInitiation := time.Since(l.InitiationTime).Seconds()


I propose to add lnd/clock instance to StaticAddressLoopIn and use it here, so this place can be fully tested (by passing a test clock). I propose to add a unit test for this method, testing it at various times.

rpcs for loop client and server are added to facilitate static address swap requests.

We add CRU procedures for static address swaps.

The static address state machine adds states to reflect the state of a deposit during a loop-in swap.

In this commit we introduce maximum fee percentages for the static loop-in htlc transactions. Since the server has the ability to publish htlc transactions without settling the swap payment we have to restrict the amount the server allocates for fees of these transactions.

In this commit we add the static address loop-in state machine and its orchestration through the manager.

hieblmi added the static-loop-in label Jun 28, 2024

hieblmi self-assigned this Jun 28, 2024

hieblmi marked this pull request as draft June 28, 2024 12:59

hieblmi force-pushed the static-addr-7 branch from ff4b3d8 to b30e992 Compare June 28, 2024 13:02

hieblmi force-pushed the static-addr-staging branch 2 times, most recently from d807a95 to e635d63 Compare July 1, 2024 10:51

hieblmi force-pushed the static-addr-7 branch 2 times, most recently from d813b20 to 447d70d Compare July 9, 2024 10:13

hieblmi force-pushed the static-addr-7 branch 3 times, most recently from 7ee61f6 to 3eb5bd2 Compare July 17, 2024 07:11

hieblmi force-pushed the static-addr-staging branch from e635d63 to 2c94110 Compare July 17, 2024 10:13

hieblmi force-pushed the static-addr-7 branch 4 times, most recently from a151ffd to d69e9d5 Compare July 18, 2024 08:19

hieblmi force-pushed the static-addr-staging branch from 2c94110 to ae3a983 Compare July 22, 2024 08:43

hieblmi force-pushed the static-addr-7 branch from d69e9d5 to cf15745 Compare July 28, 2024 18:32

hieblmi force-pushed the static-addr-staging branch from ae3a983 to 0e92035 Compare July 29, 2024 07:29

hieblmi force-pushed the static-addr-7 branch 6 times, most recently from 127d76c to a9d9e6f Compare July 30, 2024 14:38

hieblmi marked this pull request as ready for review July 30, 2024 14:38

hieblmi requested review from bhandras and starius July 30, 2024 14:38

starius reviewed Aug 2, 2024

View reviewed changes

starius reviewed Aug 6, 2024

View reviewed changes

bhandras approved these changes Oct 29, 2024

View reviewed changes

sputn1ck reviewed Oct 30, 2024

View reviewed changes

hieblmi commented Oct 30, 2024

View reviewed changes

hieblmi force-pushed the static-addr-7 branch 4 times, most recently from 63f4946 to a50d3d7 Compare October 30, 2024 15:58

hieblmi requested a review from sputn1ck October 31, 2024 09:55

sputn1ck approved these changes Nov 4, 2024

View reviewed changes

starius approved these changes Nov 4, 2024

View reviewed changes

hieblmi force-pushed the static-addr-staging branch from 90c9c2c to f959936 Compare November 5, 2024 08:58

hieblmi force-pushed the static-addr-7 branch 2 times, most recently from 2b5b1c4 to d4967f4 Compare November 5, 2024 09:11

hieblmi added 3 commits November 5, 2024 10:11

looprpc: static address loop-ins

7d72f71

rpcs for loop client and server are added to facilitate static address swap requests.

sqlc: loop-in tables and queries

3c0c6f8

We add CRU procedures for static address swaps.

log: static address loop-in logging

21cd934

hieblmi force-pushed the static-addr-7 branch from d4967f4 to ac65726 Compare November 5, 2024 09:52

hieblmi added 11 commits November 5, 2024 10:52

staticaddr: enable deposits for swaps

dba7796

The static address state machine adds states to reflect the state of a deposit during a loop-in swap.

staticaddr: withdrawal changes to be squashed

6840b2b

loopin: public scope for ValidateLoopInContract

e361eb5

interface: add StaticAddressLoopInRequest interface

0df0f7d

staticaddr: loopin

9ad60e4

staticaddr: sql_store

9424b04

staticaddr: swap manager and fsm

4368d94

In this commit we add the static address loop-in state machine and its orchestration through the manager.

unit: manager mocks for static address loop-in

e623061

loopd: static address loop-in support

2eec098

cmd: static address loop-in

ac65726

hieblmi merged commit d277bff into lightninglabs:static-addr-staging Nov 5, 2024
4 checks passed

hieblmi deleted the static-addr-7 branch November 5, 2024 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[7/?] StaticAddr: Loop-In #786

[7/?] StaticAddr: Loop-In #786

hieblmi commented Jun 28, 2024 •

edited

Loading

starius left a comment

starius Aug 1, 2024

hieblmi Aug 13, 2024

starius left a comment •

edited

Loading

bhandras left a comment

bhandras Oct 29, 2024

bhandras Oct 29, 2024

bhandras Oct 29, 2024

bhandras Oct 29, 2024

hieblmi Oct 30, 2024 •

edited

Loading

bhandras Oct 29, 2024

hieblmi Oct 30, 2024

bhandras Oct 29, 2024

bhandras Oct 29, 2024

hieblmi Oct 30, 2024

bhandras Oct 29, 2024

hieblmi Oct 30, 2024

bhandras Oct 29, 2024

hieblmi Oct 30, 2024 •

edited

Loading

bhandras Oct 29, 2024

sputn1ck Oct 30, 2024

hieblmi Oct 31, 2024

sputn1ck Oct 31, 2024

sputn1ck Oct 30, 2024

hieblmi Oct 31, 2024 •

edited

Loading

hieblmi Oct 30, 2024

sputn1ck left a comment

starius left a comment

starius Nov 4, 2024

starius Nov 4, 2024

hieblmi Nov 5, 2024

starius Nov 4, 2024

starius Nov 4, 2024

[7/?] StaticAddr: Loop-In #786

[7/?] StaticAddr: Loop-In #786

Conversation

hieblmi commented Jun 28, 2024 • edited Loading

starius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starius left a comment • edited Loading

Choose a reason for hiding this comment

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

hieblmi Oct 30, 2024 • edited Loading

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

Choose a reason for hiding this comment

hieblmi Oct 30, 2024 • edited Loading

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

hieblmi Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sputn1ck left a comment

Choose a reason for hiding this comment

starius 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

hieblmi commented Jun 28, 2024 •

edited

Loading

starius left a comment •

edited

Loading

hieblmi Oct 30, 2024 •

edited

Loading

hieblmi Oct 30, 2024 •

edited

Loading

hieblmi Oct 31, 2024 •

edited

Loading