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

philipr-za · 2018-10-31T15:52:42Z

Issue #204

When handling an update_fee message from the funder we must confirm that the funder can afford the new fee for the receiving nodes current commitment transaction.

This is my first PR so still new to the workings of Lightning and the architecture of rust-lightning so my reading of it is likely incorrect. I am not 100% sure about this but from my reading it seems that when a node handles a commitment_signed message it constructs a new local commitment and in that process calls channel::build_commitment_transaction(...) which runs through all the incoming and outgoing HTLCs and tests if they should be trimmed or not. The resulting untrimmed HTLC outputs are then pushed into the first position in the channel.last_local_commitment_txn vector. The length of the output field of that first transaction seems to be number of untrimmed HTLCs in the current local commitment?

Looking forward to some feedback.

TheBlueMatt

Nice, I was thinking this issue would require a bigger patch but its not too bad, actually.

TheBlueMatt · 2018-11-01T13:59:25Z

src/ln/channelmanager.rs

+		assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+		check_added_monitors!(nodes[0], 1);
+
+		nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &commitment_signed).unwrap();


Should be able to replace most of the dance here with the commitment_signed_dance! macro.

Indeed I could.

TheBlueMatt · 2018-11-01T13:59:57Z

src/ln/channelmanager.rs

+			},
+			_ => panic!("Unexpected event"),
+		};
+		println!("{:?}",nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_msg.unwrap()).is_err());


Seems to have snuck in here...

TheBlueMatt · 2018-11-01T14:00:27Z

src/ln/channelmanager.rs

+			_ => panic!("Unexpected event"),
+		};
+		println!("{:?}",nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_msg.unwrap()).is_err());
+		assert!(nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), update_msg.unwrap()).is_err());


Should check that the action is filled in and closes the channel (ie is an error action).

TheBlueMatt · 2018-11-01T14:07:12Z

src/ln/channelmanager.rs

+		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+		check_added_monitors!(nodes[1], 1);
+
+		nodes[0].node.update_fee(channel_id, feerate+2000).unwrap();


It would be nice to check the boundary condition here - can you make the first fee update one satoshi shy of hitting the feerate limit (and check that it is by getting the latest local commitment transaction out of the channel via Channel::last_local_commitment_txn) and then check that one more breaks it?

Done. See the initial channel funds and fees so it is exactly on the limit and then increase by 1 to break it.

TheBlueMatt · 2018-11-01T14:08:42Z

src/ln/channel.rs

+			0
+		};
+
+		let total_fee: u64 = msg.feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;


Hmm, the spec says we "MAY delay this check until the update_fee is committed" which may make this much easier to reason about - in other words instead of doing this check in update_fee do it in commitment_signed by looking at the resulting commitment transaction that is generated and make sure that the output to our counterparty didn't get pruned.

I have moved the check out to the commitment_signed message as it does make more sense there. Had to be careful to only check it if the fee was updated.

TheBlueMatt · 2018-11-01T14:11:02Z

Oh, also, please limit the length of your commit titles (ie the first line in the commit message with the second line blank) to something like 80ish chars. You can replace full github links with just #NNN in this case.

TheBlueMatt · 2018-11-02T20:53:23Z

Did you forget to push the updated version?

…

On November 2, 2018 12:43:36 PM UTC, Philip Robinson ***@***.***> wrote: philipr-za commented on this pull request. > @@ -2135,6 +2135,19 @@ impl Channel { } Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?; + //Confirm sender can afford the new fee rate when applied to the current local commitment transaction + let num_htlcs = if self.last_local_commitment_txn.len() > 0 { + self.last_local_commitment_txn[0].output.len() + } else { + 0 + }; + + let total_fee: u64 = msg.feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000; I have moved the check out to the commitment_signed message as it does make more sense there. Had to be careful to only check it if the fee was updated. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #231 (comment)

TheBlueMatt

Awesome! This looks great, sans a few style nits and one question about the fee calculation.

TheBlueMatt · 2018-11-13T23:34:54Z

src/ln/channel.rs

 		if msg.htlc_signatures.len() != local_commitment_tx.1.len() {
 			return Err(HandleError{err: "Got wrong number of HTLC signatures from remote", action: None});
 		}

+		println!("Len: {}, Num HTLC Sigs: {}", local_commitment_tx.0.output.len(), local_commitment_tx.1.len());


Seems to have slipped in. You can add trace log lines if you want to add new logging upstream.

Woops, ja this was just for debugging purposes to help understand what was happening. Removed.

TheBlueMatt · 2018-11-13T23:40:34Z

src/ln/channelmanager.rs

