Make the base fee configurable in ChannelConfig
authorMatt Corallo <git@bluematt.me>
Mon, 21 Jun 2021 20:20:29 +0000 (20:20 +0000)
committerMatt Corallo <git@bluematt.me>
Fri, 9 Jul 2021 00:50:30 +0000 (00:50 +0000)
Currently the base fee we apply is always the expected cost to
claim an HTLC on-chain in case of closure. This results in
significantly higher than market rate fees [1], and doesn't really
match the actual forwarding trust model anyway - as long as
channel counterparties are honest, our HTLCs shouldn't end up
on-chain no matter what the HTLC sender/recipient do.

While some users may wish to use a feerate that implies they will
not lose funds even if they go to chain (assuming no flood-and-loot
style attacks), they should do so by calculating fees themselves;
since they're already charging well above market-rate,
over-estimating some won't have a large impact.

Worse, we current re-calculate fees at forward-time, not based on
the fee we set in the channel_update. This means that the fees
others expect to pay us (and which they calculate their route based
on), is not what we actually want to charge, and that any attempt
to forward through us is inherently race-y.

This commit adds a configuration knob to set the base fee
explicitly, defaulting to 1 sat, which appears to be market-rate
today.

[1] Note that due to an msat-vs-sat bug we currently actually
    charge 1000x *less* than the calculated cost.

fuzz/src/chanmon_consistency.rs
fuzz/src/full_stack.rs
lightning-background-processor/src/lib.rs
lightning/src/chain/channelmonitor.rs
lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/functional_tests.rs
lightning/src/ln/onion_route_tests.rs
lightning/src/util/config.rs
lightning/src/util/test_utils.rs

index 4aa641d1fa4144275764a9b205ca87ea2b8921c5..dea87715e668fa492145a3865e241d833b89482a 100644 (file)
@@ -338,7 +338,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                        let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}), Arc::clone(&keys_manager)));
 
                        let mut config = UserConfig::default();
-                       config.channel_options.fee_proportional_millionths = 0;
+                       config.channel_options.forwarding_fee_proportional_millionths = 0;
                        config.channel_options.announced_channel = true;
                        let network = Network::Bitcoin;
                        let params = ChainParameters {
@@ -357,7 +357,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
                        let chain_monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}), Arc::clone(& $keys_manager)));
 
                        let mut config = UserConfig::default();
-                       config.channel_options.fee_proportional_millionths = 0;
+                       config.channel_options.forwarding_fee_proportional_millionths = 0;
                        config.channel_options.announced_channel = true;
 
                        let mut monitors = HashMap::new();
index f68cc8f3df7a16bf43d81993c2cff9ad1f963793..8d9a0d491a630442930a7ac852015b23939a5ae0 100644 (file)
@@ -359,7 +359,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 
        let keys_manager = Arc::new(KeyProvider { node_secret: our_network_key.clone(), counter: AtomicU64::new(0) });
        let mut config = UserConfig::default();
-       config.channel_options.fee_proportional_millionths =  slice_to_be32(get_slice!(4));
+       config.channel_options.forwarding_fee_proportional_millionths =  slice_to_be32(get_slice!(4));
        config.channel_options.announced_channel = get_slice!(1)[0] != 0;
        let network = Network::Bitcoin;
        let params = ChainParameters {
index 3a36bb5625eab033b05c5aec3cfa183eb2d67a7d..0b886f7bf4ea2e61bf94ff4ef54014c13b04385b 100644 (file)
@@ -238,7 +238,7 @@ mod tests {
                let mut nodes = Vec::new();
                for i in 0..num_nodes {
                        let tx_broadcaster = Arc::new(test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))});
-                       let fee_estimator = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 });
+                       let fee_estimator = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) });
                        let chain_source = Arc::new(test_utils::TestChainSource::new(Network::Testnet));
                        let logger = Arc::new(test_utils::TestLogger::with_id(format!("node {}", i)));
                        let persister = Arc::new(FilesystemPersister::new(format!("{}_persister_{}", persist_dir, i)));
index b984e521c481830a5960ec1bd6617499ad065fa3..e78faf7764025b92c2ae773cbd8d5e32e4e62395 100644 (file)
@@ -2852,7 +2852,7 @@ mod tests {
                let secp_ctx = Secp256k1::new();
                let logger = Arc::new(TestLogger::new());
                let broadcaster = Arc::new(TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))});
