Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TheBlueMatt merged 6 commits into from
Jul 9, 2021
Merged

Make the base fee configurable in ChannelConfig #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dac8b7b
Update ChannelConfig serialization to be TLV-based
TheBlueMatt Jun 21, 2021
c620944
Make the base fee configurable in ChannelConfig
TheBlueMatt Jun 21, 2021
99953ed
Update full_stack_target demo input to match new, fewer, fee gets
TheBlueMatt Jul 3, 2021
520b53e
Optionally reject HTLC forwards over priv chans with a new config
TheBlueMatt Jun 26, 2021
dbfccf0
Add a note clarifying the API guarantees of create_channel
TheBlueMatt Jun 26, 2021
4cc0d9d
Change serialization backwards compat in Channel to use new version
TheBlueMatt Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down
12 changes: 3 additions & 9 deletions fuzz/src/full_stack.rs
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion lightning-background-processor/src/lib.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down
82 changes: 47 additions & 35 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;

@ariardariardJul 8, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?

Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?

@TheBlueMattTheBlueMattJul 8, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)

const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

self.user_id.write(writer)?;
self.config.write(writer)?;

// 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.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)?;

self.channel_id.write(writer)?;
(self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
self.counterparty_dust_limit_satoshis.write(writer)?;
self.holder_dust_limit_satoshis.write(writer)?;
self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

// Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

self.counterparty_htlc_minimum_msat.write(writer)?;
self.holder_htlc_minimum_msat.write(writer)?;
self.counterparty_max_accepted_htlcs.write(writer)?;

// Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
self.minimum_depth.unwrap_or(0).write(writer)?;

match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
// override that.
(1, self.minimum_depth, option),
(3, self.counterparty_selected_channel_reserve_satoshis, option),
(5, self.config, required),
});

Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
where K::Target: KeysInterface<Signer = Signer> {
fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

let user_id = Readable::read(reader)?;
let config: ChannelConfig = Readable::read(reader)?;

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().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)?;
} else {
// Read the 8 bytes of backwards-compatibility ChannelConfig data.
let mut _val: u64 = Readable::read(reader)?;
}

let channel_id = Readable::read(reader)?;
let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
let counterparty_dust_limit_satoshis = Readable::read(reader)?;
let holder_dust_limit_satoshis = Readable::read(reader)?;
let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
if counterparty_selected_channel_reserve_satoshis == Some(0) {
// Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
// non-option, writing 0 for what we now consider None.
counterparty_selected_channel_reserve_satoshis = None;
let mut counterparty_selected_channel_reserve_satoshis = None;
if ver == 1 {
// Read the old serialization from version 0.0.98.
counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
} else {
// Read the 8 bytes of backwards-compatibility data.
let _dummy: u64 = Readable::read(reader)?;
}
let counterparty_htlc_minimum_msat = Readable::read(reader)?;
let holder_htlc_minimum_msat = Readable::read(reader)?;
let counterparty_max_accepted_htlcs = Readable::read(reader)?;
let mut minimum_depth = Some(Readable::read(reader)?);
if minimum_depth == Some(0) {
// Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
// consider None.
minimum_depth = None;

let mut minimum_depth = None;
if ver == 1 {
// Read the old serialization from version 0.0.98.
minimum_depth = Some(Readable::read(reader)?);
} else {
// Read the 4 bytes of backwards-compatibility data.
let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.
}

let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
(0, announcement_sigs, option),
(1, minimum_depth, option),
(3, counterparty_selected_channel_reserve_satoshis, option),
(5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
});

let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
Ok(Channel {
user_id,

config,
config: config.unwrap(),
channel_id,
channel_state,
secp_ctx,
Expand Down
32 changes: 23 additions & 9 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
///
/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
///
/// Note that we do not check if you are currently connected to the given peer. If no
/// connection is available, the outbound `open_channel` message may fail to send, resulting in
/// the channel eventually being silently forgotten.

@ariardariardJul 6, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?

@TheBlueMattTheBlueMattJul 6, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.

pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
if channel_value_satoshis < 1000 {
return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// short_channel_id is non-0 in any ::Forward.
if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
let forwarding_id = match id_option {
None => { // unknown_next_peer
return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
},
Some(id) => id.clone(),
};
if let Some((err, code, chan_update)) = loop {
let forwarding_id = match id_option {
None => { // unknown_next_peer
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
},
Some(id) => id.clone(),
};

let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
// Note that the behavior here should be identical to the above block - we
// should NOT reveal the existence or non-existence of a private channel if
// we don't allow forwards outbound over them.
break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));

@ariardariardJul 1, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.

Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.

@TheBlueMattTheBlueMattJul 1, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.

@ariardariardJul 5, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...

}

// Note that we could technically not return an error yet here and just hope
// that the connection is reestablished or monitor updated by the time we get
// around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
}
Expand DownExpand Up@@ -1660,7 +1674,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(),
};
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down
34 changes: 22 additions & 12 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
let logger = test_utils::TestLogger::new();
let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
&expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
recv_value, TEST_FINAL_CLTV, &logger).unwrap();
assert_eq!(route.paths.len(), 1);
assert_eq!(route.paths[0].len(), expected_route.len());
for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
}

Expand Down
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 6 commits into

from

Jul 9, 2021
 Merged


Make the base fee configurable in ChannelConfig
#975
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
dac8b7b

Update ChannelConfig serialization to be TLV-based


TheBlueMatt Jun 21, 2021
c620944

Make the base fee configurable in ChannelConfig


TheBlueMatt Jun 21, 2021
99953ed

Update full_stack_target demo input to match new, fewer, fee gets


TheBlueMatt Jul 3, 2021
520b53e

Optionally reject HTLC forwards over priv chans with a new config


TheBlueMatt Jun 26, 2021
dbfccf0

Add a note clarifying the API guarantees of create_channel


TheBlueMatt Jun 26, 2021
4cc0d9d

Change serialization backwards compat in Channel to use new version


TheBlueMatt Jul 6, 2021

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down

12 changes: 3 additions & 9 deletions
fuzz/src/full_stack.rs

View file


Open in desktop


Large diffs are not rendered by default.


2 changes: 1 addition & 1 deletion
lightning-background-processor/src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down

2 changes: 1 addition & 1 deletion
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down

82 changes: 47 additions & 35 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;
@ariardariardJul 8, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
@TheBlueMattTheBlueMattJul 8, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

 self.user_id.write(writer)?;
 self.config.write(writer)?;

 // 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.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)?;

 self.channel_id.write(writer)?;
 (self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 self.counterparty_dust_limit_satoshis.write(writer)?;
 self.holder_dust_limit_satoshis.write(writer)?;
 self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

 self.counterparty_htlc_minimum_msat.write(writer)?;
 self.holder_htlc_minimum_msat.write(writer)?;
 self.counterparty_max_accepted_htlcs.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.minimum_depth.unwrap_or(0).write(writer)?;

 match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 // override that.
 (1, self.minimum_depth, option),
 (3, self.counterparty_selected_channel_reserve_satoshis, option),
 (5, self.config, required),
 });

 Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 where K::Target: KeysInterface<Signer = Signer> {
	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

 let user_id = Readable::read(reader)?;
 let config: ChannelConfig = Readable::read(reader)?;

 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().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)?;
 } else {
 // Read the 8 bytes of backwards-compatibility ChannelConfig data.
 let mut _val: u64 = Readable::read(reader)?;
 }

 let channel_id = Readable::read(reader)?;
 let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 let holder_dust_limit_satoshis = Readable::read(reader)?;
 let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
 let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 if counterparty_selected_channel_reserve_satoshis == Some(0) {
 // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
 // non-option, writing 0 for what we now consider None.
 counterparty_selected_channel_reserve_satoshis = None;
 let mut counterparty_selected_channel_reserve_satoshis = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 } else {
 // Read the 8 bytes of backwards-compatibility data.
 let _dummy: u64 = Readable::read(reader)?;
 }
 let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 let holder_htlc_minimum_msat = Readable::read(reader)?;
 let counterparty_max_accepted_htlcs = Readable::read(reader)?;
 let mut minimum_depth = Some(Readable::read(reader)?);
 if minimum_depth == Some(0) {
 // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
 // consider None.
 minimum_depth = None;

 let mut minimum_depth = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 minimum_depth = Some(Readable::read(reader)?);
 } else {
 // Read the 4 bytes of backwards-compatibility data.
 let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.

 }

 let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 (0, announcement_sigs, option),
 (1, minimum_depth, option),
 (3, counterparty_selected_channel_reserve_satoshis, option),
 (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 });

 let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 Ok(Channel {
 user_id,

 config,
 config: config.unwrap(),
 channel_id,
 channel_state,
 secp_ctx,
Expand Down

32 changes: 23 additions & 9 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
	///
	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
	///
	/// Note that we do not check if you are currently connected to the given peer. If no
	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
	/// the channel eventually being silently forgotten.
@ariardariardJul 6, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
@TheBlueMattTheBlueMattJul 6, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.
	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 if channel_value_satoshis < 1000 {
 return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // short_channel_id is non-0 in any ::Forward.
 if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
 },
 Some(id) => id.clone(),
 };
 if let Some((err, code, chan_update)) = loop {
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
 },
 Some(id) => id.clone(),
 };

 let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

 if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 // Note that the behavior here should be identical to the above block - we
 // should NOT reveal the existence or non-existence of a private channel if
 // we don't allow forwards outbound over them.
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
@ariardariardJul 1, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
@TheBlueMattTheBlueMattJul 1, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
@ariardariardJul 5, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...
 }

 // Note that we could technically not return an error yet here and just hope
 // that the connection is reestablished or monitor updated by the time we get
 // around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
 }
Expand DownExpand Up@@ -1660,7 +1674,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(),
 };
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down

34 changes: 22 additions & 12 deletions
lightning/src/ln/functional_test_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
	let logger = test_utils::TestLogger::new();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
 &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
 Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
 recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	assert_eq!(route.paths.len(), 1);
	assert_eq!(route.paths[0].len(), expected_route.len());
	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
	}

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 6 commits into

from

Jul 9, 2021
 Merged


Make the base fee configurable in ChannelConfig
#975
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
dac8b7b

Update ChannelConfig serialization to be TLV-based


TheBlueMatt Jun 21, 2021
c620944

Make the base fee configurable in ChannelConfig


TheBlueMatt Jun 21, 2021
99953ed

Update full_stack_target demo input to match new, fewer, fee gets


TheBlueMatt Jul 3, 2021
520b53e

Optionally reject HTLC forwards over priv chans with a new config


TheBlueMatt Jun 26, 2021
dbfccf0

Add a note clarifying the API guarantees of create_channel


TheBlueMatt Jun 26, 2021
4cc0d9d

Change serialization backwards compat in Channel to use new version


TheBlueMatt Jul 6, 2021

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down

12 changes: 3 additions & 9 deletions
fuzz/src/full_stack.rs

View file


Open in desktop


Large diffs are not rendered by default.


2 changes: 1 addition & 1 deletion
lightning-background-processor/src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down

2 changes: 1 addition & 1 deletion
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down

82 changes: 47 additions & 35 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;
@ariardariardJul 8, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
@TheBlueMattTheBlueMattJul 8, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

 self.user_id.write(writer)?;
 self.config.write(writer)?;

 // 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.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)?;

 self.channel_id.write(writer)?;
 (self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 self.counterparty_dust_limit_satoshis.write(writer)?;
 self.holder_dust_limit_satoshis.write(writer)?;
 self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

 self.counterparty_htlc_minimum_msat.write(writer)?;
 self.holder_htlc_minimum_msat.write(writer)?;
 self.counterparty_max_accepted_htlcs.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.minimum_depth.unwrap_or(0).write(writer)?;

 match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 // override that.
 (1, self.minimum_depth, option),
 (3, self.counterparty_selected_channel_reserve_satoshis, option),
 (5, self.config, required),
 });

 Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 where K::Target: KeysInterface<Signer = Signer> {
	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

 let user_id = Readable::read(reader)?;
 let config: ChannelConfig = Readable::read(reader)?;

 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().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)?;
 } else {
 // Read the 8 bytes of backwards-compatibility ChannelConfig data.
 let mut _val: u64 = Readable::read(reader)?;
 }

 let channel_id = Readable::read(reader)?;
 let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 let holder_dust_limit_satoshis = Readable::read(reader)?;
 let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
 let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 if counterparty_selected_channel_reserve_satoshis == Some(0) {
 // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
 // non-option, writing 0 for what we now consider None.
 counterparty_selected_channel_reserve_satoshis = None;
 let mut counterparty_selected_channel_reserve_satoshis = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 } else {
 // Read the 8 bytes of backwards-compatibility data.
 let _dummy: u64 = Readable::read(reader)?;
 }
 let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 let holder_htlc_minimum_msat = Readable::read(reader)?;
 let counterparty_max_accepted_htlcs = Readable::read(reader)?;
 let mut minimum_depth = Some(Readable::read(reader)?);
 if minimum_depth == Some(0) {
 // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
 // consider None.
 minimum_depth = None;

 let mut minimum_depth = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 minimum_depth = Some(Readable::read(reader)?);
 } else {
 // Read the 4 bytes of backwards-compatibility data.
 let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.

 }

 let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 (0, announcement_sigs, option),
 (1, minimum_depth, option),
 (3, counterparty_selected_channel_reserve_satoshis, option),
 (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 });

 let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 Ok(Channel {
 user_id,

 config,
 config: config.unwrap(),
 channel_id,
 channel_state,
 secp_ctx,
Expand Down

32 changes: 23 additions & 9 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
	///
	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
	///
	/// Note that we do not check if you are currently connected to the given peer. If no
	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
	/// the channel eventually being silently forgotten.
@ariardariardJul 6, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
@TheBlueMattTheBlueMattJul 6, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.
	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 if channel_value_satoshis < 1000 {
 return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // short_channel_id is non-0 in any ::Forward.
 if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
 },
 Some(id) => id.clone(),
 };
 if let Some((err, code, chan_update)) = loop {
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
 },
 Some(id) => id.clone(),
 };

 let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

 if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 // Note that the behavior here should be identical to the above block - we
 // should NOT reveal the existence or non-existence of a private channel if
 // we don't allow forwards outbound over them.
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
@ariardariardJul 1, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
@TheBlueMattTheBlueMattJul 1, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
@ariardariardJul 5, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...
 }

 // Note that we could technically not return an error yet here and just hope
 // that the connection is reestablished or monitor updated by the time we get
 // around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
 }
Expand DownExpand Up@@ -1660,7 +1674,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(),
 };
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down

34 changes: 22 additions & 12 deletions
lightning/src/ln/functional_test_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
	let logger = test_utils::TestLogger::new();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
 &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
 Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
 recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	assert_eq!(route.paths.len(), 1);
	assert_eq!(route.paths[0].len(), expected_route.len());
	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
	}

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 6 commits into

from

Jul 9, 2021
 Merged


Make the base fee configurable in ChannelConfig
#975
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
dac8b7b

Update ChannelConfig serialization to be TLV-based


TheBlueMatt Jun 21, 2021
c620944

Make the base fee configurable in ChannelConfig


TheBlueMatt Jun 21, 2021
99953ed

Update full_stack_target demo input to match new, fewer, fee gets


TheBlueMatt Jul 3, 2021
520b53e

Optionally reject HTLC forwards over priv chans with a new config


TheBlueMatt Jun 26, 2021
dbfccf0

Add a note clarifying the API guarantees of create_channel


TheBlueMatt Jun 26, 2021
4cc0d9d

Change serialization backwards compat in Channel to use new version


TheBlueMatt Jul 6, 2021

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down

12 changes: 3 additions & 9 deletions
fuzz/src/full_stack.rs

View file


Open in desktop


Large diffs are not rendered by default.


2 changes: 1 addition & 1 deletion
lightning-background-processor/src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down

2 changes: 1 addition & 1 deletion
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down

