Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set label #283

Merged
merged 2 commits into from
Mar 5, 2024
Merged

Set label #283

merged 2 commits into from
Mar 5, 2024

Conversation

YusukeShimizu
Copy link
Contributor

@YusukeShimizu YusukeShimizu commented Feb 13, 2024

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.
#282

go.mod Outdated
@@ -151,3 +151,5 @@ require (

// This fork contains some options we need to reconnect to lnd.
replace github.com/grpc-ecosystem/go-grpc-middleware => github.com/nepet/go-grpc-middleware v1.3.1-0.20220824133300-340e95267339

replace github.com/elementsproject/glightning => github.com/yusukeshimizu/glightning v0.0.0-20240213204501-f17161a057dc
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[todo]remove this once glightning is updated

@wtogami
Copy link
Contributor

wtogami commented Feb 20, 2024

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 add SetLabel rpc glightning#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
Copy link
Contributor Author

YusukeShimizu commented Feb 20, 2024

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 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.

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 #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
Copy link
Contributor

wtogami commented Feb 20, 2024

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
Copy link
Contributor

wtogami commented Feb 21, 2024

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 ?

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
Copy link
Contributor

wtogami commented Feb 21, 2024

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
Copy link
Contributor Author

YusukeShimizu commented Feb 22, 2024

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
Loading

My understanding is that #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
Loading

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
Loading

@YusukeShimizu
Copy link
Contributor Author

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
Copy link
Contributor

wtogami commented Feb 22, 2024

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
Copy link
Contributor Author

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.

I was confusing the bitcoin daemon with the bitcoin wallet.
#283 (comment) 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.

Ensure that all cln/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.
Add RpcPasswordFile field to configuration for Bitcoin RPC connection.
If no user name or passphrase is set, obtained from a cookie.
If bitcoind or elementsd had restarted then their
respective cookies would have changed.

Since it is not possible to label the cln wallet,
 set the label only on the cln elementd only.
@YusukeShimizu YusukeShimizu marked this pull request as ready for review February 24, 2024 08:49
@wtogami wtogami merged commit f3a6489 into ElementsProject:master Mar 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants