[r2r] Lightning channels and payments history by shamardy · Pull Request #1240 · GLEECBTC/komodo-defi-framework

shamardy · 2022-03-31T13:42:52Z

Fixed Lightning Payments PR post-merge comments Lightning payments #1210 (review)
Added SqlStorage trait to lightning persister
Implemented the Persisting of payments and channels history across node restarts
Added pagination and filtration to RPCs for listing channels and payments
Updated rust-lightning to the latest version

#1045

…mp id

…event

… response

…o sql in the future

artemii235

Few changes requests and questions 🙂

mm2src/coins/lightning.rs

mm2src/coins/lightning/ln_platform.rs

artemii235 · 2022-04-18T07:28:07Z

mm2src/coins/lightning/ln_platform.rs

+                    .any(|info| info.txid == tx_info.tx.txid())
+                {
+                    let rpc_txid = H256Json::from(tx_info.tx.txid().as_hash().into_inner()).reversed();
+                    let index = ok_or_retry_after_sleep!(


It seems that it's worth creating RetryableFuture instead of using macro. So it will look like

client .blockchain_transaction_get_merkle(rpc_txid, tx_info.block_height) .compat() .retry_every(TRY_LOOP_INTERVAL) .await;

I think this can be moved to the next PR as I mentioned here #1240 (comment) since this might be part of the code that will be changed. If ok_or_retry_after_sleep is to be used I will look into creating RetryableFuture instead for sure :)

Ok, I added it to the issue checklist #1045. Please also update it if something is done or not actual anymore, etc.

Please also update it if something is done or not actual anymore, etc.

Sure, I always do that, especially after a related PR merge to dev. For unresolved comments like this, I usually just reference them in the next PR that resolves them as I did here #1240 (comment).

mm2src/coins/lightning_persister/src/lib.rs

sergeyboyko0791

Thanks for the huge fixes!
Second review iteration 😅

mm2src/coins/lightning.rs

sergeyboyko0791 · 2022-04-18T13:37:58Z

mm2src/coins/lightning/ln_events.rs

+            let claiming_txid = spending_tx.txid().to_string();
+            let persister = self.persister.clone();
+            spawn(async move {
+                ok_or_retry_after_sleep!(


Should this macro be used elsewhere in ln_events.rs? Am I right this is only one critical section if you decided to use the macro?

Am I right this is only one critical section if you decided to use the macro?

This is related to this comment #1240 (comment)

sergeyboyko0791 · 2022-04-18T14:40:20Z

mm2src/coins/lightning/ln_platform.rs

+        let mut confirmed_registered_txs = Vec::new();
+        for (txid, scripts) in registered_txs {
+            if let Some(transaction) =
+                ok_or_continue_after_sleep!(self.get_tx_if_onchain(txid).await, TRY_LOOP_INTERVAL)


Is it needed to sleep before continue? If I'm not mistaken, in this case we sleep for TRY_LOOP_INTERVAL and just skip this transaction.

My thought process was that if there was an error it would mostly be because there is a problem connecting to electrums (internet connection down, electrums down, etc..), and waiting a bit might be better than continuing before reconnecting.

If we didn't wait, this might lead to passing through all the registered_txs transactions in this loop without confirming any and waiting for the next block to confirm it, I actually encountered this case while testing when my internet went down for a few minutes.

I realize now that there should be a big change in the confirmation process of registered_txs and the watching for spending of registered_outputs and that I should revise this whole process. I have a few ideas to solve this problem but I think this should be implemented in another PR.

One of these ideas is once a transaction or an output is registered through the register functions https://github.com/KomodoPlatform/atomicDEX-API/blob/9b3339a7e6e2e49ae98345de6162b4d6c31cbfed/mm2src/coins/lightning/ln_platform.rs#L527 https://github.com/KomodoPlatform/atomicDEX-API/blob/9b3339a7e6e2e49ae98345de6162b4d6c31cbfed/mm2src/coins/lightning/ln_platform.rs#L530 I should just spawn a separate thread for this transaction/output while retrying each step that involves calls to electrums until confirmation. One problem with that approach is this error for instance #1240 (comment) where if a call fails for a reason unrelated to electrum connections, knowing that a certain output is spent might not resolve due to getting stuck in retrying such calls forever. This watched output might be related to an old commitment transaction broadcasted by a channel's malicious counterparty and we will not be able to broadcast a transaction to punish this counterparty for broadcasting an old commitment transaction without detecting the spending of the channel opening transaction by this old commitment transaction, what makes this worse is that there is a window for broadcasting this punishing transaction and if passed some of our funds will be stolen.

These problems can all be solved after watchtowers are implemented since watchtowers should be running a full node and if a call to electrums fails for whatever reason we can delegate the watching for a certain transaction or a spending of an output to a number of watchtowers while providing them with the transactions that should be broadcasted in such cases.

I think the left unresolved comments are related to the above problem and should be resolved in a new PR since this code might be changed. The ok_or_retry_after_sleep was used for instance in here #1240 (comment) to avoid the above situation.

The idea with the spawned thread or just a future sounds good! This will allow us to avoid infinite loop and to be sure that we'll try to broadcast transactions until they're accepted.

Just only one thing - I think that one separated thread is enough for this :D

I should just spawn a separate thread for this transaction/output

Just only one thing - I think that one separated thread is enough for this :D

Big lightning nodes can have a lot of open channels, the most connected nodes have over 2000 open channels with them, which in turn means that they can watch for more than 2000 outputs if they are spent or not with every new block. Of course, these nodes should always use a full node as their chain source and not electrums, but I think it's the right way to at least spawn a thread for multiple transactions at a time but not all so as not to take a long time to finish the requests when a new block connects if a user who uses electrum decided to open a lot of channels.

sergeyboyko0791 · 2022-04-18T14:44:44Z

mm2src/coins/lightning/ln_platform.rs

+                for (_, vout) in transaction.output.iter().enumerate() {
+                    if scripts.contains(&vout.script_pubkey) {
+                        let script_hash = hex::encode(electrum_script_hash(vout.script_pubkey.as_ref()));
+                        let history = ok_or_retry_after_sleep!(


We can never return from this function if scripthash_get_history always fails. One particular case is when the RPC fails with history too large
https://github.com/KomodoPlatform/atomicDEX-API/blob/322dcbdeea76336dc699056b7f4f8f53476793c4/mm2src/coins/utxo/utxo_common.rs#L50-L53

Even if there is a network error, it's worth to return from the function with an error. How about adding a number of attemptions?

I think this should be fixed in the next PR, please refer to #1240 (comment)
Also, I think the history too large error will not be returned from this since the script hashes are mostly for P2WSH addresses which are used once. I guess in the confirmation of the claiming transaction this error might appear since we claim back to our address and the script_pubkey of our address will be used in the scripthash_get_history, I guess I should find a solution for this but as I said this code might change and spawning these calls might be one of the solutions.

Got it! There can be other permanent errors like invalid Electrum state etc.

I think a good solution for electrum errors can be to request electrums sequentially if there is an error returned, so if the user is connected to 3 electrums if an error is returned from one then the request should be retried to the next electrum in the list. what do you think?

I think a good solution for electrum errors can be to request electrums sequentially

Actually, all electrum requests are sequential through the rpc_func! macro, I guess I just forgot that :)

sergeyboyko0791 · 2022-04-18T14:45:28Z

mm2src/coins/lightning/ln_platform.rs

+                            if item.tx_hash == rpc_txid && item.height > 0 {
+                                let height = item.height as u64;
+                                let header =
+                                    ok_or_retry_after_sleep!(get_block_header(client, height).await, TRY_LOOP_INTERVAL);


I think this should be fixed in the next PR, please refer to #1240 (comment)

mm2src/coins/lightning_persister/src/lib.rs

sergeyboyko0791 · 2022-04-18T16:31:55Z

mm2src/coins/lightning_persister/src/lib.rs

+            destination: row
+                .get::<_, String>(1)
+                .ok()
+                .map(|d| PublicKey::from_str(&d).expect("PublicKey from str should not fail!")),


It can fail easily if the user change this field in the database 😄
Could you please make sure if it's safe to unwrap this and other lines in this function/file?

sergeyboyko0791 · 2022-04-18T16:59:48Z

mm2src/coins/lightning_persister/src/lib.rs

+
+    let sql = "UPDATE ".to_owned()
+        + &table_name
+        + " SET funding_tx = ?2, funding_value = ?3, funding_generated_in_block = ?4, last_updated = ?5 WHERE rpc_id = ?1;";


I think such order may help and confuse the user at the same time.
How about ordering the query params or adding a # Usage comment about the order?
Same question for other places in the file.

mm2src/coins/lightning_persister/src/storage.rs

sergeyboyko0791 · 2022-04-19T06:59:13Z

mm2src/coins/sql_tx_history_storage.rs

        async_blocking(move || {
            let conn = selfi.0.lock().unwrap();
-            query_single_row(&conn, &sql, params, tx_details_from_row)
+            query_single_row(&conn, &sql, params, tx_details_from_row).map_err(From::from)


Not critical at all, but I prefer to specify the target type to make the code clearer

query_single_row(&conn, &sql, params, tx_details_from_row).map_err(SqlError::from)

onur-ozkan

All looks good to me, just couple of minor suggestions. 🙂

(please also consider this suggestion from Artem's PR.)

mm2src/coins/lightning/ln_p2p.rs

mm2src/coins/lightning_persister/src/lib.rs

artemii235

🔥

sergeyboyko0791

Last review iteration.
There are no critical comments, so If it will take some time to implement, I think they can be postponed to the next impl iteration.

sergeyboyko0791 · 2022-04-26T13:19:37Z

mm2src/coins/lightning_persister/src/lib.rs

        PaymentType::OutboundPayment {
-            destination: PublicKey::from_str(&row.get::<_, String>(1)?).expect("PublicKey from str should not fail!"),
+            destination: PublicKey::from_str(&row.get::<_, String>(1)?)
+                .map_err(|e| SqlError::FromSqlConversionFailure(1, SqlType::Text, Box::new(e)))?,


Creating such error is repeating in this file. I have several ideas how it can be simplified:

Return a custom error type:

enum MmSqlError { ErrorParsing { // Both or one of these: field_name: String, field_id: usize, error: String, } } impl LnStorageError { pub fn error_parsing<E>(field_name: String, field_id: usize, error: E) -> Self where E: Error { MmSqlError::ErrorParsing {field_name, field_id, error: error.to_string()} } }

Use a function like:

pub fn sql_text_conversion_err<E>(field_id: usize, e: E) -> SqlError { SqlError::FromSqlConversionFailure(field_id, SqlType::Text, Box::new(e)) }

What do you think?

pub fn sql_text_conversion_err<E>(field_id: usize, e: E) -> SqlError { SqlError::FromSqlConversionFailure(field_id, SqlType::Text, Box::new(e)) }

I prefer this idea since this function can be used easily for any such error. I will add it to our db_common lib.

sergeyboyko0791 · 2022-04-26T13:29:00Z

mm2src/coins/lightning_persister/src/lib.rs

+    let mut hash_slice = [0u8; 32];
+    hex::decode_to_slice(row.get::<_, String>(0)?, &mut hash_slice as &mut [u8])
+        .map_err(|e| SqlError::FromSqlConversionFailure(0, SqlType::Text, Box::new(e)))?;
+    let payment_hash = PaymentHash(hash_slice);


Do you think this code snippet will be used elsewhere? How about adding a separate function like fn payment_hash_from_row(row, column_id)?
Same question for the below fields :)

artemii235

Reapproving

sergeyboyko0791

Thank you for the fixes! Approve!

shamardy added 30 commits March 1, 2022 14:30

WIP: Channels history in SQL storage

94feb06

review fixes: specific types, add serialization tests

8fa6bde

WIP: refactor start_lightning fn

16b80bc

WIP: continue refactoring start_lightning fn

4f432a3

WIP: refactoring, add init_channel_manager fn

eb62785

WIP: refactoring, ln_p2p

88009fa

WIP: refactoring, ln_platform

b13d64f

Refactor completed

1ff8f86

WIP: use user_channel_id as channel id in rpc calls to avoid using te…

ccd53a6

…mp id

Channels in sql WIP: add pending outbound channel to sql

628921e

Channels sql WIP: get channel and add funding_tx

35667fa

Channels sql WIP: add funding tx to sql after FundingGenerationReady …

5069239

…event

Channels sql WIP: add closure_reason to sql table

15047e0

Channels sql WIP: update channel to closed in sql

4378f7d

Channels sql WIP: get closed channels in list_channels response

fdc54f7

Channels sql WIP: get closed channel by rpc_id in get_channel_details…

4c6b479

… response

Channels sql WIP: add closing transaction to sql

be882bf

outbound channels history in sql completed

ab8dc45

Payments in sql WIP: add/get payment to/from sql

5eb9306

Payments in sql completed

61c9493

Merge remote-tracking branch 'origin/dev' into channels-payments-history

1e60810

update rust-lightning to v0.0.105

9936e25

set manually_accept_inbound_channels to true to be able to add them t…

7bd7b92

…o sql in the future

correct calculation of claimed balances

bf11e7a

add destination to payment info

25fc9e2

add description to payment info

892e2b9

add created/updated at to payment info, wip: list payments by filter

316200a

list_payments_by_filter completed, pagination completed

70bcc06

list channels by filter for open and closed

dc54493

Merge remote-tracking branch 'origin/dev' into channels-payments-history

3b8b3f3

shamardy added 2 commits April 11, 2022 15:02

review fixes wip: default_fee_per_kb, refactors

c5b4c90

review fixes: use try_loop_with_sleep macro wip

55d2ab6

shamardy mentioned this pull request Apr 12, 2022

[r2r] broadcast tx to txhlp / refactor tx errors #1245

Merged

4 tasks

shamardy added 3 commits April 13, 2022 17:21

review fixes

7026db7

Merge remote-tracking branch 'origin/dev' into channels-payments-history

68a70b2

remove wasm warnings

f1001a0

shamardy changed the title ~~Lightning channels and payments history~~ [r2r] Lightning channels and payments history Apr 14, 2022

artemii235 suggested changes Apr 18, 2022

View reviewed changes

shamardy added 3 commits April 18, 2022 15:31

destination can't be None in OutboundPayment

1800112

empty string for description instead of an option

d9e6b6a

Review fixes: refactors

6ccb32f

sergeyboyko0791 suggested changes Apr 19, 2022

View reviewed changes

review fixes wip

21a04d7

shamardy changed the title ~~[r2r] Lightning channels and payments history~~ [wip] Lightning channels and payments history Apr 20, 2022

wip: more review fixes

9b3339a

shamardy changed the title ~~[wip] Lightning channels and payments history~~ [r2r] Lightning channels and payments history Apr 20, 2022

shamardy mentioned this pull request Apr 21, 2022

[r2r] UTXO RPC batch requests #1255

Merged

onur-ozkan reviewed Apr 22, 2022

View reviewed changes

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

mm2src/coins/lightning_persister/src/lib.rs Outdated Show resolved Hide resolved

mm2src/coins/lightning_persister/src/lib.rs Outdated Show resolved Hide resolved

review fixes: inline, refactors

6d9df92

artemii235 mentioned this pull request Apr 25, 2022

Lightning integration #1045

Open

24 tasks

artemii235 previously approved these changes Apr 25, 2022

View reviewed changes

sergeyboyko0791 reviewed Apr 26, 2022

View reviewed changes

sql_text_conversion_err, h256_slice_from_row

88c68ea

shamardy dismissed artemii235’s stale review via 88c68ea April 26, 2022 16:06

Merge remote-tracking branch 'origin/dev' into channels-payments-history

b71a7e4

artemii235 approved these changes Apr 27, 2022

View reviewed changes

sergeyboyko0791 approved these changes Apr 28, 2022

View reviewed changes

artemii235 merged commit 7dace4e into dev Apr 28, 2022

artemii235 deleted the channels-payments-history branch April 28, 2022 11:35

shamardy mentioned this pull request Nov 28, 2022

Consider adding repeatable future PoC #1558

Closed

Conversation

shamardy commented Mar 31, 2022

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergeyboyko0791 Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

sergeyboyko0791 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemii235 left a comment

sergeyboyko0791 Apr 21, 2022 •

edited

Loading