82 changes: 47 additions & 35 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;
@ariardariardJul 8, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
@TheBlueMattTheBlueMattJul 8, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

 self.user_id.write(writer)?;
 self.config.write(writer)?;

 // 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.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)?;

 self.channel_id.write(writer)?;
 (self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 self.counterparty_dust_limit_satoshis.write(writer)?;
 self.holder_dust_limit_satoshis.write(writer)?;
 self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

 self.counterparty_htlc_minimum_msat.write(writer)?;
 self.holder_htlc_minimum_msat.write(writer)?;
 self.counterparty_max_accepted_htlcs.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.minimum_depth.unwrap_or(0).write(writer)?;

 match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 // override that.
 (1, self.minimum_depth, option),
 (3, self.counterparty_selected_channel_reserve_satoshis, option),
 (5, self.config, required),
 });

 Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 where K::Target: KeysInterface<Signer = Signer> {
	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

 let user_id = Readable::read(reader)?;
 let config: ChannelConfig = Readable::read(reader)?;

 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().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)?;
 } else {
 // Read the 8 bytes of backwards-compatibility ChannelConfig data.
 let mut _val: u64 = Readable::read(reader)?;
 }

 let channel_id = Readable::read(reader)?;
 let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 let holder_dust_limit_satoshis = Readable::read(reader)?;
 let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
 let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 if counterparty_selected_channel_reserve_satoshis == Some(0) {
 // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
 // non-option, writing 0 for what we now consider None.
 counterparty_selected_channel_reserve_satoshis = None;
 let mut counterparty_selected_channel_reserve_satoshis = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 } else {
 // Read the 8 bytes of backwards-compatibility data.
 let _dummy: u64 = Readable::read(reader)?;
 }
 let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 let holder_htlc_minimum_msat = Readable::read(reader)?;
 let counterparty_max_accepted_htlcs = Readable::read(reader)?;
 let mut minimum_depth = Some(Readable::read(reader)?);
 if minimum_depth == Some(0) {
 // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
 // consider None.
 minimum_depth = None;

 let mut minimum_depth = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 minimum_depth = Some(Readable::read(reader)?);
 } else {
 // Read the 4 bytes of backwards-compatibility data.
 let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.

 }

 let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 (0, announcement_sigs, option),
 (1, minimum_depth, option),
 (3, counterparty_selected_channel_reserve_satoshis, option),
 (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 });

 let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 Ok(Channel {
 user_id,

 config,
 config: config.unwrap(),
 channel_id,
 channel_state,
 secp_ctx,
Expand Down

32 changes: 23 additions & 9 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
	///
	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
	///
	/// Note that we do not check if you are currently connected to the given peer. If no
	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
	/// the channel eventually being silently forgotten.
@ariardariardJul 6, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
@TheBlueMattTheBlueMattJul 6, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.
	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 if channel_value_satoshis < 1000 {
 return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // short_channel_id is non-0 in any ::Forward.
 if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
 },
 Some(id) => id.clone(),
 };
 if let Some((err, code, chan_update)) = loop {
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
 },
 Some(id) => id.clone(),
 };

 let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

 if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 // Note that the behavior here should be identical to the above block - we
 // should NOT reveal the existence or non-existence of a private channel if
 // we don't allow forwards outbound over them.
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
@ariardariardJul 1, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
@TheBlueMattTheBlueMattJul 1, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
@ariardariardJul 5, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...
 }

 // Note that we could technically not return an error yet here and just hope
 // that the connection is reestablished or monitor updated by the time we get
 // around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
 }
Expand DownExpand Up@@ -1660,7 +1674,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(),
 };
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down

34 changes: 22 additions & 12 deletions
lightning/src/ln/functional_test_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
	let logger = test_utils::TestLogger::new();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
 &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
 Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
 recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	assert_eq!(route.paths.len(), 1);
	assert_eq!(route.paths[0].len(), expected_route.len());
	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
	}

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 6 commits into

from

Jul 9, 2021
 Merged


Make the base fee configurable in ChannelConfig
#975
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
dac8b7b

Update ChannelConfig serialization to be TLV-based


TheBlueMatt Jun 21, 2021
c620944

Make the base fee configurable in ChannelConfig


TheBlueMatt Jun 21, 2021
99953ed

Update full_stack_target demo input to match new, fewer, fee gets


TheBlueMatt Jul 3, 2021
520b53e

Optionally reject HTLC forwards over priv chans with a new config


TheBlueMatt Jun 26, 2021
dbfccf0

Add a note clarifying the API guarantees of create_channel


TheBlueMatt Jun 26, 2021
4cc0d9d

Change serialization backwards compat in Channel to use new version


TheBlueMatt Jul 6, 2021

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down

12 changes: 3 additions & 9 deletions
fuzz/src/full_stack.rs

View file


Open in desktop


Large diffs are not rendered by default.


2 changes: 1 addition & 1 deletion
lightning-background-processor/src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down

2 changes: 1 addition & 1 deletion
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down

