From 11c746fc55d6a7a95f17bce61d4989a48454d581 Mon Sep 17 00:00:00 2001 From: Tibo-lg Date: Mon, 19 Jun 2023 14:41:07 +0900 Subject: [PATCH] Fix off chain close reconnect issue --- dlc-manager/src/sub_channel_manager.rs | 195 ++++++++---------- dlc-manager/src/subchannel/mod.rs | 2 + dlc-manager/src/subchannel/ser.rs | 2 +- .../tests/ln_dlc_channel_execution_tests.rs | 76 ++++++- dlc-messages/src/sub_channel.rs | 10 +- 5 files changed, 170 insertions(+), 115 deletions(-) diff --git a/dlc-manager/src/sub_channel_manager.rs b/dlc-manager/src/sub_channel_manager.rs index f84f5dde..17ea7da7 100644 --- a/dlc-manager/src/sub_channel_manager.rs +++ b/dlc-manager/src/sub_channel_manager.rs @@ -38,12 +38,12 @@ use crate::{ chain_monitor::{ChannelInfo, RevokedTxType, TxType}, channel::{ generate_temporary_contract_id, offered_channel::OfferedChannel, - party_points::PartyBasePoints, signed_channel::SignedChannelState, Channel, + party_points::PartyBasePoints, Channel, }, channel_updater::{ self, FundingInfo, SubChannelSignInfo, SubChannelSignVerifyInfo, SubChannelVerifyInfo, }, - contract::{contract_input::ContractInput, ClosedContract, Contract, FundingInputInfo}, + contract::{contract_input::ContractInput, Contract, FundingInputInfo}, error::Error, manager::{get_channel_in_state, get_contract_in_state, Manager, CET_NSEQUENCE}, subchannel::{ @@ -129,17 +129,16 @@ pub enum Action { /// The balance of the local party in the DLC sub channel for settling it. own_balance: u64, }, - /// The sub channel with specified [`ChannelId`] should be marked as - /// [`SubChannelState::OffChainClosed`] as the revoke and ack must have been given by the peer - /// during the reestablishment of the LN channel. - ForceOffChainClosed(ChannelId), + /// The given [`SubChannelCloseFinalize`] message should be re-sent to the peer with given + /// `PublicKey`. + ResendCloseFinalize((SubChannelCloseFinalize, PublicKey)), } impl_dlc_writeable_enum!(Action, (0, ResendOffer), (2, ForceSign), (3, ResendCloseOffer), - (5, ForceOffChainClosed); + (5, ResendCloseFinalize); (1, ReAccept, { (channel_id, writeable), (party_params, { cb_writeable, dlc_messages::ser_impls::party_params::write, dlc_messages::ser_impls::party_params::read }), @@ -1173,81 +1172,6 @@ where Ok(()) } - fn mark_channel_offchain_closed(&self, channel_id: ChannelId) -> Result<(), Error> { - let (mut signed_sub_channel, state) = get_sub_channel_in_state!( - self.dlc_channel_manager, - channel_id, - CloseConfirmed, - None:: - )?; - - let dlc_channel_id = - signed_sub_channel - .get_dlc_channel_id(0) - .ok_or(Error::InvalidState( - "Could not get dlc channel id".to_string(), - ))?; - let mut channel = get_channel_in_state!( - self.dlc_channel_manager, - &dlc_channel_id, - Signed, - None:: - )?; - let contract = match channel.state { - SignedChannelState::Established { .. } => { - let contract = get_contract_in_state!( - self.dlc_channel_manager, - &channel - .get_contract_id() - .ok_or_else(|| Error::InvalidState( - "No contract id in on_sub_channel_finalize".to_string() - ))?, - Signed, - None:: - )?; - - let offer = &contract.accepted_contract.offered_contract; - let party_params = if offer.is_offer_party { - &offer.offer_params - } else { - &contract.accepted_contract.accept_params - }; - let collateral = party_params.collateral as i64; - - let closed_contract = ClosedContract { - attestations: None, - signed_cet: None, - contract_id: contract.accepted_contract.get_contract_id(), - temporary_contract_id: contract.accepted_contract.offered_contract.id, - counter_party_id: contract.accepted_contract.offered_contract.counter_party, - pnl: collateral - (state.own_balance as i64), - }; - Some(Contract::Closed(closed_contract)) - } - SignedChannelState::Settled { .. } => None, - _ => { - return Err(Error::InvalidState(format!( - "Expected established of settled state but was in {:?} state", - channel.state - ))); - } - }; - - channel.state = SignedChannelState::CollaborativelyClosed; - - self.dlc_channel_manager - .get_store() - .upsert_channel(Channel::Signed(channel), contract)?; - - signed_sub_channel.state = SubChannelState::OffChainClosed; - - self.dlc_channel_manager - .get_store() - .upsert_sub_channel(&signed_sub_channel)?; - - Ok(()) - } - fn on_subchannel_offer( &self, sub_channel_offer: &SubChannelOffer, @@ -2072,6 +1996,7 @@ where own_balance: state.offer_balance, counter_balance: state.accept_balance, ln_rollback: (&channel_details).into(), + check_ln_secret: true, }; sub_channel.state = SubChannelState::CloseConfirmed(updated_channel); @@ -2167,9 +2092,11 @@ where let finalize = SubChannelCloseFinalize { channel_id: confirm.channel_id, split_revocation_secret: per_split_secret, - commit_revocation_secret: SecretKey::from_slice(&own_raa.per_commitment_secret) - .expect("a valid secret key"), - next_per_commitment_point: own_raa.next_per_commitment_point, + commit_revocation_secret: Some( + SecretKey::from_slice(&own_raa.per_commitment_secret) + .expect("a valid secret key"), + ), + next_per_commitment_point: Some(own_raa.next_per_commitment_point), }; self.dlc_channel_manager @@ -2244,14 +2171,27 @@ where .dlc_channel_manager .get_closed_sub_dlc_channel(dlc_channel_id, state.own_balance)?; - let revoke_and_ack = RevokeAndACK { - channel_id: finalize.channel_id, - per_commitment_secret: *finalize.commit_revocation_secret.as_ref(), - next_per_commitment_point: finalize.next_per_commitment_point, - }; + if state.check_ln_secret { + match ( + finalize.commit_revocation_secret.as_ref(), + finalize.next_per_commitment_point.as_ref(), + ) { + (Some(secret), Some(point)) => { + let revoke_and_ack = RevokeAndACK { + channel_id: finalize.channel_id, + per_commitment_secret: *secret.as_ref(), + next_per_commitment_point: *point, + }; - self.ln_channel_manager - .revoke_and_ack(channel_lock, &revoke_and_ack)?; + self.ln_channel_manager + .revoke_and_ack(channel_lock, &revoke_and_ack)?; + } + _ => return Err(APIError::ExternalError { + err: "Did not get expected revocation secret and next commitment point" + .to_string(), + }), + }; + } sub_channel.state = SubChannelState::OffChainClosed; @@ -2385,19 +2325,8 @@ where ); }; } - Action::ForceOffChainClosed(id) => { - if let Some(details) = self.ln_channel_manager.get_channel_details(&id) { - if details.is_usable { - if let Err(e) = self.mark_channel_offchain_closed(id) { - error!("Unexpected error {} making channel {:?} as offchain closed, keeping the action to retry.", e, id); - retain.push(Action::ForceOffChainClosed(id)); - } - } else { - retain.push(Action::ForceOffChainClosed(id)); - } - } else { - error!("Could not get channel details for id: {:?}", id); - }; + Action::ResendCloseFinalize((msg, pk)) => { + msgs.push((SubChannelMessage::CloseFinalize(msg), pk)); } }; } @@ -2704,6 +2633,18 @@ where let has_not_received = match peer_state { None => true, Some(state) if state == ReestablishFlag::OffChainClosed as u8 => true, + Some(state) if state == ReestablishFlag::CloseConfirmed as u8 => { + let finalize = self.get_reconnect_close_finalize(&channel)?; + + self.actions + .lock() + .unwrap() + .push(Action::ResendCloseFinalize(( + finalize, + channel.counter_party, + ))); + true + } _ => false, }; if has_not_received { @@ -2952,7 +2893,24 @@ where }, )?; } else { - debug_assert_eq!(ReestablishFlag::OffChainClosed as u8, counter_state); + // LDK will provide the revocation secret through reestablishment. + let mut confirmed = confirmed.clone(); + confirmed.check_ln_secret = false; + updated_state = Some(SubChannelState::CloseConfirmed(confirmed)); + } + } + } + SubChannelState::OffChainClosed => { + if let Some(counter_state) = peer_state { + let finalize = self.get_reconnect_close_finalize(&channel)?; + if counter_state == ReestablishFlag::CloseConfirmed as u8 { + self.actions + .lock() + .unwrap() + .push(Action::ResendCloseFinalize(( + finalize, + channel.counter_party, + ))); } } } @@ -2978,6 +2936,31 @@ where Ok(()) } + fn get_reconnect_close_finalize( + &self, + channel: &SubChannel, + ) -> Result { + let per_split_seed = self + .dlc_channel_manager + .get_wallet() + .get_secret_key_for_pubkey( + &channel.per_split_seed.expect("to have a per split seed"), + )?; + + let per_split_secret = SecretKey::from_slice(&build_commitment_secret( + per_split_seed.as_ref(), + channel.update_idx, + )) + .map_err(|e| APIError::ExternalError { err: e.to_string() })?; + + Ok(SubChannelCloseFinalize { + channel_id: channel.channel_id, + split_revocation_secret: per_split_secret, + commit_revocation_secret: None, + next_per_commitment_point: None, + }) + } + fn on_sub_channel_reject(&self, reject: &Reject, peer_id: &PublicKey) -> Result<(), Error> { let sub_channel = self .dlc_channel_manager diff --git a/dlc-manager/src/subchannel/mod.rs b/dlc-manager/src/subchannel/mod.rs index fe00aa12..18dace7e 100644 --- a/dlc-manager/src/subchannel/mod.rs +++ b/dlc-manager/src/subchannel/mod.rs @@ -286,6 +286,8 @@ pub struct CloseConfirmedSubChannel { pub counter_balance: u64, /// Rollback information about the split channel pub ln_rollback: LnRollBackInfo, + /// Whether to check for LN secret (to deal with reestblishments) + pub check_ln_secret: bool, } /// Information about a sub channel that is in the process of being unilateraly closed. diff --git a/dlc-manager/src/subchannel/ser.rs b/dlc-manager/src/subchannel/ser.rs index ea637482..29017120 100644 --- a/dlc-manager/src/subchannel/ser.rs +++ b/dlc-manager/src/subchannel/ser.rs @@ -79,6 +79,6 @@ impl_dlc_writeable!(CloseOfferedSubChannel, { impl_dlc_writeable!(CloseAcceptedSubChannel, { (signed_subchannel, writeable), (own_balance, writeable), (counter_balance, writeable), (ln_rollback, writeable) }); -impl_dlc_writeable!(CloseConfirmedSubChannel, { (signed_subchannel, writeable), (own_balance, writeable), (counter_balance, writeable), (ln_rollback, writeable) }); +impl_dlc_writeable!(CloseConfirmedSubChannel, { (signed_subchannel, writeable), (own_balance, writeable), (counter_balance, writeable), (ln_rollback, writeable), (check_ln_secret, writeable) }); impl_dlc_writeable!(ClosingSubChannel, { (signed_sub_channel, writeable), (is_initiator, writeable) }); diff --git a/dlc-manager/tests/ln_dlc_channel_execution_tests.rs b/dlc-manager/tests/ln_dlc_channel_execution_tests.rs index e079b834..26514ef1 100644 --- a/dlc-manager/tests/ln_dlc_channel_execution_tests.rs +++ b/dlc-manager/tests/ln_dlc_channel_execution_tests.rs @@ -160,6 +160,7 @@ enum TestPath { OfferRejected, CloseRejected, Reconnect, + ReconnectReOfferAfterClose, } impl LnDlcParty { @@ -575,6 +576,12 @@ fn ln_dlc_off_chain_close_open_close() { ln_dlc_test(TestPath::OffChainCloseOpenClose); } +#[test] +#[ignore] +fn ln_dlc_offer_after_offchain_close_disconnect() { + ln_dlc_test(TestPath::ReconnectReOfferAfterClose); +} + // #[derive(Debug)] // pub struct TestParams { // pub oracles: Vec, @@ -929,7 +936,8 @@ fn ln_dlc_test(test_path: TestPath) { | TestPath::SplitCheat | TestPath::CloseRejected | TestPath::OffChainCloseOpenClose - | TestPath::Reconnect = test_path + | TestPath::Reconnect + | TestPath::ReconnectReOfferAfterClose = test_path { if let TestPath::SplitCheat = test_path { alice_node.dlc_manager.get_store().save(); @@ -972,8 +980,13 @@ fn ln_dlc_test(test_path: TestPath) { channel_id, alice_descriptor.clone(), bob_descriptor.clone(), + &test_params, ); + if let TestPath::ReconnectReOfferAfterClose = test_path { + return; + } + if let TestPath::OffChainCloseOpenClose = test_path { offer_sub_channel( &test_path, @@ -1000,6 +1013,7 @@ fn ln_dlc_test(test_path: TestPath) { channel_id, alice_descriptor.clone(), bob_descriptor.clone(), + &test_params, ); } @@ -1740,6 +1754,7 @@ fn off_chain_close_finalize( channel_id: ChannelId, alice_descriptor: MockSocketDescriptor, bob_descriptor: MockSocketDescriptor, + test_params: &TestParams, ) { let sub_channel = alice_node .dlc_manager @@ -1788,7 +1803,12 @@ fn off_chain_close_finalize( .unwrap(); if let TestPath::Reconnect = test_path { - reconnect(alice_node, bob_node, alice_descriptor, bob_descriptor); + reconnect( + alice_node, + bob_node, + alice_descriptor.clone(), + bob_descriptor.clone(), + ); assert_sub_channel_state!(alice_node.sub_channel_manager, &channel_id, CloseOffered); assert_sub_channel_state!(bob_node.sub_channel_manager, &channel_id, CloseOffered); @@ -1811,7 +1831,7 @@ fn off_chain_close_finalize( } } - let close_finalize = bob_node + let mut close_finalize = bob_node .sub_channel_manager .on_sub_channel_message( &close_confirm, @@ -1820,6 +1840,56 @@ fn off_chain_close_finalize( .unwrap() .unwrap(); + if let TestPath::Reconnect = test_path { + reconnect(alice_node, bob_node, alice_descriptor, bob_descriptor); + + assert_eq!(0, alice_node.sub_channel_manager.periodic_check().len()); + let mut msgs = bob_node.sub_channel_manager.periodic_check(); + assert_eq!(1, msgs.len()); + if let (SubChannelMessage::CloseFinalize(c), _) = msgs.pop().unwrap() { + close_finalize = SubChannelMessage::CloseFinalize(c); + } else { + panic!("Expected a close finalize message"); + } + } else if let TestPath::ReconnectReOfferAfterClose = test_path { + offer_common(test_params, bob_node, &channel_id); + assert_sub_channel_state!(bob_node.sub_channel_manager, &channel_id, Offered); + + reconnect(alice_node, bob_node, alice_descriptor, bob_descriptor); + + assert_sub_channel_state!(bob_node.sub_channel_manager, &channel_id, Offered); + + assert_eq!(0, alice_node.sub_channel_manager.periodic_check().len()); + let mut msgs = bob_node.sub_channel_manager.periodic_check(); + assert_eq!(2, msgs.len()); + if let (SubChannelMessage::CloseFinalize(c), _) = msgs.remove(0) { + close_finalize = SubChannelMessage::CloseFinalize(c); + alice_node + .sub_channel_manager + .on_sub_channel_message( + &close_finalize, + &bob_node.channel_manager.get_our_node_id(), + ) + .unwrap(); + assert_sub_channel_state!(alice_node.sub_channel_manager, &channel_id; OffChainClosed); + if let (SubChannelMessage::Offer(o), _) = msgs.pop().unwrap() { + alice_node + .sub_channel_manager + .on_sub_channel_message( + &SubChannelMessage::Offer(o), + &bob_node.channel_manager.get_our_node_id(), + ) + .unwrap(); + assert_sub_channel_state!(alice_node.sub_channel_manager, &channel_id, Offered); + return; + } else { + panic!("Expected an offer message"); + } + } else { + panic!("Expected a close finalize message"); + } + } + alice_node .sub_channel_manager .on_sub_channel_message(&close_finalize, &bob_node.channel_manager.get_our_node_id()) diff --git a/dlc-messages/src/sub_channel.rs b/dlc-messages/src/sub_channel.rs index 3716d6da..97083a43 100644 --- a/dlc-messages/src/sub_channel.rs +++ b/dlc-messages/src/sub_channel.rs @@ -303,7 +303,7 @@ impl_dlc_writeable!(SubChannelCloseConfirm, { /// A message to finalize the collaborative closing of a DLC channel embedded within a Lightning /// channel. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Eq, PartialEq)] #[cfg_attr( feature = "serde", derive(serde::Serialize, serde::Deserialize), @@ -315,16 +315,16 @@ pub struct SubChannelCloseFinalize { /// The pre-image of the split transaction revocation point. pub split_revocation_secret: SecretKey, /// The pre-image of the commit transaction revocation point. - pub commit_revocation_secret: SecretKey, + pub commit_revocation_secret: Option, /// The point to be used for computing the revocation of the next commitment transaction. - pub next_per_commitment_point: PublicKey, + pub next_per_commitment_point: Option, } impl_dlc_writeable!(SubChannelCloseFinalize, { (channel_id, writeable), (split_revocation_secret, writeable), - (commit_revocation_secret, writeable), - (next_per_commitment_point, writeable) + (commit_revocation_secret, option), + (next_per_commitment_point, option) }); /// A message to reject an offer to collaboratively close a DLC channel embedded within a Lightning