chain service: use safe AddrManager address accessor

[chappjc]

This PR requires btcsuite/btcd#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]

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]

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]

An update that I've proposed an alternate btcd fix that requires no code changes from neutrino, just a btcd require bump: btcsuite/btcd#1763. dcrd had fixed it that way (add a mutex instead of making copies for external consumers). Either way seems fine to me, although btcsuite/btcd#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]

Good news, the alternate btcd fix that requires no consumer code changes was merged btcsuite/btcd#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]

@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