82 changes: 47 additions & 35 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;
@ariardariardJul 8, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
@TheBlueMattTheBlueMattJul 8, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

 self.user_id.write(writer)?;
 self.config.write(writer)?;

 // 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.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)?;

 self.channel_id.write(writer)?;
 (self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 self.counterparty_dust_limit_satoshis.write(writer)?;
 self.holder_dust_limit_satoshis.write(writer)?;
 self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

 self.counterparty_htlc_minimum_msat.write(writer)?;
 self.holder_htlc_minimum_msat.write(writer)?;
 self.counterparty_max_accepted_htlcs.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.minimum_depth.unwrap_or(0).write(writer)?;

 match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 // override that.
 (1, self.minimum_depth, option),
 (3, self.counterparty_selected_channel_reserve_satoshis, option),
 (5, self.config, required),
 });

 Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 where K::Target: KeysInterface<Signer = Signer> {
	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

 let user_id = Readable::read(reader)?;
 let config: ChannelConfig = Readable::read(reader)?;

 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().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)?;
 } else {
 // Read the 8 bytes of backwards-compatibility ChannelConfig data.
 let mut _val: u64 = Readable::read(reader)?;
 }

 let channel_id = Readable::read(reader)?;
 let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 let holder_dust_limit_satoshis = Readable::read(reader)?;
 let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
 let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 if counterparty_selected_channel_reserve_satoshis == Some(0) {
 // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
 // non-option, writing 0 for what we now consider None.
 counterparty_selected_channel_reserve_satoshis = None;
 let mut counterparty_selected_channel_reserve_satoshis = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 } else {
 // Read the 8 bytes of backwards-compatibility data.
 let _dummy: u64 = Readable::read(reader)?;
 }
 let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 let holder_htlc_minimum_msat = Readable::read(reader)?;
 let counterparty_max_accepted_htlcs = Readable::read(reader)?;
 let mut minimum_depth = Some(Readable::read(reader)?);
 if minimum_depth == Some(0) {
 // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
 // consider None.
 minimum_depth = None;

 let mut minimum_depth = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 minimum_depth = Some(Readable::read(reader)?);
 } else {
 // Read the 4 bytes of backwards-compatibility data.
 let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.

 }

 let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 (0, announcement_sigs, option),
 (1, minimum_depth, option),
 (3, counterparty_selected_channel_reserve_satoshis, option),
 (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 });

 let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 Ok(Channel {
 user_id,

 config,
 config: config.unwrap(),
 channel_id,
 channel_state,
 secp_ctx,
Expand Down

32 changes: 23 additions & 9 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
	///
	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
	///
	/// Note that we do not check if you are currently connected to the given peer. If no
	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
	/// the channel eventually being silently forgotten.
@ariardariardJul 6, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
@TheBlueMattTheBlueMattJul 6, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.
	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 if channel_value_satoshis < 1000 {
 return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // short_channel_id is non-0 in any ::Forward.
 if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
 },
 Some(id) => id.clone(),
 };
 if let Some((err, code, chan_update)) = loop {
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
 },
 Some(id) => id.clone(),
 };

 let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

 if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 // Note that the behavior here should be identical to the above block - we
 // should NOT reveal the existence or non-existence of a private channel if
 // we don't allow forwards outbound over them.
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
@ariardariardJul 1, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
@TheBlueMattTheBlueMattJul 1, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
@ariardariardJul 5, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...
 }

 // Note that we could technically not return an error yet here and just hope
 // that the connection is reestablished or monitor updated by the time we get
 // around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
 }
Expand DownExpand Up@@ -1660,7 +1674,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(),
 };
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down

34 changes: 22 additions & 12 deletions
lightning/src/ln/functional_test_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
	let logger = test_utils::TestLogger::new();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
 &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
 Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
 recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	assert_eq!(route.paths.len(), 1);
	assert_eq!(route.paths[0].len(), expected_route.len());
	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
	}

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 6 commits into

from

Jul 9, 2021
 Merged


Make the base fee configurable in ChannelConfig
#975
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
dac8b7b

Update ChannelConfig serialization to be TLV-based


TheBlueMatt Jun 21, 2021
c620944

Make the base fee configurable in ChannelConfig


TheBlueMatt Jun 21, 2021
99953ed

Update full_stack_target demo input to match new, fewer, fee gets


TheBlueMatt Jul 3, 2021
520b53e

Optionally reject HTLC forwards over priv chans with a new config


TheBlueMatt Jun 26, 2021
dbfccf0

Add a note clarifying the API guarantees of create_channel


TheBlueMatt Jun 26, 2021
4cc0d9d

Change serialization backwards compat in Channel to use new version


TheBlueMatt Jul 6, 2021

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down

12 changes: 3 additions & 9 deletions
fuzz/src/full_stack.rs

View file


Open in desktop


Large diffs are not rendered by default.


2 changes: 1 addition & 1 deletion
lightning-background-processor/src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down

2 changes: 1 addition & 1 deletion
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down

82 changes: 47 additions & 35 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;
@ariardariardJul 8, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
@TheBlueMattTheBlueMattJul 8, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

 self.user_id.write(writer)?;
 self.config.write(writer)?;

 // 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.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)?;

 self.channel_id.write(writer)?;
 (self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 self.counterparty_dust_limit_satoshis.write(writer)?;
 self.holder_dust_limit_satoshis.write(writer)?;
 self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

 self.counterparty_htlc_minimum_msat.write(writer)?;
 self.holder_htlc_minimum_msat.write(writer)?;
 self.counterparty_max_accepted_htlcs.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.minimum_depth.unwrap_or(0).write(writer)?;

 match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 // override that.
 (1, self.minimum_depth, option),
 (3, self.counterparty_selected_channel_reserve_satoshis, option),
 (5, self.config, required),
 });

 Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 where K::Target: KeysInterface<Signer = Signer> {
	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

 let user_id = Readable::read(reader)?;
 let config: ChannelConfig = Readable::read(reader)?;

 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().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)?;
 } else {
 // Read the 8 bytes of backwards-compatibility ChannelConfig data.
 let mut _val: u64 = Readable::read(reader)?;
 }

 let channel_id = Readable::read(reader)?;
 let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 let holder_dust_limit_satoshis = Readable::read(reader)?;
 let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
 let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 if counterparty_selected_channel_reserve_satoshis == Some(0) {
 // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
 // non-option, writing 0 for what we now consider None.
 counterparty_selected_channel_reserve_satoshis = None;
 let mut counterparty_selected_channel_reserve_satoshis = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 } else {
 // Read the 8 bytes of backwards-compatibility data.
 let _dummy: u64 = Readable::read(reader)?;
 }
 let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 let holder_htlc_minimum_msat = Readable::read(reader)?;
 let counterparty_max_accepted_htlcs = Readable::read(reader)?;
 let mut minimum_depth = Some(Readable::read(reader)?);
 if minimum_depth == Some(0) {
 // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
 // consider None.
 minimum_depth = None;

 let mut minimum_depth = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 minimum_depth = Some(Readable::read(reader)?);
 } else {
 // Read the 4 bytes of backwards-compatibility data.
 let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.

 }

 let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 (0, announcement_sigs, option),
 (1, minimum_depth, option),
 (3, counterparty_selected_channel_reserve_satoshis, option),
 (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 });

 let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 Ok(Channel {
 user_id,

 config,
 config: config.unwrap(),
 channel_id,
 channel_state,
 secp_ctx,
Expand Down

32 changes: 23 additions & 9 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
	///
	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
	///
	/// Note that we do not check if you are currently connected to the given peer. If no
	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
	/// the channel eventually being silently forgotten.
@ariardariardJul 6, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
@TheBlueMattTheBlueMattJul 6, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.
	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 if channel_value_satoshis < 1000 {
 return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // short_channel_id is non-0 in any ::Forward.
 if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
 },
 Some(id) => id.clone(),
 };
 if let Some((err, code, chan_update)) = loop {
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
 },
 Some(id) => id.clone(),
 };

 let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

 if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 // Note that the behavior here should be identical to the above block - we
 // should NOT reveal the existence or non-existence of a private channel if
 // we don't allow forwards outbound over them.
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
@ariardariardJul 1, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
@TheBlueMattTheBlueMattJul 1, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
@ariardariardJul 5, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...
 }

 // Note that we could technically not return an error yet here and just hope
 // that the connection is reestablished or monitor updated by the time we get
 // around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
 }