-               let fee_estimator = Arc::new(TestFeeEstimator { sat_per_kw: 253 });
+               let fee_estimator = Arc::new(TestFeeEstimator { sat_per_kw: Mutex::new(253) });
 
                let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };
index 356a88dc4c38382e1dc32dced2045a0504871ba9..dedd1eccf29a85753b7334ac579bf656e1061cab 100644 (file)
@@ -456,7 +456,6 @@ struct CommitmentTxInfoCached {
 }
 
 pub const OUR_MAX_HTLCS: u16 = 50; //TODO
-const SPENDING_INPUT_FOR_A_OUTPUT_WEIGHT: u64 = 79; // prevout: 36, nSequence: 4, script len: 1, witness lengths: (3+1)/4, sig: 73/4, if-selector: 1, redeemScript: (6 ops + 2*33 pubkeys + 1*2 delay)/4
 
 #[cfg(not(test))]
 const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;
@@ -3426,7 +3425,7 @@ impl<Signer: Sign> Channel<Signer> {
        }
 
        pub fn get_fee_proportional_millionths(&self) -> u32 {
-               self.config.fee_proportional_millionths
+               self.config.forwarding_fee_proportional_millionths
        }
 
        pub fn get_cltv_expiry_delta(&self) -> u16 {
@@ -3499,24 +3498,8 @@ impl<Signer: Sign> Channel<Signer> {
 
        /// Gets the fee we'd want to charge for adding an HTLC output to this Channel
        /// Allowed in any state (including after shutdown)
-       pub fn get_holder_fee_base_msat<F: Deref>(&self, fee_estimator: &F) -> u32
-               where F::Target: FeeEstimator
-       {
-               // For lack of a better metric, we calculate what it would cost to consolidate the new HTLC
-               // output value back into a transaction with the regular channel output:
-
-               // the fee cost of the HTLC-Success/HTLC-Timeout transaction:
-               let mut res = self.feerate_per_kw as u64 * cmp::max(HTLC_TIMEOUT_TX_WEIGHT, HTLC_SUCCESS_TX_WEIGHT) / 1000;
-
-               if self.is_outbound() {
-                       // + the marginal fee increase cost to us in the commitment transaction:
-                       res += self.feerate_per_kw as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC / 1000;
-               }
-
-               // + the marginal cost of an input which spends the HTLC-Success/HTLC-Timeout output:
-               res += fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal) as u64 * SPENDING_INPUT_FOR_A_OUTPUT_WEIGHT / 1000;
-
-               res as u32
+       pub fn get_outbound_forwarding_fee_base_msat(&self) -> u32 {
+               self.config.forwarding_fee_base_msat
        }
 
        /// Returns true if we've ever received a message from the remote end for this Channel
@@ -4505,7 +4488,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 
                // Write out the old serialization for the config object. This is read by version-1
                // deserializers, but we will read the version in the TLV at the end instead.
-               self.config.fee_proportional_millionths.write(writer)?;
+               self.config.forwarding_fee_proportional_millionths.write(writer)?;
                self.config.cltv_expiry_delta.write(writer)?;
                self.config.announced_channel.write(writer)?;
                self.config.commit_upfront_shutdown_pubkey.write(writer)?;
@@ -4724,7 +4707,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
                let mut config = Some(ChannelConfig::default());
                if ver == 1 {
                        // Read the old serialization of the ChannelConfig from version 0.0.98.
-                       config.as_mut().unwrap().fee_proportional_millionths = Readable::read(reader)?;
+                       config.as_mut().unwrap().forwarding_fee_proportional_millionths = Readable::read(reader)?;
                        config.as_mut().unwrap().cltv_expiry_delta = Readable::read(reader)?;
                        config.as_mut().unwrap().announced_channel = Readable::read(reader)?;
                        config.as_mut().unwrap().commit_upfront_shutdown_pubkey = Readable::read(reader)?;
index 2f51fbab6bb735c574d6dcaf6d246baebeb6de6f..cf1cd65bb7873a70ce9a59845017dea654716b0d 100644 (file)
@@ -1575,7 +1575,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        if *amt_to_forward < chan.get_counterparty_htlc_minimum_msat() { // amount_below_minimum
                                                break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
-                                       let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_holder_fee_base_msat(&self.fee_estimator) as u64) });
+                                       let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64)
+                                               .and_then(|prop_fee| { (prop_fee / 1000000)
+                                               .checked_add(chan.get_outbound_forwarding_fee_base_msat() as u64) });
                                        if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
                                                break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
