ElementsProject / peerswap

MIT License
110 stars 40 forks source link

Set label #283

Closed YusukeShimizu closed 8 months ago

YusukeShimizu commented 9 months ago

Ensure that all liquid transactions related to peerswaps are identifiable by the label. This makes it easier to audit the transactions. if it fails, it is logged and subsequent processing is continued.

This depends on the following PR. https://github.com/ElementsProject/peerswap/pull/282

wtogami commented 9 months ago

Let's do this ...

  1. For now let's add labels for peerswap LND only. LND already provides the needed RPC commands. Document the limitation that only peerswap LND has onchain labels for now.
  2. We will not add https://github.com/ElementsProject/glightning/pull/12 as this would enable CLN peerswap to label only elementsd. As noted there glightning is already overloaded with too many things which I consider to be a layering violation.
  3. Next CLN peerswap should talk to an optional secondary bitcoind wallet directly like #96 or talk directly to elementsd or LWK to set labels. Do this in another PR later.
YusukeShimizu commented 9 months ago

For now let's add labels for peerswap LND only. LND already provides the needed RPC commands. Document the limitation that only peerswap LND has onchain labels for now.

OK, I will create a PR to document it.

We will not add https://github.com/ElementsProject/glightning/pull/12 as this would enable CLN peerswap to label only elementsd.

This PR change calls the set label of bitcoind even if cln is used. This bitcoind is the same as the one cln depends on.

As noted there glightning is already overloaded with too many things which I consider to be a layering violation.

I agree.

Next CLN peerswap should talk to an optional secondary bitcoind wallet directly like https://github.com/ElementsProject/peerswap/issues/96 or talk directly to elementsd or LWK to set labels. Do this in another PR later.

It would be possible to offer the option to connect to the daemon used by lnd and another daemon. I understood that this should be addressed later.

wtogami commented 9 months ago

We will not add ElementsProject/glightning#12 as this would enable CLN peerswap to label only elementsd.

This PR change calls the set label of bitcoind even if cln is used. This bitcoind is the same as the one cln depends on.

CLN uses bitcoind only to read blocks/transactions. CLN has its own onchain wallet which unfortunately lacks labels entirely.

Requests like #96 have asked peerswap to offer the ability to use a different BTC onchain wallet for those who want to keep it separate from their CLN funds. Even if we add this as an option it would be confusing to have setlabel when it can't work with the default CLN onchain wallet.

That said maybe we should include ElementsProject/glightning#12 limited to elementsd for now since it would work for our default L-BTC wallet. Thoughts @nepet ?

Related question: Does our CLN peerswap currently communicate entirely via CLN RPC or does it also connect to bitcoind and elementsd directly?

wtogami commented 9 months ago

That said maybe we should include https://github.com/ElementsProject/glightning/pull/12 limited to elementsd for now since it would work for our default L-BTC wallet. Thoughts @nepet ?

For now let's go ahead with this part. Please note the CLN bitcoind setlabel is currently not supported due to CLN onchain wallet's lack of labels.

wtogami commented 9 months ago

Related question: Does our CLN peerswap currently communicate entirely via CLN RPC or does it also connect to bitcoind and elementsd directly?

How does this currently work?

YusukeShimizu commented 9 months ago

My understanding is as follows

cln

In the case of cln, peerswap starts as a cln plugin, but peerswap also has its own jsonrpc connection to bitcoind for onchain communication.(config) Thus, we can call SetLabel directly. In this case, the label is attached to the bitcoind that peerswap connects to.

The default is to connect to the same bitcoind as the bitcoind that cln connects to, but it is also possible to set up a different bitcoind.

If liquid is enabled, peerswapd calls the function via elementd jsonrpc. We can call SetLabel also. Thus, it should be possible to label only Elementsd.

graph LR
    subgraph clnplugin
    peerswap --> cln
end
cln --> bitcoind,etc
peerswap ---> elementd
peerswap ---> bitcoind,etc

My understanding is that https://github.com/ElementsProject/peerswap/issues/96 is sorted out by #153 how to set up a jsonrpc connection with bitcoind using config file.

lnd

