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

chancloser: unregister channel before db modification #4030

Merged
merged 1 commit into from Mar 4, 2020

Conversation

halseth
Copy link
Contributor

@halseth halseth commented Feb 25, 2020

This fixes a race during channel closing, where the last channel state
update was still not finished when we set the channel close bit in the
database. This lead to a flake during integration tests, where the last
state update would not finish, and the channel wasn't closes
successfully.

We fix it by first unregistering the channel, making sure it is removed
fully from the link before doing the db modification.

Fixes

--- FAIL: TestLightningNetworkDaemon (74.86s)
    lnd_test.go:15365: Running 61 integration tests
    --- FAIL: TestLightningNetworkDaemon/route_fee_cutoff (51.38s)
        lnd_test.go:103: Failed: (route fee cutoff): exited with error:
            *errors.errorString unable to close channel: timeout reached before channel close initiated
            /github.com/lightningnetwork/lnd-worktree/lntest/itest/lnd_test.go:287 (0x199fca7)
            	closeChannelAndAssert: t.Fatalf("unable to close channel: %v", err)
            /github.com/lightningnetwork/lnd-worktree/lntest/itest/lnd_test.go:13019 (0x19eaccd)
            	testRouteFeeCutoff: closeChannelAndAssert(ctxt, t, net, net.Alice, chanPointAliceBob, false)
            /github.com/lightningnetwork/lnd-worktree/lntest/itest/lnd_test.go:126 (0x199ee6a)
            	(*harnessTest).RunTestCase: testCase.test(h.lndHarness, h)
            /github.com/lightningnetwork/lnd-worktree/lntest/itest/lnd_test.go:15386 (0x1a0deec)
            	TestLightningNetworkDaemon.func4: ht.RunTestCase(testCase)
            /usr/local/Cellar/go/1.13.8/libexec/src/testing/testing.go:909 (0x1107eb9)
            	tRunner: fn(t)
            /usr/local/Cellar/go/1.13.8/libexec/src/runtime/asm_amd64.s:1357 (0x10612e1)
            	goexit: BYTE	$0x90	// NOP
FAIL

@Roasbeef Roasbeef added this to the 0.10.0 milestone Feb 27, 2020
@Roasbeef Roasbeef added this to In Progress in v0.10.0-beta via automation Feb 27, 2020
@bhandras bhandras self-requested a review March 3, 2020 13:23
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM

chancloser.go Outdated
// Before returning the shutdown message, we'll unregister the channel
// to ensure that it isn't seen as usable within the system.
//
// TODO(roasbeef): fail if err?
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i think the TODO doesn't apply anymore as unregisterChannel won't fail.

v0.10.0-beta automation moved this from In Progress to Merge Queue Mar 3, 2020
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 💪 should fix @bhandras's comment before merging

This fixes a race during channel closing, where the last channel state
update was still not finished when we set the channel close bit in the
database. This lead to a flake during integration tests, where the last
state update would not finish, and the channel wasn't closes
successfully.

We fix it by first unregistering the channel, making sure it is removed
fully from the link before doing the db modification.
@halseth halseth merged commit 42e65d4 into lightningnetwork:master Mar 4, 2020
v0.10.0-beta automation moved this from Merge Queue to Done Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v0.10.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants