From 950063840afc71894a4f076592af75827e0c2361 Mon Sep 17 00:00:00 2001 From: eugene Date: Thu, 23 Sep 2021 16:37:38 -0400 Subject: [PATCH] funding+lnwallet: validate ChannelReserve is above DustLimit This is necessary and is implied by BOLT#02. Both ChannelReserve parameters should be above both DustLimit parameters. Otherwise, it is possible for one side to have nothing at stake. --- funding/manager.go | 25 ++++++++++++++++++----- lnwallet/errors.go | 8 ++++++++ lnwallet/reservation.go | 36 +++++++++++++++++++++++++++++++-- lnwallet/test/test_interface.go | 8 ++++---- lnwallet/wallet.go | 18 ++++++++++++++--- 5 files changed, 81 insertions(+), 14 deletions(-) diff --git a/funding/manager.go b/funding/manager.go index 9cbf77d5f..9725630ee 100644 --- a/funding/manager.go +++ b/funding/manager.go @@ -1340,7 +1340,7 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, CsvDelay: msg.CsvDelay, } err = reservation.CommitConstraints( - channelConstraints, f.cfg.MaxLocalCSVDelay, + channelConstraints, f.cfg.MaxLocalCSVDelay, true, ) if err != nil { log.Errorf("Unacceptable channel constraints: %v", err) @@ -1386,7 +1386,19 @@ func (f *Manager) handleFundingOpen(peer lnpeer.Peer, remoteCsvDelay = acceptorResp.CSVDelay } - chanReserve := f.cfg.RequiredRemoteChanReserve(amt, msg.DustLimit) + // If our default dust limit was above their ChannelReserve, we change + // it to the ChannelReserve. We must make sure the ChannelReserve we + // send in the AcceptChannel message is above both dust limits. + // Therefore, take the maximum of msg.DustLimit and our dust limit. + // + // NOTE: Even with this bounding, the ChannelAcceptor may return an + // BOLT#02-invalid ChannelReserve. + maxDustLimit := reservation.OurContribution().DustLimit + if msg.DustLimit > maxDustLimit { + maxDustLimit = msg.DustLimit + } + + chanReserve := f.cfg.RequiredRemoteChanReserve(amt, maxDustLimit) if acceptorResp.Reserve != 0 { chanReserve = acceptorResp.Reserve } @@ -1576,7 +1588,7 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, CsvDelay: msg.CsvDelay, } err = resCtx.reservation.CommitConstraints( - channelConstraints, resCtx.maxLocalCsv, + channelConstraints, resCtx.maxLocalCsv, false, ) if err != nil { log.Warnf("Unacceptable channel constraints: %v", err) @@ -1586,8 +1598,11 @@ func (f *Manager) handleFundingAccept(peer lnpeer.Peer, // As they've accepted our channel constraints, we'll regenerate them // here so we can properly commit their accepted constraints to the - // reservation. - chanReserve := f.cfg.RequiredRemoteChanReserve(resCtx.chanAmt, msg.DustLimit) + // reservation. Also make sure that we re-generate the ChannelReserve + // with our dust limit or we can get stuck channels. + chanReserve := f.cfg.RequiredRemoteChanReserve( + resCtx.chanAmt, resCtx.reservation.OurContribution().DustLimit, + ) // The remote node has responded with their portion of the channel // contribution. At this point, we can process their contribution which diff --git a/lnwallet/errors.go b/lnwallet/errors.go index 96a91c169..0526acffd 100644 --- a/lnwallet/errors.go +++ b/lnwallet/errors.go @@ -152,6 +152,14 @@ func ErrChanTooLarge(chanSize, maxChanSize btcutil.Amount) ReservationError { } } +// ErrInvalidDustLimit returns an error indicating that a proposed DustLimit +// was rejected. +func ErrInvalidDustLimit(dustLimit btcutil.Amount) ReservationError { + return ReservationError{ + fmt.Errorf("dust limit %v is invalid", dustLimit), + } +} + // ErrHtlcIndexAlreadyFailed is returned when the HTLC index has already been // failed, but has not been committed by our commitment state. type ErrHtlcIndexAlreadyFailed uint64 diff --git a/lnwallet/reservation.go b/lnwallet/reservation.go index ce9110c57..00519f823 100644 --- a/lnwallet/reservation.go +++ b/lnwallet/reservation.go @@ -394,7 +394,7 @@ func (r *ChannelReservation) SetNumConfsRequired(numConfs uint16) { // will also attempt to verify the constraints for sanity, returning an error // if the parameters are seemed unsound. func (r *ChannelReservation) CommitConstraints(c *channeldb.ChannelConstraints, - maxLocalCSVDelay uint16) error { + maxLocalCSVDelay uint16, responder bool) error { r.Lock() defer r.Unlock() @@ -409,6 +409,13 @@ func (r *ChannelReservation) CommitConstraints(c *channeldb.ChannelConstraints, return ErrChanReserveTooSmall(c.ChanReserve, c.DustLimit) } + // Validate against the maximum-sized witness script dust limit, and + // also ensure that the DustLimit is not too large. + maxWitnessLimit := DustLimitForSize(input.UnknownWitnessSize) + if c.DustLimit < maxWitnessLimit || c.DustLimit > 3*maxWitnessLimit { + return ErrInvalidDustLimit(c.DustLimit) + } + // Fail if we consider the channel reserve to be too large. We // currently fail if it is greater than 20% of the channel capacity. maxChanReserve := r.partialState.Capacity / 5 @@ -449,7 +456,7 @@ func (r *ChannelReservation) CommitConstraints(c *channeldb.ChannelConstraints, // Our dust limit should always be less than or equal to our proposed // channel reserve. - if r.ourContribution.DustLimit > c.ChanReserve { + if responder && r.ourContribution.DustLimit > c.ChanReserve { r.ourContribution.DustLimit = c.ChanReserve } @@ -462,6 +469,31 @@ func (r *ChannelReservation) CommitConstraints(c *channeldb.ChannelConstraints, return nil } +// validateReserveBounds checks that both ChannelReserve values are above both +// DustLimit values. This not only avoids stuck channels, but is also mandated +// by BOLT#02 even if it's not explicit. This returns true if the bounds are +// valid. This function should be called with the lock held. +func (r *ChannelReservation) validateReserveBounds() bool { + ourDustLimit := r.ourContribution.DustLimit + ourRequiredReserve := r.ourContribution.ChanReserve + theirDustLimit := r.theirContribution.DustLimit + theirRequiredReserve := r.theirContribution.ChanReserve + + // We take the smaller of the two ChannelReserves and compare it + // against the larger of the two DustLimits. + minChanReserve := ourRequiredReserve + if minChanReserve > theirRequiredReserve { + minChanReserve = theirRequiredReserve + } + + maxDustLimit := ourDustLimit + if maxDustLimit < theirDustLimit { + maxDustLimit = theirDustLimit + } + + return minChanReserve >= maxDustLimit +} + // OurContribution returns the wallet's fully populated contribution to the // pending payment channel. See 'ChannelContribution' for further details // regarding the contents of a contribution. diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index 40ae2db23..9c251da04 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -454,7 +454,7 @@ func testDualFundingReservationWorkflow(miner *rpctest.Harness, CsvDelay: csvDelay, } err = aliceChanReservation.CommitConstraints( - channelConstraints, defaultMaxLocalCsvDelay, + channelConstraints, defaultMaxLocalCsvDelay, false, ) if err != nil { t.Fatalf("unable to verify constraints: %v", err) @@ -490,7 +490,7 @@ func testDualFundingReservationWorkflow(miner *rpctest.Harness, t.Fatalf("bob unable to init channel reservation: %v", err) } err = bobChanReservation.CommitConstraints( - channelConstraints, defaultMaxLocalCsvDelay, + channelConstraints, defaultMaxLocalCsvDelay, true, ) if err != nil { t.Fatalf("unable to verify constraints: %v", err) @@ -904,7 +904,7 @@ func testSingleFunderReservationWorkflow(miner *rpctest.Harness, CsvDelay: csvDelay, } err = aliceChanReservation.CommitConstraints( - channelConstraints, defaultMaxLocalCsvDelay, + channelConstraints, defaultMaxLocalCsvDelay, false, ) if err != nil { t.Fatalf("unable to verify constraints: %v", err) @@ -947,7 +947,7 @@ func testSingleFunderReservationWorkflow(miner *rpctest.Harness, t.Fatalf("unable to create bob reservation: %v", err) } err = bobChanReservation.CommitConstraints( - channelConstraints, defaultMaxLocalCsvDelay, + channelConstraints, defaultMaxLocalCsvDelay, true, ) if err != nil { t.Fatalf("unable to verify constraints: %v", err) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index f0a2f57f3..a99fd1b1e 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -1266,6 +1266,13 @@ func (l *LightningWallet) handleContributionMsg(req *addContributionMsg) { theirContribution := req.contribution ourContribution := pendingReservation.ourContribution + // Perform bounds-checking on both ChannelReserve and DustLimit + // parameters. + if !pendingReservation.validateReserveBounds() { + req.err <- fmt.Errorf("invalid reserve and dust bounds") + return + } + var ( chanPoint *wire.OutPoint err error @@ -1564,9 +1571,6 @@ func (l *LightningWallet) handleSingleContribution(req *addSingleContributionMsg pendingReservation.Lock() defer pendingReservation.Unlock() - // TODO(roasbeef): verify sanity of remote party's parameters, fail if - // disagree - // Validate that the remote's UpfrontShutdownScript is a valid script // if it's set. shutdown := req.contribution.UpfrontShutdown @@ -1584,6 +1588,14 @@ func (l *LightningWallet) handleSingleContribution(req *addSingleContributionMsg theirContribution := pendingReservation.theirContribution chanState := pendingReservation.partialState + // Perform bounds checking on both ChannelReserve and DustLimit + // parameters. The ChannelReserve may have been changed by the + // ChannelAcceptor RPC, so this is necessary. + if !pendingReservation.validateReserveBounds() { + req.err <- fmt.Errorf("invalid reserve and dust bounds") + return + } + // Initialize an empty sha-chain for them, tracking the current pending // revocation hash (we don't yet know the preimage so we can't add it // to the chain).