Implement the last three (relevant) unimplemented()s in ChannelManager #364

TheBlueMatt · 2019-07-27T16:32:43Z

There's still one in update_fee that hopefully will die with all the fee stuff, and all of the Watchtower-mode monitor stuff is unimplemented!() but aside from that this is the last of them!

codecov · 2019-07-27T17:11:13Z

Codecov Report

Merging #364 into master will increase coverage by 0.03%.
The diff coverage is 92.89%.

@@            Coverage Diff             @@
##           master     #364      +/-   ##
==========================================
+ Coverage   87.29%   87.33%   +0.03%     
==========================================
  Files          29       29              
  Lines       15457    15605     +148     
==========================================
+ Hits        13493    13628     +135     
- Misses       1964     1977      +13

Impacted Files	Coverage Δ
src/ln/functional_test_utils.rs	`94.61% <100%> (-0.08%)`	⬇️
src/ln/channel.rs	`84.53% <100%> (+0.14%)`	⬆️
src/ln/channelmanager.rs	`81.46% <66.66%> (-0.21%)`	⬇️
src/ln/chanmon_update_fail_tests.rs	`97.01% <98.82%> (+0.15%)`	⬆️
src/ln/functional_tests.rs	`96.22% <0%> (-0.06%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd2fef...8ba3529. Read the comment docs.

ariard

Overall seems good ! Pushing these 2 comments already, will finish review later!

Also, have to learn this codecov report now :p

ariard · 2019-07-27T22:38:42Z

src/ln/channel.rs

@@ -1540,7 +1540,7 @@ impl Channel {
 		if !self.channel_outbound {
 			return Err(ChannelError::Close("Received funding_signed for an inbound channel?"));
 		}
-		if self.channel_state != ChannelState::FundingCreated as u32 {
+		if self.channel_state & !(ChannelState::MonitorUpdateFailed as u32) != ChannelState::FundingCreated as u32 {


IMO MonitorUpdateFailed and PeerDisconnected shouldn't pollute ChannelState as they are private state to the node and not public state we expect in synchronization with the othe peer state machine. MonitorUpdateFailed forerun any other states and we stop protocol execution when we get one. It would make easier to reason on if at some point we refactor it.

That was kind of the original discussion when monitor update failure was first implemented - its either done as a ChannelManager thing where the Channel isn't touched and the ChannelManager is responsible for message retransmission, or its done at the Channel level (which is way lighter-weight, though much ore complicated) and we have to handle it everywhere. While you may have a point with PeerDisconnected, handling it on the Channel level makes it much easier to avoid DoS issues where buffers in ChannelManager for message retransmission aren't bounded. Ultimately this means a blowup in state complexity, but it does afford us a few really nice features wrt being able to take some steps forward even though a channel is frozen.

That's a good point even if I think you can bound per channel_id at the ChannelManager level to avoid DoS issues. Okay, another advantage I see tracking in Channel is you may have a per channel monitor failure, if you decide to send your updates filtered by the funding_outpoint to different set of monitors. Being able to take some steps forward even though a channel is frozen, let me think that in fact you only want to commit updates for CS/RAA, for the funding_signed case not being able to update immediately is this a real risk ? First commitment transaction doesn't have any time-sensitive output and remote peer can't broadcast a lower state

I don't think its ever much of a real risk, more of a nice-to-have. On the update_fulfill_htlc side its really nice to have, though, as it allows us to mark an HTLC fulfilled/claim upstream even if the monitor updating has failed.

ariard · 2019-07-27T23:03:10Z

src/ln/channel.rs

@@ -2345,9 +2349,10 @@ impl Channel {
 	/// Indicates that the latest ChannelMonitor update has been committed by the client
 	/// successfully and we should restore normal operation. Returns messages which should be sent
 	/// to the remote side.
-	pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>) {
+	pub fn monitor_updating_restored(&mut self) -> (Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, bool) {


Hmm I was reading again resend_order comment, which maybe should be refer by function comment, and "Note that because there can only ever be one in-flight CS and/or one in-flight RAA at any time..." is right but really obscure.
IIRC already commented on this comment, but a clearer could be "Protocol update going forward by committing to a new state then revoking older one by local peer, follow by the same sequence again while inverting jobs , when remote peer sends us CS we have to reply by RAA (so we resend_order so), when it sends us RAA, we have to reply CS.

Hmm, I'm not quite sure what your objection is?

Just a nit, was thinking aloud about a better resend_order comment now that I understand a bit better channel_reestablish logic

Hmm, happy to think about it in another pr, but didn't touch it here.

src/ln/channelmanager.rs

ariard · 2019-07-29T15:03:08Z

src/ln/channelmanager.rs

-					if let Err(_e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
-						unimplemented!();
+					if let Err(e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
+						return_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::RevokeAndACKFirst, false, false);


here RAACommitmentOrder::RevokeAndACKFirst is purely arbitrary given that we don't use it and we shouldn't have receive yet any CS/RAA ?

ariard · 2019-07-29T15:30:57Z

src/ln/channel.rs

+		// MonitorUpdateFailed, if we assume the user only broadcast the funding transaction when
+		// they received the FundingBroadcastSafe event, we can only ever hit
+		// monitor_pending_funding_locked when we're an inbound channel which failed to persist the
+		// monitor on funding_signed, and we even got the funding transaction confirmed before the


If you're an inbound channel you should receive a funding_created and send a funding_signed, not receive one?
And assert comment isn't really clear too, if you're an inbound channel you don't have to broadcast the funding transaction.

Right, oops, sorry, it should read funding_created. I think the assertion failure string is right, though? If the channel is outbound (ie the assertion fails), then the string indicating you broadcasted before you were supposed to is right.

Yes I think that's the right, but would explicit your assumption there in the above comment "we MUST NOT generate a FundingBroadcastSafe event while being a outbound channel and having failed our update after funding_created which throws us in MonitorUpdateFailed"

ariard · 2019-07-29T15:52:57Z

src/ln/channel.rs

+				 // Note that funding_signed/funding_created will have decremented both by 1!
+				 self.cur_local_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1 &&
+				 self.cur_remote_commitment_transaction_number == INITIAL_COMMITMENT_NUMBER - 1) ||
+				(self.channel_state & (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) ==


nit : maybe this should go in its own commit given that's not monitor update related

Hmmm is this covered by new tests ?

The new tests do, indeed, fail in this case. But I went ahead and pulled it into its own commit.

ariard · 2019-07-29T16:05:35Z

src/ln/channelmanager.rs

+					// channel, not the temporary_channel_id. This is compatible with ourselves, but the
+					// spec is somewhat ambiguous here. Not a huge deal since we'll send error messages for
+					// any messages referencing a previously-closed channel anyway.
+					return Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure", funding_msg.channel_id, chan.force_shutdown(), None));


Not specifically here, but in handle_monitor_err we disclose to our peer than we have a monitor failure. That's kinda overlap with our previous talk on data_loss_protection, IMO still have doubts on this. But you can't reply anyway that a serious attacker able to fingerprint a rust-lightning node can know our error call flow so even if we tell another content he will know that's a monitor failure...

Right, but in theory the point of a monitor error is to close ASAP because we still have a monitor in memory that is fully up-to-date. Its only if the node crashes that we're screwed.

This carries a surprising amount of complexity despite only being possible in the case where monitor updating failed during the processing of funding_generated. Specifically, this requires handling rebroadcasting funding_locked once we successfully persist our monitor again. As an alternative we could never send funding_signed when the monitor failed to persist, but this approach avoids needless delays during funding.

ariard · 2019-07-29T19:15:36Z

utACK 8ba3529

TheBlueMatt force-pushed the 2019-07-no-unimpl branch from 0bf609e to 55a392a Compare July 27, 2019 16:37

ariard reviewed Jul 27, 2019

View reviewed changes

ariard reviewed Jul 29, 2019

View reviewed changes

TheBlueMatt added 3 commits July 29, 2019 13:26

Handle monitor update failures during funding on the funder side

a1e0ca4

Allow a counterparty to resend their funding_locked before ours

b27f788

TheBlueMatt force-pushed the 2019-07-no-unimpl branch from 55a392a to 8ba3529 Compare July 29, 2019 17:48

TheBlueMatt merged commit 283d40f into lightningdevkit:master Jul 29, 2019

Implement the last three (relevant) unimplemented()s in ChannelManager #364

Implement the last three (relevant) unimplemented()s in ChannelManager #364

Uh oh!

Conversation

TheBlueMatt commented Jul 27, 2019

Uh oh!

codecov bot commented Jul 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ariard left a comment • 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!

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!

ariard commented Jul 29, 2019

Uh oh!

Uh oh!

codecov bot commented Jul 27, 2019 •

edited

Loading

ariard left a comment •

edited

Loading