Simplify + document the ChannelManager Err flow, fix close-outside-lock race, finish ChannelError conversion #258
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently channel_reserve_test sends a garbage update_add_htlc message and then relies on it being silently ignored to continue using the channel. This shouldn't be the case, so take the easy way out and split the test in two, at first not delivering the bogus update_add_htlc and then delivering it, but not running the rest of the test.
This converts block_connected failures to returning the ErrorMessage that needs to be sent directly, since it always results in channel closure and never results in needing to call force_shutdown. It also converts update_add_htlc and closing_signed handlers to ChannelError as the rest of the message handlers.
This moves a clone() inside Channel from ChannelManager making references simpler for the coming refactors.
If we never accessed channels for a peer outside of a message handler for that peer then this wouldn't be a problem since message handlers are required to be serialized per-peer. However, that isn't the world we live in - we may want to forward payments or we may get a send_payment call.
Best reviewed with -b
Best reviewed with -b
Technically funding_transaction_generated was fine using it, but calling force_shutdown on an empty Channel inside the channel_state lock isn't a big deal and almost any other use of it would be unsafe.
This removes all the channel-closure stuff from handle_error!() and MsgHandleErrInternal, making all the Err handling consistent by closing the channel before releasing the channel_state lock and then calling handle_error!() outside of the lock.
ariard
reviewed
Nov 24, 2018
| @@ -464,6 +464,24 @@ macro_rules! handle_error { | |||
| } | |||
| } | |||
|
|
|||
| macro_rules! break_chan_entry { | |||
There was a problem hiding this comment.
Maybe I'm exhaused but isn't break_chan_entry exactly the same as try_chan_entry ?
There was a problem hiding this comment.
break vs return in the Err conditions - one is used in let err = loop { };, the other when we just want to return.
There was a problem hiding this comment.
Ah okay good, shouldn't review after a given hour!
src/ln/channelmanager.rs
Outdated
| HandleError{err, .. } => assert_eq!(err, "Remote HTLC add would put them over their reserve value"), | ||
| } | ||
| // If we send a garbage message, the channel should get closed, making the rest of this test case fail. | ||
| /*assert_eq!(nodes[1].node.list_channels().len(), 1); |
There was a problem hiding this comment.
Hmmm, comments of the test isn't voluntary there ?
(uncommented on your branch)
There was a problem hiding this comment.
Well the test test for the correct behavior but it isnt implemented until the next commit, so the next commit just uncomments it. I can add a comment there for clarity if you want, but its just git history consistency....
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the races discovered during work on #254, as well as finishes the conversion to using ChannelError in place of HandleError in Channel Errs. It makes ChannelManager a bit more complicated, but we end up in a more consistent state than we were in previously, so hopefully its still within reason.