Expand DownExpand Up@@ -1660,7 +1674,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(),
 };
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down

34 changes: 22 additions & 12 deletions
lightning/src/ln/functional_test_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
	let logger = test_utils::TestLogger::new();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
 &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
 Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
 recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	assert_eq!(route.paths.len(), 1);
	assert_eq!(route.paths[0].len(), expected_route.len());
	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
	}

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 6 commits into

from

Jul 9, 2021
 Merged


Make the base fee configurable in ChannelConfig
#975
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
dac8b7b

Update ChannelConfig serialization to be TLV-based


TheBlueMatt Jun 21, 2021
c620944

Make the base fee configurable in ChannelConfig


TheBlueMatt Jun 21, 2021
99953ed

Update full_stack_target demo input to match new, fewer, fee gets


TheBlueMatt Jul 3, 2021
520b53e

Optionally reject HTLC forwards over priv chans with a new config


TheBlueMatt Jun 26, 2021
dbfccf0

Add a note clarifying the API guarantees of create_channel


TheBlueMatt Jun 26, 2021
4cc0d9d

Change serialization backwards compat in Channel to use new version


TheBlueMatt Jul 6, 2021

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down

12 changes: 3 additions & 9 deletions
fuzz/src/full_stack.rs

View file


Open in desktop


Large diffs are not rendered by default.


2 changes: 1 addition & 1 deletion
lightning-background-processor/src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down

2 changes: 1 addition & 1 deletion
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down

82 changes: 47 additions & 35 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;
@ariardariardJul 8, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
@TheBlueMattTheBlueMattJul 8, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

 self.user_id.write(writer)?;
 self.config.write(writer)?;

 // 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.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)?;

 self.channel_id.write(writer)?;
 (self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 self.counterparty_dust_limit_satoshis.write(writer)?;
 self.holder_dust_limit_satoshis.write(writer)?;
 self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

 self.counterparty_htlc_minimum_msat.write(writer)?;
 self.holder_htlc_minimum_msat.write(writer)?;
 self.counterparty_max_accepted_htlcs.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.minimum_depth.unwrap_or(0).write(writer)?;

 match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 // override that.
 (1, self.minimum_depth, option),
 (3, self.counterparty_selected_channel_reserve_satoshis, option),
 (5, self.config, required),
 });

 Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 where K::Target: KeysInterface<Signer = Signer> {
	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

 let user_id = Readable::read(reader)?;
 let config: ChannelConfig = Readable::read(reader)?;

 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().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)?;
 } else {
 // Read the 8 bytes of backwards-compatibility ChannelConfig data.
 let mut _val: u64 = Readable::read(reader)?;
 }

 let channel_id = Readable::read(reader)?;
 let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 let holder_dust_limit_satoshis = Readable::read(reader)?;
 let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
 let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 if counterparty_selected_channel_reserve_satoshis == Some(0) {
 // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
 // non-option, writing 0 for what we now consider None.
 counterparty_selected_channel_reserve_satoshis = None;
 let mut counterparty_selected_channel_reserve_satoshis = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 } else {
 // Read the 8 bytes of backwards-compatibility data.
 let _dummy: u64 = Readable::read(reader)?;
 }
 let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 let holder_htlc_minimum_msat = Readable::read(reader)?;
 let counterparty_max_accepted_htlcs = Readable::read(reader)?;
 let mut minimum_depth = Some(Readable::read(reader)?);
 if minimum_depth == Some(0) {
 // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
 // consider None.
 minimum_depth = None;

 let mut minimum_depth = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 minimum_depth = Some(Readable::read(reader)?);
 } else {
 // Read the 4 bytes of backwards-compatibility data.
 let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.

 }

 let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 (0, announcement_sigs, option),
 (1, minimum_depth, option),
 (3, counterparty_selected_channel_reserve_satoshis, option),
 (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 });

 let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 Ok(Channel {
 user_id,

 config,
 config: config.unwrap(),
 channel_id,
 channel_state,
 secp_ctx,
Expand Down

32 changes: 23 additions & 9 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
	///
	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
	///
	/// Note that we do not check if you are currently connected to the given peer. If no
	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
	/// the channel eventually being silently forgotten.
@ariardariardJul 6, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
@TheBlueMattTheBlueMattJul 6, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.
	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 if channel_value_satoshis < 1000 {
 return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // short_channel_id is non-0 in any ::Forward.
 if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
 },
 Some(id) => id.clone(),
 };
 if let Some((err, code, chan_update)) = loop {
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
 },
 Some(id) => id.clone(),
 };

 let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

 if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 // Note that the behavior here should be identical to the above block - we
 // should NOT reveal the existence or non-existence of a private channel if
 // we don't allow forwards outbound over them.
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
@ariardariardJul 1, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
@TheBlueMattTheBlueMattJul 1, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
@ariardariardJul 5, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...
 }

 // Note that we could technically not return an error yet here and just hope
 // that the connection is reestablished or monitor updated by the time we get
 // around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
 }
