[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 · 2024-08-02T02:54:32Z

swapserverrpc/server.proto

+    // Examples:
+    //    loopd/v0.10.0-beta/commit=3b635821
+    //    litd/v0.2.0-alpha/commit=326d754
+    string user_agent = 7;


Is StaticAddressLoopInRequest.initiator appended to this field? Can you provide an example of such a user agent in the description, denoting where is the initiator part, please?

Again thanks for poking, I was using the initator as the user agent, but there's actually a function that appends the right info so that it matches the examples. Have a look at https://github.com/lightninglabs/loop/blob/master/version.go#L57. Will update the PR

swapserverrpc/server.proto

starius · 2024-08-02T03:00:06Z

swapserverrpc/server.proto

+    // The address that the server wants to sweep the static address deposits
+    // to.
+    string sweepless_sweep_addr = 3;


Can we replace an address with an unsigned tx to provide more flexibility?

With flexibility you mean that if the sweepless sweep tx has different parameters in the future we wouldn't have to modify these rpc messages, but the client would just blindly sign any tx?

Yes, that is the idea. Then we can implement sweep batching without asking clients to update.

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

Deposit schema extension with a withdrawal sweep address. If the user selects a deposit for withdrawal the destination address of the sweep is stored here.

A new rpc for deposit withdrawals is added. A withdrawal cooperatively spends the 2/2 musig deposit outpoint to a client-specified address.

A server endpoint to obtain a partial sig to cooperatively spend a 2/2 musig deposit outpoint.

This commit adds static address deposit outpoints to the QuoteRequest message. It allows to quote for loop in swaps with the total value of specified deposits.

This commit adds the number of deposits to the ServerLoopInQuoteRequest that the client wants to quote for.

These deposit changes will be squashed into the original commits once merged into staging.

bhandras

Super clean PR, I think this is very close to ready now!

looprpc/client.proto

bhandras · 2024-10-11T10:56:11Z

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

+
+    -- quoted_swap_fee is the swap fee in sats that the server returned in
+    -- the swap quote.
+    quoted_swap_fee BIGINT NOT NULL,


Just linking to this comment since it's in the auto generated code: 9ef2c6f#r1759145732

I generally agree, we could add the unit to the name here and for the fee rate.

bhandras · 2024-10-11T11:06:05Z

loopdb/sqlc/queries/static_address_loopin.sql

+WHERE
+        swaps.swap_hash = $1;
+
+-- name: GetStaticAddressLoopInSwaps :many


nit: I assume this is to restart pending swaps. Perhaps we could filter on the state to not return everything.

I've removed GetStaticAddressLoopInSwap and added GetStaticAddressLoopInSwapsByStates which we now use in the recoverLoopIns method.

bhandras · 2024-10-11T11:12:30Z

staticaddr/deposit/deposit.go

-	defer d.Unlock()
-
-	return d.state == Expired || d.state == Withdrawn || d.state == Failed
+	return d.state == Expired || d.state == Withdrawn ||


Why remove locking (here and below)?

Good catch! I've removed the locks while working on the recover() in the withdrawal manager. There in GetActiveDepositsInState we lock the states of several deposits in lockDeposits(deposits), but then try to lock again in check if !d.IsInState(stateFilter).

I've restored the locks here, and also provided a method d.IsInStateNoLock(stateFilter) that I call in GetActiveDepositsInState in recover of the withdrawal manager, since all deposits are already locked.

bhandras · 2024-10-11T11:15:38Z

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


I assume some transitions may be blocking, in which case 5 seconds might not be enough?

This timeout is only used in TransitionDeposits of the deposit manager, which has the following caveat:

// TransitionDeposits allows a caller to transition a set of deposits to a new // state. // Caveat: The action triggered by the state transitions should not compute // heavy things or call external endpoints that can block for a long time. // Deposits will be released if a transition takes longer than // DefaultTransitionTimeout which is set to 5 seconds.

So we really only call this function to transition to 'light' states.

bhandras · 2024-10-15T14:31:11Z

staticaddr/loopin/manager.go

+
+// Run runs the static address loop-in manager.
+func (m *Manager) Run(ctx context.Context, currentHeight uint32) error {
+	m.runCtx = ctx


Good idea for hygiene, although not strictly required given the current logic.

bhandras · 2024-10-15T14:31:29Z

staticaddr/loopin/manager.go

+	log.Infof("Recovering static address loop-ins...")
+
+	// Recover loop-ins.
+	loopIns, err := m.cfg.Store.AllLoopIns(m.runCtx)


Would be nice to just select pending ones.

bhandras · 2024-10-15T14:33:43Z

staticaddr/loopin/manager.go

+
+		// Create a state machine for a given loop-in.
+		recovery := true
+		fsm, err := NewFSM(m.runCtx, loopIn, m.cfg, recovery)


We should also store the constructed FSM in a member container, e.g. a map to avoid accidental GC.

bhandras · 2024-10-15T14:36:58Z

staticaddr/loopin/manager.go

+func (m *Manager) startLoopInFsm(loopIn *StaticAddressLoopIn) error {
+	// Create a state machine for a given deposit.
+	recovery := false
+	loopInFsm, err := NewFSM(m.runCtx, loopIn, m.cfg, recovery)


Again I think we should store active FSM's in order to avoid accidental GC.

bhandras · 2024-10-15T14:37:58Z

staticaddr/loopin/sql_store.go

@@ -0,0 +1,336 @@
+package loopin


nit: This could be a separate commit.

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

hieblmi added 27 commits October 15, 2024 11:20

staticaddr: public deposit method scope

150668c

sqlc: withdrawal address for deposits

a24c162

Deposit schema extension with a withdrawal sweep address. If the user selects a deposit for withdrawal the destination address of the sweep is stored here.

looprpc: deposit withdrawal endpoint

1418757

A new rpc for deposit withdrawals is added. A withdrawal cooperatively spends the 2/2 musig deposit outpoint to a client-specified address.

swapserverrpc: server withdrawal endpoint

7695337

A server endpoint to obtain a partial sig to cooperatively spend a 2/2 musig deposit outpoint.

log: unified static address logger

433215c

staticaddr: address and deposit adjustments for withdrawals

dfa4235

staticaddr: withdrawal manager and interface

a5a3f61

loopd: static address withdrawals support

f13564e

loop: static address withdrawals support

31f5a10

looprpc: static address summary

3c6dc54

staticaddr: expose static address to client summary

f08c1ec

loopd: static address summary

ea79825

looprpc: deposit outpoints for QuoteRequest

7e051b7

This commit adds static address deposit outpoints to the QuoteRequest message. It allows to quote for loop in swaps with the total value of specified deposits.

swapserverrpc: number of deposits for ServerLoopInQuoteRequest

d744ac1

This commit adds the number of deposits to the ServerLoopInQuoteRequest that the client wants to quote for.

loopd: quoting for static address loop-ins

94bad00

cmd: quote for static address loop-ins

2e31f4e

looprpc: static address loop-ins

2497599

sqlc: loop-in tables and queries

937d7c7

log: static address loop-in logging

164546e

staticaddr: deposit changes to be squashed

9e84591

These deposit changes will be squashed into the original commits once merged into staging.

staticaddr: withdrawal changes to be squashed

a1c2f67

loopin: public scope for ValidateLoopInContract

b19cc06

interface: add StaticAddressLoopInRequest interface

21c828a

staticaddr: swap manager and fsm

2681ae1

unit: manager mocks for static address loop-in

54467c8

loopd: static address loop-in support

7cbe156

cmd: static address loop-in

4f64cda

hieblmi force-pushed the static-addr-7 branch from 0585b1e to 4f64cda Compare October 15, 2024 13:04

bhandras reviewed Oct 15, 2024

View reviewed changes

hieblmi force-pushed the static-addr-staging branch from 2e31f4e to 909b3b4 Compare October 16, 2024 18:54

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 Aug 2, 2024

hieblmi Aug 13, 2024

starius Aug 2, 2024

hieblmi Aug 13, 2024

starius Sep 10, 2024

starius left a comment •

edited

Loading

bhandras left a comment

bhandras Oct 11, 2024

hieblmi Oct 16, 2024

bhandras Oct 11, 2024

hieblmi Oct 16, 2024

bhandras Oct 11, 2024

hieblmi Oct 16, 2024

bhandras Oct 11, 2024

hieblmi Oct 16, 2024

bhandras Oct 15, 2024

bhandras Oct 15, 2024

bhandras Oct 15, 2024

bhandras Oct 15, 2024

bhandras Oct 15, 2024

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

Are you sure you want to change the base?

[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

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

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

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 commented Jun 28, 2024 •

edited

Loading

starius left a comment •

edited

Loading