[r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes #1452

shamardy · 2022-08-26T22:04:33Z

#1045

Use async_blocking for sync rust-lightning functions that calls other I/O functions or that has mutexes that can be held for some time. [r2r] Lightning batch transactions confirmations, spv (wip), refactors #1339 (comment)
Get default fees from rpc when starting lightning instead of config. [r2r] Lightning batch transactions confirmations, spv (wip), refactors #1339 (comment)
Update rust-lightning from v0.0.106 to v0.0.110.
After updating to v0.0.108, the option to enable 0 confirmations channels was added. 3 new RPCs were added to enable this feature (add_trusted_node, remove_trusted_node , list_trusted_nodes). This feature allows immediate forwarding of payments after channel opening which in turn allows writing unit tests for lightning payments. This should help unit testing swaps functions in the future.
Added update_channel RPC that updates a channel that is open without closing it. The P2P messages required for this were added to rust-lightning v0.0.110.
Moved the lightning Background processor to mm2 context so that the latest state can be persisted to the filesystem on exiting mm2, since the background can be dropped on exiting now (when calling the “stop” RPC). This problem was detected due to updating to rust-lightning v0.0.110 since now the network graph only persists on exit.
BTC headers can now be validated using the difficulty calculations.
Made block headers storage non-optional. [r2r] Lightning batch transactions confirmations, spv (wip), refactors #1339 (comment)
Fixed problems related to BTC block headers deserialization since there are some headers that use version 4, KAWPOW_VERSION.
Added spv and block headers synchronization to enable utxo v2. They are not supported in v1 since block headers should be added to the database on enabling the coin which takes too much time for enable v1.
Simple payment verification for transactions now use a validated block header from storage.
There are multiple todo comments left that should be done in the next PR.

…fee is received

…-fixes

…mediately for invoice payments that we don't have a preimage for

…PC, finally can send payments in unit tests (due to 0 confs channels

…Invoice serialization/deserialization is supported

…-fixes

…en calling stop RPC, to persist the latest states on exit

… refactors

…hat block headers validation work for the complete BTC chain

…-fixes

borngraced

Really great work! 🔥
I have only 2 non blockers

borngraced · 2022-08-27T03:53:02Z

mm2src/coins/utxo/utxo_block_header_storage.rs

+        Ok(BlockHeaderStorage {
+            inner: Box::new(SqliteBlockHeadersStorage {
+                ticker,
+                conn: sqlite_connection,


I guess this can also be rewritten as

let conn = Arc::new(Mutex::new(Connection::open_in_memory().unwrap())); let conn = ctx.sqlite_connection.clone_or(conn); Ok(BlockHeaderStorage { inner: Box::new(SqliteBlockHeadersStorage { ticker, conn, }),

borngraced · 2022-08-27T04:16:00Z

mm2src/mm2_bitcoin/serialization/src/reader.rs

+        // "LBC", "LBC-segwit", etc..
+        CoinVariant::LBC
+    } else {
+        CoinVariant::Standard


what about replacing coin_variant_by_ticker with this? to avoid having boring if/else statements

fn coin_variant_by_ticker(ticker: &str) -> CoinVariant { // "BTC", "BTC-segwit", "tBTC", "tBTC-segwit", etc.. let btc_ticker = ticker == "BTC" || ticker.contains("BTC-") || ticker.contains("BTC_"); // "LBC", "LBC-segwit", etc.. let lbc_ticker = ticker == "LBC" || ticker.contains("LBC-") || ticker.contains("LBC_"); match (btc_ticker, lbc_ticker) { (btc @ true, _) => CoinVariant::BTC, (_, lbc @ true) => CoinVariant::LBC, _ => CoinVariant::Standard, } }

until at least when this issue is resolved #1345

I think the below is even a better way of doing this :)

pub fn coin_variant_by_ticker(ticker: &str) -> CoinVariant { match ticker { // "BTC", "BTC-segwit", "tBTC", "tBTC-segwit", etc.. t if t == "BTC" || t.contains("BTC-") || t.contains("BTC_") => CoinVariant::BTC, // "LBC", "LBC-segwit", etc.. t if t == "LBC" || t.contains("LBC-") || t.contains("LBC_") => CoinVariant::LBC, _ => CoinVariant::Standard, } }

I will also remove coin_variant_by_ticker in favor of impl From<&str> for CoinVariant

onur-ozkan

Appreciated the work! Here is my first review notes :)

mm2src/coins/lightning/ln_conf.rs

onur-ozkan · 2022-08-29T11:07:21Z

mm2src/coins/lightning.rs

+}
+
+/// Updates configuration for an open channel.
+pub async fn update_channel(ctx: MmArc, req: UpdateChannelReq) -> UpdateChannelResult<String> {


What about providing proper result type instead of plain text? Or maybe even bool type can be nice.

onur-ozkan · 2022-08-29T11:08:19Z

mm2src/coins/lightning.rs

+    pub node_id: PublicKeyForRPC,
+}
+
+pub async fn add_trusted_node(ctx: MmArc, req: AddTrustedNodeReq) -> TrustedNodeResult<String> {


Same here. (About plain text result types)

onur-ozkan · 2022-08-29T11:09:08Z

mm2src/coins/lightning.rs

+    pub node_id: PublicKeyForRPC,
+}
+
+pub async fn remove_trusted_node(ctx: MmArc, req: RemoveTrustedNodeReq) -> TrustedNodeResult<String> {


Same here. (About plain text result types)

onur-ozkan · 2022-08-29T11:15:33Z

mm2src/coins/lightning.rs

+    let coin = lp_coinfind_or_err(&ctx, &req.coin).await?;
+    let ln_coin = match coin {
+        MmCoinEnum::LightningCoin(c) => c,
+        _ => return MmError::err(TrustedNodeError::UnsupportedCoin(coin.ticker().to_string())),
+    };


Since lp_coinfind_or_err always returns MmCoinEnum instance, we don't need to keep coin variable.

So we can shorten this like:

let ln_coin = match lp_coinfind_or_err(&ctx, &req.coin).await? { MmCoinEnum::LightningCoin(c) => c, e => return MmError::err(TrustedNodeError::UnsupportedCoin(e.ticker().to_string())), };

sergeyboyko0791

Huge progress!
First review iteration

Cargo.lock

mm2src/coins/Cargo.toml

sergeyboyko0791 · 2022-08-31T09:32:08Z

mm2src/coins/lightning.rs

@@ -919,6 +957,52 @@ pub async fn open_channel(ctx: MmArc, req: OpenChannelRequest) -> OpenChannelRes
    })
 }

+#[derive(Deserialize)]
+pub struct UpdateChannelReq {


Not critical.
I think all RPCs could be moved to coins/rpc_command/ directory or a new module like coins/rpc_command/lightning.

This can be done in a separate refactor PR maybe. Moving all the lightning RPCs now will add lots of changes to this PR that are not really changes but might confuse the reviewers a bit.

Added this comment to this checklist to not forget

mm2src/coins/lightning.rs

mm2src/coins/lightning/ln_filesystem_persister.rs

mm2src/coins/lightning/ln_platform.rs

onur-ozkan

Next review iteration :)

onur-ozkan · 2022-09-01T06:28:07Z

mm2src/coins/lightning.rs

-            .list_channels()
+    async fn list_channels(&self) -> Vec<ChannelDetails> {
+        let channel_manager = self.channel_manager.clone();
+        async_blocking(move || channel_manager.list_channels()).await


Just a question, does list_channels really require thread spawning? I am not sure how long it takes to finish. If the operaton is not a long-running task, we should avoid context switching and spawning threads which impacts the performance badly.

This is all due to the channel_state Mutex
https://github.com/lightningdevkit/rust-lightning/blob/00f08c910ff87fecc580750df33d3ebf5ac1fceb/lightning/src/ln/channelmanager.rs#L702

Since list_channels uses list_channels_with_filter which waits to acquire the lock here https://github.com/lightningdevkit/rust-lightning/blob/00f08c910ff87fecc580750df33d3ebf5ac1fceb/lightning/src/ln/channelmanager.rs#L1743

And I believe, this can take some time, as this lock is locked everywhere when dealing with channels in rust-lightning, so it should require using async_blocking here. An example for a function that acquires the lock for sometime is internal_funding_signed
https://github.com/lightningdevkit/rust-lightning/blob/00f08c910ff87fecc580750df33d3ebf5ac1fceb/lightning/src/ln/channelmanager.rs#L4668 https://github.com/lightningdevkit/rust-lightning/blob/00f08c910ff87fecc580750df33d3ebf5ac1fceb/lightning/src/ln/channelmanager.rs#L4679 https://github.com/lightningdevkit/rust-lightning/blob/00f08c910ff87fecc580750df33d3ebf5ac1fceb/lightning/src/chain/chainmonitor.rs#L584
Where persist_new_channel saves the new monitor to the filesystem.

It's not always the case that list_channels will wait for an I/O process to complete to acquire the lock, I even think that this happens rarely, but the frequency with which this happens should increase with the number of channels open.

onur-ozkan · 2022-09-01T07:17:32Z

mm2src/coins/lightning.rs

+    let coin = lp_coinfind_or_err(&ctx, &req.coin).await?;
+    let ln_coin = match coin {
+        MmCoinEnum::LightningCoin(c) => c,
+        _ => return MmError::err(UpdateChannelError::UnsupportedCoin(coin.ticker().to_string())),
+    };


You can apply this update here and other cases in this module. I see this block used in many functions.

onur-ozkan · 2022-09-01T07:23:54Z

mm2src/coins/lightning.rs

+        if channel_options != req.channel_options {
+            channel_options.update(req.channel_options.clone());
+        }
+        let channel_ids = vec![req.channel_id.0];


This doesn't need to be vector. Can simply be slice like &[req.channel_id.0].

onur-ozkan · 2022-09-01T07:43:37Z

mm2src/coins/lightning/ln_errors.rs

+    fn status_code(&self) -> StatusCode {
+        match self {
+            UpdateChannelError::UnsupportedCoin(_) => StatusCode::BAD_REQUEST,
+            UpdateChannelError::NoSuchCoin(_) => StatusCode::PRECONDITION_REQUIRED,


We can either return Bad Request or Not Found for UpdateChannelError::NoSuchCoin. 428 Status Code(Precondition Required) is related with conditional requests. See the details.

I see, thanks for the info :)
I think Not Found is a good fit for this then

onur-ozkan · 2022-09-01T07:45:46Z

mm2src/coins/lightning/ln_errors.rs

+    fn status_code(&self) -> StatusCode {
+        match self {
+            TrustedNodeError::UnsupportedCoin(_) => StatusCode::BAD_REQUEST,
+            TrustedNodeError::NoSuchCoin(_) => StatusCode::PRECONDITION_REQUIRED,


onur-ozkan · 2022-09-01T07:58:26Z

mm2src/coins/lightning/ln_utils.rs

+        let chain_monitor_for_args = chain_monitor.clone();
+
+        let (channel_manager_blockhash, channel_manager, channelmonitors) = async_blocking(move || {
+            let mut manager_file = match File::open(persister.manager_path()) {


Why not just File::open(persister.manager_path())? :)

onur-ozkan · 2022-09-01T08:19:14Z

mm2src/coins/utxo/rpc_clients.rs

+                        let mut reader =
+                            Reader::new_with_coin_variant(serialized.as_slice(), coin_name.as_str().into());
                        let maybe_block_headers = reader.read_list::<BlockHeader>();


A note, there is a macro called drop_mutability in common module. You can use it on such cases like this to prevent invalid data assignment to the mutable addresses.

For example:

let mut x = 10; . . x = 1; drop_mutability(x); // Since the variable no longer needs mutability . . . . . Ok(())

shamardy · 2022-09-02T20:01:40Z

@sergeyboyko0791 @ozkanonur this is ready for another review iteration.

sergeyboyko0791

Thank you for the fixes!
Next review iteration mainly consists of suggestions and questions.

mm2src/coins/lightning/ln_filesystem_persister.rs

mm2src/coins/lightning/ln_platform.rs

mm2src/coins/utxo/utxo_builder/utxo_arc_builder.rs

mm2src/coins_activation/src/utxo_activation/init_utxo_standard_activation.rs

…cher

onur-ozkan

Just minor notes/suggestions 🙂

onur-ozkan · 2022-09-07T12:19:07Z

mm2src/coins/utxo/utxo_builder/utxo_conf_builder.rs

@@ -288,4 +291,8 @@ impl<'a> UtxoConfBuilder<'a> {
    }

    fn enable_spv_proof(&self) -> bool { self.conf["enable_spv_proof"].as_bool().unwrap_or(false) }
+
+    fn block_headers_verification_params(&self) -> Option<BlockHeaderVerificationParams> {
+        json::from_value(self.conf["block_headers_verification_params"].clone()).unwrap_or(None)


How about using unwrap_or_default() instead of unwrap_or. I think unwrap_or more like if you wan't to apply non-default values. If you agree, could you please replace all of them in this module?

I like to use unwrap_or even for default values for readability purposes, it's also how it's implemented for all the other confs.

onur-ozkan · 2022-09-07T12:22:30Z

mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs

+        Option<UtxoSyncStatusLoopHandle>,
+        Option<AsyncMutex<AsyncReceiver<UtxoSyncStatus>>>,
+    ) {
+        if self.conf()["enable_spv_proof"].as_bool().unwrap_or(false) && !self.activation_params().mode.is_native() {


Same here (about unwrap_or_default())

#1452 (comment)

mm2src/mm2_main/src/mm2_tests.rs

artemii235 · 2022-09-12T06:57:36Z

@sergeyboyko0791 It's [r2r] for 6 days, please complete the review. Ideally, we should merge everything today.

artemii235 · 2022-09-12T07:23:39Z

@shamardy There are also git conflicts that appeared after merging other PRs.

…-fixes

sergeyboyko0791

LGTM!
One suggestion for the future not to forget.

sergeyboyko0791 · 2022-09-12T11:23:51Z

mm2src/coins_activation/src/utxo_activation/init_utxo_standard_activation.rs

            UtxoStandardCoin::from,
        )
        .build()
        .await
        .mm_err(|e| InitUtxoStandardError::from_build_err(e, ticker.clone()))?;

-        if let Some(mut sync_watcher) = maybe_sync_watcher {
+        if let Some(sync_watcher_mutex) = &coin.as_ref().block_headers_status_watcher {


At one of the upcoming PRs we should move this into a common function that will be used for other coins. For example, we might be interesting to use block-headers storage for QTUM coin and even probably for BCH

I agree. Added this comment to the checklist #1045 (comment) to not forget.

shamardy added 30 commits July 28, 2022 14:43

use async_blocking wip

d06391d

use async_blocking where it's needed completed

b74f4de

get default fee from rpc when starting lightning + update when a new …

a51e1df

…fee is received

wip: upgrade rust-lightning to v0.0.108

14d37d7

Merge remote-tracking branch 'origin/dev' into lightning-wallet-final…

cce603c

…-fixes

fix wasm compilation

ab5887b

wip: upgrade rust-lightning to v0.0.108, update persister code

0d80394

Merge remote-tracking branch 'origin/dev' into lightning-wallet-final…

8ef776f

…-fixes

fix clippy and udep errors

a3974eb

Implement trusted nodes for inbound 0-conf channels, release htlcs im…

6e66860

…mediately for invoice payments that we don't have a preimage for

Update to V0.0.108 complete, added more features: list_trusted_node R…

4ffe1eb

…PC, finally can send payments in unit tests (due to 0 confs channels

fix wasm compilation

b806a43

update rust-lightning to v0.0.110 wip

37efa37

update rust-lightning to v0.0.110 wip, remove InvoiceForRPC now that …

c366a33

…Invoice serialization/deserialization is supported

Merge remote-tracking branch 'origin/dev' into lightning-wallet-final…

ac0a87b

…-fixes

update rust-lightning to v0.0.110 wip: refactor some code due to update

5552dda

update rust-lightning to v0.0.110 complete: add update_channel RPC

2dfb499

move lightning background processor to MmCtx, so it can be dropped wh…

00d2af2

…en calling stop RPC, to persist the latest states on exit

validating headers WIP

eff1370

headers validation wip: make block headers storage non-optional

165d6a2

fix validate spv proof to use a validated header from storage + other…

e85eddf

… refactors

get_last_block_height from storage return 0 if db is empty

e0d6738

wip: add block headers synchronization to utxo activation v2

858c8fd

fix tests

59d1d91

wip: add block headers synchronization to utxo activation v2 + test t…

2a95fa3

…hat block headers validation work for the complete BTC chain

Merge remote-tracking branch 'origin/dev' into lightning-wallet-final…

f9c1274

…-fixes

fix wasm

c3aa951

get confirmed lightning tx from rpc if spv is not enabled

c6efe5a

fix todo comments

9283e01

fix typo

6d6ab4e

shamardy requested review from onur-ozkan and sergeyboyko0791 August 26, 2022 22:20

borngraced previously approved these changes Aug 27, 2022

View reviewed changes

onur-ozkan requested changes Aug 29, 2022

View reviewed changes

First review fixes

36fe8e5

shamardy dismissed borngraced’s stale review via 36fe8e5 August 30, 2022 21:17

fix check tests

a7e95e9

sergeyboyko0791 suggested changes Aug 31, 2022

View reviewed changes

onur-ozkan requested changes Sep 1, 2022

View reviewed changes

second review fixes wip

ded9078

shamardy changed the title ~~[r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes~~ [wip] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes Sep 2, 2022

shamardy added 2 commits September 2, 2022 20:41

second review fixes wip: fix default fees

64efb72

second review fixes complete

f092279

shamardy changed the title ~~[wip] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes~~ [r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes Sep 2, 2022

sergeyboyko0791 reviewed Sep 5, 2022

View reviewed changes

shamardy mentioned this pull request Sep 5, 2022

Lightning integration #1045

Open

24 tasks

shamardy changed the title ~~[r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes~~ [wip] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes Sep 5, 2022

shamardy added 2 commits September 6, 2022 00:29

review fixes wip: invalid_data_err, fix ordering

632401c

review fixes: block_headers_status_notifier, block_headers_status_wat…

83d34f3

…cher

shamardy changed the title ~~[wip] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes~~ [r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes Sep 6, 2022

onur-ozkan reviewed Sep 7, 2022

View reviewed changes

Merge remote-tracking branch 'origin/dev' into lightning-wallet-final…

87a223e

…-fixes

sergeyboyko0791 approved these changes Sep 12, 2022

View reviewed changes

artemii235 merged commit 1519658 into dev Sep 12, 2022

artemii235 deleted the lightning-wallet-final-fixes branch September 12, 2022 12:27

shamardy mentioned this pull request Oct 11, 2022

[r2r] Lightning taker swap p.o.c #1497

Merged

shamardy mentioned this pull request Feb 17, 2023

[release] 1.0.0-beta #1576

Merged

[r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes #1452

[r2r] Update rust-lightning to latest version, 0_confs channels, complete btc spv, multiple fixes #1452

Conversation

shamardy commented Aug 26, 2022 • edited Loading

borngraced left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced Aug 27, 2022 • edited Loading

Choose a reason for hiding this comment

shamardy Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

onur-ozkan 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

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

shamardy Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

onur-ozkan 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

shamardy commented Sep 2, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

onur-ozkan 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 commented Sep 12, 2022

artemii235 commented Sep 12, 2022

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

shamardy commented Aug 26, 2022 •

edited

Loading

borngraced Aug 27, 2022 •

edited

Loading

shamardy Aug 30, 2022 •

edited

Loading

shamardy Sep 5, 2022 •

edited

Loading

shamardy Sep 12, 2022 •

edited

Loading