search

lightninglabs / neutrino

Privacy-Preserving Bitcoin Light Client
MIT License
900 stars 183 forks source link

chain service: use safe AddrManager address accessor #230

Closed chappjc closed 2 years ago

chappjc commented 2 years ago

This PR requires https://github.com/btcsuite/btcd/pull/1760. The replace would be removed and the btcd require updated if that is merged. The data races between neutrino and btcd are described on that PR.

The *KnownAddress returned by (*AddrManager).GetAddress is not safe for use while other goroutines are using the AddrManager's methods for updating peer states e.g. Connect/Attempt/AddAddresses/SetServices.

The newAddressFunc closure created by NewChainService as the ConnManager's GetNewAddress function was using GetNetAddress concurrent with other peer management goroutines. e.g. a Peer's inHandler that has a callbacks to AddAddresses and SetServices.

This resolves the issue by using the new GetNetAddress method instead of GetAddress. This is safe since the returned wire.NetAddress is immutable, unlike the KnownAddress from GetAddress.

Roasbeef commented 2 years ago

Thanks for this PR!

Re the underlying bug, is there any concrete unintended interaction that results from it? So like us re-trying the same address over and over again, when we think it's actually a new address?

chappjc commented 2 years ago

is there any concrete unintended interaction that results from it

It's a little hard to tell if there's a specific observable malfunction other than the data race printed, but it's certainly possible that the newAddressFunc was working with invalid addresses and access times given the right circumstances. I think it's mainly about the possibility for memory corruption.

chappjc commented 2 years ago

An update that I've proposed an alternate btcd fix that requires no code changes from neutrino, just a btcd require bump: https://github.com/btcsuite/btcd/pull/1763. dcrd had fixed it that way (add a mutex instead of making copies for external consumers). Either way seems fine to me, although https://github.com/btcsuite/btcd/pull/1763 requires no code changes to neutrino, and it does not touch the API. I have btcwallet running in SPV mode with this new one and it's doing fine.

chappjc commented 2 years ago

Good news, the alternate btcd fix that requires no consumer code changes was merged https://github.com/btcsuite/btcd/pull/1763 This PR is no longer needed. neutrino may want to update the btcd required, but fortunately that's also something a consumer module can force.

Roasbeef commented 2 years ago

@chappjc ah np, thanks for looking into this in any case! Apologies wasn't able to get to it sooner, focusing on cutting the initial release candidate for lnd 0.14