chain: fix NeutrinoClient segfault on NotifyReceived call #822
Conversation
MStreet3
changed the title
MStreet3
changed the title
There was a problem hiding this comment.
Thank you for re-structuring the PR! I left a few suggestions for how to improve it even more for easier review.
But I was able to understand all the changes, nice work! Sorry it took so long to get to the first pass review.
I think we can further reduce the diff by passing in the newRescanFunc in the NewNeutrinoClient. That removes the need for any type casting or having two versions of the cs member around. See my diff with the proposed changes (might need some additional cleanup, this is just to get my idea across):
MStreet3
force-pushed
the
bug/rescan-data-race
branch
5 times, most recently
from
4d87ccb to
45a359f
Compare
MStreet3
force-pushed
the
bug/rescan-data-race
branch
from
45a359f to
54710ef
Compare
|
@ellemouton could you take a look at this as well please? |
MStreet3
force-pushed
the
bug/rescan-data-race
branch
10 times, most recently
from
a175d3f to
baead51
Compare
There was a problem hiding this comment.
The rescanMtx resolution seems fine, but this PR introduces some notable and breaking API changes and abstraction, which create some challenges and confusion for consumers.
The abstraction seems unnecessary, except for test stubbing, but even then I don't think the constructors and exported types need to change. Namely, the test code can and does set the fields directly with its mock stubs (without using the constructors), since e.g. newMockNeutrinoClient manually creates the NeutrinoClient, and the newly added NewRescanFunc isn't used. Could the exported constructor, which consumers like the btcwallet app use, stay the same as before but deal with setting up the rescanner fields from the provided chain service?
This is only from a cursory scan, so I apologize in advance if my assessment is off base, but I don't think this improves the API in its present state, and I suspect the goals of testing with a mock chain service can still be achieved without the API changes.
MStreet3
force-pushed
the
bug/rescan-data-race
branch
3 times, most recently
from
0b24151 to
a6507c0
Compare
MStreet3
force-pushed
the
bug/rescan-data-race
branch
2 times, most recently
from
47929e7 to
9d91d11
Compare
MStreet3
force-pushed
the
bug/rescan-data-race
branch
3 times, most recently
from
df17f40 to
9498860
Compare
MStreet3
force-pushed
the
bug/rescan-data-race
branch
from
9498860 to
49e8366
Compare
Yea will do. |
MStreet3
force-pushed
the
bug/rescan-data-race
branch
from
c5e8a86 to
53904c1
Compare
|
@guggero the corresponding LND PR with updated btcwallet is here: lightningnetwork/lnd#7049 |
MStreet3
force-pushed
the
bug/rescan-data-race
branch
from
53904c1 to
12cb2a6
Compare
There was a problem hiding this comment.
Still builds and runs fine in my use case.
With limited familiarity with btcwallet's code, the atomicity changes look fine too. I don't see any lock ordering pitfalls that could lead to deadlock, but it's a risk of nested locking.
I think I see some ancient data races on the rescanQuit field in the notification handlers like onBlockConnected, but that's unrelated to the issue being solved in this PR.
Define an abstract rescanner interface and a newRescanFunc constructor type. Define a constructor adapter that adapts a neutrino.NewRescan constructor into the required type. Replace direct calls in the NeutrinoClient to the neutrino.NewRescan constructor with calls to the abstract type.
Define NeutrinoChainService interface to provide abstract access to all the public methods defined for a *neutrino.ChainService object. The interface defines all public methods to ensure downstream consumers of the package maintain compatibility as the previous reference to a *neutrino.ChainService is exposed publicly on the NeutrinoClient.
Add TestNeutrinoClientSequentialStartStop to verify that the neutrino client can have Start() and Stop() called sequentially. Test case fails with -race flag enabled. Add TestNeutrinoClientNotifyReceived to verify that calls to NotifyReceived call the Update method of the rescan object. Verify that there is no race condition on any state variables of the NeutrinoClient. Pass test with -race flag enabled. Add a unit test that demonstrates a segmentation fault exists when concurrent calls to NotifyReceived, NotifyBlocks and Rescan methods of the NeutrinoClient are executed. The rescan property is set to nil and a segmentation fault arises.
Add a mutex to synchronize access to the rescan prop of the neutrino client. Make calls to Rescan and NotifyReceived require the rescan mutex lock to execute. Resolve the segmentation fault in the TestNeutrinoClientNotifyReceivedRescan unit test.
Resolve the data race in the sequential start stop unit test by ensuring that each goroutine launched in the Start method is properly accounted for in the client's waitgroup. Add a TODO to shutdown the rescan goroutine when the Stop method is called.
MStreet3
force-pushed
the
bug/rescan-data-race
branch
from
12cb2a6 to
8c31629
Compare
chain: fix NeutrinoClient segfault on NotifyReceived call
chain: fix NeutrinoClient segfault on NotifyReceived call
Fixes #819
Replaces #820
Background
There is datarace in the neutrino client implementation that is causing test
flakes in
lnd. The sharedrescanobject and the granularlocking strategy allow for the possibility of a segmentation fault when
NotifyReceivedattempts to callUpdateon arescanthat isnil. Thisis because the
clientMtxlock is released afterrescanis set tonil.Changes
The solution here is to be minimally invasive. Add a new
rescanMtxmutexand require that the
NotifyReceivedmethod and theRescanmethodhold the
rescanMtxlock for the duration of their execution. Locking andunlocking of the client is unaffected and changes to the
rescanobjectbecome atomic.
Testing
This PR adds two interfaces
rescannerandnutrinoChainServiceto abstract out the dependency on structs from the
neutrinopackage.This PR adds a file of mock implementations for these interfaces and uses them
in a new unit test file.