@@ -1660,7 +1662,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        cltv_expiry_delta: chan.get_cltv_expiry_delta(),
                        htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
                        htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
-                       fee_base_msat: chan.get_holder_fee_base_msat(&self.fee_estimator),
+                       fee_base_msat: chan.get_outbound_forwarding_fee_base_msat(),
                        fee_proportional_millionths: chan.get_fee_proportional_millionths(),
                        excess_data: Vec::new(),
                };
@@ -5107,7 +5109,7 @@ pub mod bench {
                let genesis_hash = bitcoin::blockdata::constants::genesis_block(network).header.block_hash();
 
                let tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))};
-               let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+               let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
 
                let mut config: UserConfig = Default::default();
                config.own_channel_config.minimum_depth = 1;
index 2e9439916df7b8c64760813d94685621183dba01..f6e80116bc8f1ab17510f5f93015669ce8b64619 100644 (file)
@@ -269,7 +269,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
                        // Check that if we serialize and then deserialize all our channel monitors we get the
                        // same set of outputs to watch for on chain as we have now. Note that if we write
                        // tests that fully close channels and remove the monitors at some point this may break.
-                       let feeest = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+                       let feeest = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
                        let mut deserialized_monitors = Vec::new();
                        {
                                let old_monitors = self.chain_monitor.chain_monitor.monitors.read().unwrap();
@@ -295,7 +295,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
                                <(BlockHash, ChannelManager<EnforcingSigner, &test_utils::TestChainMonitor, &test_utils::TestBroadcaster, &test_utils::TestKeysInterface, &test_utils::TestFeeEstimator, &test_utils::TestLogger>)>::read(&mut ::std::io::Cursor::new(w.0), ChannelManagerReadArgs {
                                        default_config: *self.node.get_current_default_configuration(),
                                        keys_manager: self.keys_manager,
-                                       fee_estimator: &test_utils::TestFeeEstimator { sat_per_kw: 253 },
+                                       fee_estimator: &test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) },
                                        chain_monitor: self.chain_monitor,
                                        tx_broadcaster: &test_utils::TestBroadcaster {
                                                txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()),
@@ -1316,7 +1316,7 @@ pub fn create_chanmon_cfgs(node_count: usize) -> Vec<TestChanMonCfg> {
                        txn_broadcasted: Mutex::new(Vec::new()),
                        blocks: Arc::new(Mutex::new(vec![(genesis_block(Network::Testnet).header, 0)])),
                };
-               let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+               let fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
                let chain_source = test_utils::TestChainSource::new(Network::Testnet);
                let logger = test_utils::TestLogger::with_id(format!("node {}", i));
                let persister = test_utils::TestPersister::new();
@@ -1341,22 +1341,29 @@ pub fn create_node_cfgs<'a>(node_count: usize, chanmon_cfgs: &'a Vec<TestChanMon
        nodes
 }
 
