Skip to content

Check funder can afford commitment transaction fee when receiving update_fee #231

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
merged 1 commit into from
Nov 20, 2018
Merged

Check funder can afford commitment transaction fee when receiving update_fee #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 17 additions & 4 deletions src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,8 +357,9 @@ const UNCONF_THRESHOLD: u32 = 6;
const BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 7; //TODO?
/// The amount of time we're willing to wait to claim money back to us
const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 14;
const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;
const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;
/// Exposing these two constants for use in test in ChannelMonitor
pub const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;
Copy link
Collaborator

Choose a reason for hiding this comment

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

May want to add a comment noting these are just exposed for channelmanager tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172;
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
const B_OUTPUT_PLUS_SPENDING_INPUT_WEIGHT: u64 = 104; // prevout: 40, nSequence: 4, script len: 1, witness lengths: 3/4, sig: 73/4, pubkey: 33/4, output: 31 (TODO: Wrong? Useless?)
/// Maximmum `funding_satoshis` value, according to the BOLT #2 specification
Expand Down Expand Up @@ -1653,7 +1654,9 @@ impl Channel {

let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number)?;

let mut update_fee = false;
let feerate_per_kw = if !self.channel_outbound && self.pending_update_fee.is_some() {
update_fee = true;
self.pending_update_fee.unwrap()
} else {
self.feerate_per_kw
Expand All @@ -1664,6 +1667,16 @@ impl Channel {
let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap();
secp_call!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid commitment tx signature from peer", self.channel_id());

//If channel fee was updated by funder confirm funder can afford the new fee rate when applied to the current local commitment transaction
if update_fee {
let num_htlcs = local_commitment_tx.1.len();
let total_fee: u64 = feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;

if self.channel_value_satoshis - self.value_to_self_msat / 1000 < total_fee + self.their_channel_reserve_satoshis {
return Err(HandleError { err: "Funding remote cannot afford proposed new fee", action: Some(ErrorAction::DisconnectPeer { msg: None }) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should really be an error-message-generating Err, but I'll fix it in #254 since I'm rewriting this stuff there anyway.

}
}

if msg.htlc_signatures.len() != local_commitment_tx.1.len() {
return Err(HandleError{err: "Got wrong number of HTLC signatures from remote", action: None});
}
Expand All @@ -1677,7 +1690,7 @@ impl Channel {
let mut htlc_tx = self.build_htlc_transaction(&local_commitment_txid, htlc, true, &local_keys, feerate_per_kw);
let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &local_keys);
let htlc_sighash = Message::from_slice(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]).unwrap();
secp_call!(self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key), "Invalid HTLC tx siganture from peer", self.channel_id());
secp_call!(self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key), "Invalid HTLC tx signature from peer", self.channel_id());
let htlc_sig = if htlc.offered {
let htlc_sig = self.sign_htlc_transaction(&mut htlc_tx, &msg.htlc_signatures[idx], &None, htlc, &local_keys)?;
new_local_commitment_txn.push(htlc_tx);
Expand Down Expand Up @@ -1705,6 +1718,7 @@ impl Channel {
}
}
}

if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
// This is a response to our post-monitor-failed unfreeze messages, so we can clear the
// monitor_pending_order requirement as we won't re-send the monitor_pending messages.
Expand Down Expand Up @@ -2188,7 +2202,6 @@ impl Channel {
return Err(ChannelError::Close("Peer sent update_fee when we needed a channel_reestablish"));
}
Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;

self.pending_update_fee = Some(msg.feerate_per_kw as u64);
self.channel_update_count += 1;
Ok(())
Expand Down
131 changes: 74 additions & 57 deletions src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3212,6 +3212,7 @@ mod tests {
use chain::chaininterface::{ChainListener, ChainWatchInterface};
use chain::keysinterface::KeysInterface;
use chain::keysinterface;
use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
use ln::channelmanager::{ChannelManager,ChannelManagerReadArgs,OnionKeys,PaymentFailReason,RAACommitmentOrder};
use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS, ManyChannelMonitor};
use ln::router::{Route, RouteHop, Router};
Expand Down Expand Up @@ -3501,6 +3502,17 @@ mod tests {
}
}

macro_rules! get_feerate {
($node: expr, $channel_id: expr) => {
{
let chan_lock = $node.node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&$channel_id).unwrap();
chan.get_feerate()
}
}
}


fn create_chan_between_nodes_with_value_init(node_a: &Node, node_b: &Node, channel_value: u64, push_msat: u64) -> Transaction {
node_a.node.create_channel(node_b.node.get_our_node_id(), channel_value, push_msat, 42).unwrap();
node_b.node.handle_open_channel(&node_a.node.get_our_node_id(), &get_event_msg!(node_a, MessageSendEvent::SendOpenChannel, node_b.node.get_our_node_id())).unwrap();
Expand Down Expand Up @@ -4134,14 +4146,6 @@ mod tests {
let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
let channel_id = chan.2;

macro_rules! get_feerate {
($node: expr) => {{
let chan_lock = $node.node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&channel_id).unwrap();
chan.get_feerate()
}}
}

// balancing
send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);

Expand All @@ -4163,7 +4167,7 @@ mod tests {
// (6) RAA is delivered ->

// First nodes[0] generates an update_fee
nodes[0].node.update_fee(channel_id, get_feerate!(nodes[0]) + 20).unwrap();
nodes[0].node.update_fee(channel_id, get_feerate!(nodes[0], channel_id) + 20).unwrap();
check_added_monitors!(nodes[0], 1);

let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
Expand Down Expand Up @@ -4252,19 +4256,11 @@ mod tests {
let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
let channel_id = chan.2;

macro_rules! get_feerate {
($node: expr) => {{
let chan_lock = $node.node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&channel_id).unwrap();
chan.get_feerate()
}}
}

// balancing
send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);

// First nodes[0] generates an update_fee
nodes[0].node.update_fee(channel_id, get_feerate!(nodes[0]) + 20).unwrap();
nodes[0].node.update_fee(channel_id, get_feerate!(nodes[0], channel_id) + 20).unwrap();
check_added_monitors!(nodes[0], 1);

let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
Expand Down Expand Up @@ -4310,14 +4306,6 @@ mod tests {
let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
let channel_id = chan.2;

macro_rules! get_feerate {
($node: expr) => {{
let chan_lock = $node.node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&channel_id).unwrap();
chan.get_feerate()
}}
}

// A B
// update_fee/commitment_signed ->
// .- send (1) RAA and (2) commitment_signed
Expand All @@ -4338,7 +4326,7 @@ mod tests {
// revoke_and_ack ->

// First nodes[0] generates an update_fee
let initial_feerate = get_feerate!(nodes[0]);
let initial_feerate = get_feerate!(nodes[0], channel_id);
nodes[0].node.update_fee(channel_id, initial_feerate + 20).unwrap();
check_added_monitors!(nodes[0], 1);

Expand Down Expand Up @@ -4422,16 +4410,8 @@ mod tests {
let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
let channel_id = chan.2;

macro_rules! get_feerate {
($node: expr) => {{
let chan_lock = $node.node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&channel_id).unwrap();
chan.get_feerate()
}}
}

let feerate = get_feerate!(nodes[0]);
nodes[0].node.update_fee(channel_id, feerate+20).unwrap();
let feerate = get_feerate!(nodes[0], channel_id);
nodes[0].node.update_fee(channel_id, feerate+25).unwrap();
check_added_monitors!(nodes[0], 1);

let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
Expand Down Expand Up @@ -4462,24 +4442,69 @@ mod tests {
check_added_monitors!(nodes[1], 1);
}

#[test]
fn test_update_fee_that_funder_cannot_afford() {
let mut nodes = create_network(2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This mut is unnecessary.

let channel_value = 1888;
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, 700000);
let channel_id = chan.2;

let feerate = 260;
nodes[0].node.update_fee(channel_id, feerate).unwrap();
check_added_monitors!(nodes[0], 1);
let update_msg = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());

nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update_msg.update_fee.unwrap()).unwrap();

