[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

lightninglabs-deploy · 2024-09-06T09:46:44Z

@bhandras: review reminder
@starius: review reminder
@sputn1ck: review reminder

bhandras

Awesome and huge work! 🥇 Did the first detailed pass, going to spend some more time with testing and reading through the whole code again now 🧪

bhandras · 2024-08-08T16:25:01Z

staticaddr/loopin/manager.go

+	// Check that the label is valid.
+	err := labels.Validate(req.Label)
+	if err != nil {
+		return fmt.Errorf("invalid label: %v", err)


nit: %w here and everywhere else where we decorate errors.

bhandras · 2024-08-08T16:32:05Z

staticaddr/loopin/manager.go

+// InitiateLoopIn initiates a loop-in swap. It passes the request to the server
+// along with all relevant loop-in information.
+func (m *Manager) InitiateLoopIn(req *loop.StaticAddressLoopInRequest) error {
+	loopIn := &StaticAddressLoopIn{}


Perhaps we could reorder the code a bit so that validation of all fields that needs to be validated is followed by the swap creation/init etc.

bhandras · 2024-09-06T13:50:00Z

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

+    deposit_outpoints BLOB NOT NULL,
+
+    -- htlc_tx contains the htlc transaction without signatures.
+    htlc_tx BLOB,


I think storing the htlc is only minimally convenient vs regenerating it at runtime. I'd consider not storing it as a better alternative.

bhandras · 2024-09-06T14:00:42Z

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

+    htlc_tx_fee_rate BIGINT NOT NULL,
+
+    -- htlc_timeout_sweep_tx_hash contains the htlc timeout sweep tx hash.
+    htlc_timeout_sweep_tx_hash BLOB,


I propose to store the string converted tx id instead as it is easier to use if queried manually.

bhandras · 2024-09-06T14:01:27Z

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

+
+    -- deposit_outpoints is a concatenated list of outpoints that are used for
+    -- this swap. The list has the format txid1:idx;txid2:idx;...
+    deposit_outpoints BLOB NOT NULL,


Perhaps consider human readable storage so it is easier to use if queried manually.

bhandras · 2024-09-06T15:45:26Z

staticaddr/loopin/actions.go

+	err = f.cfg.WalletKit.PublishTransaction(f.ctx, timeoutTx, txLabel)
+	if err != nil {
+		if !strings.Contains(err.Error(), "output already spent") {
+			log.Errorf("%v: %v", txLabel, err)


nit: f.Errorf, and some more context could be useful.

bhandras · 2024-09-06T15:47:17Z

staticaddr/loopin/manager.go

+
+	// Retrieve all deposits referenced by the outpoints and ensure that
+	// they are in state Deposited.
+	deposits, active := m.cfg.DepositManager.AllStringOutpointsActiveDeposits( //nolint:lll


bhandras · 2024-09-06T15:48:57Z

staticaddr/loopin/actions.go

@@ -118,21 +118,53 @@ func (f *FSM) InitHtlcAction(_ fsm.EventContext) fsm.EventType {
 	}

 	f.loopIn.HtlcCltvExpiry = loopInResp.HtlcExpiry
+


nit: could you pls add more detail to the commit message?

bhandras · 2024-09-06T15:49:50Z

staticaddr/loopin/actions.go

-	fee := f.loopIn.HtlcTxFeeRate.FeeForWeight(f.loopIn.htlcWeight())
+	feeRate := chainfee.SatPerKWeight(loopInResp.HtlcFeeRate)
+	fee := feeRate.FeeForWeight(f.loopIn.htlcWeight())
+	if fee > f.loopIn.TotalDepositAmount()/5 {


nit: instead of writing TotalDepositAmount()/5 everywhere, please add a variable tracking this value.

staticaddr/loopin/actions.go

hieblmi · 2024-09-09T19:34:26Z

Thanks @bhandras for your review and the nice suggestions. I've addressed your comments.

starius · 2024-09-13T14:41:50Z

swapserverrpc/server.proto

+    repeated bytes sweepless_client_sigs = 3;
+}
+
+message PushStaticAddressSweeplessSigsResponse {
 }


Add line end

starius · 2024-09-13T15:58:41Z

cmd/loop/main.go

@@ -280,6 +280,12 @@ func displayInDetails(req *looprpc.QuoteRequest,
 			"wallet.\n\n")
 	}

+	if req.DepositOutpoints != nil {
+		fmt.Printf("On-chain fees for static address loop-ins is not " +


s/is not/are not/

starius · 2024-09-13T16:00:29Z

cmd/loop/staticaddr.go

@@ -229,9 +257,21 @@ func summary(ctx *cli.Context) error {
 	case "withdrawn":
 		filterState = looprpc.DepositState_WITHDRAWN

+	case "loopingin":


s/loopingin/looping_in/
s/loopedin/looped_in/

here and in the help message.

starius · 2024-09-13T16:08:24Z

cmd/loop/staticaddr.go

+	for _, outpoint := range outpoints {
+		if deposit, ok := depositMap[outpoint]; ok {
+			sum += deposit.Value
+		}


Should we do something if the outpoint is not in depositMap? Log or return an error?

added an error and fail the loop-in attempt if that scenario hits.

starius · 2024-09-13T16:11:35Z

cmd/loop/staticaddr.go

+	return nil
+}
+
+func sumDeposits(outpoints []string, deposits []*looprpc.Deposit) int64 {


What happens if an outpoint or a deposit is non-unique? Should we check this and return an error if this is detected?

added containsDuplicates after the outpoint selection is done.

starius · 2024-09-14T02:57:47Z

staticaddr/loopin/sql_store.go

+		ClientKeyIndex:       int32(loopIn.HtlcKeyLocator.Index),
+	}
+
+	joinedOutpoints := strings.Join(


Check that elements of loopIn.DepositOutpoints do not contain outpointSeparator

starius · 2024-09-14T02:59:20Z

staticaddr/loopin/sql_store.go

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


Add unit test.

starius · 2024-09-14T03:01:46Z

staticaddr/withdraw/manager.go

@@ -430,20 +408,19 @@ func (m *Manager) handleWithdrawal(deposits []*deposit.Deposit,
 				deposit.Withdrawn,
 			)
 			if err != nil {
-				log.Errorf("Error transitioning deposits: %v",
+				log.Errorf("Error transitioning deposits: %w",


%w should not be used in log. It will be logged as a wrong format.

I found more instances of this issue:

loop$ git grep -w log | grep %w staticaddr/deposit/actions.go: log.Errorf("%v: %w", txLabel, err) staticaddr/deposit/actions.go: log.Debugf("error while sweeping expired deposit: %w", err) staticaddr/deposit/manager.go: log.Errorf("Error sending OnStart event: %w", staticaddr/deposit/manager.go: log.Errorf("Error sending OnStart event: %w", err) staticaddr/loopin/actions.go: log.Warnf("unable to decode sweep address: %w", err) staticaddr/loopin/actions.go: log.Errorf("%v: %w", txLabel, err) staticaddr/loopin/fsm.go: log.Errorf("Error checking if loop-in is stored: %w", err) staticaddr/loopin/fsm.go: log.Errorf("Error updating loop-in: %w", err) staticaddr/loopin/manager.go: log.Errorf("Error sending OnStart event: %w", staticaddr/loopin/manager.go: log.Errorf("Error sending OnNewRequest event: %w", err) staticaddr/withdraw/manager.go: log.Errorf("Error republishing withdrawals: %w", staticaddr/withdraw/manager.go: log.Errorf("%v: %w", txLabel, err) staticaddr/withdraw/manager.go: log.Errorf("Error transitioning deposits: %w", staticaddr/withdraw/manager.go: log.Errorf("Error waiting for confirmation: %w", err) staticaddr/withdraw/manager.go: log.Errorf("Error republishing withdrawal: %w", err)

thanks for the hints, fixed.

starius · 2024-09-14T03:05:01Z

swapserverrpc/server.proto

+    bytes swap_hash = 1;
+
+    // The nonces that the client used to generate the htlc sigs.
+    repeated bytes htlc_client_nonces = 2;


I propose to make a sub-message including two fields: htlc_client_nonces and sigs. Then we can use it 3 times for normal, high and extremely high fee levels.

starius · 2024-09-14T03:06:01Z

swapserverrpc/server.proto

+    int32 htlc_expiry = 2;
+
+    // The nonces that the server used to generate the partial htlc tx sigs.
+    repeated bytes htlc_server_nonces = 3;


I propose to group the fields: make a sub-message including two fields: htlc_server_nonces and fee_rate. Then we can use it 3 times here for normal, high and extremely high fee levels.

very good suggestion, thanks

sputn1ck · 2024-09-17T11:20:20Z

Doing some manual testing now, gonna add some notes here and update them:

loopcli s does not show static address help, but an error
loopcli s w --utxo=a19442026905827007c97bf7eb286e872db6d47dc24a61d83ac96fb806254696:1 does not work when less than 3 confs, maybe show different error message or do not show unconfirmed utxos, or add bool ready to spend
I think it makes sense to move the static loop in in under the static command
I sometimes I encounter an error with looping in and just see swap canceled without any further information, I cannot pinpoint why this error is happening yet (not trying to double spend an outpoint)
loop-cli in s --all does not send correct error message when i have deposits with less than 3 confs

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

hieblmi · 2024-09-19T12:36:03Z

All addressed @sputn1ck, would be great if you give it another go. Also consider testing this PR within #814 which gives you better look-up commands.

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 reviewed Sep 6, 2024

View reviewed changes

hieblmi force-pushed the static-addr-staging branch from ce7be09 to 6cee2fe Compare September 8, 2024 17:48

hieblmi force-pushed the static-addr-7 branch 2 times, most recently from bd416b7 to 04f8541 Compare September 9, 2024 19:33

hieblmi requested a review from bhandras September 9, 2024 19:33

hieblmi force-pushed the static-addr-7 branch 4 times, most recently from d394459 to 2fe6195 Compare September 11, 2024 13:44

starius reviewed Sep 14, 2024

View reviewed changes

hieblmi force-pushed the static-addr-7 branch from 2fe6195 to 93db4df Compare September 16, 2024 10:39

hieblmi added 3 commits September 16, 2024 17:15

looprpc: static address loop-ins

ee0ba74

sqlc: loop-in tables and queries

7876171

log: static address loop-in logging

38d93ec

hieblmi force-pushed the static-addr-7 branch from 93db4df to c8c1462 Compare September 17, 2024 13:18

hieblmi requested a review from starius September 17, 2024 13:34

hieblmi force-pushed the static-addr-7 branch 3 times, most recently from 96c7526 to c6e7a43 Compare September 17, 2024 14:30

hieblmi added 5 commits September 17, 2024 16:30

staticaddr: deposit changes to be squashed

4522638

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

staticaddr: withdrawal changes to be squashed

2524e67

staticaddr: loop-in manager and fsm

475796a

unit: manager mocks for static address loop-in

b531787

loopd: static address loop-in support

b0e38c7

hieblmi force-pushed the static-addr-7 branch from c6e7a43 to 2a89748 Compare September 19, 2024 09:08

cmd: static address loop-in

2a89748

		@@ -118,21 +118,53 @@ func (f *FSM) InitHtlcAction(_ fsm.EventContext) fsm.EventType {
		}

		f.loopIn.HtlcCltvExpiry = loopInResp.HtlcExpiry

[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

lightninglabs-deploy commented Sep 6, 2024

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

hieblmi commented Sep 9, 2024

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 Sep 17, 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

sputn1ck commented Sep 17, 2024 • edited by hieblmi Loading

hieblmi commented Sep 19, 2024

hieblmi commented Jun 28, 2024 •

edited

Loading

starius left a comment •

edited

Loading

hieblmi Sep 17, 2024 •

edited

Loading

sputn1ck commented Sep 17, 2024 •

edited by hieblmi

Loading