Lightning Network Channels #1133

shamardy · 2021-11-11T14:23:34Z

Part of #1045

Features:

Withdraw to p2wsh addresses
Ability to restart a lightning node and continue operation
Ability to run multiple lightning nodes for different coins (only BTC and tBTC are supported for now)
connect_to_lightning_node RPC
open_channel RPC

artemii235

The first review iteration. I may have more remarks/questions in the future.

mm2src/coins/lightning/ln_utils.rs

mm2src/coins/lightning.rs

mm2src/coins/lightning/ln_rpc.rs

mm2src/coins/lightning/ln_utils.rs

mm2src/coins/lightning.rs

mm2src/coins/lightning/ln_utils.rs

shamardy · 2021-11-29T12:37:31Z

@artemii235 this is not ready for the next review yet. I need to use the correct size and fee here https://github.com/KomodoPlatform/atomicDEX-API/blob/7ff4b28217a2e12a1e9369ae883b7f2e145e9071/mm2src/coins/lightning.rs#L515

artemii235

Few minor questions and refactoring suggestions.

mm2src/coins/lightning/ln_rpc.rs

mm2src/coins/lightning/ln_utils.rs

artemii235 · 2021-11-29T12:30:41Z

mm2src/coins/lightning/ln_utils.rs

-        network: conf.network,
-        best_block: BestBlock::new(BlockHash::from_hash(best_block_hash), best_block.height as u32),
+    let mut restarting_node = true;
+    let rpc_client = match &filter.clone().unwrap().platform_coin.as_ref().rpc_client {


Is unwrap safe here? Please add a comment if yes or return the error otherwise.

What if rpc_client is initialized before platform_coin is moved?

Is unwrap safe here? Please add a comment if yes or return the error otherwise.

filter which implements the Filter trait https://github.com/KomodoPlatform/atomicDEX-API/blob/7ff4b28217a2e12a1e9369ae883b7f2e145e9071/mm2src/coins/lightning/ln_rpc.rs#L138
is only needed for electrum clients that's why I chose to make it Some here https://github.com/KomodoPlatform/atomicDEX-API/blob/7ff4b28217a2e12a1e9369ae883b7f2e145e9071/mm2src/coins/lightning/ln_utils.rs#L247 for now until Native client is implemented then this code will be changed depending on which client is used.

What if rpc_client is initialized before platform_coin is moved?

I chose to use filter in places where the code will be used for electrum clients only in the future as a reminder. Maybe I should add a comment in the code as a reminder also.

mm2src/coins/lightning/ln_utils.rs

artemii235 · 2021-11-29T12:49:40Z

mm2src/coins_activation/src/lightning_activation.rs

+        activation_params: Self::ActivationParams,
+        protocol_conf: Self::ProtocolInfo,
+    ) -> Result<(Self, Self::ActivationResult), MmError<Self::ActivationError>> {
+        let network = network_from_string(protocol_conf.network)?;


This can be moved to the request deserialization stage by adding BitcoinNetwork enum that implements Into<bitcoin::network::constants::Network>.

artemii235 · 2021-11-29T12:52:25Z

mm2src/coins_activation/src/lightning_activation.rs

+        protocol_conf: Self::ProtocolInfo,
+    ) -> Result<(Self, Self::ActivationResult), MmError<Self::ActivationError>> {
+        let network = network_from_string(protocol_conf.network)?;
+        let params = LightningActivationParams::from_activation_req(platform_coin.clone(), activation_params)?;


from_activation_req performs additional validation of the platform config and activation params. I think it will be useful to have validate_platform_configuration and validate_activation_params methods in L2ActivationOps instead.

sergeyboyko0791

The set of comments from me 😅

mm2src/coins/lightning/ln_rpc.rs

mm2src/coins/lightning.rs

mm2src/coins/lightning/ln_utils.rs

sergeyboyko0791 · 2021-11-29T18:53:29Z

mm2src/coins/lightning/ln_utils.rs

+                                if item.height > 0 && item.height <= current_height as i64 {
+                                    let header = match client
+                                        .blockchain_block_header(
+                                            item.height.try_into().expect("Convertion to u64 should not fail"),


I'm afraid that electrum can return a negative height in some implementation.

As I understand the negative height gets returned if the transaction is unconfirmed which is what's needed here. I also used blockchain.scripthash.get_history because this is the only electrum method that returns the height. Please correct me If I am wrong.

We never know what kind of trash some external service may return :D

mm2src/coins/lightning/ln_utils.rs

shamardy · 2021-12-02T19:36:29Z

@artemii235 @sergeyboyko0791 this is ready for the next review iteration

artemii235

I have the last question for now 🙂

mm2src/coins/lightning/ln_utils.rs

artemii235

One question in regards to the latest change.

mm2src/coins/lightning/ln_utils.rs

artemii235

Great! @sergeyboyko0791 Could you please check that all your remarks are resolved too?

shamardy · 2021-12-09T18:15:30Z

@artemii235 @sergeyboyko0791 this is not ready for merge yet if the review fixes are accepted as I found a mistake here after I reviewed my code one more time https://github.com/KomodoPlatform/atomicDEX-API/blob/6ab6a976cf1841a2b88ea66a6025993458dc65e2/mm2src/coins/lightning/ln_utils.rs#L655
This is supposed to be the transaction index in the block, not the vout index. I will add a new electrum method to get the transaction index using the blockchain.transaction.get_merkle RPC method then use it to get the correct index.

artemii235

👍

sergeyboyko0791 · 2021-12-13T17:38:15Z

mm2src/coins/lightning/ln_utils.rs

+        process_tx_for_unconfirmation(txid, filter.clone(), channel_manager.clone()).await;
+    }
+
+    for txid in chain_monitor_relevant_txids {


Not blocking comment: I guess you can use channel_manager_relevant_txids.chain(chain_monitor_relevant_txids) or something like this. Just for the next iteration :)

sergeyboyko0791 · 2021-12-13T17:40:31Z

mm2src/coins/lightning/ln_utils.rs

+}
+
+#[cfg(not(target_arch = "wasm32"))]
+async fn process_txs_confirmations(


This function looks complicated. Could you please refactor it at the next iteration?

sergeyboyko0791 · 2021-12-13T17:43:17Z

mm2src/coins/lightning/ln_utils.rs

+}
+
+#[cfg(not(target_arch = "wasm32"))]
+async fn get_best_header(best_header_listener: &ElectrumClient) -> EnableLightningResult<ElectrumBlockHeader> {


Can this be a part of the ElectrumClient or a trait that is implemented by the client?

sergeyboyko0791 · 2021-12-13T17:49:08Z

mm2src/coins/lightning/ln_utils.rs

+            Err(e) => log::error!("{}", e.to_string()),
+        }
+
+        Timer::sleep(TRY_RECONNECTING_TO_NODE_INTERVAL as f64).await;


If MmArc is stopping, the context may live until this sleep ends. In the worst case, it's 60 seconds.
Please take MmWeak and drop MmArc before the sleep starts.

sergeyboyko0791 · 2021-12-13T18:21:31Z

mm2src/coins/lightning/ln_utils.rs

+                                if item.height > 0 && item.height <= current_height as i64 {
+                                    let header = match client
+                                        .blockchain_block_header(
+                                            item.height.try_into().expect("Convertion to u64 should not fail"),


We never know what kind of trash some external service may return :D

sergeyboyko0791

This is a ton of work 🤪
I leave some non-blocking comments, could you please take them into account?
Only one thing I thing should be fixed: please avoid keeping MmArc during a Timer future is used :) #1162

shamardy added 19 commits October 19, 2021 19:24

WIP restarting node/read channel_manager from file

e7dc46c

WIP: LN Registry for register_tx & register_output

9e5ab02

update transactions confirmations on node restart

bdcd988

reconnect to channel peers on node restart

e13f593

connect to LN node RPC + WIP lightning ctx

3ea3aaf

continued lightning ctx impl for multiple coins

846ecee

open_channel RPC

cef620d

withdraw to p2wsh addresses + funding tx WIP

36691ce

fix zhtlc build

7d6d9dd

extract_destinations for p2wsh

4cf3cd9

generate/broadcast funding tx (channel opening tx)

a0b0751

update rust-lightning to v0.0.102

9202cf3

update rust-lightning to v0.0.103

fce783b

fix reconnection to open channels nodes on restart

dcd8843

save nodes to file fix + order txs to confirm

135db9c

add fee param to open_channel RPC + refactors

424947a

use scripthash_get_history to get tx block height

fde1c1c

remove unneeded comments

e8d92b8

Merge remote-tracking branch 'origin/dev' into ln-channels

9d621c2

shamardy requested review from artemii235, Milerius and sergeyboyko0791 November 11, 2021 14:28

artemii235 requested changes Nov 15, 2021

View reviewed changes

shamardy added 7 commits November 16, 2021 01:13

WIP:Review fixes, ChannelOpenAmount, UtxoTxBuilder

f1fd177

WIP: Review fixes, use get_tx_fee for funding tx

5137f90

remove WithdrawFee, fix unit test

05ad87d

WIP: review fixes, use find_output_spend fn

ab911f0

WIP: review fixes, separate dirs for tickers

b0a2d50

WIP: Review fixes, request_id fix on restart

c0caf10

WIP: review fixes, LightningCoin

80fa54f

shamardy added 3 commits November 25, 2021 20:03

Review fixes: enable_l2, enable_lightning

3694e19

remove lightning from lp_coininit

8ae7ecd

Merge remote-tracking branch 'origin/dev' into ln-channels

31aa280

shamardy mentioned this pull request Nov 29, 2021

Hotfix: Use virtual bytes to calculate correct tx fees for Segwit UTXO generate_transaction #1156

Merged

artemii235 closed this in #1156 Nov 29, 2021

artemii235 reopened this Nov 29, 2021

use virtual bytes to calculate tx fees

7ff4b28

artemii235 reviewed Nov 29, 2021

View reviewed changes

sergeyboyko0791 suggested changes Nov 29, 2021

View reviewed changes

shamardy added 5 commits November 30, 2021 19:28

WIP: more review fixes

3429e11

WIP: review fixes, remove block_on

c35da25

WIP: review fixes, impl EventHandler trait

e8948a4

WIP: add validation methods to L2ActivationOps

adc12a8

Review fixes, add unsigned_funding_txs mutex

794d4b1

artemii235 reviewed Dec 6, 2021

View reviewed changes

mm2src/coins/lightning/ln_utils.rs Show resolved Hide resolved

review fixes: NO_SUCH_TRANSACTION_ERROR

1c8f6ae

artemii235 reviewed Dec 7, 2021

View reviewed changes

mm2src/coins/lightning/ln_utils.rs Outdated Show resolved Hide resolved

use 'code': -5 to check for tx unconfirmation

6ab6a97

artemii235 previously approved these changes Dec 8, 2021

View reviewed changes

add get_merkle method, use tx index to confirm it

4996e6f

shamardy dismissed artemii235’s stale review via 4996e6f December 9, 2021 21:38

artemii235 approved these changes Dec 13, 2021

View reviewed changes

sergeyboyko0791 reviewed Dec 13, 2021

View reviewed changes

artemii235 merged commit 9cd082d into dev Dec 14, 2021

artemii235 deleted the ln-channels branch December 14, 2021 08:21

shamardy mentioned this pull request Apr 11, 2022

[r2r] Lightning channels and payments history #1240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lightning Network Channels #1133

Lightning Network Channels #1133

shamardy commented Nov 11, 2021

artemii235 left a comment

shamardy commented Nov 29, 2021

artemii235 left a comment

artemii235 Nov 29, 2021

sergeyboyko0791 Nov 29, 2021

shamardy Nov 30, 2021

artemii235 Nov 29, 2021

artemii235 Nov 29, 2021 •

edited

Loading

sergeyboyko0791 left a comment

sergeyboyko0791 Nov 29, 2021

shamardy Nov 30, 2021

sergeyboyko0791 Dec 13, 2021

shamardy commented Dec 2, 2021

artemii235 left a comment

artemii235 left a comment

artemii235 left a comment

shamardy commented Dec 9, 2021

artemii235 left a comment

sergeyboyko0791 Dec 13, 2021

sergeyboyko0791 Dec 13, 2021

sergeyboyko0791 Dec 13, 2021 •

edited

Loading

sergeyboyko0791 Dec 13, 2021

sergeyboyko0791 Dec 13, 2021

sergeyboyko0791 left a comment

Lightning Network Channels #1133

Lightning Network Channels #1133

Conversation

shamardy commented Nov 11, 2021

Features:

artemii235 left a comment

Choose a reason for hiding this comment

shamardy commented Nov 29, 2021

artemii235 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

artemii235 Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

sergeyboyko0791 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

shamardy commented Dec 2, 2021

artemii235 left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

shamardy commented Dec 9, 2021

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

artemii235 Nov 29, 2021 •

edited

Loading

sergeyboyko0791 Dec 13, 2021 •

edited

Loading