probe payment as sanity check #260

YusukeShimizu · 2023-11-24T00:18:48Z

What

I added sanity check of ProbePayment.
If the result of ProbePayment is assumed to be a payment failure, log the reason.

Also, change MinSwapAmountMsat to be able to set by policy to test stuck channels in test.

Why

LN channels can get stuck in rare.
The onchain fees were wasted because they could not be detected.
By probing payment, sanity checks can detect them.

Change MinSwapAmountMsat to be set by policy.

Added test case to confirm that swap fails on StuckChannels. Allow pushAmt to be set during setup. Set min_swap_amount_msat to 1.

Replace sanity check with probe payment in SpendableMsat. ProbePayment trying to pay via a route with a random payment hash that the receiver doesn't have the preimage of. The receiver node aren't able to settle the payment. When the probe is successful, the receiver will return a incorrect_or_unknown_payment_details error to the sender. If the result of ProbePayment is assumed to be a payment failure, log the reason as known by ProbePayment

wtogami · 2023-11-24T04:37:34Z

Asking for opinions @nepet.

I think removing the previous sanity check isn't ideal. The existing balance checking current sanity check can tell us for sure that a probe would fail. The cost isn't high to probe but it is not costless. The current sanity check prevents a tiny bit of database bloat in situations where it is guaranteed to fail.

Probing seems to be the correct check after the costless sanity check. That is worthwhile to do at this step as it that prevents on-chain fee wasting.

Route may not be built even when channel is active. Since this causes flaky test, Added to check route construction in advance.

Restored sanity check of the maximum htlc limit. * To check max htlc more strictly than CLN's own limits. * To prevents a tiny bit of database bloat in situations where it is guaranteed to fail.

YusukeShimizu · 2023-11-26T04:55:58Z

I restored sanity check of the SpendableMsat.
Because the maximum htlc was checked by it more strictly than the CLN's own limit.

grubles · 2023-11-29T17:47:08Z

On Liquid testnet I tried L-BTC swaps:

CLN swap-out initiator:

Can confirm the probe occurs (at least I assume this is the probe):

2023-11-29T14:23:34.399Z INFO    035ca2fe4793a5e789ce846062eb4834f573c060d9200ce77544a29b48a0aa5923-chan#4: htlc 143 failed from 0th node with code 0x400f (WIRE_INCORRECT_OR_UNKNOWN_P
AYMENT_DETAILS)

The swap will complete successfully:

         "state": "State_ClaimedPreimage",

Restored max HTLC sanity check works:

{
   "code": -1,
   "message": "exceeding spendable amount_msat: 1"
}

LND swap-out initiator:

Probe occurs twice?

2023-11-29 11:51:51.466 [WRN] CRTR: Attempt 26002 for payment cf0684ff0b2ef2b71b8e3aec9fb1b70452df4a2bc0a23694e4d7ccd9a78f034c failed: IncorrectOrUnknownPaymentDetails(amt=100000000000 mSAT, height=171455)@1
2023-11-29 11:51:51.479 [INF] CRTR: Payment cf0684ff0b2ef2b71b8e3aec9fb1b70452df4a2bc0a23694e4d7ccd9a78f034c failed: final_outcome=incorrect_payment_details, raw_err=IncorrectOrUnknownPaymentDetails(amt=100000000000 mSAT, height=171455)@1
2023-11-29 11:51:51.503 [WRN] RPCS: Unable to send payment: IncorrectOrUnknownPaymentDetails(amt=100000000000 mSAT, height=171455)@1
2023-11-29 11:51:52.398 [WRN] CRTR: Attempt 26003 for payment 8088cad49156ae9c94c177180b63f77b828a8a8abc3bcde6a1b7ddd83d5835e0 failed: IncorrectOrUnknownPaymentDetails(amt=100000000000 mSAT, height=171455)@1
2023-11-29 11:51:52.410 [INF] CRTR: Payment 8088cad49156ae9c94c177180b63f77b828a8a8abc3bcde6a1b7ddd83d5835e0 failed: final_outcome=incorrect_payment_details, raw_err=IncorrectOrUnknownPaymentDetails(amt=100000000000 mSAT, height=171455)@1
2023-11-29 11:51:52.434 [WRN] RPCS: Unable to send payment: IncorrectOrUnknownPaymentDetails(amt=100000000000 mSAT, height=171455)@1

Swap is able to finish:

2023/11/29 16:53:13 [INFO] [Swap:f59b1983e3b9e63fb1d16528574bdcb9aad6a32979e0909a7cb52aa8e9cc5f10] Swap claimed with preimage eabf819c4c651d68345a5efb8f73db5e5ec7ae20ffb4b0856081755c50d65cba

