Drop Result for ChannelMessageHandler methods #398

ariard · 2019-11-22T00:00:04Z

Further work to simplify our error management by removing ChannelMessageHandler errors and instead sending back HandleError for the P2PHandler.

If user, need more errors we should return APIErrors, but at least we should be have a clear distinction between errors for the user and errors sent to the peer.

TheBlueMatt · 2019-11-22T20:52:59Z

I think I like this idea, pending making sure every callsite can actually support this, but why not just do it inside handle_error!()? It seems to me you can just do all the logic we have now, but instead of returning the LightningError, we can just push it as an event (as long as its the last thing that happens during the message processing).

TheBlueMatt · 2019-12-13T16:56:16Z

lightning/src/ln/channelmanager.rs

@@ -429,19 +429,21 @@ pub struct ChannelDetails {
 }

 macro_rules! handle_error {
-	($self: ident, $internal: expr) => {
+	($self: ident, $internal: expr, $their_node_id: expr, $locked_channel_state: expr) => {


Hmm, this does end up taking an extra lock in the happy case. Maybe split the macro into handle_error_locked and handle_error_unlocked?

They were 2 callsites where we may don't have an error, modify a bit to avoid superfluous lock tacking if this is the case. Better than yet another error macro

Hmm? No, nearly ever message-handling callsite this is the case?

Ah yeah corrected, it's a bit ugly rn but ofc I'm still planning a dry-up following PR

TheBlueMatt · 2019-12-14T21:25:11Z

fuzz/src/chanmon_consistency.rs

@@ -516,6 +506,10 @@ pub fn do_test(data: &[u8]) {
 							// Can be generated due to a payment forward being rejected due to a
 							// channel having previously failed a monitor update
 						},
+						events::MessageSendEvent::HandleError { .. } => {


This really needs to check that the error in question is an IgnoreError action, and panic otherwise.

TheBlueMatt · 2019-12-14T21:25:36Z

fuzz/src/chanmon_consistency.rs

@@ -532,6 +526,7 @@ pub fn do_test(data: &[u8]) {
 							events::MessageSendEvent::SendChannelReestablish { .. } => {},
 							events::MessageSendEvent::SendFundingLocked { .. } => {},
 							events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
+							events::MessageSendEvent::HandleError { .. } => {},


Same here - this indicates exactly the type of failure this fuzz target exists to catch.

Oops, still missed this one.

TheBlueMatt · 2019-12-14T21:26:27Z

lightning/src/ln/chanmon_update_fail_tests.rs

-				if let Err(msgs::LightningError{err, action: msgs::ErrorAction::IgnoreError }) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed) {
-					assert_eq!(err, "Previous monitor update failure prevented generation of RAA");
+				nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed);
+				if let MessageSendEvent::HandleError { .. } = nodes[0].node.get_and_clear_pending_msg_events()[0] {


Can you restore the error-message-checking that was here previously?

Looks like this is still missing the IgnoreError check that was here?

TheBlueMatt

High level looks good but it looks like there were a ton of tests that were removed in this refactor. Can you re-add them (a few are commented, but there appear to be a lot).

ariard · 2019-12-30T23:16:12Z

Should be good now, modified Logger trait a bit to access logged lines and so keep test hardness as it is. Also added panic-if-not-IgnoreError in fuzz/src/chanmon_consistency.rs

TheBlueMatt · 2020-01-02T19:12:55Z

Why do you need to extend the visibility of the ChannelManager::logger? Its a shared reference so you should be able to just keep the original copy and check against that, no?

TheBlueMatt

I'm still a little confused about a few of the changes here, sadly. Also see the previous comment about the new logger.

TheBlueMatt · 2020-01-03T19:56:28Z

lightning/src/ln/channelmanager.rs

-				Err(APIError::ChannelUnavailable { err: e.err })
-			},
+		{
+			let mut channel_lock = self.channel_state.lock().unwrap();


nit: just extend the above lock to include this chunk. No reason to release and retake for one if statement in the happy path.

TheBlueMatt · 2020-01-03T20:03:45Z

lightning/src/ln/channelmanager.rs

-		match handle_error!(self, err) {
-			Ok(_) => unreachable!(),
-			Err(e) => {
-				if let msgs::ErrorAction::IgnoreError = e.action {


Hmm, there's a bunch of places where we match out IgnoreError and don't generate a HandleError event when we're returning an APIError anyway (so they user already saw it, no need to pass it through to peer_handler), but that didn't make it over here. Can you re-enable that somehow? (maybe leave the if, or maybe add a wrapper macro).

IIRC, that was quite intentional. Because you do want to log error even if it's a IgnoreError one but then was planning in further commit to match it out in the handle_error macro to avoid passing for nothing to peer_handler. I think that's good for now to pass it to peer_handler, it's just logged and ignored..

No? The IgnoreErrors that are generated today are all things where you didn't already get back an APIError directly (as the user). There is no need to send duplicative errors (though logging them is, of course, fine IMO).

I think there is already duplicative errors right now in send_payment/update_fee for the non-IgnoreError cases? Inside handle_error, you don't know if an APIError is going to be returned, so for not generating a HandleError when we're returning an APIError anyway I need to filter out all IgnoreError, and so modify test in consequence again ?

TheBlueMatt · 2020-01-03T23:13:09Z

lightning/src/ln/chanmon_update_fail_tests.rs

-				if let Err(msgs::LightningError{err, action: msgs::ErrorAction::IgnoreError }) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed) {
-					assert_eq!(err, "Previous monitor update failure prevented generation of RAA");
+				nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed);
+				if let MessageSendEvent::HandleError { .. } = nodes[0].node.get_and_clear_pending_msg_events()[0] {


Looks like this is still missing the IgnoreError check that was here?

TheBlueMatt · 2020-01-03T23:14:10Z

lightning/src/ln/chanmon_update_fail_tests.rs

-			if let msgs::LightningError { err, action: msgs::ErrorAction::IgnoreError } = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &updates.commitment_signed).unwrap_err() {
-				assert_eq!(err, "Failed to update ChannelMonitor");
+			nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &updates.commitment_signed);
+			if let MessageSendEvent::HandleError { .. } = nodes[0].node.get_and_clear_pending_msg_events()[0] {


Same here as above - why not keep the IgnoreError and message-in-HandleError check that was here (in addition to the new log message tests)?

TheBlueMatt

Oops, there's still quite a few things worth fixing here, IMO. No need to break things "to fix them in a later followup".

TheBlueMatt · 2020-01-04T01:47:33Z

fuzz/src/chanmon_consistency.rs

@@ -532,6 +526,7 @@ pub fn do_test(data: &[u8]) {
 							events::MessageSendEvent::SendChannelReestablish { .. } => {},
 							events::MessageSendEvent::SendFundingLocked { .. } => {},
 							events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
+							events::MessageSendEvent::HandleError { .. } => {},


Oops, still missed this one.

TheBlueMatt · 2020-01-04T01:47:53Z

lightning/src/util/logger.rs

@@ -119,6 +119,8 @@ impl<'a> Record<'a> {
 pub trait Logger: Sync + Send {
 	/// Logs the `Record`
 	fn log(&self, record: &Record);
+	/// Assert log count (used to harden test)
+	fn assert_log(&self, module: String, line: String, count: usize);


Oof, please dont expose a testing-only method in the public interface.

TheBlueMatt · 2020-01-04T01:50:25Z

fuzz/src/chanmon_consistency.rs

@@ -565,6 +570,7 @@ pub fn do_test(data: &[u8]) {
 						},
 						events::MessageSendEvent::SendFundingLocked { .. } => false,
 						events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => false,
+						events::MessageSendEvent::HandleError { .. } => false,


Oops, should also check this one for IgnoreError

TheBlueMatt · 2020-01-04T01:50:53Z

fuzz/src/chanmon_consistency.rs

+						events::MessageSendEvent::HandleError { action, .. } => {
+							// Can be generated at any processing step to send back an error, disconnect
+							// peer or just ignore
+							match action {


Note that these could be significantly more succinct with just a match for events::MessageSendEvent::HandleError { action: ErrorAction::IgnoreError, .. } instead of a general one.

TheBlueMatt · 2020-01-04T01:53:09Z

lightning/src/ln/chanmon_update_fail_tests.rs

-				if let Err(msgs::LightningError{err, action: msgs::ErrorAction::IgnoreError }) = nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed) {
-					assert_eq!(err, "Previous monitor update failure prevented generation of RAA");
+				nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), commitment_signed);
+				if let MessageSendEvent::HandleError { ref action, .. } = nodes[0].node.get_and_clear_pending_msg_events()[0] {


This (and in many other places) only checks the first error, if there are more than one.

TheBlueMatt · 2020-01-04T02:09:22Z

lightning/src/ln/channelmanager.rs

-		match handle_error!(self, err) {
-			Ok(_) => unreachable!(),
-			Err(e) => {
-				if let msgs::ErrorAction::IgnoreError = e.action {


No? The IgnoreErrors that are generated today are all things where you didn't already get back an APIError directly (as the user). There is no need to send duplicative errors (though logging them is, of course, fine IMO).

Extend mock Node with logger.

Simplify interfaces between ChannelMessageHandler and PeerManager, by switching all ChannelMessageHandler errors to HandleError sent internally instead of being return. With further refactors in Router and PeerChannelEncryptor, errors management on the PeerManager-side won't be splitted between try_potential_handleerror and HandleError processing. Inside ChannelManager, we now log MsgHandleErrInternal and send ErrorAction to PeerManager. On a high-level, it should allow client using API to be more flexible by polling events instead of waiting function call returns. We also update handle_error macro to take channel_state_lock from caller which should avoid some deadlock potential for some edges cases. Filter out IgnoreError in handle_error macro, update test in consequence.

TheBlueMatt

Will take as #439 with the following nits changed.

TheBlueMatt · 2020-01-06T00:08:25Z

lightning/src/ln/channelmanager.rs

@@ -1116,71 +1119,62 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {

 		let _ = self.total_consistency_lock.read().unwrap();

-		let err: Result<(), _> = loop {
+		{


Hmm, the new scope here is unnecessary.

TheBlueMatt · 2020-01-06T00:11:51Z

lightning/src/ln/channelmanager.rs

@@ -1197,8 +1191,8 @@ impl<ChanSigner: ChannelKeys> ChannelManager<ChanSigner> {
 		let _ = self.total_consistency_lock.read().unwrap();

 		let (mut chan, msg, chan_monitor) = {
+			let mut channel_state = self.channel_state.lock().unwrap();
 			let (res, chan) = {


This scope can go away now that its not releasing the lock.

TheBlueMatt · 2020-01-06T00:19:02Z

lightning/src/ln/channelmanager.rs

+			let mut channel_state_lock = self.channel_state.lock().unwrap();
+			for (their_node_id, err) in handle_errors.drain(..) {
+				match handle_error!(self, err, their_node_id, channel_state_lock) {
+					Ok(_) => {},


Dummy match?

TheBlueMatt · 2020-01-06T00:21:34Z

lightning/src/ln/channelmanager.rs

-				Err(APIError::APIMisuseError { err: e.err })
-			},
+		{
+			let mut channel_state_lock = self.channel_state.lock().unwrap();


No need to drop the lock before here and add a new scope.

TheBlueMatt · 2020-01-06T00:26:38Z

lightning/src/ln/channelmanager.rs

+		let res = self.internal_open_channel(their_node_id, their_local_features, msg);
+		if res.is_err() {
+			let mut channel_state_lock = self.channel_state.lock().unwrap();
+			if let Err(_) = handle_error!(self, res, *their_node_id, channel_state_lock) {}


This can be a bit more succinct with just let _ = handle_error!().

TheBlueMatt · 2020-01-06T01:13:37Z

lightning/src/ln/chanmon_update_fail_tests.rs

 	commitment_signed_dance!(nodes[1], nodes[2], payment_event.commitment_msg, false);

 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
 	expect_pending_htlcs_forwardable!(nodes[1]);
 	check_added_monitors!(nodes[1], 1);
-	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());


lol you added an extra line to do the same thing :(. Also, there's a bunch of let msg_events = ... assert!(msg_events.len(), 0) that could be rewritten as just a .is_empty().

TheBlueMatt · 2020-01-06T01:16:45Z

lightning/src/ln/chanmon_update_fail_tests.rs

-			}
-		} else { panic!(); }
-
-		assert!(nodes[0].node.get_and_clear_pending_events().is_empty());


Hmm, seem to have lost this?

TheBlueMatt · 2020-01-06T01:20:21Z

lightning/src/ln/functional_test_utils.rs

@@ -491,8 +495,8 @@ macro_rules! commitment_signed_dance {
 			};
 			check_added_monitors!($node_b, 1);
 			if $fail_backwards {
-				assert!($node_a.node.get_and_clear_pending_events().is_empty());
-				assert!($node_a.node.get_and_clear_pending_msg_events().is_empty());
+				//assert!($node_a.node.get_and_clear_pending_events().is_empty());


Looks like you didn't finish working on this?

TheBlueMatt · 2020-01-06T01:30:10Z

lightning/src/ln/functional_tests.rs

-			let msgs::ErrorMessage {ref channel_id, ..} = msg;
-			assert_eq!(*channel_id, chan_1.2);
+		nodes[1].node.handle_channel_reestablish(&nodes[0].node.get_our_node_id(), &node_0_2nd_reestablish);
+		if let MessageSendEvent::HandleError { ref action, .. } = nodes[1].node.get_and_clear_pending_msg_events()[0] {


Should check that there's only one msg event here.

TheBlueMatt · 2020-01-06T01:44:27Z

lightning/src/ln/functional_tests.rs

+			if let &MessageSendEvent::HandleError { ref action, .. } = handle_error {
+				match action {
+					&ErrorAction::SendErrorMessage { ref msg } => {
+						assert_eq!(msg.data, "Remote HTLC add would put them over their reserve value");


You can DRY this pattern up a ton (and fix a few cases where you forgot the trailing } else { panic!() }) by moving these matches into the macro.

ariard force-pushed the 2019-11-drop-api-errors branch from b8c3a2f to d02a615 Compare December 12, 2019 19:58

ariard changed the title ~~[WIP] Drop Result for ChannelMessageHandler methods~~ Drop Result for ChannelMessageHandler methods Dec 12, 2019

ariard force-pushed the 2019-11-drop-api-errors branch 2 times, most recently from 493f667 to a565e9c Compare December 12, 2019 23:53

TheBlueMatt reviewed Dec 13, 2019

View reviewed changes

ariard force-pushed the 2019-11-drop-api-errors branch 4 times, most recently from 64d3f58 to 07fc987 Compare December 13, 2019 23:01

TheBlueMatt mentioned this pull request Dec 14, 2019

Per-channel mutexes #422

Closed

TheBlueMatt reviewed Dec 14, 2019

View reviewed changes

ariard force-pushed the 2019-11-drop-api-errors branch 2 times, most recently from 7e77375 to ce12586 Compare December 30, 2019 23:15

TheBlueMatt reviewed Jan 3, 2020

View reviewed changes

ariard force-pushed the 2019-11-drop-api-errors branch from ce12586 to fa6f891 Compare January 4, 2020 01:24

TheBlueMatt reviewed Jan 4, 2020

View reviewed changes

ariard force-pushed the 2019-11-drop-api-errors branch 3 times, most recently from cc422e8 to 4837917 Compare January 5, 2020 20:58

Antoine Riard added 2 commits January 5, 2020 17:23

Add assert_log method to TestLogger to harden test

29ace59

Extend mock Node with logger.

ariard force-pushed the 2019-11-drop-api-errors branch from 4837917 to a15ec7a Compare January 5, 2020 22:23

TheBlueMatt reviewed Jan 6, 2020

View reviewed changes

TheBlueMatt closed this Jan 6, 2020

Drop Result for ChannelMessageHandler methods #398

Drop Result for ChannelMessageHandler methods #398

Conversation

ariard commented Nov 22, 2019

TheBlueMatt commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

ariard commented Dec 30, 2019

TheBlueMatt commented Jan 2, 2020

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt Jan 3, 2020 •

edited

Loading

TheBlueMatt Jan 3, 2020 •

edited

Loading

TheBlueMatt Jan 3, 2020 •

edited

Loading