Improve error message. #644

joemphilips · 2020-06-21T08:03:42Z

... for ChannelError and APIMisuseError
Before this commit, When rl returns error, we don't know
The actual parameter which caused the error.
By returning parameterised String instead of predefined &'static str,
We can give a caller improved error message.

This will make my life easier when I'm debugging my server by connecting to other lightning nodes.
But I'm not sure if this is a sane approach. So I want to have a concept ACK before continuing my work.
The code does not compile but I suppose it is good enough to show what I'm trying to do to a reviewer.

jkczyz · 2020-06-22T17:41:10Z

Thanks for taking the initiative on this, @joemphilips!

I'd also like to see some improvements around our error API. I opened up issue #576 awhile back to discuss such improvements. Would love to get your input on this. Having feedback from someone actively experimenting with Rust Lightning will be valuable. And, of course, I'd welcome any contributions you'd like to make.

Regarding this PR, I agree that the message should provide more details to the user. I'd probably opt for using explicit conditions and format! over using the check_or constructs. Adding a local variable for capturing the expected should be fine.

joemphilips · 2020-06-23T13:43:10Z

@jkczyz
Right now for my FFI code, type information about an error is completely lost in FFI boundary, this is because passing smart enum (enum with fields) over FFI boundary are not straightforward. So instead I've just returning its message as a buffer.

So far I'm happy with this approach, so I don't have much opinion about how we compose error types. But thanks for letting me know about #576 anyway, I may have something to say in the future.

Regarding this PR, I agree that the message should provide more details to the user. I'd probably opt for using explicit conditions and format! over using the check_or constructs. Adding a local variable for capturing the expected should be fine.

Thanks, I will continue with that approach.

joemphilips · 2020-07-02T07:35:52Z

Ready for review. I don't know why Travis CI is failing. I think it has nothing to do with my change.

Rebase on lightningdevkit/rust-lightning#644

valentinewallace

partial review

I'm a fan of these changes, they would've definitely helped me a debug more quickly in the past

One thing is, I think we prefer every commit to compile and pass tests -- I'm guessing commits prior to f2b53e9c99dda14fbb6e3d820de147ee1e08a9ff wouldn't pass at the moment?

lightning/src/ln/channel.rs

joemphilips · 2020-07-09T10:56:27Z

Addressed @valentinewallace 's review and squashed some commits so that every commits pass tests.

valentinewallace

Thanks for the quick feedback turnaround!

lightning/src/ln/channel.rs

lightning/src/util/test_utils.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

valentinewallace · 2020-07-10T17:44:29Z

Great thanks, could you squash in the last two commits then I'll approve?

jkczyz

Sorry for the delay. Thanks for going through all these error strings!

jkczyz · 2020-07-12T01:47:00Z

lightning/src/routing/router.rs

@@ -881,7 +881,7 @@ mod tests {
 		assert_eq!(route.paths[0][0].fee_msat, 200);
 		assert_eq!(route.paths[0][0].cltv_expiry_delta, (13 << 8) | 1);
 		assert_eq!(route.paths[0][0].node_features.le_flags(), &vec![0b11]); // it should also override our view of their features
-		assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::new()); // No feature flags will meet the relevant-to-channel conversion
+		assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::<u8>::new()); // No feature flags will meet the relevant-to-channel conversion


Are these changes needed?

Yes, my compiler complains that it cannot infer the type. (in every rustc version.). I have absolutely no idea why this happens only in this branch, it works fine in master. But giving an type annotation does not hurt.

Strange. I was able to reproduce this but am unsure what would cause rustc to complain.

lightning/src/util/test_utils.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

joemphilips · 2020-07-13T04:32:44Z

Addressed @jkczyz 's review and re-split commits.

valentinewallace

Thanks for this detailed PR!

lightning/src/ln/channelmanager.rs

lightning/src/util/test_utils.rs

TheBlueMatt

Didnt check every line in the diff, but seems good pending val and Jeff's review.

TheBlueMatt · 2020-07-13T22:25:22Z

lightning/Cargo.toml

@@ -28,3 +28,4 @@ features = ["bitcoinconsensus"]

 [dev-dependencies]
 hex = "0.3"
+regex = "0.1.80"


Why not regex 1.3?

This was the latest release version which compiles in rustc 1.22.0

