Implement update_fee handling #161 #187

yuntai · 2018-09-18T12:29:37Z

I'm not confident I know what I am doing with this PR. Pushed to get any feedback first, please. State stuff is complex!

TheBlueMatt

At a high level looks really good, but I think there's some race conditions here. The commitment_signed/revoke_and_ack dances are all really subtle. Generally, we should upate Channel::feerate_per_kw upon revoke_and_ack coming from the peer that did not generate the update_fee. In other words, if the channel is inbound, we should update self.feerate_per_kw when we receive a commitment_signed and respond with a revoke_and_ack (which you do), and if the channel is outbound, we should update self.feerate_per_kw when we receive an inbound revoke_and_ack.

src/ln/channel.rs

TheBlueMatt · 2018-09-18T15:57:29Z

src/ln/channel.rs

@@ -1800,6 +1818,20 @@ impl Channel {
 		}
 		self.value_to_self_msat = (self.value_to_self_msat as i64 + value_to_self_msat_diff) as u64;

+		if let Some(feerate) = self.pending_update_fee.take() {


I think you need a self.channel_outbound check here just like you have in commitment_signed (but, obviously, opposite). Otherwise you might have:

Us remote <- update_fee commitment_signed (incl new fee) ->

which would be invalid because the remote side needs to send us their commitment_signed before we use the new feerate.

With current logic, for an inbound channel, self.pending_update_fee.is_none()==true inside revoke_and_ack(). I'm worried that the logic may be correct for passing tests but only with our implementation, but the implemented logic is in sync with your general description given in your first comment.

TheBlueMatt · 2018-09-18T16:06:52Z

src/ln/channel.rs

+	pub fn send_update_fee_and_commit(&mut self, feerate_per_kw: u64) -> Result<Option<(msgs::UpdateFee, msgs::CommitmentSigned, ChannelMonitor)>, HandleError> {
+		match self.send_update_fee(feerate_per_kw)? {
+			Some(update_fee) => {
+				let (commitment_signed, monitor_update) = self.send_commitment_no_status_check()?;


Hmm, I don't think this is quite right:
When we call send_update_fee_and_commit we'll create the update_fee message, send it to our peer, then create a commitment_signed message with the new fee (that is all correct). However, when we create the commitment_signed message we'll set self.feerate_per_kw, which means we'll be incorrect in some race conditions.

What should happen is this:

Us remote update_fee -> commitment_signed (incl new fee) -> <- update_add_htlc <- commitment_signed (not incl new fee)

But we'll expect their commitment_signed to use the new feerate. This is incorrect and we should instead only expect that after they send a new revoke_and_ack.

You may want to add a test which tests the above message exchanges.

Not sure how to generate the condition above with our code. The handler of commitment_signed generates revoke_and_ack and commitment_signed, and put the channel in AwaitingRemoteRevoke state. So the second commitment signed will use new fee after 'remote' having received revoke_and_ack from 'Us.'

If you meant update_add_htlc/commitment_signed arriving to 'Us' when the first commitment_signed in transit, then it's covered, I think, because 'Us' does not update self.feerate until it gets revoke_and_ack from 'remote.'

You should be able to create two channelmanagers - one call update_fee and get the three messages out of it, on the other send_payment. Now forward the messages to both ends but only after calling both and ensure they all behave correctly.

TheBlueMatt · 2018-09-18T16:09:19Z

src/ln/peer_handler.rs

@@ -591,6 +591,9 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 														for resp in resps.update_fail_htlcs {
 															encode_and_send_msg!(resp, 131);
 														}
+														if let Some(resp) = resps.update_fee {
+															encode_and_send_msg!(resp, 131);


update_fee is type 134, not 131.

The 131 should be changed to a 134?

This still isnt changed?

src/ln/peer_handler.rs

TheBlueMatt · 2018-09-18T16:15:31Z

src/ln/channel.rs

@@ -284,6 +284,9 @@ pub struct Channel {
 	pending_inbound_htlcs: Vec<InboundHTLCOutput>,
 	pending_outbound_htlcs: Vec<OutboundHTLCOutput>,
 	holding_cell_htlc_updates: Vec<HTLCUpdateAwaitingACK>,
+
+	pending_update_fee: Option<u64>,


Can you add a comment above these describing at a high level what the conditions are for updating them?

TheBlueMatt · 2018-09-18T16:15:57Z

src/ln/channel.rs

@@ -1705,6 +1722,7 @@ impl Channel {
 						update_fulfill_htlcs,
 						update_fail_htlcs,
 						update_fail_malformed_htlcs: Vec::new(),
+						update_fee: None,


I think we need to release the holding_cell_fee_update here, actually.

src/ln/channel.rs

TheBlueMatt · 2018-09-19T21:52:57Z

src/ln/channel.rs

@@ -1705,6 +1730,17 @@ impl Channel {
 						update_fulfill_htlcs,
 						update_fail_htlcs,
 						update_fail_malformed_htlcs: Vec::new(),
+						update_fee: {
+							if let Some(feerate) = self.holding_cell_update_fee {


This needs to happen before we generate the commitment_signed message as our new commitment must use the newly-updated fee.

TheBlueMatt · 2018-09-19T21:58:50Z

src/ln/channel.rs

+
+	pub fn send_update_fee(&mut self, feerate_per_kw: u64) -> Result<Option<msgs::UpdateFee>, HandleError> {
+		if !self.channel_outbound {
+			return Err(HandleError{err: "Cannot send fee from inbound channel", action: None});


I think this can just panic.

TheBlueMatt · 2018-09-19T22:00:06Z

src/ln/channel.rs

+
+	}
+
+	pub fn send_update_fee(&mut self, feerate_per_kw: u64) -> Result<Option<msgs::UpdateFee>, HandleError> {


nit: dont make this pub unless its actually used somewhere in ChannelManager (and document the "cannot be called twice without a send_commitment in between" invariant).

TheBlueMatt · 2018-09-19T22:01:52Z

src/ln/channelmanager.rs

@@ -1923,6 +1927,36 @@ impl ChannelManager {
 		}
 		res
 	}
+
+	pub fn update_fee(&self, channel_id: [u8;32], feerate_per_kw: u64) -> Result<(), HandleError> {


Can we return an APIError instead here?

TheBlueMatt · 2018-09-19T22:02:21Z

src/ln/channelmanager.rs

+		match channel_state.by_id.get_mut(&channel_id) {
+			None => return Err(HandleError{err: "Failed to find corresponding channel", action: None}),
+			Some(chan) => {
+				if !chan.is_usable() {


Should also check if the channel is outbound.

TheBlueMatt · 2018-09-19T22:03:09Z

src/ln/peer_handler.rs

@@ -591,6 +591,9 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 														for resp in resps.update_fail_htlcs {
 															encode_and_send_msg!(resp, 131);
 														}
+														if let Some(resp) = resps.update_fee {
+															encode_and_send_msg!(resp, 131);


The 131 should be changed to a 134?

yuntai · 2018-09-20T16:34:28Z

ahh.. one more test (update mixed w/ payment) need to be added before a review.

yuntai · 2018-09-21T14:44:14Z

One more test added. seems better now.

TheBlueMatt

I spent some real quality time with this one, since some interaction is super subtle. Ended up writing a few new tests to make sure I got everything. You can find a squashed copy of this PR (with a few macros pulled out to simplify the tests) as well as fixes for most of my comments here, as well as a few new tests, one of which currently fails and needs some work to make pass at https://github.com/TheBlueMatt/rust-lightning/commits/2018-09-187-tweaks. I think if you add another boolean for tracking whether the current pending feerate update has been acked on inbound channels it should be easy to make pass.

TheBlueMatt · 2018-09-26T17:36:12Z

src/ln/channel.rs

@@ -1582,7 +1613,7 @@ impl Channel {

 		let mut htlcs_and_sigs = Vec::with_capacity(local_commitment_tx.1.len());
 		for (idx, ref htlc) in local_commitment_tx.1.iter().enumerate() {
-			let mut htlc_tx = self.build_htlc_transaction(&local_commitment_txid, htlc, true, &local_keys);
+			let mut htlc_tx = self.build_htlc_transaction(&local_commitment_txid, htlc, true, &local_keys, self.feerate_per_kw);


Shouldn't this use the local copy instead of the old version?

TheBlueMatt · 2018-09-26T18:04:13Z

src/ln/channel.rs

+		if !self.channel_outbound {
+			if let Some(fee_update) = self.pending_update_fee {
+				self.feerate_per_kw = fee_update;
+				if (self.channel_state & ChannelState::AwaitingRemoteRevoke as u32) == 0 {


I think this is right, but its super subtle. Can you add a comment like "We later use the presence of pending_update_fee to indicate we should generate a commitment_signed upon receipt of revoke_and_ack, so we can only set it to None if we're not awaiting a revoke".

TheBlueMatt · 2018-09-26T18:16:49Z

src/ln/channel.rs

@@ -1694,18 +1736,28 @@ impl Channel {
 			//fail it back the route, if its a temporary issue we can ignore it...
 			match err {
 				None => {
-					if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() {
+					if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() && self.holding_cell_update_fee.is_none() {


Note that this interacts with #189 (well, ok, the send_htlc call above does). If we have pending fee updates they may invalidate our sends and may make this commitment_signed we're about to generate invalid. I'm fine with leaving it for #189 or post-merge-of-both, but should at least file an issue for it once they get merged.

TheBlueMatt · 2018-09-26T18:48:49Z

src/ln/channel.rs

+			if self.channel_outbound {
+				self.feerate_per_kw = feerate;
+			} else {
+				assert_eq!(feerate, self.feerate_per_kw);


I think this is wrong, if a peer sends us an update_fee then revoke_and_ack prior to a commitment_signed (which is allowed if, eg, they are waiting on something else to send a commitment_signed) then we'll immediately crash.

Knee-jerk reaction is that we'll need another flag or another pending_fee_update variable, but I think it'd actually be fine to just check if feerate == self.feerate_per_kw { require_commitment = true; } and then add a comment about how we left pending_update_fee set because we were in AwaitingRemoteRevoke and the equality indicates we need a commitment. This is only OK because they aren't allowed to send a duplicative commitment_signed with an update_fee that simply hasn't changed.

After fighting with some new tests for a while, I think we need a new boolean field to track this correctly.

TheBlueMatt · 2018-09-26T19:06:46Z

src/ln/channel.rs

+			return None;
+		}
+
+		debug_assert!(self.pending_update_fee.is_none());


Can you add a comment describing why, since it's not quite clear why this is OK otherwise - if we just sent an update_fee then we should be in AwaitingRemoteRevoke as we should have been committed prior to any other updates, otherwise the revoke_and_ack should have wiped our pending_update_fee.

TheBlueMatt · 2018-09-26T21:36:07Z

src/ln/channelmanager.rs

+	/// Begin Update fee process. Allowed only on an outbound channel.
+	/// If successful, will generate a UpdateHTLCs event, so you should probably poll
+	/// PeerManager::process_events afterwards.
+	pub fn update_fee(&self, channel_id: [u8;32], feerate_per_kw: u64) -> Result<(), APIError> {


Can we make this priv for now for testing? I think the eventual API is gonna look something more like ChannelManager::notify_fees_changed_materially() and that will update all our channels and check if inbound channels have sane fees.

Or just mark it doc(hidden) and note that it'll change eventually.

TheBlueMatt · 2018-09-26T21:36:37Z

src/ln/peer_handler.rs

@@ -591,6 +591,9 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 														for resp in resps.update_fail_htlcs {
 															encode_and_send_msg!(resp, 131);
 														}
+														if let Some(resp) = resps.update_fee {
+															encode_and_send_msg!(resp, 131);


This still isnt changed?

TheBlueMatt · 2018-09-26T23:54:56Z

src/ln/peer_handler.rs

+						if let &Some(ref msg) = update_fee {
+							peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encode_msg!(msg, 134)));
+						}
+						debug_assert!(update_fee.is_none());


I presume this was just here for testing and should be removed (given ChannelManager::update_fee generates such an event)?

TheBlueMatt · 2018-09-27T15:37:45Z

src/ln/channel.rs

 						// This should never actually happen and indicates we got some Errs back
 						// from update_fulfill_htlc/update_fail_htlc, but we handle it anyway in
 						// case there is some strange way to hit duplicate HTLC removes.
 						return Ok(None);
 					}
+					let update_fee = if let Some(feerate) = self.holding_cell_update_fee {


This is never actually hit because of the if self.holding_cell_htlc_updates.len() != 0 { check at the top of the function.

TheBlueMatt · 2018-09-27T20:03:06Z

src/ln/channelmanager.rs

+		check_added_monitors_and_clear!(nodes[0]);
+
+		let resp_option = nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &revoke_msg_0).unwrap();
+		let commitment_signed = resp_option.unwrap().commitment_signed;


Why does nodes[1] generate a new commitment_signed here? AFAICT the contents are identical.

to free_holding_cell_htlcs to be consistent with others

for fee to make pending_update_fee updated when building commitment tx

to use APIError unwrap send_update_fee add is_outbound() accessor

fundee update_add_htlc before the first revoke for update_fee being completed

yuntai · 2018-09-29T20:40:31Z

I applied the fixes in your local branch and made some tweaks for the inbound channel. Somehow this fix passes all the tests without additional variable but can't explain why!

TheBlueMatt · 2018-09-30T00:00:03Z

Ah, yea, I think your changes are right, see new comments in #200. I'll take that once travis passes.

#187 rebased + squashed

TheBlueMatt reviewed Sep 18, 2018

View reviewed changes

TheBlueMatt reviewed Sep 19, 2018

View reviewed changes

yuntai force-pushed the 201809-update-fee branch 2 times, most recently from 42b5b02 to f73e2cc Compare September 20, 2018 16:32

yuntai force-pushed the 201809-update-fee branch from f73e2cc to 9dfb15d Compare September 21, 2018 14:39

TheBlueMatt reviewed Sep 28, 2018

View reviewed changes

yuntai added 14 commits September 30, 2018 05:28

Implement update_fee handling

f0fc392

Use UpdateHTLCs instead of SendUpdateFee

9206a61

Remove feerate_per_kw arg from add_htlc_output!()

4f104c2

Remove one TODO and add a white space line

c1e6c4a

move holding_cell_feerate logic

98044df

to free_holding_cell_htlcs to be consistent with others

add comments about pending_update_fee

a6bbcab

make rustc1.22.0 happy

d15e1f0

Update comments

29d3533

Update update_fee logic

46f78d0

move up updaing fee when freeing holding_cell

1d7a563

for fee to make pending_update_fee updated when building commitment tx

Update update_fee API signature

ec77da2

to use APIError unwrap send_update_fee add is_outbound() accessor

Add/update test for update_fee

2fa0af7

add another test for update_fee

decac48

fundee update_add_htlc before the first revoke for update_fee being completed

apply fixes in TheBlueMatts and update_fee state tweak

8f5d09c

yuntai force-pushed the 201809-update-fee branch from 9dfb15d to 8f5d09c Compare September 29, 2018 20:34

TheBlueMatt closed this Sep 30, 2018

TheBlueMatt added a commit that referenced this pull request Sep 30, 2018

Merge pull request #200 from TheBlueMatt/2018-09-187-tweaks

8bfe18c

#187 rebased + squashed

yuntai mentioned this pull request Oct 3, 2018

Check commitment transaction fee when receiving update_fee #204

Closed


		}

		pub fn send_update_fee(&mut self, feerate_per_kw: u64) -> Result<Option<msgs::UpdateFee>, HandleError> {

Implement update_fee handling #161 #187

Implement update_fee handling #161 #187

Uh oh!

Conversation

yuntai commented Sep 18, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuntai Sep 19, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

yuntai commented Sep 20, 2018

Uh oh!

yuntai commented Sep 21, 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!

yuntai commented Sep 29, 2018

Uh oh!

TheBlueMatt commented Sep 30, 2018

Uh oh!

yuntai Sep 19, 2018 •

edited

Loading