+pub fn test_default_channel_config() -> UserConfig {
+       let mut default_config = UserConfig::default();
+       // Set cltv_expiry_delta slightly lower to keep the final CLTV values inside one byte in our
+       // tests so that our script-length checks don't fail (see ACCEPTED_HTLC_SCRIPT_WEIGHT).
+       default_config.channel_options.cltv_expiry_delta = 6*6;
+       default_config.channel_options.announced_channel = true;
+       default_config.peer_channel_config_limits.force_announced_channel_preference = false;
+       // When most of our tests were written, the default HTLC minimum was fixed at 1000.
+       // It now defaults to 1, so we simply set it to the expected value here.
+       default_config.own_channel_config.our_htlc_minimum_msat = 1000;
+       default_config
+}
+
 pub fn create_node_chanmgrs<'a, 'b>(node_count: usize, cfgs: &'a Vec<NodeCfg<'b>>, node_config: &[Option<UserConfig>]) -> Vec<ChannelManager<EnforcingSigner, &'a TestChainMonitor<'b>, &'b test_utils::TestBroadcaster, &'a test_utils::TestKeysInterface, &'b test_utils::TestFeeEstimator, &'b test_utils::TestLogger>> {
        let mut chanmgrs = Vec::new();
        for i in 0..node_count {
-               let mut default_config = UserConfig::default();
-               // Set cltv_expiry_delta slightly lower to keep the final CLTV values inside one byte in our
-               // tests so that our script-length checks don't fail (see ACCEPTED_HTLC_SCRIPT_WEIGHT).
-               default_config.channel_options.cltv_expiry_delta = 6*6;
-               default_config.channel_options.announced_channel = true;
-               default_config.peer_channel_config_limits.force_announced_channel_preference = false;
-               default_config.own_channel_config.our_htlc_minimum_msat = 1000; // sanitization being done by the sender, to exerce receiver logic we need to lift of limit
                let network = Network::Testnet;
                let params = ChainParameters {
                        network,
                        best_block: BestBlock::from_genesis(network),
                };
-               let node = ChannelManager::new(cfgs[i].fee_estimator, &cfgs[i].chain_monitor, cfgs[i].tx_broadcaster, cfgs[i].logger, cfgs[i].keys_manager, if node_config[i].is_some() { node_config[i].clone().unwrap() } else { default_config }, params);
+               let node = ChannelManager::new(cfgs[i].fee_estimator, &cfgs[i].chain_monitor, cfgs[i].tx_broadcaster, cfgs[i].logger, cfgs[i].keys_manager,
+                       if node_config[i].is_some() { node_config[i].clone().unwrap() } else { test_default_channel_config() }, params);
                chanmgrs.push(node);
        }
 
index 073f3578de60b4419ed196d4b39ae5758942d0fe..9b57ebe0bced8278b41529882f5b6c9b73067008 100644 (file)
@@ -1699,8 +1699,8 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
        // sending any above-dust amount would result in a channel reserve violation.
        // In this test we check that we would be prevented from sending an HTLC in
        // this situation.
-       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
-       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
+       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(6000) };
+       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(6000) };
        let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
@@ -1721,8 +1721,8 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
        // to channel reserve violation. This close could also happen if the fee went
        // up a more realistic amount, but many HTLCs were outstanding at the time of
        // the update_add_htlc.
-       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
-       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
+       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(6000) };
+       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(6000) };
        let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
@@ -1895,7 +1895,11 @@ fn commit_tx_fee_msat(feerate: u32, num_htlcs: u64) -> u64 {
 fn test_channel_reserve_holding_cell_htlcs() {
        let chanmon_cfgs = create_chanmon_cfgs(3);
        let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+       // When this test was written, the default base fee floated based on the HTLC count.
+       // It is now fixed, so we simply set the fee to the expected value here.
+       let mut config = test_default_channel_config();
+       config.channel_options.forwarding_fee_base_msat = 239;
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config.clone()), Some(config.clone()), Some(config.clone())]);
        let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
        let chan_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 190000, 1001, InitFeatures::known(), InitFeatures::known());
        let chan_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 190000, 1001, InitFeatures::known(), InitFeatures::known());
@@ -1916,7 +1920,7 @@ fn test_channel_reserve_holding_cell_htlcs() {
                }}
        }
 
-       let feemsat = 239; // somehow we know?
+       let feemsat = 239; // set above
        let total_fee_msat = (nodes.len() - 2) as u64 * feemsat;
        let feerate = get_feerate!(nodes[0], chan_1.2);
 
@@ -4363,7 +4367,7 @@ fn test_no_txn_manager_serialize_deserialize() {
        nodes[0].chain_monitor.chain_monitor.monitors.read().unwrap().iter().next().unwrap().1.write(&mut chan_0_monitor_serialized).unwrap();
 
        logger = test_utils::TestLogger::new();
-       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
        persister = test_utils::TestPersister::new();
        let keys_manager = &chanmon_cfgs[0].keys_manager;
        new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager);
@@ -4578,7 +4582,7 @@ fn test_manager_serialize_deserialize_events() {
        let mut chan_0_monitor_serialized = test_utils::TestVecWriter(Vec::new());
        nodes[0].chain_monitor.chain_monitor.monitors.read().unwrap().iter().next().unwrap().1.write(&mut chan_0_monitor_serialized).unwrap();
 
-       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
        logger = test_utils::TestLogger::new();
        persister = test_utils::TestPersister::new();
        let keys_manager = &chanmon_cfgs[0].keys_manager;
@@ -4666,7 +4670,7 @@ fn test_simple_manager_serialize_deserialize() {
        nodes[0].chain_monitor.chain_monitor.monitors.read().unwrap().iter().next().unwrap().1.write(&mut chan_0_monitor_serialized).unwrap();
 
        logger = test_utils::TestLogger::new();
-       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
        persister = test_utils::TestPersister::new();
        let keys_manager = &chanmon_cfgs[0].keys_manager;
        new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager);
@@ -4746,7 +4750,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() {
        }
 
        logger = test_utils::TestLogger::new();
-       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
        persister = test_utils::TestPersister::new();
        let keys_manager = &chanmon_cfgs[0].keys_manager;
        new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager);