Ugh. Hmm, looks like their MSRV is still 1.28 (https://github.com/rust-lang/regex/blob/master/.github/workflows/ci.yml#L34) maybe lets just finally bump the MSRV to that given rust-bitcoin may bump to 1.29 sooner or later anyway.

I suppose bumping the MSRV should be done with other PR so should I rebase after that PR gets merged? I think it is fine to use old regex though.

Fair enough.

jkczyz

Looks good!

In the future, it would be less daunting for reviewers if the PR could be broken into more atomic commits. For instance, here one commit to switch from &'static str to String and another to update the error messages.

jkczyz · 2020-07-14T04:55:34Z

lightning/src/routing/router.rs

@@ -881,7 +881,7 @@ mod tests {
 		assert_eq!(route.paths[0][0].fee_msat, 200);
 		assert_eq!(route.paths[0][0].cltv_expiry_delta, (13 << 8) | 1);
 		assert_eq!(route.paths[0][0].node_features.le_flags(), &vec![0b11]); // it should also override our view of their features
-		assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::new()); // No feature flags will meet the relevant-to-channel conversion
+		assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::<u8>::new()); // No feature flags will meet the relevant-to-channel conversion


Strange. I was able to reproduce this but am unsure what would cause rustc to complain.

TheBlueMatt

Looks good!

TheBlueMatt · 2020-07-20T17:09:42Z

lightning/Cargo.toml

@@ -28,3 +28,4 @@ features = ["bitcoinconsensus"]

 [dev-dependencies]
 hex = "0.3"
+regex = "0.1.80"


Fair enough.

lightning/src/ln/channel.rs

... for ChannelError and APIMisuseError Before this commit, When rl returns error, we don't know The actual parameter which caused the error. By returning parameterised `String` instead of predefined `&'static str`, We can give a caller improved error message. TestLogger now has two additional methods 1. `assert_log_contains` which checks the logged messsage has how many entry which includes the specified string as a substring. 2. `aasert_log_regex` mostly the same with `assert_log_contains` but it is more flexible that caller specifies regex which has to be satisfied instead of just a substring. For regex, tests now includes `regex` as dev-dependency.

For making debugging easy. If the user gives a different node_secret for transport layer (`PeerManager`) and for routing msg, internal_announcement_signatures is the first place it causes an error. By giving a detailed error message, user will be able to fix the bug quickly.

jkczyz self-requested a review June 22, 2020 17:41

ariard added the Easy to Review label Jun 23, 2020

joemphilips force-pushed the improve_error_message branch from 9b67db5 to cd7c94d Compare July 2, 2020 05:47

joemphilips force-pushed the improve_error_message branch 4 times, most recently from aac2c8d to f2b53e9 Compare July 3, 2020 03:30

joemphilips changed the title ~~WIP: improve error message.~~ Improve error message. Jul 3, 2020

joemphilips force-pushed the improve_error_message branch from 1b21715 to 11a2d7f Compare July 4, 2020 10:42

joemphilips added a commit to joemphilips/NRustLightning that referenced this pull request Jul 6, 2020

bump rust-lightning

8fbf350

Rebase on lightningdevkit/rust-lightning#644

valentinewallace self-requested a review July 8, 2020 20:05

valentinewallace reviewed Jul 8, 2020

View reviewed changes

joemphilips force-pushed the improve_error_message branch from 11a2d7f to bc1d1a8 Compare July 9, 2020 10:51

valentinewallace reviewed Jul 9, 2020

View reviewed changes

joemphilips force-pushed the improve_error_message branch from 8f89271 to 68b3a4f Compare July 11, 2020 02:46

jkczyz reviewed Jul 12, 2020

View reviewed changes

joemphilips force-pushed the improve_error_message branch 2 times, most recently from 9c04b33 to e79317d Compare July 13, 2020 04:30

valentinewallace approved these changes Jul 13, 2020

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/util/test_utils.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Jul 13, 2020

View reviewed changes

joemphilips force-pushed the improve_error_message branch from e79317d to 6a2ae52 Compare July 14, 2020 01:52

jkczyz approved these changes Jul 14, 2020

View reviewed changes

TheBlueMatt approved these changes Jul 20, 2020

View reviewed changes

TheBlueMatt reviewed Jul 21, 2020

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

joemphilips added 2 commits July 22, 2020 10:34

joemphilips force-pushed the improve_error_message branch from 6a2ae52 to 407e306 Compare July 22, 2020 01:35

TheBlueMatt merged commit 50df4cf into lightningdevkit:master Jul 22, 2020

joemphilips deleted the improve_error_message branch August 8, 2020 02:38

Improve error message. #644

Improve error message. #644

Uh oh!

Conversation

joemphilips commented Jun 21, 2020

Uh oh!

jkczyz commented Jun 22, 2020

Uh oh!

joemphilips commented Jun 23, 2020

Uh oh!

joemphilips commented Jul 2, 2020

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joemphilips commented Jul 9, 2020

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joemphilips commented Jul 13, 2020

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

valentinewallace commented Jul 10, 2020 •

edited

Loading