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

[r2r] Lightning channels and payments history #1240

Merged
merged 54 commits into from
Apr 28, 2022
Merged

Conversation

shamardy
Copy link
Collaborator

  • 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

shamardy added 30 commits March 1, 2022 14:30
@shamardy shamardy changed the title Lightning channels and payments history [r2r] Lightning channels and payments history Apr 14, 2022
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Few changes requests and questions 🙂

mm2src/coins/lightning.rs Outdated Show resolved Hide resolved
mm2src/coins/lightning.rs Outdated Show resolved Hide resolved
mm2src/coins/lightning.rs Show resolved Hide resolved
mm2src/coins/lightning/ln_platform.rs Outdated Show resolved Hide resolved
mm2src/coins/lightning/ln_platform.rs Outdated Show resolved Hide resolved
mm2src/coins/lightning/ln_platform.rs Outdated Show resolved Hide resolved
.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!(
Copy link
Member

Choose a reason for hiding this comment

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

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;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Thanks for the huge fixes!
Second review iteration 😅

mm2src/coins/lightning.rs Show resolved Hide resolved
let claiming_txid = spending_tx.txid().to_string();
let persister = self.persister.clone();
spawn(async move {
ok_or_retry_after_sleep!(

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

This is related to this comment #1240 (comment)

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)

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link

@sergeyboyko0791 sergeyboyko0791 Apr 21, 2022

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

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!(

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 :)

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);

Choose a reason for hiding this comment

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

Same here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

mm2src/coins/lightning_persister/src/lib.rs Outdated Show resolved Hide resolved
destination: row
.get::<_, String>(1)
.ok()
.map(|d| PublicKey::from_str(&d).expect("PublicKey from str should not fail!")),

Choose a reason for hiding this comment

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

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?


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;";

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@@ -315,7 +301,7 @@ impl TxHistoryStorage for SqliteTxHistoryStorage {

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)

Choose a reason for hiding this comment

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

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)

@shamardy shamardy changed the title [r2r] Lightning channels and payments history [wip] Lightning channels and payments history Apr 20, 2022
@shamardy shamardy changed the title [wip] Lightning channels and payments history [r2r] Lightning channels and payments history Apr 20, 2022
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

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 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
@artemii235 artemii235 mentioned this pull request Apr 25, 2022
24 tasks
artemii235
artemii235 previously approved these changes Apr 25, 2022
Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

🔥

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

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.

@@ -278,38 +279,48 @@ fn payment_info_from_row(row: &Row<'_>) -> Result<PaymentInfo, SqlError> {
let is_outbound = row.get::<_, bool>(8)?;
let payment_type = if is_outbound {
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)))?,

Choose a reason for hiding this comment

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

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

  1. 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()}
  }
}
  1. 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Comment on lines 289 to 292
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);

Choose a reason for hiding this comment

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

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 :)

Copy link
Member

@artemii235 artemii235 left a comment

Choose a reason for hiding this comment

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

Reapproving

Copy link

@sergeyboyko0791 sergeyboyko0791 left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes! Approve!

@artemii235 artemii235 merged commit 7dace4e into dev Apr 28, 2022
@artemii235 artemii235 deleted the channels-payments-history branch April 28, 2022 11:35
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.

5 participants