@@ -5344,7 +5348,12 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
        // we forward one of the payments onwards to D.
        let chanmon_cfgs = create_chanmon_cfgs(4);
        let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
+       // When this test was written, the default base fee floated based on the HTLC count.
+       // It is now fixed, so we simply set the fee to the expected value here.
+       let mut config = test_default_channel_config();
+       config.channel_options.forwarding_fee_base_msat = 196;
+       let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs,
+               &[Some(config.clone()), Some(config.clone()), Some(config.clone()), Some(config.clone())]);
        let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs);
 
        create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
@@ -5531,7 +5540,12 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
        // And test where C fails back to A/B when D announces its latest commitment transaction
        let chanmon_cfgs = create_chanmon_cfgs(6);
        let node_cfgs = create_node_cfgs(6, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(6, &node_cfgs, &[None, None, None, None, None, None]);
+       // When this test was written, the default base fee floated based on the HTLC count.
+       // It is now fixed, so we simply set the fee to the expected value here.
+       let mut config = test_default_channel_config();
+       config.channel_options.forwarding_fee_base_msat = 196;
+       let node_chanmgrs = create_node_chanmgrs(6, &node_cfgs,
+               &[Some(config.clone()), Some(config.clone()), Some(config.clone()), Some(config.clone()), Some(config.clone()), Some(config.clone())]);
        let nodes = create_network(6, &node_cfgs, &node_chanmgrs);
        let logger = test_utils::TestLogger::new();
 
@@ -6381,7 +6395,11 @@ fn test_free_and_fail_holding_cell_htlcs() {
 fn test_fail_holding_cell_htlc_upon_free_multihop() {
        let chanmon_cfgs = create_chanmon_cfgs(3);
        let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+       // When this test was written, the default base fee floated based on the HTLC count.
+       // It is now fixed, so we simply set the fee to the expected value here.
+       let mut config = test_default_channel_config();
+       config.channel_options.forwarding_fee_base_msat = 196;
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config.clone()), Some(config.clone()), Some(config.clone())]);
        let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
        let chan_0_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
        let chan_1_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
@@ -7599,7 +7617,7 @@ fn test_user_configurable_csv_delay() {
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
        // We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound()
-       if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), 1000000, 1000000, 0, &low_our_to_self_config) {
+       if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), 1000000, 1000000, 0, &low_our_to_self_config) {
                match error {
                        APIError::APIMisuseError { err } => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); },
                        _ => panic!("Unexpected event"),
@@ -7610,7 +7628,7 @@ fn test_user_configurable_csv_delay() {
        nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap();
        let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id());
        open_channel.to_self_delay = 200;
-       if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &low_our_to_self_config) {
+       if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &low_our_to_self_config) {
                match error {
                        ChannelError::Close(err) => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str()));  },
                        _ => panic!("Unexpected event"),
@@ -7636,7 +7654,7 @@ fn test_user_configurable_csv_delay() {
        nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap();
        let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id());
        open_channel.to_self_delay = 200;
-       if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &high_their_to_self_config) {
+       if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &high_their_to_self_config) {
                match error {
                        ChannelError::Close(err) => { assert!(regex::Regex::new(r"They wanted our payments to be delayed by a needlessly long period\. Upper limit: \d+\. Actual: \d+").unwrap().is_match(err.as_str())); },
                        _ => panic!("Unexpected event"),
@@ -7686,7 +7704,7 @@ fn test_data_loss_protect() {
        let mut chain_monitor = <(BlockHash, ChannelMonitor<EnforcingSigner>)>::read(&mut ::std::io::Cursor::new(previous_chain_monitor_state.0), keys_manager).unwrap().1;
        chain_source = test_utils::TestChainSource::new(Network::Testnet);
        tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))};