Max HTLC sanity check works:

2023/11/29 12:31:39 rpc error: code = Unknown desc = exceeding spendable amount_msat: 1000

I will add another comment with mainnet test results.

grubles · 2023-11-29T20:40:23Z

On mainnet the probing / checks in this PR seem to still allow impossible swaps to occur when using a stuck channel:

"state": "State_ClaimedCoop",

"cancel_message": "could not pay invoice: timeout, last err: 204:failed: WIRE_TEMPORARY_CHANNEL_FAILURE (WIRE_TEMPORARY_CHANNEL_FAILURE: Capacity exceeded - HTLC fee: 6978sat)",

More info:

$ lightning-cli peerswap-swap-out <short chan id> 1015624 lbtc

      {
         "peer_id": "redacted",
         "connected": true,
         "state": "CHANNELD_NORMAL",
         "channel_id": "redacted",
         "short_channel_id": "redacted",
         "our_amount_msat": 157147510,
         "amount_msat": 13715301000,
         "funding_txid": "redacted",
         "funding_output": 0
      }

YusukeShimizu · 2023-11-30T04:11:56Z

#260 (comment)
Thanks for the test.

Probe occurs twice?

Probe occurs twice, which is the expected behavior.
The sanity check is timed twice in case of swap out.
The first time is when the swap command is started, and the second time is before the fee invoice is paid.

YusukeShimizu · 2023-11-30T04:13:17Z

#260 (comment)
I do not know the cause of the behavior change in test and mainnet.
Will investigate.

YusukeShimizu · 2023-11-30T07:21:38Z

I think it is possible that the stuck channel may have been caused by the payment of the fee invoice.

If you could share the results of the lightning-listpeerchannels and the amount of the fee invoice payment with me, I try to find out more.

wtogami · 2023-11-30T15:33:34Z

Probe occurs twice, which is the expected behavior.
The sanity check is timed twice in case of swap out.
The first time is when the swap command is started, and the second time is before the fee invoice is paid.

What does the second probe gain us?

If I understand it correctly no harm is done if the payment fails? The preimage is not revealed. It can still fallback to Coop Close to CSV after timeout?

YusukeShimizu · 2023-12-01T03:23:52Z

What does the second probe gain us?

When we receive the swap_out_agreement, we again confirm our own payment availability of the swap amt.
I understand that this is to prevent fee invoice payment and tx broadcast by reconfirming payment availability of swap amt just before fee invoice payment.

If I understand it correctly no harm is done if the payment fails? The preimage is not revealed. It can still fallback to Coop Close to CSV after timeout?

If payment to fee invoice fails, no extra cost is incurred.
However, if payment on the fee invoice is carried out and payment on the claim invoice which is swap amt fails, refund will be happened, onchain costs will be wasted.

wtogami · 2023-12-13T03:56:55Z

OHHH I read the description wrong. This sounds correct. I would strongly prefer that @nepet reviews this though.

nepet · 2023-12-18T12:18:06Z

test/bitcoin_cln_test.go

+	require := require.New(t)
+	// repro by using the push_msat in the open_channel.
+	// Assumption that feperkw is 253perkw in reg test.
+	bitcoind, lightningds, scid := clnclnSetupWithConfig(t, 3794, 3573, []string{


Numbers checked. They match 1% reserve 37940 msat and a CommitTx fee of 183172 msat.
This gives the funder 221112 msat for a stuck channel. The configuration leaves 221 sat on the funders side which results in a stuck channel.

nepet

This looks pretty solid! Good work! I am just confused about the way you construct the route (see below).

nepet · 2023-12-18T12:29:43Z

clightning/clightning.go

+		}
+	}
+
+	route, err := cl.glightning.GetRoute(channel.PeerId, amountMsat, 1, 0, cl.nodeId, 0, nil, 1)


I fail to see the reason in choosing a route that core lightning constructs for us? Should we not construct the route that uses the exact channel for probing that we also use for the payment?

Yes, I agree with your suggestion.

Like PayInvoiceViaChannel, I have modified to ensure that invoice payments are probed through a direct channel to peers.

nepet · 2023-12-18T12:34:02Z

clightning/clightning.go

+		if !ok {
+			return false, "", fmt.Errorf("WaitSendPay() %w", err)
+		}
+		faiCodeWireIncorrectOrUnknownPaymentDetails := 203


nit: typo failCodeWire...

To ensures that the invoice payment is probed via the direct channel to the peer.

YusukeShimizu · 2023-12-19T01:38:59Z

