Refactor #28
Open
Refactor #28
Conversation
This file contains 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
This field is unused.
This field was unused.
Rather than having separate `ChannelState`s for waiting for funding
confirmed/locked we can merge these into the Normal state. The Normal
state's ShortChannelId and RemoteNextCommitInfo have been turned into
options where a value of `None` indicates that funding is not confirmed
or that the peer has not sent funding_locked respectively.
The advantages of this are:
(a) It simplifies the code by removing two more channel states.
(b) it allows us to handle channel shutdown before funding has been
locked. This is required by the lightning spec but, prior to this
change, attempting to apply a remote's `shutdown` message before
the funding is confirmed and locked would have crashed DNL.
(c) It's a step towards refactoring the channel type into two layers -
one which is saved to the wallet and persisted across reconnects,
and another which is discarded when the connection is dropped.
…ngHTLCs This event doesn't actually change the commitments. It's constructed using the channel's current commmitments and then replaces the channel's commitments when applied. So the channel's commitments are left unchanged, and the field is redundant.
This state is redundant. The `Normal` state already has `Option` fields to indicate whether we have sent and recieved shutdown messages but, as is, never sets both of them to `Some` at once and instead transitions to the Shutdown state when the second one would be set to `Some`. We can instead just remove this state and have two `Some` values be the indicator that we are shutting down. This removes a lot of code-duplication between how messages and events were handled between the two states.
These values were being thrown away and weren't being cross-checked against the script we receive in their shutdown message. We now store them in the channel state and raise an error if the peer sends us a different message in their shutdown.
We don't need to store the original messages that we sent/recieved, just the shutdown scripts within them.
The local/remote shutdown scripts were being held in two places in the channel data structure. We now hold them in a single place and structure the types so that we always have a side's shutdown script when they have entered shutdown.
These fields were completely unused.
This field was a List<List<ClosingTxProposed>>, however the inner List only ever contained zero or one elements. As such, it makes more sense to make it an Option.
This field had type List<Option<ClosingTxPerformed>>, however a None could only appear as the final element in this list where it was ignored anyway. The program logic is identical if we simply make this field a List<ClosingTxPerformed> and never insert a None at the end of the list.
This field was set but never read.
Remove ClosingTxProposed type and change the ClosingTxProposed field of NegotiatingData to just be a list of proposed fees instead. We don't need to store copies of the closing_signed messages and nor the closing transactions that we've proposed. We don't currently use them and we can always re-create them on demand anyway.
The lightning spec requires us to fail a channel if the remote peer proposes a closing fee which is not strictly between the previous fees proposed by both sides. This was not being enforced. This commit adds a field to NegotiatingData which tracks this value so that a protocol violation of this sort can be caught.
This type was a field of every variant of ChannelState. So rather than keeping it in ChannelState it can be moved up into Channel. Also remove the Opt suffix from the field name.
The ShortChannelId isn't tied to being in a non-closing state and we may need to access/modify it when we are in a negotiating state (for instance, if the channel is shutdown before funding gets locked). So move it up into Channel. Also remove the Opt suffix from the field name.
All this function did is ignore its arguments and return MutualClosePerformed, so we can just inline that instead.
This function was unfinished and unused.
Rather than storing the channel flags in a uint8 and accessing the flags through bit positions, use a structured type to hold the flags. This makes the code clearer and less bug-prone, and also hightlights that the announce_channel flag is duplicated in two places in the Channel structure.
Re-duplicate the local/remote ShutdownScriptPubKey fields since shutdown scripts sent during channel initialization need to be remembered for the lifetime of the channel but shutdown scripts sent during shutdown should be forgotten if the connection is broken before shutdown is complete. Factor the negotiating data out into the Channel type and remove ChannelState. The last other remaining state (NormalState) was now pointless since we store the static shutdown scripts in Channel so that they can be remembered regardless of what state we're in. The NegotiatingData field of Channel indicates that we are in shutdown if both peers have shared shutdown messages.
This type represents all channel configuration which is established
during the handshake and remains unchanged through the lifetime of the
channel.
The following fields are moved into this type: AnounceChannel,
RemoteNodeId, Network, FundingTxMinimumDepth, the shutdown scripts,
IsFunder, FundingScriptCoin, {Local,Remote}Params, and
RemoteChannelPubKeys. In doing so, we reduce the number of fields in
Commitments and the channel types.
Also, in CommitmentsModule, rather than passing around all the fields
individually pass the entire StaticChannelConfig object.
Remove the channelPrivKeys argument from the NewInbound and NewOutbound methods on Channel. This argument is redundant since it is computed from the NodeMasterKey that we pass to these methods. Also de-tuple the function arguments and remove a unnecessary level of indentation.
Change the CreateChannelReestablish operation into a method.
functions into methods
These are no longer used since we now do everything through methods on Channel.
This field was unused.
This was mis-named (it's the temporary channel id, not a "temporary failure") and also unused (we don't need the temporary channel id anymore at this point).
Fields in this message were duplicated elsewhere in this type.
Commitment and htlc transactions only have a single input, so the WhichInput field is always set to zero and is meaningless.
There may be both an incoming and an outgoing htlc for any given htlc id, so we can't store both incoming and outgoing htlcs in the same map under the same id. Separating these into two maps also makes the code a bit less strange since we no longer have to keep filtering the map for the htlc-direction that we want.
Similar refactoring as this past one: joemphilips@242a25b
This module defined the following interfaces:
type IState = interface end
type IStateData = interface end
type ICommand = interface end
type IEvent = interface end
These were not used in the code anywhere
Previously SignCommitment would return None instead of a CommitmentSignedMsg if either: (a) There were not updates to sign or (b) A revoke-and-ack for a previous commitment was not yet received Since these cases both indicate a misuse of the API they have been changed to errors.
If the depth of the funding tx is less than the minimum depth then this function will now return an error, rather than None.
An earlier refactor commit accidently removed the applyEvent handler for the BothFundingLocked event. As a result, when the executeCommand and applyEvent branches for the ApplyFundingLocked even got concatenated into an ApplyFundingLocked method, the branch of code which previously raised the BothFundingLocked event got replaced with a branch of code that did nothing. This meant that ApplyFundingLocked ended up doing nothing whenever we had already called ApplyFundingConfirmedOnBC. This commit fixes the mistake.
Similar to the last commit. This function was erroneously doing nothing if ApplyFundingConfirmedOnBC had previously been called.
Pre-refactor DNL was not giving geewallet the shutdown message when it should when RemoteShutdown was called. This was hacked around in geewallet's ClosedChannel code by reaching into DNL's internal state to get the message. Post-refactor, the message was no longer stored in DNL's internal state because it wasn't being used, nor was it being given to geewallet because that wasn't changed. This commit fixes this by simply handing it to geewallet when it should.
checkRemoteProposedFeeWithinNegotiatedRange was erroneously not allowing the remote peer to propose a fee equal to the one we had sent it. This is now fixed.
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.
No description provided.