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

contractcourt: fix off-by-one error in closeObserver #2766

Merged

Conversation

Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Mar 13, 2019

In this PR, we fix an off-by-one error when handling force closes from the remote party. Before this commit, if the remote party broadcasts state 2, and we were on state 1, then we wouldn't act at all. This is due to an extraneous +1 in the comparison, causing us to only detect this DLP case if the remote party's state is two beyond what we know atm. A test that fails when run on master has been added to demonstrate the bug, and confirm that it has been fixed.

Fixes #2764.

@Roasbeef Roasbeef added P2 should be fixed if one has time bug fix labels Mar 13, 2019
contractcourt/chain_watcher.go Show resolved Hide resolved
contractcourt/chain_watcher_test.go Show resolved Hide resolved
contractcourt/chain_watcher_test.go Outdated Show resolved Hide resolved
contractcourt/chain_watcher_test.go Outdated Show resolved Hide resolved
contractcourt/chain_watcher_test.go Outdated Show resolved Hide resolved
contractcourt/chain_watcher_test.go Show resolved Hide resolved
@roeierez
Copy link
Contributor

@Roasbeef following conversation in slack, I applied this fix to my node that had this issue (funds were not swept) and I confirm that now after restarting the funds were swept correctly.
Will wait for this to be merged but regardless of that the fast response and quick solution are very appreciated here.

@Roasbeef Roasbeef force-pushed the chain-watcher-fix-off-by-one branch from 7a0619c to fe8f3f0 Compare March 13, 2019 23:35
@Roasbeef
Copy link
Member Author

PTAL @halseth @cfromknecht

In this commit, we export the `ForceStateTransition` for tests outside
the package that need to interact with actual channel state machines.
In this commit, we abstract the call to `GetStateNumHint` within the
`closeObserver` method to a function closure in the primary config. This
allows us to feed in an arbitrary broadcast state number within unit
tests.
In this commit, we add a new test case to exercise the way we handle the
DLP detection and dispatch within the chain watcher. Briefly, we use
the `testing/quick` package to ensure that the following invariant is
always held: "if we do N state updates, then state M is broadcast, iff M
> N, we'll execute the DLP protocol". We limit the number of iterations
to 10 for now, as the tests can take a bit of time to execute, since it
actually does proper state transitions.
In this commit, we fix an off-by-one error when handling force closes
from the remote party. Before this commit, if the remote party
broadcasts state 2, and we were on state 1, then we wouldn't act at all.
This is due to an extraneous +1 in the comparison, causing us to only
detect this DLP case if the remote party's state is two beyond what we
know atm. Before this commit, the test added in the prior commit failed.
@Roasbeef Roasbeef force-pushed the chain-watcher-fix-off-by-one branch from fe8f3f0 to 6983a9e Compare March 14, 2019 00:34
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.

LGTB 💰

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.

LGTB 👍

@Roasbeef Roasbeef merged commit 5ef95a5 into lightningnetwork:master Mar 15, 2019
@MykelSIlver
Copy link

I am not very common with github. Are there any binaries available for this fix branch? Or is the only solution to build this fix from source? Thank you

@halseth
Copy link
Contributor

halseth commented Mar 28, 2019

@MykelSIlver currently only from source. But the v0.6 release is just around the corner 🤞

@MykelSIlver
Copy link

MykelSIlver commented Apr 12, 2019

@MykelSIlver currently only from source. But the v0.6 release is just around the corner

lnd version 0.6.0-beta commit=v0.6-beta-rc4 did indeed solve the problem 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix P2 should be fixed if one has time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

possible off-by-one error in chain watcher DLP detection
5 participants