taproot swaps #28

JssDWt · 2024-12-13T16:06:24Z

This PR adds taproot swap support.

Changes in a nutshell:

the term 'redeem' was renamed to 'claim'
the public api has changed
swaps are now taproot swaps, with
- a claim script spend path
- a refund script spend path
- a cooperative musig key spend path
there's a cooperative refund added
lock time is now absolute. The default locktime is 1008 blocks from when the swap address was made.

Note about the musig2 implementation:

There is no musig2 implementation in rust-bitcoin at the moment. There is a PR in rust-secp256k1 though. Combining the version from that PR with the taproot parts of rust-bitcoin goes well, as long as keys are serialized/deserialized to convert the incompatible structs from one package to another.

review notes

Apologies for the huge PR. For review I think the best option is:

check the public server interface in swap.proto
check the pay and refund logic in public_server.rs
check the taproot logic in swap_service.rs

swapd/proto/swap/swap.proto

MusigSessionId was renamed to MusigSecRand.

roeierez

Looks really good. Everything seems to be in place.
Added review comments.

roeierez · 2024-12-26T14:46:18Z

swapd/src/public_server.rs

+        {
+            Ok(_) => {}
+            Err(LockSwapError::AlreadyLocked) => {
+                return Err(Status::failed_precondition("swap is locked"))


Does this mean we only have one attempt to pay the user? I am thinking of a case where payment failed and user tries again to complete the swap.

It is supposed to unlock after the payment, see #28 (comment)

roeierez · 2024-12-26T14:52:00Z

swapd/src/public_server.rs

                    "failed to persist pay result: {:?}",
                    e
                );
            }
        };

-        Ok(Response::new(GetSwapPaymentReply::default()))
+        let _ = self.swap_repository.unlock_swap(&swap_state.swap).await;


What happens if we attempted to pay and there was some error before getting into this point? For example if the lightning_client.pay failed? Will the swap stay locked forever?

It would remain locked forever, yes.