Corrected the parts that were reviewed.

nepet

Thanks a lot!
Ack 52e2ff3

YusukeShimizu added 3 commits November 24, 2023 08:59

policy: be able to set MinSwapAmountMsat

da1fe17

Change MinSwapAmountMsat to be set by policy.

test: swap in StuckChannels

da8ecf3

Added test case to confirm that swap fails on StuckChannels. Allow pushAmt to be set during setup. Set min_swap_amount_msat to 1.

YusukeShimizu force-pushed the probe-payment branch from 138a0dc to e5cbb6b Compare November 24, 2023 00:26

testframework: verify that route is constructed

5c30387

Route may not be built even when channel is active. Since this causes flaky test, Added to check route construction in advance.

YusukeShimizu force-pushed the probe-payment branch from 969bf95 to 5d79e9e Compare November 26, 2023 04:24

swap: restored sanity check

dbec5bb

Restored sanity check of the maximum htlc limit. * To check max htlc more strictly than CLN's own limits. * To prevents a tiny bit of database bloat in situations where it is guaranteed to fail.

YusukeShimizu force-pushed the probe-payment branch 2 times, most recently from cd42ebd to dbec5bb Compare November 26, 2023 04:41

YusukeShimizu marked this pull request as ready for review November 26, 2023 04:56

wtogami requested a review from nepet December 13, 2023 03:57

nepet reviewed Dec 18, 2023

View reviewed changes

YusukeShimizu force-pushed the probe-payment branch from 474f260 to 0b59602 Compare December 19, 2023 00:42

YusukeShimizu added 2 commits December 19, 2023 10:05

cln: uses the exact channel to probe payment

a6af654

To ensures that the invoice payment is probed via the direct channel to the peer.

cln: Fix typo in probe payment

52e2ff3

YusukeShimizu force-pushed the probe-payment branch from 0b59602 to 52e2ff3 Compare December 19, 2023 01:06

YusukeShimizu requested a review from nepet December 19, 2023 01:18

nepet approved these changes Dec 19, 2023

View reviewed changes

nepet merged commit a42d611 into ElementsProject:master Dec 19, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

probe payment as sanity check #260

probe payment as sanity check #260

YusukeShimizu commented Nov 24, 2023 •

edited

Loading

wtogami commented Nov 24, 2023

YusukeShimizu commented Nov 26, 2023 •

edited

Loading

grubles commented Nov 29, 2023

grubles commented Nov 29, 2023

YusukeShimizu commented Nov 30, 2023

YusukeShimizu commented Nov 30, 2023 •

edited

Loading

YusukeShimizu commented Nov 30, 2023

wtogami commented Nov 30, 2023

YusukeShimizu commented Dec 1, 2023 •

edited

Loading

wtogami commented Dec 13, 2023

nepet Dec 18, 2023

nepet left a comment •

edited

Loading

nepet Dec 18, 2023 •

edited

Loading

YusukeShimizu Dec 19, 2023 •

edited

Loading

nepet Dec 18, 2023

YusukeShimizu commented Dec 19, 2023

nepet left a comment

probe payment as sanity check #260

probe payment as sanity check #260

Conversation

YusukeShimizu commented Nov 24, 2023 • edited Loading

What

Why

wtogami commented Nov 24, 2023

YusukeShimizu commented Nov 26, 2023 • edited Loading

grubles commented Nov 29, 2023

CLN swap-out initiator:

LND swap-out initiator:

grubles commented Nov 29, 2023

YusukeShimizu commented Nov 30, 2023

YusukeShimizu commented Nov 30, 2023 • edited Loading

YusukeShimizu commented Nov 30, 2023

wtogami commented Nov 30, 2023

YusukeShimizu commented Dec 1, 2023 • edited Loading

wtogami commented Dec 13, 2023

nepet Dec 18, 2023

Choose a reason for hiding this comment

nepet left a comment • edited Loading

Choose a reason for hiding this comment

nepet Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

YusukeShimizu Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

nepet Dec 18, 2023

Choose a reason for hiding this comment

YusukeShimizu commented Dec 19, 2023

nepet left a comment

Choose a reason for hiding this comment

YusukeShimizu commented Nov 24, 2023 •

edited

Loading

YusukeShimizu commented Nov 26, 2023 •

edited

Loading

YusukeShimizu commented Nov 30, 2023 •

edited

Loading

YusukeShimizu commented Dec 1, 2023 •

edited

Loading

nepet left a comment •

edited

Loading

nepet Dec 18, 2023 •

edited

Loading

YusukeShimizu Dec 19, 2023 •

edited

Loading