Expand DownExpand Up@@ -1660,7 +1674,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(),
 };
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down

34 changes: 22 additions & 12 deletions
lightning/src/ln/functional_test_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
	let logger = test_utils::TestLogger::new();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
 &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
 Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
 recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	assert_eq!(route.paths.len(), 1);
	assert_eq!(route.paths[0].len(), expected_route.len());
	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
	}

Expand Down
Loading

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Make the base fee configurable in ChannelConfig#975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

TheBlueMatt
merged 6 commits into

from

Jul 9, 2021
 Merged


Make the base fee configurable in ChannelConfig
#975
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
dac8b7b

Update ChannelConfig serialization to be TLV-based


TheBlueMatt Jun 21, 2021
c620944

Make the base fee configurable in ChannelConfig


TheBlueMatt Jun 21, 2021
99953ed

Update full_stack_target demo input to match new, fewer, fee gets


TheBlueMatt Jul 3, 2021
520b53e

Optionally reject HTLC forwards over priv chans with a new config


TheBlueMatt Jun 26, 2021
dbfccf0

Add a note clarifying the API guarantees of create_channel


TheBlueMatt Jun 26, 2021
4cc0d9d

Change serialization backwards compat in Channel to use new version


TheBlueMatt Jul 6, 2021

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


4 changes: 2 additions & 2 deletions
fuzz/src/chanmon_consistency.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {
Expand All@@ -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();
Expand Down

12 changes: 3 additions & 9 deletions
fuzz/src/full_stack.rs

View file


Open in desktop


Large diffs are not rendered by default.


2 changes: 1 addition & 1 deletion
lightning-background-processor/src/lib.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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)));
Expand Down

2 changes: 1 addition & 1 deletion
lightning/src/chain/channelmonitor.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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() };
Expand Down

82 changes: 47 additions & 35 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
Expand DownExpand Up@@ -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 {
Expand DownExpand Up@@ -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
Expand DownExpand Up@@ -4460,7 +4443,7 @@ fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script
	return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const SERIALIZATION_VERSION: u8 = 2;
@ariardariardJul 8, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One follow-up is documenting our serialization philosophy in a doc/serialization.md, explain when/why we introduce changes, the odd/even scheme with TLV, that bindings should stay in-sync and what actions user should take in consequence?
Maybe it could be part of LDK docs, but when it's a library interface I would favor the stability guarantees to be documented in-tree ?
@TheBlueMattTheBlueMattJul 8, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we're kinda still figuring it out and doing a case-by-case "what do you want the old clients to do". Def could add to CONTRIBUTING that we have to meticulously document any backwards compat concerns in the CHANGELOG :)
const MIN_SERIALIZATION_VERSION: u8 = 1;

impl_writeable_tlv_based_enum!(InboundHTLCRemovalReason,;
Expand DownExpand Up@@ -4502,7 +4485,13 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);

 self.user_id.write(writer)?;
 self.config.write(writer)?;

 // 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.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)?;

 self.channel_id.write(writer)?;
 (self.channel_state | ChannelState::PeerDisconnected as u32).write(writer)?;
Expand DownExpand Up@@ -4661,10 +4650,15 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 self.counterparty_dust_limit_satoshis.write(writer)?;
 self.holder_dust_limit_satoshis.write(writer)?;
 self.counterparty_max_htlc_value_in_flight_msat.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.counterparty_selected_channel_reserve_satoshis.unwrap_or(0).write(writer)?;

 self.counterparty_htlc_minimum_msat.write(writer)?;
 self.holder_htlc_minimum_msat.write(writer)?;
 self.counterparty_max_accepted_htlcs.write(writer)?;

 // Note that this field is ignored by 0.0.99+ as the TLV Optional variant is used instead.
 self.minimum_depth.unwrap_or(0).write(writer)?;

 match &self.counterparty_forwarding_info {
Expand DownExpand Up@@ -4700,6 +4694,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 // override that.
 (1, self.minimum_depth, option),
 (3, self.counterparty_selected_channel_reserve_satoshis, option),
 (5, self.config, required),
 });

 Ok(())
Expand All@@ -4710,10 +4705,21 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 where K::Target: KeysInterface<Signer = Signer> {
	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);

 let user_id = Readable::read(reader)?;
 let config: ChannelConfig = Readable::read(reader)?;

 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().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)?;
 } else {
 // Read the 8 bytes of backwards-compatibility ChannelConfig data.
 let mut _val: u64 = Readable::read(reader)?;
 }

 let channel_id = Readable::read(reader)?;
 let channel_state = Readable::read(reader)?;
Expand DownExpand Up@@ -4843,20 +4849,25 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 let counterparty_dust_limit_satoshis = Readable::read(reader)?;
 let holder_dust_limit_satoshis = Readable::read(reader)?;
 let counterparty_max_htlc_value_in_flight_msat = Readable::read(reader)?;
 let mut counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 if counterparty_selected_channel_reserve_satoshis == Some(0) {
 // Versions up to 0.0.98 had counterparty_selected_channel_reserve_satoshis as a
 // non-option, writing 0 for what we now consider None.
 counterparty_selected_channel_reserve_satoshis = None;
 let mut counterparty_selected_channel_reserve_satoshis = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 counterparty_selected_channel_reserve_satoshis = Some(Readable::read(reader)?);
 } else {
 // Read the 8 bytes of backwards-compatibility data.
 let _dummy: u64 = Readable::read(reader)?;
 }
 let counterparty_htlc_minimum_msat = Readable::read(reader)?;
 let holder_htlc_minimum_msat = Readable::read(reader)?;
 let counterparty_max_accepted_htlcs = Readable::read(reader)?;
 let mut minimum_depth = Some(Readable::read(reader)?);
 if minimum_depth == Some(0) {
 // Versions up to 0.0.98 had minimum_depth as a non-option, writing 0 for what we now
 // consider None.
 minimum_depth = None;

 let mut minimum_depth = None;
 if ver == 1 {
 // Read the old serialization from version 0.0.98.
 minimum_depth = Some(Readable::read(reader)?);
 } else {
 // Read the 4 bytes of backwards-compatibility data.
 let _dummy: u32 = Readable::read(reader)?;
Comment thread
TheBlueMatt marked this conversation as resolved.

 }

 let counterparty_forwarding_info = match <u8 as Readable>::read(reader)? {
Expand DownExpand Up@@ -4887,6 +4898,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 (0, announcement_sigs, option),
 (1, minimum_depth, option),
 (3, counterparty_selected_channel_reserve_satoshis, option),
 (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
 });

 let mut secp_ctx = Secp256k1::new();
Expand All@@ -4895,7 +4907,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
 Ok(Channel {
 user_id,

 config,
 config: config.unwrap(),
 channel_id,
 channel_state,
 secp_ctx,
Expand Down

32 changes: 23 additions & 9 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1137,6 +1137,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
	///
	/// Raises APIError::APIMisuseError when channel_value_satoshis > 2**24 or push_msat is
	/// greater than channel_value_satoshis * 1k or channel_value_satoshis is < 1000.
	///
	/// Note that we do not check if you are currently connected to the given peer. If no
	/// connection is available, the outbound `open_channel` message may fail to send, resulting in
	/// the channel eventually being silently forgotten.
@ariardariardJul 6, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "being silently forgotten", can't remember this case is covered by peer_disconnected ?
@TheBlueMattTheBlueMattJul 6, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, it will be silently forgotten when we disconnect or restart.
	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_id: u64, override_config: Option<UserConfig>) -> Result<(), APIError> {
 if channel_value_satoshis < 1000 {
 return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
Expand DownExpand Up@@ -1555,15 +1559,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 // short_channel_id is non-0 in any ::Forward.
 if let &PendingHTLCRouting::Forward { ref short_channel_id, .. } = routing {
 let id_option = channel_state.as_ref().unwrap().short_to_id.get(&short_channel_id).cloned();
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 return_err!("Don't have available channel for forwarding as requested.", 0x4000 | 10, &[0;0]);
 },
 Some(id) => id.clone(),
 };
 if let Some((err, code, chan_update)) = loop {
 let forwarding_id = match id_option {
 None => { // unknown_next_peer
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
 },
 Some(id) => id.clone(),
 };

 let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap();

 if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels {
 // Note that the behavior here should be identical to the above block - we
 // should NOT reveal the existence or non-existence of a private channel if
 // we don't allow forwards outbound over them.
 break Some(("Don't have available channel for forwarding as requested.", 0x4000 | 10, None));
@ariardariardJul 1, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note, if we allow forwards outbound, i believe you still a timing attack to break the confidentiality of private channel by forwarding probing HTLC through a hub with tlv_payload's short_channel_id built from observed P2WSH outputs on the transaction logs. If the hub consumes a long delay to sent back an error, it means an update_add_htlc has been tried and rejected (for lack of payment_secret) by a counterparty on the other-side. If the hub consumes a short delay to sent back an error, it means the error has been generated by the hub, no such private channel is attached to the hub.
Does it work ? I've never tried it though i do remember talking about it with @naumenkogs a while back.
@TheBlueMattTheBlueMattJul 1, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, for something with accept_forwards_to_priv_channels set we do have such a timing attack. At some point we need to move more things behind the forwarding delay (including claims, this, etc), its just low on the priority list :/.
@ariardariardJul 5, 2021

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked in #680, yeah agree low-priority for now...
 }

 // Note that we could technically not return an error yet here and just hope
 // that the connection is reestablished or monitor updated by the time we get
 // around to doing the actual forward, but better to fail early if we can and
Expand All@@ -1575,7 +1587,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())));
 }
Expand DownExpand Up@@ -1660,7 +1674,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(),
 };
Expand DownExpand Up@@ -5107,7 +5121,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;
Expand Down

34 changes: 22 additions & 12 deletions
lightning/src/ln/functional_test_utils.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -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();
Expand All@@ -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()),
Expand DownExpand Up@@ -1206,7 +1206,10 @@ pub const TEST_FINAL_CLTV: u32 = 70;
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;
	let logger = test_utils::TestLogger::new();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	let route = get_route(&origin_node.node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(),
 &expected_route.last().unwrap().node.get_our_node_id(), Some(InvoiceFeatures::known()),
 Some(&origin_node.node.list_usable_channels().iter().collect::<Vec<_>>()), &[],
 recv_value, TEST_FINAL_CLTV, &logger).unwrap();
	assert_eq!(route.paths.len(), 1);
	assert_eq!(route.paths[0].len(), expected_route.len());
	for (node, hop) in expected_route.iter().zip(route.paths[0].iter()) {
Expand DownExpand Up@@ -1316,7 +1319,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();
Expand All@@ -1341,22 +1344,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);
	}

Expand Down
Loading