The lightning clients should return PaymentResult::Failure, rather than an error in most circumstances. (Which they don't! TODO!).

The thing with the locking is, I want to avoid races. If this line here is reached, you know the swap was locked by the current call to pay_swap. So it's safe to unlock in this function. We could add some logic to allow relocking the swap for payment, to allow payment in case of a crash. Which should be fine if there are no htlcs pending. But then the unlock mechanism needs to change, because the 'unlock' just releases the lock, it has no counter. You definitely want to make sure the cooperative refund can't happen while paying.

I don't like this whole database lock, but I don't really see a good way to do this safely, across restarts as well. If we start reasoning from the refund:

The fact there was a refund definitely needs to be stored. If there ever was a refund issued, you never want to pay again.

In order to avoid races with refund/pay, the refund and pay have to use the same lock.

If you have a better solution, please share!

I was thinking using the lock only for refund means that before we refund we lock and by that we mark the swap as eligible for refund and not eligible for payment anymore. This covers one side of the race.
For the other side (not refunding while payment in progress) perhaps we can use the payment attempts table?
I see that before payment we add an entry there and after we record the results (which is only handled by the preimage monitor so it should fix things after crashes while payment is in progress) so perhaps only allow cooperative refund if there is no payment attempt without result?

roeierez · 2024-12-26T14:55:46Z

swapd/src/public_server.rs

+        {
+            Ok(false) => {}
+            Ok(true) => {
+                if !was_locked {


Seems that if the server has some unplanned crash we can end up with the locked state forever?

Yes, in this case, for refund, that seems like the only viable solution though. If you don't know whether you returned a refund signature or not, you have to consider it refunded.

Swaps are now locked by a row in the database. This allows simultaneous refund locks and allows removing payment locks based on their label.

roeierez

Locking mechanism looks good to me. Added one comment regarding refund unlocking.

roeierez · 2025-01-14T08:32:17Z

swapd/src/postgresql/swap_repository.rs

+        Ok(())
+    }
+
+    async fn unlock_swap_refund(&self, swap: &Swap, refund_id: &str) -> Result<(), LockSwapError> {


I wonder if we need this function. Now that we have reliable locking mechanism do you see a case where we need to unlock the refund?

Theoretically no. I do like the extra safety check in refund_swap for the has_pending_or_complete_payment. It shouldn't happen due to the way the locking works, but perhaps a manual intervention or a bug causes the swap to be accidentally refundable.

Makes sense.

roeierez

LGTM

JssDWt marked this pull request as ready for review December 13, 2024 17:17

JssDWt requested review from roeierez, yaslama and hydra-yse December 13, 2024 17:17

JssDWt added 17 commits December 16, 2024 10:53

implement taproot swaps

91c17c5

fix payment result after cln payment failure

16cfbfa

itest: add random suffix to postgres containers

46745e8

itest: ensure cln nodes are killed

ba4c8fc

itest: add cltv tests

2fc1061

itest: reduce block times for quicker tests

3f1b51a

implement cooperative refund

82c5365

initial coop refund test

927d793

ensure not pay and refund can't happen together

3ec288b

itest: log swapd commandline arguments

c9be6d0

itest: fix commandline log formatting

88a8951

itest: log swapd start

ebc3be0

itest: fix commandline startup log

399a412

itest: formatting

adf9b32

fix claim input witness size

97a504f

remove todo

bd6046c

itest: fix expected fee rates

2ee3174

JssDWt force-pushed the jssdwt-taproot-swaps branch from a3c38b3 to 2ee3174 Compare December 16, 2024 09:53

roeierez reviewed Dec 16, 2024

View reviewed changes

swapd/proto/swap/swap.proto Show resolved Hide resolved

bump secp256k1 to latest

b5f4c3f

MusigSessionId was renamed to MusigSecRand.

roeierez reviewed Dec 26, 2024

View reviewed changes

JssDWt mentioned this pull request Jan 13, 2025

Synchronize all payment attempt results #30

Open

JssDWt requested a review from roeierez January 13, 2025 10:33

JssDWt force-pushed the jssdwt-taproot-swaps branch 2 times, most recently from d0365a8 to 2e365e5 Compare January 13, 2025 10:52

more robust swap locks

f1b5523

Swaps are now locked by a row in the database. This allows simultaneous refund locks and allows removing payment locks based on their label.

fix clippy

e36c0e5

JssDWt force-pushed the jssdwt-taproot-swaps branch from 2e365e5 to e36c0e5 Compare January 13, 2025 11:16

roeierez reviewed Jan 14, 2025

View reviewed changes

roeierez approved these changes Jan 16, 2025

View reviewed changes

JssDWt merged commit e36c0e5 into main Jan 16, 2025
16 checks passed

JssDWt deleted the jssdwt-taproot-swaps branch January 16, 2025 09:59

This was referenced Jan 16, 2025

taproot swaps #23

Closed

Add LND support #22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

taproot swaps #28

taproot swaps #28

JssDWt commented Dec 13, 2024 •

edited

Loading

roeierez left a comment

roeierez Dec 26, 2024

JssDWt Dec 27, 2024

roeierez Dec 26, 2024

JssDWt Dec 27, 2024

roeierez Dec 31, 2024 •

edited

Loading

roeierez Dec 26, 2024

JssDWt Dec 27, 2024

roeierez left a comment

roeierez Jan 14, 2025

JssDWt Jan 16, 2025

roeierez Jan 16, 2025

roeierez left a comment

taproot swaps #28

taproot swaps #28

Conversation

JssDWt commented Dec 13, 2024 • edited Loading

Note about the musig2 implementation:

review notes

roeierez 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

roeierez Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeierez 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

roeierez left a comment

Choose a reason for hiding this comment

JssDWt commented Dec 13, 2024 •

edited

Loading

roeierez Dec 31, 2024 •

edited

Loading