Add support for Peerswap atomic swap based local liquidity management protocol #2342

remyers · 2022-07-05T09:54:18Z

This PR implements the draft PeerSwap protocol .

We create a SwapRegister actor at setup which will spawn swap sender actors in response to user requests. The SwapRegister will also spawn swap receiver actors in response to a swap requests received from peers. The SwapRegister forwards swap messages based on their swapId field to the corresponding local swap actor or remote peer. When a swap actor has completed it simply stops which triggers the SwapRegister to remove it.

Project TODOs:

Protocol implementation TODOs:

Only a single swap should be active on a given channel at one time.
Channels should fail updates that would remove liquidity needed for an active swap.

PeerSwap specification suggestions/questions to discuss:

custom byte encoded message rather than json #1
use 32-byte channel ids instead of short channel ids #2
nit: rename “OpeningTxBroadcasted” to grammatically correct “OpeningTxBroadcast” #3
send a signature instead of a private key for cooperative closes #4

sstone · 2022-07-05T09:58:19Z

eclair-core/src/main/scala/fr/acinq/eclair/NodeParams.scala

@@ -138,6 +140,7 @@ object NodeParams extends Logging {
    val oldSeedPath = new File(datadir, "seed.dat")
    val nodeSeedFilename: String = "node_seed.dat"
    val channelSeedFilename: String = "channel_seed.dat"
+    val swapSeedFilename: String = "swap_seed.dat"


I don't think that we need a new seed and key manager, we could probably reuse/extend the existing channel key manager

sstone · 2022-07-05T10:04:24Z

eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala

@@ -402,6 +408,11 @@ class Peer(val nodeParams: NodeParams, remoteNodeId: PublicKey, wallet: OnChainA
    self ! Peer.OutgoingMessage(msg, peerConnection)
  }

+  def replyUnknownSwap(peerConnection: ActorRef, unknownSwapId: String): Unit = {


this method does not seem to be used anywhere yet ?

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapCommands.scala

sstone · 2022-07-05T10:53:51Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapData.scala

+
+  final case class SwapInSenderData(channelId: ByteVector32, request: SwapInRequest, agreement: SwapInAgreement, invoice: Bolt11Invoice, openingTxBroadcasted: OpeningTxBroadcasted)
+
+  final case class SwapInReceiverData(shortChannelId: ShortChannelId, request: SwapInRequest, agreement: SwapInAgreement, invoice: Bolt11Invoice, openingTxBroadcasted: OpeningTxBroadcasted)


Why a not a 32 bytes channel id ? because of the PeerSwap specs that currently uses short channel ids ?

Ideally the PeerSwap spec would only use the 32 byte channel id which would be in the SwapInRequest message so we could then drop channelId and shortChannelId from these data classes.

sstone · 2022-07-05T10:57:38Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapHelpers.scala

+  def forwardShortIdAdapter(context: ActorContext[SwapCommand]): ActorRef[Register.ForwardShortId[HasSwapId]] =
+    context.messageAdapter[Register.ForwardShortId[HasSwapId]](ForwardFailure)
+
+  def fundOpening(request: SwapInRequest, agreement: SwapInAgreement, invoice: Bolt11Invoice)(implicit context: ActorContext[SwapCommand], wallet: OnChainWallet, feeRatePerKw: FeeratePerKw): Unit = {


nit: most methods in this file rely on implicit parameters, which can make the code seem more compact but also harder to read and extend.

I couldn't filed any explanation for how to use pilots to pass local fixed parameters, so I instead I just used currying to delineate them instead of using implicit. I still pass context as an implicit parameter though. see 4fcceb5.

I could also define and use a local function like the following, but it seemed clearer to use the helper directly.

private def mySend(message: HasSwapId) = send(register, shortChannelId)(message)

Does this approach look ok?

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapInReceiver.scala

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapInSender.scala

sstone · 2022-07-05T11:07:09Z

eclair-core/src/main/scala/fr/acinq/eclair/wire/protocol/LightningMessageTypes.scala

+  }
+}
+
+case class SwapInRequest(protocolVersion: Long, swapId: String, asset: String, network: String, scid: String, amount: Long, pubkey: String) extends JSonBlobMessage with HasSwapId


Should we use more restrictive types here, ByteVector32 for swap ids for example ?

Until the spec is changed from json to byte encodings our codec will have to work with swapId as a string. I couldn't figure out a way to parse the json into messages with fields of a different type.

eclair-core/src/test/scala/fr/acinq/eclair/swap/SwapTransactionsSpec.scala

t-bast

Very nice work, I think you clearly understood how to architect such a system: the flow between actors looks good.

The gist of my comments for this first review is that we should really focus this PR to be an MVP, as simple as possible. I think we can remove some nice-to-haves that can come later which would simplify the number of events that need to be handled, and will let us focus on the core of the peer swap protocol. This is important to ensure we don't have a bug in the core protocol as it could lead to loss of funds. I know it's tempting to make things better whenever you see a potential improvement (such as making state checks before accepting swaps), but once we have an MVP we can add those later (just keep a list somewhere of all the improvements you want to add in follow-up PRs). Otherwise the PR will be cluttered with a lot of comments on non-critical parts, and we may miss more important protocol issues.

Once that's done and we only have the core protocol to focus on, I think it's important to add the DB support: how we interact with the DB can be critical to avoid loss of funds, so that's a really important part to review and get right.

Also a small comment on types: you can do more typing work on the codecs to make the case classes directly contain the most accurate type (e.g. ByteVector32 for swapId, Satoshi for amounts, etc).

t-bast · 2022-07-28T08:52:30Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapData.scala

+
+object SwapData {
+
+  final case class SwapInSenderData(channelId: ByteVector32, request: SwapInRequest, agreement: SwapInAgreement, invoice: Bolt11Invoice, openingTxBroadcasted: OpeningTxBroadcasted)


nit: don't we say broadcast, not broadcasted?

It's on my list of proposed spec changes, but right now it matches their spec.

t-bast · 2022-07-28T09:16:45Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapRegister.scala

+    def replyTo: ActorRef[Response]
+  }
+  sealed trait InitializingMessages extends ReplyToMessages
+  case class RestoreData(replyTo: ActorRef[Response], swaps: Set[SwapInSenderData]) extends Command with InitializingMessages


Why do we only care about swap-in? Shouldn't we restore swap-out as well?

I've generalized SwapData to be the same for both actors, see c612e69. I assume that until the opening transaction is broadcast there is nothing relevant to backup - it's just a failed negotiation until that point.

t-bast · 2022-07-28T09:18:05Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapRegister.scala

+  sealed trait SwapId {
+    def id: String
+  }
+  case class SwapInSenderId(id: String) extends SwapId {
+    def toByteVector32: ByteVector32 = ByteVector32(ByteVector.fromValidHex(id))
+  }


Can't we simplify this to just use a ByteVector32 all the time since we already have an assumption in toByteVector32?

More generally, shouldn't this directly be a constructor argument of the swap-in actors (SwapInSender and SwapInReceiver) so that we can integrate them into logging automatically? In that case we probably don't need a dedicated case class, it can simply be a swapId: ByteVector32 or swapId: UUID, right?

Currently the json based PeerSwap protocol spec uses a string to represent swapId in protocol messages. Once the spec evolves to use byte encodings then that would clean up this sort of awkward reinterpretation. Perhaps to simplify review until then, the register should just map from String, the native type of swapId, to an actor, for example:

private def registering(swaps: Map[String, ActorRef[SwapCommands.SwapCommand]]): Behavior[Command] = {

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapRegister.scala

t-bast · 2022-07-28T09:22:06Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapRegister.scala

+    }
+
+  private def registering(swaps: Map[SwapInSenderId, ActorRef[Any]]): Behavior[Command] = {
+    // TODO: fail requests for swaps on a channel if one already exists for the channel; keep a list of channels with active swaps


Instead of a separate map, can't we put the channelId of the channel we're swapping on directly in SwapInSenderId? Then we only need to check the keys of the swaps map to extract the list of impacted channelIds, it's better than having to maintain two maps that can become inconsistent if we forget to update one in one case.

I think this is fixed in the latest PeerSwap plugin branch: SwapRegister.scala

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapResponses.scala

t-bast · 2022-07-28T10:01:05Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapInSender.scala

+      case CancelReceived(c) if c.swapId == swapId => swapCanceled(PeerCanceled(swapId))
+      case CancelReceived(_) => Behaviors.same


Aren't CancelReceived and CancelRequested redundant with AbortSwapInSender? There should be only one way of requesting this actor to gracefully stop from the outside, this would reduce the number of events we have to handle here.

Fixed in recent versions.

t-bast · 2022-07-28T10:05:40Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapInSender.scala

+    queryOnChainBalance(wallet)
+
+    receiveSwapMessage[CreateSwapMessages](context, "createSwap") {
+      case QueryOnChainBalance(Success(onChainBalance: OnChainBalance)) if onChainBalance.confirmed < amount + maxPremium.sat =>


I don't know if this check and the case ChannelInfoResponse(RES_GET_CHANNEL_INFO(_, _, _, d: DATA_NORMAL)) if d.commitments.availableBalanceForReceive.truncateToSatoshi < amount are really necessary, especially for a first version.

I'd rather stay simple at first and just abort later when we realize we actually cannot pay (either when failing to fund an on-chain transaction or failing to send an HTLC).

We can add nice-to-have checks later, I think we should just focus on an MVP at first, which will help focus on the core of the protocol.

Good point; this check is removed in recent versions.

t-bast · 2022-07-28T10:13:59Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapResponses.scala

+  case class SwapInStatus(swapId: String, actor: String, behavior: String, channelId: ByteVector32, request: SwapInRequest, agreement_opt: Option[SwapInAgreement] = None, invoice_opt: Option[Bolt11Invoice] = None, openingTxBroadcasted_opt: Option[OpeningTxBroadcasted] = None) extends Status {
+    override def toString: String = s"$actor[$behavior]: $swapId, $channelId, $request, $agreement_opt, $invoice_opt, $openingTxBroadcasted_opt"
+  }


I don't think we should expose that much internal information, actor and behavior are really internal details IMO.
But I think it would be valuable to have something like:

sealed trait SwapInStatus extends Status case class AwaitingSwapInAgreement(...) extends SwapInStatus case class AwaitOpeningTxConfirmation(...) extends SwapInStatus ...

The list of statuses we should expose should map to the protocol statuses where we're waiting for an external event (a response from our peer or a blockchain event like tx confirmation). We should not include transient statuses (such as CreatingOpeningTx for example), because these statuses will automatically move to a "real" status once an internal call completes, they would only clutter the status trait for no good reason. When the actor is in one of these transient statuses and receives a GetStatus request, it should simply delay it and it will eventually be handled once we're in a "real" status.

This is quite subtle, does it make sense (and do you think it's a good idea)?

Good point! I've updated GetStatus to emit the minimal set of user meaningful messages, and moved the verbose information to the debug log: 3403272

t-bast · 2022-07-28T10:16:58Z

eclair-core/src/main/scala/fr/acinq/eclair/swap/SwapHelpers.scala

+  def payInvoice(nodeParams: NodeParams)(paymentInitiator: actor.ActorRef, swapId: String, invoice: Bolt11Invoice): Unit =
+    paymentInitiator ! SendPaymentToNode(invoice.amount_opt.get, invoice, nodeParams.maxPaymentAttempts, Some(swapId), assistedRoutes = invoice.routingInfo, nodeParams.routerConf.pathFindingExperimentConf.getRandomConf().getDefaultRouteParams, blockUntilComplete = true)
+
+  def watchForPayment(watch: Boolean)(implicit context: ActorContext[SwapCommand]): Unit =


This is a good idea! But I think we should be more specific instead of registering to PaymentEvent. We should explicitly register only to PaymentReceived when we generated an invoice and wait for it to be paid, and we should only register to PaymentSent and PaymentFailed when we're paying an invoice.

Fixed in ad33a79. This also forced some reorganization of the coded that handles resume/restart scenarios because a Taker actor check points right before trying to pay the invoice.

Now the payInvoice method first checks if the payment is in the database before paying the invoice. This ensures the Taker will not be pay the fee or claim invoices twice after a restart or resume.

remyers · 2022-08-30T14:04:26Z

My latest push is rebased on master (from c1daaf3), cleans up the commit history and has been tested on signet between two eclair nodes.

codecov-commenter · 2022-08-30T14:20:58Z

Codecov Report

Merging #2342 (1f14ed1) into master (fb6eb48) will decrease coverage by 0.59%.
The diff coverage is 65.87%.

@@            Coverage Diff             @@
##           master    #2342      +/-   ##
==========================================
- Coverage   84.79%   84.19%   -0.60%     
==========================================
  Files         199      209      +10     
  Lines       15584    16081     +497     
  Branches      662      638      -24     
==========================================
+ Hits        13214    13540     +326     
- Misses       2370     2541     +171

Impacted Files	Coverage Δ
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala	`49.73% <0.00%> (-0.82%)`	⬇️
...la/fr/acinq/eclair/transactions/Transactions.scala	`96.04% <0.00%> (-0.77%)`	⬇️
...in/scala/fr/acinq/eclair/swap/SwapInReceiver.scala	`52.50% <52.50%> (ø)`
...ala/fr/acinq/eclair/swap/LocalSwapKeyManager.scala	`52.94% <52.94%> (ø)`
...main/scala/fr/acinq/eclair/swap/SwapInSender.scala	`54.48% <54.48%> (ø)`
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala	`88.70% <60.00%> (-0.64%)`	⬇️
.../main/scala/fr/acinq/eclair/swap/SwapHelpers.scala	`71.05% <71.05%> (ø)`
...main/scala/fr/acinq/eclair/swap/SwapRegister.scala	`75.60% <75.60%> (ø)`
...q/eclair/wire/protocol/PeerSwapMessageCodecs.scala	`82.50% <82.50%> (ø)`
.../scala/fr/acinq/eclair/swap/SwapTransactions.scala	`98.03% <98.03%> (ø)`
... and 16 more

- use different watch message to wait for csv - fix handling of received CancelSwap message - send OpeningTxBroadcasted message before opening confirmed - fixed claim by coop and csv transactions by including premium - do not handle user commands during swap-in create - do not send CancelSwap message for failures during swap-in create - reverse txid when making transaction outpoints - clarify comments to distinguish when tx is published vs confirmed

…Register Add support for testing swaps

- use different watch message to wait for csv - fix handling of received CancelSwap messages - send OpeningTxBroadcasted message before opening confirmed - fixed claim by coop and csv transactions by including premium - remove user commands during createSwap - do not send CancelSwap message for failures during createSwap - reverse txid when making transaction outpoints - clarify comments to distinguish when tx is published vs confirmed

- use only shortChannelId, not channelId - add isInitiator so same SwapData class works for both senders and receivers

remyers · 2022-09-14T09:42:15Z

Very nice work, I think you clearly understood how to architect such a system: the flow between actors looks good.

The gist of my comments for this first review is that we should really focus this PR to be an MVP, as simple as possible. I think we can remove some nice-to-haves that can come later which would simplify the number of events that need to be handled, and will let us focus on the core of the peer swap protocol. This is important to ensure we don't have a bug in the core protocol as it could lead to loss of funds. I know it's tempting to make things better whenever you see a potential improvement (such as making state checks before accepting swaps), but once we have an MVP we can add those later (just keep a list somewhere of all the improvements you want to add in follow-up PRs). Otherwise the PR will be cluttered with a lot of comments on non-critical parts, and we may miss more important protocol issues.

Thanks for the detailed review so far and helpful advice. I agree that it make sense to strip it down as much as possible to the critical protocol elements. Now that the basic architecture appears correct, I have gone ahead and finished off the protocol by adding the remaining states necessary to support the swap-out workflow. This workflow is the same as swap-in but begins with a Lightning fee payment from the swap initiator instead of a premium paid by the on-chain (maker) actor.

I will now look at incorporating your SwapRegister suggestions as a prelude to persisting the swap states to a DB.

remyers · 2022-09-14T09:48:35Z

I'd also like to rename my two main swap actors as follows:

SwapInSender -> SwapMaker
SwapInReceiver -> SwapTaker

The maker actor is the one that swaps on-chain UTXO value via the opening tx for off-chain Lightning liquidity. The taker actor always swaps off-chain Lightning liquidity for on-chain UTXO value. This naming would be more consistent with the terminology used in the current PeerSwap specification and is a necessary change now that my SwapIn* actors participate in both swap-in and swap-out workflows.

remyers · 2022-11-21T08:35:30Z

Closing this PR and opening #2496 which moves this feature into an Eclair Plugin with a cleaner commit history.

remyers added the enhancement label Jul 5, 2022

remyers requested a review from pm47 July 5, 2022 09:54

remyers self-assigned this Jul 5, 2022

sstone reviewed Jul 5, 2022

View reviewed changes

sstone self-requested a review July 5, 2022 11:14

t-bast added the Optech Make Me Famous! label Jul 25, 2022

t-bast reviewed Jul 28, 2022

View reviewed changes

remyers force-pushed the peerswap branch from 8e5651f to 1f14ed1 Compare August 30, 2022 13:58

remyers added 13 commits September 7, 2022 11:16

Add serialization for PeerSwap messages

7678204

Add on-chain transactions for peerswap

ef88c3e

Add shared peerswap helper functions and data structures

584ef48

Add new SwapKeyManager for peerswap swaps

ed0b0f8

Add SwapInSender peerswap workflow actor

559a2da

Add SwapInReceiver peerswap workflow actor

2e17561

Add SwapRegister to forward peerswap messages to swap actors

d7e6c5b

Update Peer actor to forward peerswap messages from peers to the Swap…

522fd3a

…Register Add support for testing swaps

Update Channel actor to forward peerswap messages to Peer

a3dae29

Add SwapInSend/Receive integration test

df69b97

add cli/api for peerswap swapin, status and cancel

dc6ad3c

remyers force-pushed the peerswap branch from 1f14ed1 to 1ef5a99 Compare September 7, 2022 10:36

Add swap-out support and basic tests

d8c41d2

- use only shortChannelId, not channelId - add isInitiator so same SwapData class works for both senders and receivers

remyers force-pushed the peerswap branch from c612e69 to d8c41d2 Compare September 14, 2022 09:29

remyers added 2 commits September 16, 2022 13:08

Change Register to use simple swapId String for key

c07b552

Rename actors to SwapMaker and SwapTaker

00032d6

Add swaps db and update swap actors to use it

1270c07

remyers force-pushed the peerswap branch from 875b5ff to 1270c07 Compare September 30, 2022 15:45

Modify to work with PeerSwap as a plugin

32c2a96

remyers force-pushed the peerswap branch 3 times, most recently from fb4ea5f to 6ca8fa5 Compare October 21, 2022 14:42

Move Peerswap functionality into a plugin

eb91acb

remyers force-pushed the peerswap branch from 6ca8fa5 to eb91acb Compare October 21, 2022 14:54

remyers added 4 commits October 26, 2022 14:27

Create random seed file if seed file not found

d3118f4

Clean up handling/responses for 'listswaps' and 'cancelswap'

c34a722

Add some TODOs to discuss later

4da5074

Cleanup register and prevent multiple swaps per channel

805e602

remyers force-pushed the peerswap branch from ddef11d to 805e602 Compare October 26, 2022 21:39

remyers mentioned this pull request Nov 21, 2022

Add plugin for Peerswap atomic swap based local liquidity management protocol #2496

Closed

8 tasks

remyers closed this Nov 21, 2022

remyers deleted the peerswap branch November 21, 2022 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Peerswap atomic swap based local liquidity management protocol #2342

Add support for Peerswap atomic swap based local liquidity management protocol #2342

remyers commented Jul 5, 2022 •

edited

Loading

sstone Jul 5, 2022

sstone Jul 5, 2022

remyers Nov 23, 2022

sstone Jul 5, 2022

remyers Jul 5, 2022

sstone Jul 5, 2022

remyers Jul 6, 2022 •

edited

Loading

sstone Jul 5, 2022

remyers Jul 5, 2022

t-bast left a comment

t-bast Jul 28, 2022

remyers Aug 30, 2022

t-bast Jul 28, 2022

remyers Sep 14, 2022

t-bast Jul 28, 2022

remyers Sep 14, 2022

t-bast Jul 28, 2022

remyers Dec 7, 2022

t-bast Jul 28, 2022

remyers Dec 13, 2022

t-bast Jul 28, 2022

remyers Dec 13, 2022

t-bast Jul 28, 2022

remyers Dec 14, 2022

t-bast Jul 28, 2022

remyers Dec 13, 2022

remyers commented Aug 30, 2022

codecov-commenter commented Aug 30, 2022

remyers commented Sep 14, 2022 •

edited

Loading

remyers commented Sep 14, 2022 •

edited

Loading

remyers commented Nov 21, 2022


		final case class SwapInSenderData(channelId: ByteVector32, request: SwapInRequest, agreement: SwapInAgreement, invoice: Bolt11Invoice, openingTxBroadcasted: OpeningTxBroadcasted)

		final case class SwapInReceiverData(shortChannelId: ShortChannelId, request: SwapInRequest, agreement: SwapInAgreement, invoice: Bolt11Invoice, openingTxBroadcasted: OpeningTxBroadcasted)


		object SwapData {

		final case class SwapInSenderData(channelId: ByteVector32, request: SwapInRequest, agreement: SwapInAgreement, invoice: Bolt11Invoice, openingTxBroadcasted: OpeningTxBroadcasted)

		case CancelReceived(c) if c.swapId == swapId => swapCanceled(PeerCanceled(swapId))
		case CancelReceived(_) => Behaviors.same

Add support for Peerswap atomic swap based local liquidity management protocol #2342

Add support for Peerswap atomic swap based local liquidity management protocol #2342

Conversation

remyers commented Jul 5, 2022 • 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

remyers Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-bast 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

remyers commented Aug 30, 2022

codecov-commenter commented Aug 30, 2022

Codecov Report

remyers commented Sep 14, 2022 • edited Loading

remyers commented Sep 14, 2022 • edited Loading

remyers commented Nov 21, 2022

remyers commented Jul 5, 2022 •

edited

Loading

remyers Jul 6, 2022 •

edited

Loading

remyers commented Sep 14, 2022 •

edited

Loading

remyers commented Sep 14, 2022 •

edited

Loading