-       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 };
+       fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(253) };
        persister = test_utils::TestPersister::new();
        monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, keys_manager);
        node_state_0 = {
index 3fc01f1bb8b4ea7ce285091d9c2a4aea7170ec5d..620864308c4db7c247c67c1ffb7d87d483faf0ac 100644 (file)
 
 use chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS};
 use ln::{PaymentPreimage, PaymentHash, PaymentSecret};
-use ln::channelmanager::HTLCForwardInfo;
+use ln::channelmanager::{HTLCForwardInfo, CLTV_FAR_FAR_AWAY};
 use ln::onion_utils;
 use routing::router::{Route, get_route};
 use ln::features::{InitFeatures, InvoiceFeatures};
 use ln::msgs;
-use ln::msgs::{ChannelMessageHandler, HTLCFailChannelUpdate, OptionalField};
+use ln::msgs::{ChannelMessageHandler, ChannelUpdate, HTLCFailChannelUpdate, OptionalField};
 use util::test_utils;
 use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
 use util::ser::{Writeable, Writer};
@@ -29,6 +29,7 @@ use bitcoin::hash_types::BlockHash;
 use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hashes::Hash;
 
+use bitcoin::secp256k1;
 use bitcoin::secp256k1::Secp256k1;
 use bitcoin::secp256k1::key::SecretKey;
 
@@ -240,17 +241,58 @@ impl Writeable for BogusOnionHopData {
        }
 }
 
+const BADONION: u16 = 0x8000;
+const PERM: u16 = 0x4000;
+const NODE: u16 = 0x2000;
+const UPDATE: u16 = 0x1000;
+
 #[test]
-fn test_onion_failure() {
-       use ln::msgs::ChannelUpdate;
-       use ln::channelmanager::CLTV_FAR_FAR_AWAY;
-       use bitcoin::secp256k1;
+fn test_fee_failures() {
+       // Tests that the fee required when forwarding remains consistent over time. This was
+       // previously broken, with forwarding fees floating based on the fee estimator at the time of
+       // forwarding.
+       //
+       // When this test was written, the default base fee floated based on the HTLC count.
+       // It is now fixed, so we simply set the fee to the expected value here.
+       let mut config = test_default_channel_config();
+       config.channel_options.forwarding_fee_base_msat = 196;
+
+       let chanmon_cfgs = create_chanmon_cfgs(3);
+       let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config), Some(config), Some(config)]);
+       let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+       let channels = [create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()), create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known())];
+       let logger = test_utils::TestLogger::new();
+       let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 40_000, TEST_FINAL_CLTV, &logger).unwrap();
+
+       // positive case
+       let (payment_preimage_success, payment_hash_success, payment_secret_success) = get_payment_preimage_hash!(nodes[2]);
+       nodes[0].node.send_payment(&route, payment_hash_success, &Some(payment_secret_success)).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       pass_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], 40_000, payment_hash_success, payment_secret_success);
+       claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_success);
+
+       let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]);
+       run_onion_failure_test("fee_insufficient", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| {
+               msg.amount_msat -= 1;
+       }, || {}, true, Some(UPDATE|12), Some(msgs::HTLCFailChannelUpdate::ChannelClosed { short_channel_id: channels[0].0.contents.short_channel_id, is_permanent: true}));
 
-       const BADONION: u16 = 0x8000;
-       const PERM: u16 = 0x4000;
-       const NODE: u16 = 0x2000;
-       const UPDATE: u16 = 0x1000;
+       // In an earlier version, we spuriously failed to forward payments if the expected feerate
+       // changed between the channel open and the payment.
+       {
+               let mut feerate_lock = chanmon_cfgs[1].fee_estimator.sat_per_kw.lock().unwrap();
+               *feerate_lock *= 2;
+       }
 