+
+		let events_0 = nodes[0].node.get_and_clear_pending_msg_events();
+		assert_eq!(events_0.len(), 1);
+		let (update_msg, commitment_signed) = match events_0[0] {


May be simpler to use get_htlc_update_msgs here.

Ahh yes that works.

TheBlueMatt · 2018-11-13T23:42:12Z

src/ln/channel.rs

@@ -357,8 +357,8 @@ 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;
+pub const COMMITMENT_TX_BASE_WEIGHT: u64 = 724;


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

TheBlueMatt · 2018-11-13T23:43:27Z

src/ln/channelmanager.rs

@@ -22,7 +22,7 @@ use secp256k1;

 use chain::chaininterface::{BroadcasterInterface,ChainListener,ChainWatchInterface,FeeEstimator};
 use chain::transaction::OutPoint;
-use ln::channel::{Channel, ChannelError};
+use ln::channel::{Channel, ChannelError, COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};


This results in unused import warnings - you should just add these imports inside the tests module and drop the "super::" paths where they're used.

TheBlueMatt · 2018-11-13T23:45:24Z

src/ln/channel.rs

 		let mut local_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, feerate_per_kw);
 		let local_commitment_txid = local_commitment_tx.0.txid();
 		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.0.output.len();


num_htlcs should be output.len() - 2, no? The test should probably catch this. Note that build_commitment_transaction is somewhat confusingly written - the fees are calculated before the non-HTLC outputs are added to the txouts Vec.

Ahh! This did confuse me! I thought that the way I had setup the network that we had only done the initial funding and a few update_fees so I expected there to be no HTLCs in the channel yet but my testing revealed there was 1. So I think it will actually be max(outout.len() - 2, 0) because if either of the non-HTLC outputs is below the dust limit it is not added to the txn.

Actually I should probably just use the length of Vec<HTLCOutputInCommitment> returned from build_commitment.

TheBlueMatt · 2018-11-13T23:45:54Z

src/ln/channelmanager.rs

+		assert_eq!(events_0.len(), 1);
+		let (update_msg, commitment_signed) = match events_0[0] {
+			MessageSendEvent::UpdateHTLCs {
+				node_id: _, updates: msgs::CommitmentUpdate {


Note that you can generally replace "a_thing: _, b_thing" with "b_thing, .." to keep things shorter.

TheBlueMatt · 2018-11-13T23:46:43Z

src/ln/channelmanager.rs

+			let num_htlcs = chan.last_local_commitment_txn[0].output.len();
+			let total_fee: u64 = feerate * (super::COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * super::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
+			let fee_for_single_update_to_commitment = feerate * (super::COMMITMENT_TX_BASE_WEIGHT + 1 * super::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
+			assert_eq!(total_fee, fee_for_single_update_to_commitment);


Should probably assert against the real tx fee here - total the output values and subtract that from the input.

Makes sense. Done.

TheBlueMatt · 2018-11-13T23:48:47Z

src/ln/channelmanager.rs

+		//Should produce and error.
+		let err = nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &commitment_signed).unwrap_err();
+
+		assert!(match err.action.unwrap() {


May be easier to match against the err message here - eventually all Channel-generated Errs will be ChannelErrors which will change the action to SendErrorMessage/Disconnect (with error message). Technically this should send an error message, though I don't really care about getting the error action right right now given that change incoming.

…#204

TheBlueMatt · 2018-11-20T21:29:05Z

src/ln/channelmanager.rs

@@ -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);


This mut is unnecessary.

TheBlueMatt · 2018-11-20T21:29:07Z

src/ln/channel.rs

+			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 }) });


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

TheBlueMatt reviewed Nov 1, 2018

View reviewed changes

SWvheerden mentioned this pull request Nov 6, 2018

Check Bolt 2 spec #240

Closed

26 tasks

philipr-za force-pushed the philip-204-check-commitment-transaction-fee branch 2 times, most recently from ea0ddc0 to 8142175 Compare November 13, 2018 09:20

TheBlueMatt reviewed Nov 13, 2018

View reviewed changes

Check affordability of new fee during update_fee call lightningdevkit…

a51dbb4

…#204

philipr-za force-pushed the philip-204-check-commitment-transaction-fee branch from 8da5521 to a51dbb4 Compare November 14, 2018 12:14

TheBlueMatt reviewed Nov 20, 2018

View reviewed changes

TheBlueMatt merged commit 86944d3 into lightningdevkit:master Nov 20, 2018

This was referenced Nov 20, 2018

Check commitment transaction fee when receiving update_fee #204

Closed

Move a ton of Channel functions to ChannelError from HandleError #254

Merged

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

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

Uh oh!

Conversation

philipr-za commented Oct 31, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 1, 2018

Uh oh!

TheBlueMatt commented Nov 2, 2018 via email

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipr-za Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philipr-za Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philipr-za Nov 14, 2018 •

edited

Loading

philipr-za Nov 14, 2018 •

edited

Loading