Skip to content
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

fundingmanager: rejects funding requests with number of confirmations too large #2275

Merged
merged 7 commits into from Jan 15, 2019

Conversation

@wpaulino
Copy link
Collaborator

@wpaulino wpaulino commented Dec 5, 2018

In this PR, we allow the funding manager to reject any funding requests which impose a number of confirmations that is deemed as too large by the underlying ChainNotifier. This will properly prevent the funding state machine from continuing and avoid bugs like #2273, where we're unable to ever see a channel confirm as the notification registration will always fail due to a too large number of confirmations.

Fixes #2273.

@wpaulino wpaulino force-pushed the wpaulino:funding-max-confs branch 2 times, most recently from 7242d27 to 92f50cb Dec 5, 2018
fundingmanager.go Outdated Show resolved Hide resolved
chainntnfs/txnotifier.go Show resolved Hide resolved
chainntnfs/txnotifier_test.go Show resolved Hide resolved
chainntnfs/txnotifier.go Show resolved Hide resolved
msg.CsvDelay, msg.MaxAcceptedHTLCs, msg.MaxValueInFlight,
msg.HtlcMinimum, msg.ChannelReserve, msg.DustLimit,
)
channelConstraints := &channeldb.ChannelConstraints{

This comment has been minimized.

@halseth

halseth Dec 11, 2018
Collaborator

ooooh, this is nice

This comment has been minimized.

@Roasbeef

Roasbeef Jan 12, 2019
Member

Ah yeh see the rationale now.

channeldb/channel.go Show resolved Hide resolved
channeldb/channel.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef added this to In progress in High Priority via automation Dec 18, 2018
@Roasbeef Roasbeef moved this from In progress to Needs review in High Priority Dec 18, 2018
chainntnfs/txnotifier.go Show resolved Hide resolved
lnwallet/reservation.go Outdated Show resolved Hide resolved
lnwallet/reservation.go Outdated Show resolved Hide resolved
@@ -1241,6 +1243,7 @@ func (f *fundingManager) handleFundingAccept(fmsg *fundingAcceptMsg) {
MinHTLC: msg.HtlcMinimum,
MaxAcceptedHtlcs: msg.MaxAcceptedHTLCs,
CsvDelay: msg.CsvDelay,
NumConfs: msg.MinAcceptDepth,

This comment has been minimized.

@halseth

halseth Jan 8, 2019
Collaborator

We can just check the value here instead of making it part of the constraints.

This comment has been minimized.

@wpaulino

wpaulino Jan 12, 2019
Author Collaborator

Fixed.

channeldb/channel.go Outdated Show resolved Hide resolved
@wpaulino wpaulino force-pushed the wpaulino:funding-max-confs branch from c1bdd27 to 4f953af Jan 12, 2019
Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🍯

channeldb/channel.go Show resolved Hide resolved
msg.CsvDelay, msg.MaxAcceptedHTLCs, msg.MaxValueInFlight,
msg.HtlcMinimum, msg.ChannelReserve, msg.DustLimit,
)
channelConstraints := &channeldb.ChannelConstraints{

This comment has been minimized.

@Roasbeef

Roasbeef Jan 12, 2019
Member

Ah yeh see the rationale now.

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge Jan 14, 2019
Copy link
Collaborator

@halseth halseth left a comment

LGTM 💯

@@ -17,6 +17,10 @@ const (
// handled correctly. The average number of blocks in a day is a
// reasonable value to use.
ReorgSafetyLimit = 144

// MaxNumConfs is the maximum number of confirmations that can be
// requested on a transaction.

This comment has been minimized.

@halseth

halseth Jan 14, 2019
Collaborator

nit: would add the rationale for setting it equal to the comment.

// maximum number of confirmations required by the ChainNotifier to
// properly dispatch confirmations.
if msg.MinAcceptDepth > chainntnfs.MaxNumConfs {
err := lnwallet.ErrNumConfsTooLarge(

This comment has been minimized.

@halseth

halseth Jan 14, 2019
Collaborator

We should probably find a better pattern than having these errors defined in lnwallet, but this is okay for now.

@Roasbeef Roasbeef merged commit 815c4a9 into lightningnetwork:master Jan 15, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0007%) to 56.317%
Details
High Priority automation moved this from Final Testing -- Ready For Merge to Done Jan 15, 2019
@wpaulino wpaulino deleted the wpaulino:funding-max-confs branch Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
High Priority
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants