Skip to content

lnwallet/chancloser: remove the commit fee clamp, introduce max fee #6770

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Aug 9, 2022
Merged

lnwallet/chancloser: remove the commit fee clamp, introduce max fee #6770

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c140da8
lnwallet/chancloser: remove the commit fee clamp, introduce max fee
Roasbeef Jul 26, 2022
4a997bf
lnwallet: add new AbsoluteThawHeight method
Roasbeef Jul 26, 2022
106912b
lnwallet/chancloser: remove raw state machine pointer, add channel in…
Roasbeef Jul 26, 2022
9eb1e87
lnwallet/chancloser: add tests for the new max fee behavior
Roasbeef Jul 26, 2022
c791b18
lnrpc: add a new max_fee field to the CloseChannel RPC call
Roasbeef Jul 26, 2022
1e556aa
multi: thread through the new max fee field for co-op close
Roasbeef Jul 26, 2022
046aa7f
docs/release-notes: add entry for 0.15.1
Roasbeef Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 33 additions & 14 deletions lnwallet/chancloser/chancloser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ var (
// shutdown script previously set for that party.
ErrUpfrontShutdownScriptMismatch = fmt.Errorf("shutdown script does not " +
"match upfront shutdown script")

// ErrProposalExeceedsMaxFee is returned when as the initiator, the
// latest fee proposal sent by the responder exceed our max fee.
// responder.
ErrProposalExeceedsMaxFee = fmt.Errorf("latest fee proposal exceeds " +
"max fee")
)

// closeState represents all the possible states the channel closer state
Expand Down Expand Up @@ -73,6 +79,14 @@ const (
closeFinished
)

const (
// defaultMaxFeeMultiplier is a multiplier we'll apply to the ideal fee
// of the initiator, to decide when the negotiated fee is too high. By
// default, we want to bail out if we attempt to negotiate a fee that's
// 3x higher than our max fee.
defaultMaxFeeMultiplier = 3
)

// ChanCloseCfg holds all the items that a ChanCloser requires to carry out its
// duties.
type ChanCloseCfg struct {
Expand All @@ -89,6 +103,9 @@ type ChanCloseCfg struct {
// Disconnect will disconnect from the remote peer in this close.
Disconnect func() error

// MaxFee, is non-zero represents the highest fee that the initiator is
// willing to pay to close the channel.
MaxFee chainfee.SatPerKWeight
// Quit is a channel that should be sent upon in the occasion the state
// machine should cease all progress and shutdown.
Quit chan struct{}
Expand Down Expand Up @@ -122,6 +139,11 @@ type ChanCloser struct {
// offer when starting negotiation. This will be used as a baseline.
idealFeeSat btcutil.Amount

// maxFee is the highest fee the initiator is willing to pay to close
// out the channel. This is either a use specified value, or a default
// multiplier based of the initial starting ideal fee.
maxFee btcutil.Amount

// lastFeeProposal is the last fee that we proposed to the remote party.
// We'll use this as a pivot point to ratchet our next offer up, down, or
// simply accept the remote party's prior offer.
Expand Down Expand Up @@ -168,17 +190,12 @@ func NewChanCloser(cfg ChanCloseCfg, deliveryScript []byte,
// TODO(roasbeef): should factor in minimal commit
idealFeeSat := cfg.Channel.CalcFee(idealFeePerKw)

// If this fee is greater than the fee currently present within the
// commitment transaction, then we'll clamp it down to be within the proper
// range.
//
// TODO(roasbeef): clamp fee func?
channelCommitFee := cfg.Channel.StateSnapshot().CommitFee
if idealFeeSat > channelCommitFee {
chancloserLog.Infof("Ideal starting fee of %v is greater than commit "+
"fee of %v, clamping", int64(idealFeeSat), int64(channelCommitFee))

idealFeeSat = channelCommitFee
// When we're the initiator, we'll want to also factor in the highest
// fee we want to pay. This'll either be 3x the ideal fee, or the
// specified explicit max fee.
maxFee := idealFeeSat * defaultMaxFeeMultiplier
if cfg.MaxFee > 0 {
maxFee = cfg.Channel.CalcFee(cfg.MaxFee)
Copy link
Collaborator

@Crypt-iQ Crypt-iQ Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the ideal fee, the max fee isn't respected on restart

Copy link
Member Author

@Roasbeef Roasbeef Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think one way to resolve this would be to commit this information to disk when we call MarkCoopBroadcasted.

Copy link
Member Author

@Roasbeef Roasbeef Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we'd store persistently the preferences of the user at that point, and for the case where we're the responder (but still the initiator), we'd do something like base it off some (larger?) multiplier of the max commitment fee.

Copy link
Collaborator

@Crypt-iQ Crypt-iQ Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be stored in MarkCoopBroadcasted since #6760 will end up changing that function's usage. This is because the link may still need to be loaded, but we still want the user's preferences. I made a function called PersistDeliveryScript which could be extended to persist other information before calling MarkCoopBroadcasted

I also think we could check here that the ideal fee isn't greater than the max fee if one exists

Copy link
Collaborator

@Crypt-iQ Crypt-iQ Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where we're the responder (but still the initiator), we'd do something like base it off some (larger?) multiplier of the max commitment fee.

and/or a config option?

Copy link
Member Author

@Roasbeef Roasbeef Aug 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from my reading, we'll retain the "ideal" fee on restart, since we'll compute the "ideal" fee based on CoopCloseTargetConfs (defaults to 6 blocks). At this point, the req will be nil, so we'll then based the max fee off 3x this ideal fee.

So I think we can either leave it as is, persist it in #6760, or add a config option (yet another one, that ppl prob won't set tbh) to let ppl express what this max multiplier should be.

Copy link
Collaborator

@Crypt-iQ Crypt-iQ Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two ways the ideal fee is set:

  • through rpc if initiating the close
  • via EstimateFeePerKW on CoopCloseTargetConfs

The first case probably won't have the same fee on restart. The second may if the fee estimation doesn't change. I don't think this should be persisted in this PR. It could be done in #6760. But I think any persistence changes should be done in the same release so we don't have to check whether something is stored or not

Copy link
Member Author

@Roasbeef Roasbeef Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. I'd say a user's expectation would be for things to resume with their desired fee, so we should strive for that. Agree we can do it in another PR, though rn #6760 is marked for 0.16 (and in a draft state).

}

chancloserLog.Infof("Ideal fee for closure of ChannelPoint(%v) is: %v sat",
Expand All @@ -193,6 +210,7 @@ func NewChanCloser(cfg ChanCloseCfg, deliveryScript []byte,
cfg: cfg,
negotiationHeight: negotiationHeight,
idealFeeSat: idealFeeSat,
maxFee: maxFee,
localDeliveryScript: deliveryScript,
priorFeeOffers: make(map[btcutil.Amount]*lnwire.ClosingSigned),
locallyInitiated: locallyInitiated,
Expand Down Expand Up @@ -483,9 +501,10 @@ func (c *ChanCloser) ProcessCloseMsg(msg lnwire.Message) ([]lnwire.Message,
feeProposal := calcCompromiseFee(c.chanPoint, c.idealFeeSat,
c.lastFeeProposal, remoteProposedFee,
)
if feeProposal > c.idealFeeSat*3 {
return nil, false, fmt.Errorf("couldn't find" +
" compromise fee")
if c.cfg.Channel.IsInitiator() && feeProposal > c.maxFee {
return nil, false, fmt.Errorf("%w: %v > %v",
ErrProposalExeceedsMaxFee, feeProposal,
c.maxFee)
}

// With our new fee proposal calculated, we'll craft a new close
Expand Down