From 6862ab8c7f974f7c433a3c1e241a293cb35339ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Granh=C3=A3o?= Date: Wed, 28 Jun 2023 12:34:40 +0100 Subject: [PATCH 1/2] Persist and expose payment fail reason --- eel/src/data_store.rs | 43 +++++++++++++++++++++++++++++----- eel/src/errors.rs | 19 ++++++++++++++- eel/src/event_handler.rs | 7 ++++-- eel/src/lib.rs | 45 ++++++++++++++++++++++-------------- eel/src/migrations.rs | 11 +++++++++ eel/src/payment.rs | 2 ++ eel/tests/data_store_test.rs | 2 ++ examples/3l-node/cli.rs | 4 ++++ src/lib.rs | 2 ++ src/lipalightninglib.udl | 3 +++ 10 files changed, 112 insertions(+), 26 deletions(-) diff --git a/eel/src/data_store.rs b/eel/src/data_store.rs index 49aa3208..f3ef0c4d 100644 --- a/eel/src/data_store.rs +++ b/eel/src/data_store.rs @@ -1,5 +1,5 @@ use crate::config::TzConfig; -use crate::errors::{Error, Result}; +use crate::errors::{Error, PayErrorCode, Result}; use crate::interfaces::ExchangeRate; use crate::migrations::get_migrations; use crate::payment::{Payment, PaymentState, PaymentType, TzTime}; @@ -156,6 +156,27 @@ impl DataStore { self.fill_network_fees(hash, network_fees_msat) } + pub fn outgoing_payment_failed(&self, hash: &str, fail_reason: PayErrorCode) -> Result<()> { + self.db_conn + .execute( + "\ + INSERT INTO events (payment_id, type, timezone_id, timezone_utc_offset_secs, fail_reason) \ + VALUES ( + (SELECT payment_id FROM payments WHERE hash=?1), ?2, ?3, ?4, ?5) + ", + ( + hash, + PaymentState::Failed as u8, + &self.timezone_config.timezone_id, + self.timezone_config.timezone_utc_offset_secs, + fail_reason as u8, + ), + ) + .map_to_invalid_input("Failed to add payment failed event to db")?; + + Ok(()) + } + pub fn new_payment_state(&self, hash: &str, state: PaymentState) -> Result<()> { self.db_conn .execute( @@ -171,7 +192,7 @@ impl DataStore { self.timezone_config.timezone_utc_offset_secs, ), ) - .map_to_invalid_input("Failed to add payment retrying event to payments db")?; + .map_to_invalid_input("Failed to add payment event to db")?; Ok(()) } @@ -216,7 +237,7 @@ impl DataStore { lsp_fees_msat, invoice, metadata, recent_events.type as state, recent_events.inserted_at, \ recent_events.timezone_id, recent_events.timezone_utc_offset_secs, description, \ creation_events.inserted_at, creation_events.timezone_id, creation_events.timezone_utc_offset_secs, \ - h.fiat_currency, h.rate, h.updated_at \ + h.fiat_currency, h.rate, h.updated_at, recent_events.fail_reason \ FROM payments \ JOIN recent_events ON payments.payment_id=recent_events.payment_id \ JOIN creation_events ON payments.payment_id=creation_events.payment_id \ @@ -248,7 +269,7 @@ impl DataStore { lsp_fees_msat, invoice, metadata, recent_events.type as state, recent_events.inserted_at, \ recent_events.timezone_id, recent_events.timezone_utc_offset_secs, description, \ creation_events.inserted_at, creation_events.timezone_id, creation_events.timezone_utc_offset_secs, \ - h.fiat_currency, h.rate, h.updated_at \ + h.fiat_currency, h.rate, h.updated_at, recent_events.fail_reason \ FROM payments \ JOIN recent_events ON payments.payment_id=recent_events.payment_id \ JOIN creation_events ON payments.payment_id=creation_events.payment_id \ @@ -278,7 +299,7 @@ impl DataStore { lsp_fees_msat, invoice, metadata, recent_events.type as state, recent_events.inserted_at, \ recent_events.timezone_id, recent_events.timezone_utc_offset_secs, description, \ creation_events.inserted_at, creation_events.timezone_id, creation_events.timezone_utc_offset_secs, \ - h.fiat_currency, h.rate, h.updated_at \ + h.fiat_currency, h.rate, h.updated_at, recent_events.fail_reason \ FROM payments \ JOIN recent_events ON payments.payment_id=recent_events.payment_id \ JOIN creation_events ON payments.payment_id=creation_events.payment_id \ @@ -475,9 +496,17 @@ fn payment_from_row(row: &Row) -> rusqlite::Result { } else { None }; + let fail_reason: Option = row.get(20)?; + let fail_reason = match fail_reason { + None => None, + Some(r) => Some(PayErrorCode::try_from(r).map_err(|e| { + rusqlite::Error::FromSqlConversionFailure(1, Type::Integer, Box::new(e)) + })?), + }; Ok(Payment { payment_type, payment_state, + fail_reason, hash, amount_msat, invoice, @@ -526,6 +555,7 @@ mod tests { use crate::interfaces::ExchangeRate; use crate::payment::{FiatValues, PaymentState, PaymentType}; + use crate::errors::PayErrorCode; use lightning::chain::keysinterface::SpendableOutputDescriptor; use lightning::ln::PaymentSecret; use lightning::util::ser::Readable; @@ -711,12 +741,13 @@ mod tests { sleep(Duration::from_secs(1)); data_store - .new_payment_state(hash, PaymentState::Failed) + .outgoing_payment_failed(hash, PayErrorCode::NoMoreRoutes) .unwrap(); let payments = data_store.get_latest_payments(100).unwrap(); assert_eq!(payments.len(), 2); let payment = payments.get(0).unwrap(); assert_eq!(payment.payment_state, PaymentState::Failed); + assert_eq!(payment.fail_reason, Some(PayErrorCode::NoMoreRoutes)); assert_eq!(payment.created_at.timezone_id, TEST_TZ_ID); assert_eq!(payment.created_at.timezone_utc_offset_secs, TEST_TZ_OFFSET); assert_eq!(payment.latest_state_change_at.timezone_id, TEST_TZ_ID); diff --git a/eel/src/errors.rs b/eel/src/errors.rs index 4c659bf3..61f9e365 100644 --- a/eel/src/errors.rs +++ b/eel/src/errors.rs @@ -1,3 +1,5 @@ +use lightning::events::PaymentFailureReason; +use num_enum::TryFromPrimitive; use std::fmt::{Display, Formatter}; #[derive(Debug, PartialEq, Eq)] @@ -18,7 +20,8 @@ impl Display for RuntimeErrorCode { pub type Error = perro::Error; pub type Result = std::result::Result; -#[derive(Debug, PartialEq, Eq)] +#[derive(PartialEq, Eq, Debug, TryFromPrimitive, Clone)] +#[repr(u8)] pub enum PayErrorCode { InvoiceExpired, AlreadyUsedInvoice, @@ -27,6 +30,20 @@ pub enum PayErrorCode { RecipientRejected, RetriesExhausted, NoMoreRoutes, + UnexpectedError, +} + +impl PayErrorCode { + pub(crate) fn from_failure_reason(reason: PaymentFailureReason) -> Self { + match reason { + PaymentFailureReason::RecipientRejected => Self::RecipientRejected, + PaymentFailureReason::UserAbandoned => Self::UnexpectedError, + PaymentFailureReason::RetriesExhausted => Self::RetriesExhausted, + PaymentFailureReason::PaymentExpired => Self::InvoiceExpired, + PaymentFailureReason::RouteNotFound => Self::NoMoreRoutes, + PaymentFailureReason::UnexpectedError => Self::UnexpectedError, + } + } } impl Display for PayErrorCode { diff --git a/eel/src/event_handler.rs b/eel/src/event_handler.rs index ab684d47..468ea01f 100644 --- a/eel/src/event_handler.rs +++ b/eel/src/event_handler.rs @@ -1,5 +1,5 @@ use crate::data_store::DataStore; -use crate::errors::Result; +use crate::errors::{PayErrorCode, Result}; use crate::interfaces; use crate::payment::PaymentState; use crate::task_manager::TaskManager; @@ -155,7 +155,10 @@ impl EventHandler for LipaEventHandler { .data_store .lock() .unwrap() - .new_payment_state(&payment_hash, PaymentState::Failed) + .outgoing_payment_failed( + &payment_hash, + PayErrorCode::from_failure_reason(reason.unwrap()), // `reason` could only be None if we deserialize events from old LDK versions + ) .is_err() { error!("Failed to persist in the payment db that sending payment with hash {payment_hash} has failed"); diff --git a/eel/src/lib.rs b/eel/src/lib.rs index d51a9633..bad3c77f 100644 --- a/eel/src/lib.rs +++ b/eel/src/lib.rs @@ -506,27 +506,38 @@ impl LightningNode { error: PaymentError, payment_hash: &str, ) -> PayResult<()> { - self.data_store - .lock() - .unwrap() - .new_payment_state(payment_hash, PaymentState::Failed) - .map_to_permanent_failure("Failed to persist payment result")?; - match error { - PaymentError::Invoice(e) => Err(invalid_input(format!("Invalid invoice - {e}"))), + let (error, error_code) = match error { + PaymentError::Invoice(e) => ( + Err(invalid_input(format!("Invalid invoice - {e}"))), + PayErrorCode::UnexpectedError, + ), PaymentError::Sending(e) => match e { - RetryableSendFailure::PaymentExpired => Err(runtime_error( + RetryableSendFailure::PaymentExpired => ( + Err(runtime_error( + PayErrorCode::InvoiceExpired, + "Invoice has expired", + )), PayErrorCode::InvoiceExpired, - "Invoice has expired", - )), - RetryableSendFailure::RouteNotFound => Err(runtime_error( + ), + RetryableSendFailure::RouteNotFound => ( + Err(runtime_error( + PayErrorCode::NoRouteFound, + "Failed to find any route", + )), PayErrorCode::NoRouteFound, - "Failed to find any route", - )), - RetryableSendFailure::DuplicatePayment => { - Err(permanent_failure("Duplicate payment")) - } + ), + RetryableSendFailure::DuplicatePayment => ( + Err(permanent_failure("Duplicate payment")), + PayErrorCode::UnexpectedError, + ), }, - } + }; + self.data_store + .lock() + .unwrap() + .outgoing_payment_failed(payment_hash, error_code) + .map_to_permanent_failure("Failed to persist payment result")?; + error } fn validate_persist_new_outgoing_payment_attempt( diff --git a/eel/src/migrations.rs b/eel/src/migrations.rs index 34534671..4cef9eb2 100644 --- a/eel/src/migrations.rs +++ b/eel/src/migrations.rs @@ -10,6 +10,7 @@ pub(crate) fn get_migrations() -> Vec { migration_02_store_exchange_rates, migration_03_store_spendable_outputs, migration_04_store_exchange_rates_history, + migration_05_store_payment_failure_reason, ] } @@ -116,3 +117,13 @@ fn migration_04_store_exchange_rates_history(connection: &Connection) -> Result< ) .map_to_permanent_failure("Failed to set up local database") } + +fn migration_05_store_payment_failure_reason(connection: &Connection) -> Result<()> { + connection + .execute_batch( + "\ + ALTER TABLE events ADD COLUMN fail_reason INTEGER NULL; + ", + ) + .map_to_permanent_failure("Failed to set up local database") +} diff --git a/eel/src/payment.rs b/eel/src/payment.rs index 51f084b3..df406685 100644 --- a/eel/src/payment.rs +++ b/eel/src/payment.rs @@ -1,6 +1,7 @@ use crate::interfaces::ExchangeRate; use lightning_invoice::Invoice; +use crate::errors::PayErrorCode; use num_enum::TryFromPrimitive; use std::time::SystemTime; @@ -56,6 +57,7 @@ impl FiatValues { pub struct Payment { pub payment_type: PaymentType, pub payment_state: PaymentState, + pub fail_reason: Option, pub hash: String, pub amount_msat: u64, pub invoice: Invoice, diff --git a/eel/tests/data_store_test.rs b/eel/tests/data_store_test.rs index a50c2473..e8248ed9 100644 --- a/eel/tests/data_store_test.rs +++ b/eel/tests/data_store_test.rs @@ -122,6 +122,7 @@ mod data_store_test { let payment_dummy = Payment { payment_type: PaymentType::Receiving, payment_state: PaymentState::Succeeded, + fail_reason: None, hash: "".to_string(), amount_msat: TWENTY_K_SATS, invoice: invoice.clone(), @@ -163,6 +164,7 @@ mod data_store_test { let mut payment_dummy = Payment { payment_type: PaymentType::Sending, payment_state: PaymentState::Created, + fail_reason: None, hash: "".to_string(), amount_msat: TWO_K_SATS, invoice: Invoice::from_str(&invoice).unwrap(), diff --git a/examples/3l-node/cli.rs b/examples/3l-node/cli.rs index 51c9f445..89a303e9 100644 --- a/examples/3l-node/cli.rs +++ b/examples/3l-node/cli.rs @@ -6,6 +6,7 @@ use bitcoin::secp256k1::PublicKey; use chrono::offset::FixedOffset; use chrono::{DateTime, Utc}; use colored::Colorize; +use eel::payment::PaymentState; use rustyline::config::{Builder, CompletionType}; use rustyline::error::ReadlineError; use rustyline::history::DefaultHistory; @@ -500,6 +501,9 @@ fn list_payments(node: &LightningNode) -> Result<(), String> { payment.latest_state_change_at.timezone_id ); println!(" State: {:?}", payment.payment_state); + if payment.payment_state == PaymentState::Failed { + println!(" Fail reason: {:?}", payment.fail_reason); + } println!(" Amount: {}", amount_to_string(payment.amount)); println!( " Network fees: {:?}", diff --git a/src/lib.rs b/src/lib.rs index 34f7bdd8..ef7a7d1b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -84,6 +84,7 @@ pub struct ChannelsInfo { pub struct Payment { pub payment_type: PaymentType, pub payment_state: PaymentState, + pub fail_reason: Option, pub hash: String, pub amount: Amount, pub invoice_details: InvoiceDetails, @@ -338,6 +339,7 @@ fn to_payment(payment: eel::payment::Payment) -> Payment { Payment { payment_type: payment.payment_type, payment_state: payment.payment_state, + fail_reason: payment.fail_reason, hash: payment.hash, amount, invoice_details, diff --git a/src/lipalightninglib.udl b/src/lipalightninglib.udl index 6cd2c6ce..88124499 100644 --- a/src/lipalightninglib.udl +++ b/src/lipalightninglib.udl @@ -292,6 +292,7 @@ interface MaxRoutingFeeMode { dictionary Payment { PaymentType payment_type; PaymentState payment_state; + PayErrorCode? fail_reason; string hash; // Hex representation of payment hash Amount amount; // Nominal amount specified in the invoice InvoiceDetails invoice_details; @@ -479,4 +480,6 @@ enum PayErrorCode { "RecipientRejected", // The recipient has rejected the payment. "RetriesExhausted", // Retry attempts or timeout was reached. "NoMoreRoutes", // All possible routes failed. + + "UnexpectedError", // An unexpected error occurred. This likely is a result of a bug within 3L/LDK and should be reported to lipa. }; From 2c3145873426b7a37c08e77455eac19a80fb9556 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Granh=C3=A3o?= <32176319+danielgranhao@users.noreply.github.com> Date: Wed, 28 Jun 2023 15:24:22 +0100 Subject: [PATCH 2/2] Remove unwrap of reason optional Co-authored-by: Andrei <92177534+andrei-21@users.noreply.github.com> --- eel/src/event_handler.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/eel/src/event_handler.rs b/eel/src/event_handler.rs index 468ea01f..cb58c064 100644 --- a/eel/src/event_handler.rs +++ b/eel/src/event_handler.rs @@ -157,7 +157,11 @@ impl EventHandler for LipaEventHandler { .unwrap() .outgoing_payment_failed( &payment_hash, - PayErrorCode::from_failure_reason(reason.unwrap()), // `reason` could only be None if we deserialize events from old LDK versions + PayErrorCode::from_failure_reason( + reason.unwrap_or( + lightning::events::PaymentFailureReason::UnexpectedError, + ), + ), // `reason` could only be None if we deserialize events from old LDK versions ) .is_err() {