+       let (payment_preimage_success, payment_hash_success, payment_secret_success) = get_payment_preimage_hash!(nodes[2]);
+       nodes[0].node.send_payment(&route, payment_hash_success, &Some(payment_secret_success)).unwrap();
+       check_added_monitors!(nodes[0], 1);
+       pass_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], 40_000, payment_hash_success, payment_secret_success);
+       claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_success);
+}
+
+#[test]
+fn test_onion_failure() {
        // When we check for amount_below_minimum below, we want to test that we're using the *right*
        // amount, thus we need different htlc_minimum_msat values. We set node[2]'s htlc_minimum_msat
        // to 2000, which is above the default value of 1000 set in create_node_chanmgrs.
@@ -261,9 +303,14 @@ fn test_onion_failure() {
        node_2_cfg.channel_options.announced_channel = true;
        node_2_cfg.peer_channel_config_limits.force_announced_channel_preference = false;
 
+       // When this test was written, the default base fee floated based on the HTLC count.
+       // It is now fixed, so we simply set the fee to the expected value here.
+       let mut config = test_default_channel_config();
+       config.channel_options.forwarding_fee_base_msat = 196;
+
        let chanmon_cfgs = create_chanmon_cfgs(3);
        let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
-       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, Some(node_2_cfg)]);
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config), Some(config), Some(node_2_cfg)]);
        let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
        for node in nodes.iter() {
                *node.keys_manager.override_session_priv.lock().unwrap() = Some([3; 32]);
index d5ff2885cc4ae4131c95a63a9b8ab2095fd7bb15..f3fd0ce70793451fbe6e58ce3e2efc81ca84b3b5 100644 (file)
@@ -140,12 +140,26 @@ impl Default for ChannelHandshakeLimits {
 /// with our counterparty.
 #[derive(Copy, Clone, Debug)]
 pub struct ChannelConfig {
-       /// Amount (in millionths of a satoshi) the channel will charge per transferred satoshi.
+       /// Amount (in millionths of a satoshi) charged per satoshi for payments forwarded outbound
+       /// over the channel.
        /// This may be allowed to change at runtime in a later update, however doing so must result in
        /// update messages sent to notify all nodes of our updated relay fee.
        ///
        /// Default value: 0.
-       pub fee_proportional_millionths: u32,
+       pub forwarding_fee_proportional_millionths: u32,
+       /// Amount (in milli-satoshi) charged for payments forwarded outbound over the channel, in
+       /// excess of [`forwarding_fee_proportional_millionths`].
+       /// This may be allowed to change at runtime in a later update, however doing so must result in
+       /// update messages sent to notify all nodes of our updated relay fee.
+       ///
+       /// The default value of a single satoshi roughly matches the market rate on many routing nodes
+       /// as of July 2021. Adjusting it upwards or downwards may change whether nodes route through
+       /// this node.
+       ///
+       /// Default value: 1000.
+       ///
+       /// [`forwarding_fee_proportional_millionths`]: ChannelConfig::forwarding_fee_proportional_millionths
+       pub forwarding_fee_base_msat: u32,
        /// The difference in the CLTV value between incoming HTLCs and an outbound HTLC forwarded over
        /// the channel this config applies to.
        ///
@@ -196,7 +210,8 @@ impl Default for ChannelConfig {
        /// Provides sane defaults for most configurations (but with zero relay fees!).
        fn default() -> Self {
                ChannelConfig {
-                       fee_proportional_millionths: 0,
+                       forwarding_fee_proportional_millionths: 0,
+                       forwarding_fee_base_msat: 1000,
                        cltv_expiry_delta: 6 * 12, // 6 blocks/hour * 12 hours
                        announced_channel: false,
                        commit_upfront_shutdown_pubkey: true,
@@ -205,10 +220,11 @@ impl Default for ChannelConfig {
 }
 
 impl_writeable_tlv_based!(ChannelConfig, {
-       (0, fee_proportional_millionths, required),
+       (0, forwarding_fee_proportional_millionths, required),
        (2, cltv_expiry_delta, required),
        (4, announced_channel, required),
        (6, commit_upfront_shutdown_pubkey, required),
+       (8, forwarding_fee_base_msat, required),
 });
 
 /// Top-level config which holds ChannelHandshakeLimits and ChannelConfig.
index adaf631f8a93b4f63d68ae71bde3441e47635b92..2775697b1dbb1d672e396341848b08b6b5c059bf 100644 (file)
@@ -56,11 +56,11 @@ impl Writer for TestVecWriter {
 }
 
 pub struct TestFeeEstimator {
-       pub sat_per_kw: u32,
+       pub sat_per_kw: Mutex<u32>,
 }
 impl chaininterface::FeeEstimator for TestFeeEstimator {
        fn get_est_sat_per_1000_weight(&self, _confirmation_target: ConfirmationTarget) -> u32 {
-               self.sat_per_kw
+               *self.sat_per_kw.lock().unwrap()
        }
 }