chain/neutrino: improve rescan locking #695
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpaulino
approved these changes
Apr 14, 2020
|
I tried running our itests against this PR (rebased onto current btcwallet master) and it looks like something's deadlocking now. The tests that do rescan don't even start properly, not sure what's going on. |
cfromknecht
force-pushed
the
rescan-nil-ptr
branch
from
0c4fca6
to
a2a90dd
Compare
cfromknecht
added a commit
to cfromknecht/lnd
that referenced
this pull request
Nov 12, 2020
cfromknecht
force-pushed
the
rescan-nil-ptr
branch
from
a2a90dd
to
1eb58d9
Compare
cfromknecht
added a commit
to cfromknecht/lnd
that referenced
this pull request
Nov 18, 2020
cfromknecht
force-pushed
the
rescan-nil-ptr
branch
from
1eb58d9
to
8d41d76
Compare
cfromknecht
added a commit
to cfromknecht/lnd
that referenced
this pull request
Nov 19, 2020
cfromknecht
added a commit
to cfromknecht/lnd
that referenced
this pull request
Nov 24, 2020
This commit fixes a potential nil pointer dereference in the neutrino chain client. Currently the Rescan method blindly overwrites the rescan member with nil _after_ reacquiring the lock. However, the field may have been populated with a new rescan while the mutex wasn't held, causing a panic later on where we assume the rescan is non-nil. To remedy, we add a simple check that asserts we are nilling the same rescan we were shutting down while the mutex was unlocked. If we find a differing value, we will continue to this process until we arrive at matching rescan pointers, at which point we know we can safely proceed in creating a new rescan.
The scanning variable is only ever set to true, and it happens in the same paths that the rescanQuit is initialized. Since rescanQuit is also only ever set to a non-nil value, we instead use this to determine whether we are "scanning".
The rescanQuit and rescan objects are set under different mutex acquisitions, hence it's possible for rescanQuit to be non-nil while rescan is nil. This commit ensures that we only try to wait for shutdown of non-nil rescans.
This also fixes an unprotected access of s.rescan when calling Update without the lock held.
Currently NotifyBlock releases the clientMtx before calling a public version of NotifyReceived that reacquires clientMtx. This can have unexpected behavior because the value of isScanning() could change between lock acquisitions. We switch to using the internal notifyReceived so that our read on isScanning() is consistent for the duration of the call.
cfromknecht
force-pushed
the
rescan-nil-ptr
branch
from
8d41d76
to
faf925f
Compare
cfromknecht
added a commit
to cfromknecht/lnd
that referenced
this pull request
Dec 8, 2020
cfromknecht
added a commit
to cfromknecht/lnd
that referenced
this pull request
Dec 27, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes 5 or so issues around locking the rescan-related member variables, see commit messages for more details. The issue fixed by the first commit was triggered by an lnd itest, the others are just observed from looking through the code.