commitment_signed_dance!(nodes[1], nodes[0], update_msg.commitment_signed, false);

//Confirm that the new fee based on the last local commitment txn is what we expected based on the feerate of 260 set above.
//This value results in a fee that is exactly what the funder can afford (277 sat + 1000 sat channel reserve)
{
let chan_lock = nodes[1].node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&channel_id).unwrap();

//We made sure neither party's funds are below the dust limit so -2 non-HTLC txns from number of outputs
let num_htlcs = chan.last_local_commitment_txn[0].output.len() - 2;
let total_fee: u64 = feerate * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
let mut actual_fee = chan.last_local_commitment_txn[0].output.iter().fold(0, |acc, output| acc + output.value);
actual_fee = channel_value - actual_fee;
assert_eq!(total_fee, actual_fee);
} //drop the mutex

//Add 2 to the previous fee rate to the final fee increases by 1 (with no HTLCs the fee is essentially
//fee_rate*(724/1000) so the increment of 1*0.724 is rounded back down)
nodes[0].node.update_fee(channel_id, feerate+2).unwrap();
check_added_monitors!(nodes[0], 1);

let update2_msg = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());

nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update2_msg.update_fee.unwrap());

//While producing the commitment_signed response after handling a received update_fee request the
//check to see if the funder, who sent the update_fee request, can afford the new fee (funder_balance >= fee+channel_reserve)
//Should produce and error.
let err = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &update2_msg.commitment_signed).unwrap_err();

assert!(match err.err {
"Funding remote cannot afford proposed new fee" => true,
_ => false,
});

//clear the message we could not handle
nodes[1].node.get_and_clear_pending_msg_events();
}

#[test]
fn test_update_fee_with_fundee_update_add_htlc() {
let mut nodes = create_network(2);
let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
let channel_id = chan.2;

macro_rules! get_feerate {
($node: expr) => {{
let chan_lock = $node.node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&channel_id).unwrap();
chan.get_feerate()
}}
}

// balancing
send_payment(&nodes[0], &vec!(&nodes[1])[..], 8000000);

let feerate = get_feerate!(nodes[0]);
let feerate = get_feerate!(nodes[0], channel_id);
nodes[0].node.update_fee(channel_id, feerate+20).unwrap();
check_added_monitors!(nodes[0], 1);

Expand Down Expand Up @@ -4577,14 +4602,6 @@ mod tests {
let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
let channel_id = chan.2;

macro_rules! get_feerate {
($node: expr) => {{
let chan_lock = $node.node.channel_state.lock().unwrap();
let chan = chan_lock.by_id.get(&channel_id).unwrap();
chan.get_feerate()
}}
}

// A B
// (1) update_fee/commitment_signed ->
// <- (2) revoke_and_ack
Expand All @@ -4600,7 +4617,7 @@ mod tests {
// revoke_and_ack ->

// Create and deliver (1)...
let feerate = get_feerate!(nodes[0]);
let feerate = get_feerate!(nodes[0], channel_id);
nodes[0].node.update_fee(channel_id, feerate+20).unwrap();
check_added_monitors!(nodes[0], 1);

Expand Down Expand Up @@ -4674,8 +4691,8 @@ mod tests {
check_added_monitors!(nodes[1], 1);
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

assert_eq!(get_feerate!(nodes[0]), feerate + 30);
assert_eq!(get_feerate!(nodes[1]), feerate + 30);
assert_eq!(get_feerate!(nodes[0], channel_id), feerate + 30);
assert_eq!(get_feerate!(nodes[1], channel_id), feerate + 30);
close_channel(&nodes[0], &nodes[1], &chan.2, chan.3, true);
}

Expand Down