Merge pull request #874 from quake/quake/tweak-default-commitment-delay-epochs

chore: tweak DEFAULT_TLC_EXPIRY_DELTA to 4 hours

Author: quake

crates/fiber-lib/src/fiber/channel.rs

@@ -1,5 +1,6 @@
-use super::config::DEFAULT_FUNDING_TIMEOUT_SECONDS;
-use super::config::DEFAULT_HOLD_TLC_TIMEOUT;
+use super::config::{
+    DEFAULT_COMMITMENT_DELAY_EPOCHS, DEFAULT_FUNDING_TIMEOUT_SECONDS, DEFAULT_HOLD_TLC_TIMEOUT,
+};
 use super::{
     gossip::SOFT_BROADCAST_MESSAGES_CONSIDERED_STALE_DURATION, graph::ChannelUpdateInfo,
     types::ForwardTlcResult,
@@ -282,8 +283,6 @@ pub struct ChannelCommandWithId {
 
 pub const DEFAULT_FEE_RATE: u64 = 1_000;
 pub const DEFAULT_COMMITMENT_FEE_RATE: u64 = 1_000;
-// The default commitment delay is 6 epochs = 24 hours.
-pub const DEFAULT_COMMITMENT_DELAY_EPOCHS: u64 = 6;
 // The min commitment delay is 1 epoch = 4 hours.
 pub const MIN_COMMITMENT_DELAY_EPOCHS: u64 = 1;
 // The max commitment delay is 84 epochs = 14 days.
@@ -5713,8 +5712,8 @@ impl ChannelActorState {
         let current_time = now_timestamp_as_millis_u64();
         if expiry <= current_time + MIN_TLC_EXPIRY_DELTA {
             error!(
-                "TLC expiry {} is too soon, current time: {}",
-                expiry, current_time
+                "TLC expiry {} is too soon, current time: {}, MIN_TLC_EXPIRY_DELTA: {}",
+                expiry, current_time, MIN_TLC_EXPIRY_DELTA
             );
             return Err(ProcessingChannelError::TlcExpirySoon);
         }
@@ -5726,6 +5725,11 @@ impl ChannelActorState {
             .all_tlcs()
             .filter(|tlc| tlc.removed_confirmed_at.is_none())
             .count() as u64;
+        debug!(
+            "here debug pending_tlc_count: {} => delay: {}",
+            pending_tlc_count,
+            epoch_delay_milliseconds * (pending_tlc_count + 1)
+        );
         let expect_expiry = current_time + epoch_delay_milliseconds * (pending_tlc_count + 1);
         if expiry < expect_expiry {
             error!(

crates/fiber-lib/src/fiber/config.rs

@@ -29,8 +29,14 @@ pub const DEFAULT_AUTO_ACCEPT_CHANNEL_CKB_FUNDING_AMOUNT: u64 =
 /// Default minimum ckb funding amount for auto accepting an open channel request.
 pub const DEFAULT_OPEN_CHANNEL_AUTO_ACCEPT_MIN_CKB_FUNDING_AMOUNT: u64 = 100 * CKB_SHANNONS;
 
-/// The expiry delta to forward a tlc, in milliseconds, default to 1 day.
-pub const DEFAULT_TLC_EXPIRY_DELTA: u64 = 24 * 60 * 60 * 1000;
+// The default commitment delay is 1 epochs.
+pub const DEFAULT_COMMITMENT_DELAY_EPOCHS: u64 = 1;
+
+/// The expiry delta to forward a tlc, in milliseconds, default to 4 hours.
+pub const DEFAULT_TLC_EXPIRY_DELTA: u64 = 4 * 60 * 60 * 1000;
+
+/// The final hop expiry delta for a tlc, in milliseconds, default to 24 hours.
+pub const DEFAULT_FINAL_TLC_EXPIRY_DELTA: u64 = 24 * 60 * 60 * 1000; // 24 hours
 
 /// 4 hours for each epoch
 #[cfg(not(debug_assertions))]
@@ -40,14 +46,10 @@ pub const MILLI_SECONDS_PER_EPOCH: u64 = 4 * 60 * 60 * 1000;
 // we need to make sure 2/3 commitment_delay_epoch is greater than MIN_TLC_EXPIRY_DELTA
 pub const MILLI_SECONDS_PER_EPOCH: u64 = 2 * 1000;
 
-#[cfg(not(debug_assertions))]
-/// The minimal expiry delta to forward a tlc, in milliseconds. 16 hours
-/// expect it >= 2/3 commitment_delay_epoch, default DEFAULT_COMMITMENT_DELAY_EPOCHS is 6 epoch
-/// so 2/3 * 6 = 4 epoch, 4 * 4 hours = 16 hours
-pub const MIN_TLC_EXPIRY_DELTA: u64 = 4 * MILLI_SECONDS_PER_EPOCH;
-#[cfg(debug_assertions)]
-// 5 seconds for testing environment
-pub const MIN_TLC_EXPIRY_DELTA: u64 = 5 * 1000;
+/// The minimal expiry delta to forward a tlc, in milliseconds, 160 minutes.
+/// expect it >= 2/3 commitment_delay_epoch, default DEFAULT_COMMITMENT_DELAY_EPOCHS is 1 epoch
+pub const MIN_TLC_EXPIRY_DELTA: u64 =
+    DEFAULT_COMMITMENT_DELAY_EPOCHS * MILLI_SECONDS_PER_EPOCH * 2 / 3;
 
 /// The maximum expiry delta for a payment, in milliseconds. 2 weeks
 pub const MAX_PAYMENT_TLC_EXPIRY_LIMIT: u64 = 14 * 24 * 60 * 60 * 1000; // 2 weeks

crates/fiber-lib/src/fiber/fee.rs

@@ -229,14 +229,14 @@ pub(crate) fn check_tlc_delta_with_epochs(
 ) -> Result<(), ProcessingChannelError> {
     if tlc_expiry_delta < MIN_TLC_EXPIRY_DELTA {
         return Err(ProcessingChannelError::InvalidParameter(format!(
-            "TLC expiry delta is too small, expect larger than {}",
-            MIN_TLC_EXPIRY_DELTA
+            "TLC expiry delta is too small, expect larger than {}, got {}",
+            MIN_TLC_EXPIRY_DELTA, tlc_expiry_delta
         )));
     }
     if tlc_expiry_delta > MAX_PAYMENT_TLC_EXPIRY_LIMIT {
         return Err(ProcessingChannelError::InvalidParameter(format!(
-            "TLC expiry delta is too large, expected to be smaller than {}",
-            MAX_PAYMENT_TLC_EXPIRY_LIMIT
+            "TLC expiry delta is too large, expected to be smaller than {}, got {}",
+            MAX_PAYMENT_TLC_EXPIRY_LIMIT, tlc_expiry_delta
         )));
     }
 

crates/fiber-lib/src/fiber/graph.rs

@@ -11,7 +11,9 @@ use super::types::{
 };
 use super::types::{Cursor, Pubkey, TlcErr};
 use crate::ckb::config::UdtCfgInfos;
-use crate::fiber::config::DEFAULT_TLC_EXPIRY_DELTA;
+use crate::fiber::config::{
+    DEFAULT_FINAL_TLC_EXPIRY_DELTA, DEFAULT_TLC_EXPIRY_DELTA, MAX_PAYMENT_TLC_EXPIRY_LIMIT,
+};
 use crate::fiber::fee::calculate_tlc_forward_fee;
 use crate::fiber::history::SentNode;
 use crate::fiber::path::NodeHeapElement;
@@ -744,10 +746,12 @@ where
         // but a malicious node may send a channel update with a too large expiry delta
         // which makes the network graph contains a channel update with a too large expiry delta.
         // We need to check it again here to avoid any malicious channel update
-        if channel_update.tlc_expiry_delta > DEFAULT_TLC_EXPIRY_DELTA {
+        // Note: we don't check the tlc_expiry_delta is too small here, because it does not effect
+        // the path finding, and a too small tlc_expiry_delta only makes the hop itself more risky.
+        if channel_update.tlc_expiry_delta > MAX_PAYMENT_TLC_EXPIRY_LIMIT {
             error!(
                 "Channel update has too large expiry delta: {} > {}, channel update: {:?}",
-                channel_update.tlc_expiry_delta, DEFAULT_TLC_EXPIRY_DELTA, &channel_update
+                channel_update.tlc_expiry_delta, MAX_PAYMENT_TLC_EXPIRY_LIMIT, &channel_update
             );
             return None;
         }
@@ -1928,7 +1932,7 @@ where
             ));
         }
 
-        let mut agg_tlc_expiry = final_tlc_expiry_delta.unwrap_or(DEFAULT_TLC_EXPIRY_DELTA);
+        let mut agg_tlc_expiry = final_tlc_expiry_delta.unwrap_or(DEFAULT_FINAL_TLC_EXPIRY_DELTA);
         for (idx, cur_hop) in router_hops.iter().enumerate() {
             let prev_hop_pubkey = router_hops.get(idx + 1).map(|h| h.pubkey).unwrap_or(source);
 

crates/fiber-lib/src/fiber/network.rs

@@ -77,14 +77,13 @@ use crate::ckb::{
 use crate::fiber::channel::{
     AddTlcCommand, AddTlcResponse, ChannelActorState, ChannelEphemeralConfig,
     ChannelInitializationOperation, ShutdownCommand, TxCollaborationCommand, TxUpdateCommand,
-    DEFAULT_COMMITMENT_DELAY_EPOCHS,
 };
 use crate::fiber::channel::{
     AwaitingTxSignaturesFlags, ShuttingDownFlags, MAX_TLC_NUMBER_IN_FLIGHT,
 };
 use crate::fiber::config::{
-    DEFAULT_MAX_PARTS, DEFAULT_TLC_EXPIRY_DELTA, MAX_PAYMENT_TLC_EXPIRY_LIMIT,
-    MIN_TLC_EXPIRY_DELTA, PAYMENT_MAX_PARTS_LIMIT,
+    DEFAULT_COMMITMENT_DELAY_EPOCHS, DEFAULT_FINAL_TLC_EXPIRY_DELTA, DEFAULT_MAX_PARTS,
+    MAX_PAYMENT_TLC_EXPIRY_LIMIT, MIN_TLC_EXPIRY_DELTA, PAYMENT_MAX_PARTS_LIMIT,
 };
 use crate::fiber::fee::{check_open_channel_parameters, check_tlc_delta_with_epochs};
 use crate::fiber::gossip::{GossipConfig, GossipService, SubscribableGossipMessageStore};
@@ -566,14 +565,11 @@ impl SendPaymentData {
         };
 
         // check htlc expiry delta and limit are both valid if it is set
-        let final_tlc_expiry_delta = command
-            .final_tlc_expiry_delta
-            .or_else(|| {
-                invoice
-                    .as_ref()
-                    .and_then(|i| i.final_tlc_minimum_expiry_delta().copied())
-            })
-            .unwrap_or(DEFAULT_TLC_EXPIRY_DELTA);
+        let final_tlc_expiry_delta = invoice
+            .as_ref()
+            .and_then(|i| i.final_tlc_minimum_expiry_delta().copied())
+            .or(command.final_tlc_expiry_delta)
+            .unwrap_or(DEFAULT_FINAL_TLC_EXPIRY_DELTA);
         if !(MIN_TLC_EXPIRY_DELTA..=MAX_PAYMENT_TLC_EXPIRY_LIMIT).contains(&final_tlc_expiry_delta)
         {
             return Err(format!(
@@ -587,7 +583,10 @@ impl SendPaymentData {
             .unwrap_or(MAX_PAYMENT_TLC_EXPIRY_LIMIT);
 
         if tlc_expiry_limit < final_tlc_expiry_delta || tlc_expiry_limit < MIN_TLC_EXPIRY_DELTA {
-            return Err("tlc_expiry_limit is too small".to_string());
+            return Err(format!(
+                "tlc_expiry_limit is too small, final_tlc_expiry_delta: {}, tlc_expiry_limit: {}",
+                final_tlc_expiry_delta, tlc_expiry_limit
+            ));
         }
         if tlc_expiry_limit > MAX_PAYMENT_TLC_EXPIRY_LIMIT {
             return Err(format!(
@@ -3357,8 +3356,9 @@ where
 
         if tlc_expiry_delta.is_some_and(|d| d < MIN_TLC_EXPIRY_DELTA) {
             return Err(ProcessingChannelError::InvalidParameter(format!(
-                "TLC expiry delta is too small, expect larger than {}",
-                MIN_TLC_EXPIRY_DELTA
+                "TLC expiry delta is too small, expect larger than {}, got {}",
+                MIN_TLC_EXPIRY_DELTA,
+                tlc_expiry_delta.unwrap()
             )));
         }
 

crates/fiber-lib/src/fiber/tests/channel.rs

@@ -1,12 +1,11 @@
 use crate::ckb::tests::test_utils::complete_commitment_tx;
 use crate::fiber::channel::{
     AddTlcResponse, ChannelState, CloseFlags, OutboundTlcStatus, TLCId, TlcStatus, UpdateCommand,
-    DEFAULT_COMMITMENT_DELAY_EPOCHS, MAX_COMMITMENT_DELAY_EPOCHS, MIN_COMMITMENT_DELAY_EPOCHS,
-    XUDT_COMPATIBLE_WITNESS,
+    MAX_COMMITMENT_DELAY_EPOCHS, MIN_COMMITMENT_DELAY_EPOCHS, XUDT_COMPATIBLE_WITNESS,
 };
 use crate::fiber::config::{
-    DEFAULT_TLC_EXPIRY_DELTA, MAX_PAYMENT_TLC_EXPIRY_LIMIT, MILLI_SECONDS_PER_EPOCH,
-    MIN_TLC_EXPIRY_DELTA,
+    DEFAULT_COMMITMENT_DELAY_EPOCHS, DEFAULT_FINAL_TLC_EXPIRY_DELTA, DEFAULT_TLC_EXPIRY_DELTA,
+    MAX_PAYMENT_TLC_EXPIRY_LIMIT, MILLI_SECONDS_PER_EPOCH, MIN_TLC_EXPIRY_DELTA,
 };
 use crate::fiber::features::FeatureVector;
 use crate::fiber::graph::ChannelInfo;
@@ -5669,7 +5668,8 @@ async fn test_send_payment_will_succeed_with_large_tlc_expiry_limit() {
     let source_node = &mut node_0;
     let target_pubkey = node_3.pubkey;
 
-    let expected_minimal_tlc_expiry_limit = (24 * 60 * 60 * 1000) * 3;
+    let expected_minimal_tlc_expiry_limit =
+        DEFAULT_TLC_EXPIRY_DELTA * 2 + DEFAULT_FINAL_TLC_EXPIRY_DELTA;
 
     let res = source_node
         .send_payment(SendPaymentCommand {

crates/fiber-lib/src/fiber/tests/mpp.rs

@@ -3916,3 +3916,70 @@ async fn test_send_mpp_send_each_other_with_no_fee() {
         tokio::time::sleep(Duration::from_secs(1)).await;
     }
 }
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+#[ignore = "need to modify MILLI_SECONDS_PER_EPOCH and MIN_TLC_EXPIRY_DELTA to run this test"]
+async fn test_send_mpp_send_each_other_expire_soon() {
+    init_tracing();
+
+    let (nodes, _channels) = create_n_nodes_network_with_params(
+        &[
+            (
+                (0, 1),
+                ChannelParameters {
+                    public: true,
+                    node_a_funding_amount: MIN_RESERVED_CKB + 1000 * 100000000,
+                    node_b_funding_amount: MIN_RESERVED_CKB,
+                    a_tlc_fee_proportional_millionths: Some(0),
+                    b_tlc_fee_proportional_millionths: Some(0),
+                    ..Default::default()
+                },
+            ),
+            (
+                (0, 1),
+                ChannelParameters {
+                    public: true,
+                    node_a_funding_amount: MIN_RESERVED_CKB + 1000 * 100000000,
+                    node_b_funding_amount: MIN_RESERVED_CKB,
+                    a_tlc_fee_proportional_millionths: Some(0),
+                    b_tlc_fee_proportional_millionths: Some(0),
+                    ..Default::default()
+                },
+            ),
+            (
+                (1, 2),
+                ChannelParameters {
+                    public: true,
+                    node_a_funding_amount: MIN_RESERVED_CKB + 2000 * 100000000,
+                    node_b_funding_amount: MIN_RESERVED_CKB,
+                    a_tlc_fee_proportional_millionths: Some(0),
+                    b_tlc_fee_proportional_millionths: Some(0),
+                    ..Default::default()
+                },
+            ),
+        ],
+        3,
+        None,
+    )
+    .await;
+    let [node_0, _node_1, node_2] = nodes.try_into().expect("3 nodes");
+
+    for _i in 0..2 {
+        let res = node_0
+            .send_mpp_payment(&node_2, 2000 * 100000000, None)
+            .await;
+
+        let payment_hash = res.unwrap().payment_hash;
+        node_0.wait_until_success(payment_hash).await;
+
+        tokio::time::sleep(Duration::from_secs(1)).await;
+
+        let res = node_2
+            .send_mpp_payment(&node_0, 2000 * 100000000, None)
+            .await;
+
+        let payment_hash = res.unwrap().payment_hash;
+        node_2.wait_until_success(payment_hash).await;
+        tokio::time::sleep(Duration::from_secs(1)).await;
+    }
+}

crates/fiber-lib/src/fiber/tests/network.rs

@@ -10,7 +10,6 @@ use crate::{
     },
     fiber::{
         channel::ShutdownInfo,
-        config::DEFAULT_TLC_EXPIRY_DELTA,
         gossip::{GossipActorMessage, GossipMessageStore},
         graph::ChannelUpdateInfo,
         network::{
@@ -865,9 +864,6 @@ fn test_send_payment_validate_invoice() {
         .expiry_time(Duration::from_secs(1024))
         .payee_pub_key(public_key)
         .add_attr(Attribute::FinalHtlcTimeout(5))
-        .add_attr(Attribute::FinalHtlcMinimumExpiryDelta(
-            DEFAULT_TLC_EXPIRY_DELTA,
-        ))
         .add_attr(Attribute::Description("description".to_string()))
         .build_with_sign(|hash| Secp256k1::new().sign_ecdsa_recoverable(hash, &private_key))
         .unwrap();

crates/fiber-lib/src/fiber/tests/payment.rs

@@ -1,5 +1,6 @@
 #![allow(clippy::needless_range_loop)]
 use crate::fiber::channel::*;
+use crate::fiber::config::DEFAULT_FINAL_TLC_EXPIRY_DELTA;
 use crate::fiber::config::DEFAULT_TLC_EXPIRY_DELTA;
 use crate::fiber::config::DEFAULT_TLC_FEE_PROPORTIONAL_MILLIONTHS;
 use crate::fiber::config::MAX_PAYMENT_TLC_EXPIRY_LIMIT;
@@ -3558,7 +3559,7 @@ async fn test_send_payment_with_invalid_tlc_expiry() {
             target_pubkey: Some(nodes[1].pubkey),
             amount: Some(1000),
             keysend: Some(true),
-            tlc_expiry_limit: Some(DEFAULT_TLC_EXPIRY_DELTA + 1), // Ok now
+            tlc_expiry_limit: Some(DEFAULT_FINAL_TLC_EXPIRY_DELTA + 1), // Ok now
             ..Default::default()
         })
         .await;
@@ -3617,7 +3618,7 @@ async fn test_send_payself_with_invalid_tlc_expiry() {
             amount: Some(1000),
             keysend: Some(true),
             allow_self_payment: true,
-            tlc_expiry_limit: Some(DEFAULT_TLC_EXPIRY_DELTA),
+            tlc_expiry_limit: Some(DEFAULT_FINAL_TLC_EXPIRY_DELTA),
             ..Default::default()
         })
         .await;
@@ -6137,7 +6138,7 @@ async fn test_network_with_hops_max_number_limit() {
             keysend: Some(true),
             allow_self_payment: false,
             dry_run: false,
-            tlc_expiry_limit: Some(13 * 24 * 60 * 60 * 1000), // 13 days
+            tlc_expiry_limit: Some(DEFAULT_TLC_EXPIRY_DELTA * 12 + DEFAULT_FINAL_TLC_EXPIRY_DELTA), // 13 hops limit
             ..Default::default()
         })
         .await;
@@ -6152,7 +6153,7 @@ async fn test_network_with_hops_max_number_limit() {
             keysend: Some(true),
             allow_self_payment: false,
             dry_run: false,
-            tlc_expiry_limit: Some(13 * 24 * 60 * 60 * 1000), // 13 days
+            tlc_expiry_limit: Some(DEFAULT_TLC_EXPIRY_DELTA * 12 + DEFAULT_FINAL_TLC_EXPIRY_DELTA), // 13 hops limit
             ..Default::default()
         })
         .await

crates/fiber-lib/src/rpc/README.md

@@ -203,11 +203,11 @@ Attempts to open a channel with a peer.
 * `public` - <em>`Option<bool>`</em>, Whether this is a public channel (will be broadcasted to network, and can be used to forward TLCs), an optional parameter, default value is true.
 * `funding_udt_type_script` - <em>`Option<Script>`</em>, The type script of the UDT to fund the channel with, an optional parameter.
 * `shutdown_script` - <em>`Option<Script>`</em>, The script used to receive the channel balance, an optional parameter, default value is the secp256k1_blake160_sighash_all script corresponding to the configured private key.
-* `commitment_delay_epoch` - <em>`Option<EpochNumberWithFraction>`</em>, The delay time for the commitment transaction, must be an [EpochNumberWithFraction](https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0017-tx-valid-since/e-i-l-encoding.png) in u64 format, an optional parameter, default value is 24 hours, which is 6 epochs.
+* `commitment_delay_epoch` - <em>`Option<EpochNumberWithFraction>`</em>, The delay time for the commitment transaction, must be an [EpochNumberWithFraction](https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0017-tx-valid-since/e-i-l-encoding.png) in u64 format, an optional parameter, default value is 1 epoch, which is 4 hours.
 * `commitment_fee_rate` - <em>`Option<u64>`</em>, The fee rate for the commitment transaction, an optional parameter.
 * `funding_fee_rate` - <em>`Option<u64>`</em>, The fee rate for the funding transaction, an optional parameter.
-* `tlc_expiry_delta` - <em>`Option<u64>`</em>, The expiry delta to forward a tlc, in milliseconds, default to 1 day, which is 24 * 60 * 60 * 1000 milliseconds
- Expect it >= 2/3 commitment_delay_epoch, minimum is 16 hours.
+* `tlc_expiry_delta` - <em>`Option<u64>`</em>, The expiry delta to forward a tlc, in milliseconds, default to 4 hours, which is 4 * 60 * 60 * 1000 milliseconds
+ Expect it >= 2/3 commitment_delay_epoch.
  This parameter can be updated with rpc `update_channel` later.
 * `tlc_min_value` - <em>`Option<u128>`</em>, The minimum value for a TLC our side can send,
  an optional parameter, default is 0, which means we can send any TLC is larger than 0.