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

channeldb/db: prevent filtering channels with unconfirmed close txs #2248

Merged

Conversation

wpaulino
Copy link
Contributor

In this commit, we address an issue with the FetchWaitingCloseChannels
method where it would not properly return channels that are unconfirmed
and also have an unconfirmed closing transaction because they were
filtered out. We fix this by modifying the filtering logic to only apply
when the channel has already confirmed.

Related to #2217.

cfromknecht
cfromknecht previously approved these changes Nov 30, 2018
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM

cfromknecht
cfromknecht previously approved these changes Nov 30, 2018
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM

channeldb/db.go Outdated
@@ -511,7 +511,7 @@ func fetchChannels(d *DB, pending, waitingClose bool) ([]*OpenChannel, error) {
"node_key=%x: %v", chainHash[:], k, err)
}
for _, channel := range nodeChans {
if channel.IsPending != pending {
if !waitingClose && channel.IsPending != pending {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. this code is not straight forward to follow. Would it be better to instead add the channel if either

  1. channel.IsPending == pending
  2. channelWaitingClose == waitingClose

Another option is to only consider the pending argument if it is true, and use both pending and waitingClose as a way of filter channels.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure either is equivalent to the new behavior? Switching from 1 && 2 to 1 || 2 doesn't offer enough constraint to target a single quadrant of the state space. The new behavior ignores pending if we are looking for waitingClose=true channels, which is not the same as only considering pending if pending=true.

Docs + comments would help out here for sure

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, but with this ther will also be a quadrant impossible to target? (1 && 2)

Copy link
Contributor

Choose a reason for hiding this comment

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

The simplest change (the one proposed) is to treat channels that are pending and waiting close as waiting close, so it’s collapsing the two quadrants where waiting close is true.

We could leave the filtering logic as is, and just query for (pending=true, waitingClose=true) and (pending=false, waitingClose=true) and combine the results, which would be equivalent

Copy link
Contributor

Choose a reason for hiding this comment

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

Latter suggestion SGTM :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I think that keeps the filtering logic clearer
cc @wpaulino

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@Roasbeef Roasbeef added database Related to the database/storage of LND P2 should be fixed if one has time needs review PR needs review by regular contributors bug fix labels Dec 3, 2018
@wpaulino wpaulino force-pushed the fetch-waiting-close-channels branch 2 times, most recently from 03fcd7f to 346bb1d Compare December 11, 2018 03:44
@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
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Should be accompanied by a basic test to demonstrate the prior blind case (test fails on master right now) and ensure this change fixes it. Other than that, modified logic is straight forward.

@Roasbeef
Copy link
Member

Roasbeef commented Jan 3, 2019

Ping!

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM after test addition!

…annels

In this commit, we use random outpoints when creating new test channels
to ensure we can uniquely identify them.
@wpaulino wpaulino force-pushed the fetch-waiting-close-channels branch 2 times, most recently from 864cc8c to 70a917d Compare January 4, 2019 01:40
In this commit, we add a test case for FetchWaitingCloseChannels to
ensure it behaves as intended. Currently, this test fails due to not
fetching channels which are pending to be closed but are also pending to
be opened. This will be fixed in the following commit and should allow
the test to pass.
In this commit, we address an issue with the FetchWaitingCloseChannels
method where it would not properly return channels that are unconfirmed
and also have an unconfirmed closing transaction because they were
filtered out. We fix this by fetching channels that remain unconfirmed
that are also waiting for a confirmed closing transaction.

This will allow the recently added test TestFetchWaitingCloseChannels to
pass.
@wpaulino wpaulino force-pushed the fetch-waiting-close-channels branch from 70a917d to 70d3fc6 Compare January 4, 2019 01:48
@wpaulino
Copy link
Contributor Author

wpaulino commented Jan 4, 2019

@Roasbeef @halseth test added!

Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -852,6 +852,79 @@ func TestFetchClosedChannels(t *testing.T) {
}
}

// TestFetchWaitingCloseChannels ensures that the correct channels that are
// waiting to be closed are returned.
func TestFetchWaitingCloseChannels(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, the fixing commit should be added before the test case, to ensure the commit history is always building/passing. Not a blocker for this PR tho.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🌦

High Priority automation moved this from Needs review to Final Testing -- Ready For Merge Jan 4, 2019
@Roasbeef Roasbeef merged commit 6787ba2 into lightningnetwork:master Jan 4, 2019
High Priority automation moved this from Final Testing -- Ready For Merge to Done Jan 4, 2019
@wpaulino wpaulino deleted the fetch-waiting-close-channels branch January 4, 2019 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix database Related to the database/storage of LND needs review PR needs review by regular contributors P2 should be fixed if one has time
Projects
No open projects
High Priority
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants