From 219dcc4ef0883ad5f10a931981e1eea94230ab98 Mon Sep 17 00:00:00 2001 From: yukang Date: Fri, 6 Dec 2024 18:42:54 +0800 Subject: [PATCH 1/3] fix tlc space issue caused by remote_nonces and remote_commitment_points --- src/fiber/channel.rs | 94 ++++++++++++++++++++++++++------------ src/fiber/tests/channel.rs | 36 +++++++++++++-- src/store/tests/store.rs | 7 ++- 3 files changed, 102 insertions(+), 35 deletions(-) diff --git a/src/fiber/channel.rs b/src/fiber/channel.rs index f34028880..3ad1c4385 100644 --- a/src/fiber/channel.rs +++ b/src/fiber/channel.rs @@ -3,6 +3,7 @@ use ckb_jsonrpc_types::BlockNumber; use secp256k1::XOnlyPublicKey; use tracing::{debug, error, info, trace, warn}; +use crate::fiber::serde_utils::U64Hex; use crate::{ fiber::{ fee::calculate_tlc_forward_fee, @@ -2622,8 +2623,8 @@ pub struct ChannelActorState { #[serde_as(as = "Option")] pub last_used_nonce_in_commitment_signed: Option, - #[serde_as(as = "Vec")] - pub remote_nonces: Vec, + #[serde_as(as = "Vec<(U64Hex, PubNonceAsBytes)>")] + pub remote_nonces: Vec<(u64, PubNonce)>, // The latest commitment transaction we're holding #[serde_as(as = "Option")] @@ -2631,7 +2632,7 @@ pub struct ChannelActorState { // All the commitment point that are sent from the counterparty. // We need to save all these points to derive the keys for the commitment transactions. - pub remote_commitment_points: Vec, + pub remote_commitment_points: Vec<(u64, Pubkey)>, pub remote_channel_public_keys: Option, // The shutdown info for both local and remote, they are setup by the shutdown command or message. @@ -3343,8 +3344,11 @@ impl ChannelActorState { commitment_numbers: Default::default(), remote_shutdown_script: Some(remote_shutdown_script), last_used_nonce_in_commitment_signed: None, - remote_nonces: vec![remote_nonce], - remote_commitment_points: vec![first_commitment_point, second_commitment_point], + remote_nonces: vec![(0, remote_nonce)], + remote_commitment_points: vec![ + (0, first_commitment_point), + (1, second_commitment_point), + ], local_shutdown_info: None, remote_shutdown_info: None, local_reserved_ckb_amount, @@ -3905,15 +3909,39 @@ impl ChannelActorState { "Getting remote nonce: commitment number {}, current nonces: {:?}", comitment_number, &self.remote_nonces ); - self.remote_nonces[comitment_number as usize].clone() + self.remote_nonces + .iter() + .rev() + .find_map(|(number, nonce)| { + if *number == comitment_number { + Some(nonce.clone()) + } else { + None + } + }) + .unwrap_or_else(|| { + self.remote_nonces + .last() + .expect("expect remote nonce") + .1 + .clone() + }) } fn save_remote_nonce(&mut self, nonce: PubNonce) { debug!( - "Saving remote nonce: new nonce {:?}, current nonces {:?}", - &nonce, &self.remote_nonces + "Saving remote nonce: new nonce {:?}, current nonces {:?}, commitment numbers {:?}", + &nonce, &self.remote_nonces, self.commitment_numbers ); - self.remote_nonces.push(nonce); + self.remote_nonces + .push((self.get_remote_commitment_number() + 1, nonce)); + loop { + let len = self.remote_nonces.len(); + if len <= 2 { + break; + } + self.remote_nonces.remove(0); + } } fn save_remote_nonce_for_raa(&mut self) { @@ -4197,16 +4225,20 @@ impl ChannelActorState { /// Get the counterparty commitment point for the given commitment number. fn get_remote_commitment_point(&self, commitment_number: u64) -> Pubkey { - debug!("Getting remote commitment point #{}", commitment_number); - let index = commitment_number as usize; - let commitment_point = self.remote_commitment_points[index]; debug!( - "Obtained remote commitment point #{} (counting from 0) out of total {} commitment points: {:?}", - index, - self.remote_commitment_points.len(), - commitment_point + "Getting remote commitment point #{} from remote_commitment_points: {:?}", + commitment_number, &self.remote_commitment_points ); - commitment_point + self.remote_commitment_points + .iter() + .find_map(|(number, point)| { + if *number == commitment_number { + Some(point.clone()) + } else { + None + } + }) + .expect("remote commitment point should exist") } fn get_current_local_commitment_point(&self) -> Pubkey { @@ -4780,8 +4812,8 @@ impl ChannelActorState { let remote_pubkeys = (&accept_channel).into(); self.remote_channel_public_keys = Some(remote_pubkeys); self.remote_commitment_points = vec![ - accept_channel.first_per_commitment_point, - accept_channel.second_per_commitment_point, + (0, accept_channel.first_per_commitment_point), + (1, accept_channel.second_per_commitment_point), ]; self.remote_shutdown_script = Some(accept_channel.shutdown_script.clone()); self.check_accept_channel_parameters()?; @@ -5214,16 +5246,20 @@ impl ChannelActorState { } fn append_remote_commitment_point(&mut self, commitment_point: Pubkey) { - debug!( - "Setting remote commitment point #{} (counting from 0)): {:?}", - self.remote_commitment_points.len(), - commitment_point - ); - assert_eq!( - self.remote_commitment_points.len() as u64, - self.get_local_commitment_number() - ); - self.remote_commitment_points.push(commitment_point); + self.remote_commitment_points + .push((self.get_local_commitment_number(), commitment_point)); + + let len = self.remote_commitment_points.len(); + if len > 5 { + let min_remote_commitment = self + .tlc_state + .all_tlcs() + .map(|x| x.created_at.remote) + .min() + .unwrap_or_default(); + self.remote_commitment_points + .retain(|(num, _)| *num >= min_remote_commitment); + } } fn handle_revoke_and_ack_message( diff --git a/src/fiber/tests/channel.rs b/src/fiber/tests/channel.rs index 9810ccc1c..a445cecf0 100644 --- a/src/fiber/tests/channel.rs +++ b/src/fiber/tests/channel.rs @@ -2073,14 +2073,13 @@ async fn do_test_add_tlc_waiting_ack() { let node_a_funding_amount = 100000000000; let node_b_funding_amount = 6200000000; - let (node_a, _node_b, new_channel_id) = + let (node_a, node_b, new_channel_id) = create_nodes_with_established_channel(node_a_funding_amount, node_b_funding_amount, false) .await; let tlc_amount = 1000000000; for i in 1..=2 { - std::thread::sleep(std::time::Duration::from_millis(400)); let add_tlc_command = AddTlcCommand { amount: tlc_amount, hash_algorithm: HashAlgorithm::CkbHash, @@ -2107,6 +2106,35 @@ async fn do_test_add_tlc_waiting_ack() { assert!(add_tlc_result.is_ok()); } } + + // send from b to a + for i in 1..=2 { + let add_tlc_command = AddTlcCommand { + amount: tlc_amount, + hash_algorithm: HashAlgorithm::CkbHash, + payment_hash: gen_sha256_hash().into(), + expiry: now_timestamp_as_millis_u64() + 100000000, + onion_packet: None, + previous_tlc: None, + }; + let add_tlc_result = call!(node_b.network_actor, |rpc_reply| { + NetworkActorMessage::Command(NetworkActorCommand::ControlFiberChannel( + ChannelCommandWithId { + channel_id: new_channel_id, + command: ChannelCommand::AddTlc(add_tlc_command, rpc_reply), + }, + )) + }) + .expect("node_b alive"); + if i == 2 { + // we are sending AddTlc constantly, so we should get a TemporaryChannelFailure + assert!(add_tlc_result.is_err()); + let code = add_tlc_result.unwrap_err().decode().unwrap(); + assert_eq!(code.error_code, TlcErrorCode::TemporaryChannelFailure); + } else { + assert!(add_tlc_result.is_ok()); + } + } } #[tokio::test] @@ -2116,14 +2144,14 @@ async fn do_test_add_tlc_number_limit() { let [mut node_a, mut node_b] = NetworkNode::new_n_interconnected_nodes().await; - let max_tlc_number = 3; + let max_tlc_number = 6; let (new_channel_id, _funding_tx) = establish_channel_between_nodes( &mut node_a, &mut node_b, true, node_a_funding_amount, node_b_funding_amount, - Some(3), + Some(max_tlc_number), None, ) .await; diff --git a/src/store/tests/store.rs b/src/store/tests/store.rs index cb2d2e229..3ec421a7c 100644 --- a/src/store/tests/store.rs +++ b/src/store/tests/store.rs @@ -299,8 +299,11 @@ fn test_channel_actor_state_store() { commitment_numbers: Default::default(), remote_shutdown_script: Some(Script::default()), last_used_nonce_in_commitment_signed: None, - remote_nonces: vec![pub_nonce.clone()], - remote_commitment_points: vec![generate_pubkey().into(), generate_pubkey().into()], + remote_nonces: vec![(0, pub_nonce.clone())], + remote_commitment_points: vec![ + (0, generate_pubkey().into()), + (1, generate_pubkey().into()), + ], local_shutdown_info: None, remote_shutdown_info: None, local_reserved_ckb_amount: 100, From de42e8e458caf8fbcc9d81c312835be9a386200d Mon Sep 17 00:00:00 2001 From: yukang Date: Fri, 6 Dec 2024 18:47:01 +0800 Subject: [PATCH 2/3] merge main and resolve conflicts --- src/fiber/tests/channel.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/fiber/tests/channel.rs b/src/fiber/tests/channel.rs index ece1a5cc1..681e20834 100644 --- a/src/fiber/tests/channel.rs +++ b/src/fiber/tests/channel.rs @@ -2132,7 +2132,10 @@ async fn do_test_add_tlc_waiting_ack() { if i == 2 { // we are sending AddTlc constantly, so we should get a TemporaryChannelFailure assert!(add_tlc_result.is_err()); - let code = add_tlc_result.unwrap_err().decode().unwrap(); + let code = add_tlc_result + .unwrap_err() + .decode(&NO_SHARED_SECRET, vec![]) + .unwrap(); assert_eq!(code.error_code, TlcErrorCode::TemporaryChannelFailure); } else { assert!(add_tlc_result.is_ok()); From e287995ed09b8a1fd00a4783d5de2cef29939c45 Mon Sep 17 00:00:00 2001 From: yukang Date: Tue, 10 Dec 2024 09:20:57 +0800 Subject: [PATCH 3/3] fix remote_nonces and add tests for two directions --- src/fiber/channel.rs | 31 ++++++++++++++++--------------- src/fiber/tests/channel.rs | 11 ++++++++--- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/fiber/channel.rs b/src/fiber/channel.rs index e46b44c3a..5cc3c0ec4 100644 --- a/src/fiber/channel.rs +++ b/src/fiber/channel.rs @@ -2626,6 +2626,7 @@ pub struct ChannelActorState { #[serde_as(as = "Option")] pub last_used_nonce_in_commitment_signed: Option, + // The nonces that are sent by the counterparty, the length is at most 2 #[serde_as(as = "Vec<(U64Hex, PubNonceAsBytes)>")] pub remote_nonces: Vec<(u64, PubNonce)>, @@ -2635,6 +2636,7 @@ pub struct ChannelActorState { // All the commitment point that are sent from the counterparty. // We need to save all these points to derive the keys for the commitment transactions. + // The length of this vector is at most the maximum number of flighting tlcs. pub remote_commitment_points: Vec<(u64, Pubkey)>, pub remote_channel_public_keys: Option, @@ -3912,6 +3914,7 @@ impl ChannelActorState { "Getting remote nonce: commitment number {}, current nonces: {:?}", comitment_number, &self.remote_nonces ); + assert!(self.remote_nonces.len() <= 2); self.remote_nonces .iter() .rev() @@ -3922,13 +3925,7 @@ impl ChannelActorState { None } }) - .unwrap_or_else(|| { - self.remote_nonces - .last() - .expect("expect remote nonce") - .1 - .clone() - }) + .expect("get_remote_nonce") } fn save_remote_nonce(&mut self, nonce: PubNonce) { @@ -3936,13 +3933,14 @@ impl ChannelActorState { "Saving remote nonce: new nonce {:?}, current nonces {:?}, commitment numbers {:?}", &nonce, &self.remote_nonces, self.commitment_numbers ); - self.remote_nonces - .push((self.get_remote_commitment_number() + 1, nonce)); - loop { - let len = self.remote_nonces.len(); - if len <= 2 { - break; - } + + let next_remote_number = if self.remote_nonces.is_empty() { + 0 + } else { + self.get_remote_commitment_number() + 1 + }; + self.remote_nonces.push((next_remote_number, nonce)); + if self.remote_nonces.len() > 2 { self.remote_nonces.remove(0); } } @@ -5253,7 +5251,7 @@ impl ChannelActorState { .push((self.get_local_commitment_number(), commitment_point)); let len = self.remote_commitment_points.len(); - if len > 5 { + if len > (self.max_tlc_number_in_flight + 1) as usize { let min_remote_commitment = self .tlc_state .all_tlcs() @@ -5263,6 +5261,9 @@ impl ChannelActorState { self.remote_commitment_points .retain(|(num, _)| *num >= min_remote_commitment); } + assert!( + self.remote_commitment_points.len() <= (self.max_tlc_number_in_flight + 1) as usize + ); } fn handle_revoke_and_ack_message( diff --git a/src/fiber/tests/channel.rs b/src/fiber/tests/channel.rs index 681e20834..ffbc1d415 100644 --- a/src/fiber/tests/channel.rs +++ b/src/fiber/tests/channel.rs @@ -33,6 +33,7 @@ use ckb_types::{ prelude::{AsTransactionBuilder, Builder, Entity, IntoTransactionView, Pack, Unpack}, }; use ractor::call; +use rand::Rng; use secp256k1::Secp256k1; use std::collections::HashSet; @@ -2146,7 +2147,7 @@ async fn do_test_add_tlc_waiting_ack() { #[tokio::test] async fn do_test_add_tlc_number_limit() { let node_a_funding_amount = 100000000000; - let node_b_funding_amount = 6200000000; + let node_b_funding_amount = 100000000000; let [mut node_a, mut node_b] = NetworkNode::new_n_interconnected_nodes().await; @@ -2163,7 +2164,10 @@ async fn do_test_add_tlc_number_limit() { .await; let tlc_amount = 1000000000; + let rand_in_100 = rand::thread_rng().gen_range(1..=100); + // both tlc sent from a -> b or b -> a will be charged as tlc numbers + // TODO: we should consider the tlc number limit for both direction for i in 1..=max_tlc_number + 1 { std::thread::sleep(std::time::Duration::from_millis(400)); let add_tlc_command = AddTlcCommand { @@ -2174,7 +2178,8 @@ async fn do_test_add_tlc_number_limit() { onion_packet: None, previous_tlc: None, }; - let add_tlc_result = call!(node_a.network_actor, |rpc_reply| { + let source_node = if rand_in_100 > 50 { &node_a } else { &node_b }; + let add_tlc_result = call!(source_node.network_actor, |rpc_reply| { NetworkActorMessage::Command(NetworkActorCommand::ControlFiberChannel( ChannelCommandWithId { channel_id: new_channel_id, @@ -2182,7 +2187,7 @@ async fn do_test_add_tlc_number_limit() { }, )) }) - .expect("node_b alive"); + .expect("source node alive"); tokio::time::sleep(tokio::time::Duration::from_millis(300)).await; if i == max_tlc_number + 1 { assert!(add_tlc_result.is_err());