In lnd, onchain exchange is done using Wallet in lnd's grpc, and connection with bitcoind is not provided. LabelTransaction is a function provided as a lnd wallet.

graph LR
peerswapd --> lnd
peerswapd --> elementd
subgraph lndnode
    lnd --> bitcoind,btcd,neutrino
end

Therefore, for lnd, there is currently no way to set up another onchain wallet, so this is something to consider.

graph LR
classDef color font-style:italic,stroke:yellow

peerswapd --> lnd
peerswapd --> elementd
subgraph lndnode
    lnd --> bitcoind,btcd,neutrino
end
peerswapd --> alternativebitcoindamon:::color
YusukeShimizu commented 9 months ago

When swapout is performed in cln with this PR, the label can be confirmed in bitcoind connected to peerswap as follows.

./bin/clncli peerswap-listswaps
{
   "swaps": [
      {
         "id": "085b1368810005a30610adf6ba00ce01258d2d2605798ec20663feb16023a5b9",
         "created_at": 1708562187,
         "asset": "btc",
         "type": "swap-out",
         "role": "sender",
         "state": "State_ClaimedPreimage",
         "initiator_node_id": "022d6b27d4577dad8ee17466be3e9e4af6e2ac80a600453c28f8be2ae66af8b548",
         "peer_node_id": "02496a469ebf47bce20b7c0ac661f306b84cde3bbf6860d868e117d8ff3f18062c",
         "amount": 1000000,
         "channel_id": "231x1x0",
         "opening_tx_id": "4ae28ab0e547fcf23579059a1689a9f5c91dc97c2c9e804ba0d476171085be7f",
         "claim_tx_id": "d3ceb004d9464e266a181ea4639e4e836820d5ff8fdfe19b7d846c53d46362da",
         "lnd_chan_id": 253987186081792
      }
   ]
}
./bin/bitcoin-cli listlabels
[
  "",
  "peerswap -- ClaimByInvoice(swap id=085b13)"
]
./bin/bitcoin-cli getaddressesbylabel "peerswap -- ClaimByInvoice(swap id=085b13)"
{
  "bcrt1q7y2zlrysa3qjsqz0x4m6jh6w8tun996y3cfzw3": {
    "purpose": "send"
  }
}
wtogami commented 9 months ago

In the case of cln, peerswap starts as a cln plugin, but peerswap also has its own jsonrpc connection to bitcoind for onchain communication. Thus, we can call SetLabel directly. In this case, the label is attached to the bitcoind that peerswap connects to.

The default CLN onchain wallet does not use bitcoind's wallet. CLN works with bitcoind disablewallet=1.

My understanding is that #96 is resolved with #153 for cln.(Cause)

I'm confused how this is possible. #153 was not intended to do anything like #96. If it did so it was an accident. Supporting #96 would require lots of extra work because it would be confusing and difficult to support querying two different bitcoinds.

If CLN peerswap currently can do a swap with a secondary bitcoind wallet that is NOT INTENDED. CLN would not see those UTXO's because it is not within its own onchain wallet. The user would need to manually spend it to make it available to CLN again.

Therefore, for lnd, there is currently no way to set up another onchain wallet, so this is something to consider.

Nobody requested this feature for LND peerswap.

YusukeShimizu commented 9 months ago

I'm confused how this is possible. https://github.com/ElementsProject/peerswap/pull/153 was not intended to do anything like https://github.com/ElementsProject/peerswap/issues/96. If it did so it was an accident. Supporting https://github.com/ElementsProject/peerswap/issues/96 would require lots of extra work because it would be confusing and difficult to support querying two different bitcoinds.

I was confusing the bitcoin daemon with the bitcoin wallet. https://github.com/ElementsProject/peerswap/pull/283#issuecomment-1958464183 is different from cln's wallet, which is labeled bitcoind's wallet.

If CLN peerswap currently can do a swap with a secondary bitcoind wallet that is NOT INTENDED. CLN would not see those UTXO's because it is not within its own onchain wallet. The user would need to manually spend it to make it available to CLN again.

Currently it is not possible to swap with multiple wallets.

Since it is not possible to label the cln wallet, I will try to set the label only on the cln elementd only, as discussed separately.