-
Notifications
You must be signed in to change notification settings - Fork 2.3k
multi: log additional information for local force closures #8072
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
844681d
c60ea99
b22d72d
e67dabd
6824a82
6fc6915
65e7d6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1057,13 +1057,74 @@ type forceCloseReq struct { | |
| closeTx chan *wire.MsgTx | ||
| } | ||
|
|
||
| // ForceCloseContractOption is used for providing functional options to callers | ||
| // of ForceCloseContract(). | ||
| type ForceCloseContractOption func(*ChannelArbitrator) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the right strategy to go for here. Using continuations to implement this is "too powerful" and we probably just want to explicitly enumerate the cases.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay could you please explain why this is so? I got that it is "too powerful" but that was not backed up by anything really.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is too powerful because you are using a full continuation here. Someone could supply a function that does anything and it is delegating the process of calling the "log" functions to the argument. If you look at ChainArbitrator.ForceCloseContract it is simply calling out to the supplied Continuation passing has its uses but I don't think this is a good instance of it. That said, I wouldn't waste time taking a look at these smaller comments before zooming out and reworking the approach. See my other comment. I think @ziggie1984 has the right idea of what to do here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also don't think it's the right approach - 1) it's not really an option, but an extra step in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite get this @yyforyongyu. Care explaining more about this? |
||
|
|
||
| // UserInitiatedForceClose is a functional option for ForceCloseContract() that | ||
| // signifies that the force close is purely user-initiated. | ||
| func UserInitiatedForceClose(ca *ChannelArbitrator) { | ||
| // Just log errors since this is not a critical part of the force close | ||
| // flow. | ||
| err := ca.log.LogLocalForceCloseInitiator(channeldb.UserInitiated) | ||
| if err != nil { | ||
| log.Errorf("Error logging user initiated force "+ | ||
| "close initiator: %v", err.Error()) | ||
| } | ||
|
|
||
| log.Info("Logged user initiated force close.") | ||
| } | ||
|
|
||
| // LinkFailureErrorForceClose is a functional option for ForceCloseContract() | ||
| // that signifies that the force close is due to a link failure. Requires the | ||
| // error message of the link failure to be specified. | ||
| func LinkFailureErrorForceClose(errorMsg string) ForceCloseContractOption { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should accept a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please explain why this should be stored as a
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not that it needs to be stored as a LinkFailureError. But this function should accept a LinkFailureError for type safety reasons. It enforces stricter discipline at potential callsites |
||
| return func(ca *ChannelArbitrator) { | ||
| // Just log errors since this is not a critical part of the | ||
| // force close flow. | ||
| err := ca.log.LogLocalForceCloseInitiator( | ||
| channeldb.LocalForceCloseInitiator(errorMsg)) | ||
|
|
||
| if err != nil { | ||
| log.Errorf("Error logging link failure error force "+ | ||
| "close initiator: %v", | ||
| err.Error()) | ||
| } | ||
| log.Infof("Logged link failure error force close: %v", errorMsg) | ||
| } | ||
| } | ||
|
|
||
| // ChainTriggerForceClose is a functional option for ForceCloseContract() that | ||
| // signifies that the force close is due to a chain trigger. | ||
| func ChainTriggerForceClose( | ||
| chainActions ChainActionMap) ForceCloseContractOption { | ||
|
|
||
| return func(ca *ChannelArbitrator) { | ||
| // Ignore errors since logging force close info is not a | ||
| // critical part of the force close flow. | ||
| err := ca.log.LogLocalForceCloseInitiator( | ||
| channeldb.ChainActionsInitiated) | ||
|
|
||
| if err != nil { | ||
| log.Errorf("Error logging chain action force "+ | ||
| "close initiator: %v", | ||
| err.Error()) | ||
| } | ||
| ca.log.LogLocalFCChainActions(chainActions) | ||
|
|
||
| log.Info("Logged chain trigger initiated force close.") | ||
| } | ||
| } | ||
|
|
||
| // ForceCloseContract attempts to force close the channel infield by the passed | ||
| // channel point. A force close will immediately terminate the contract, | ||
| // causing it to enter the resolution phase. If the force close was successful, | ||
| // then the force close transaction itself will be returned. | ||
| // | ||
| // TODO(roasbeef): just return the summary itself? | ||
| func (c *ChainArbitrator) ForceCloseContract(chanPoint wire.OutPoint) (*wire.MsgTx, error) { | ||
| func (c *ChainArbitrator) ForceCloseContract(chanPoint wire.OutPoint, | ||
| opt ForceCloseContractOption) (*wire.MsgTx, error) { | ||
|
|
||
| c.Lock() | ||
| arbitrator, ok := c.activeChannels[chanPoint] | ||
| c.Unlock() | ||
|
|
@@ -1072,6 +1133,7 @@ func (c *ChainArbitrator) ForceCloseContract(chanPoint wire.OutPoint) (*wire.Msg | |
| } | ||
|
|
||
| log.Infof("Attempting to force close ChannelPoint(%v)", chanPoint) | ||
| opt(arbitrator) | ||
|
|
||
| // Before closing, we'll attempt to send a disable update for the | ||
| // channel. We do so before closing the channel as otherwise the current | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.