-
Notifications
You must be signed in to change notification settings - Fork 100
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
core,btc,app: Manage SPV wallet peers #1931
Conversation
I really like the Manage Peers view. Good stuff @martonp! |
@chappjc This will be ready to review once dcrlabs/neutrino-bch#9 and dcrlabs/neutrino-ltc#6 are merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK, for sure. This is really well done.
client/asset/btc/spv_peer_manager.go
Outdated
// Peers returns the list of peers that the wallet is connected to. It also | ||
// returns the peers that the user added that the wallet may not currently | ||
// be connected to. | ||
func (w *SPVPeerManager) Peers() ([]*asset.WalletPeer, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why w
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update, I had these methods on the btcSPVWallet type earlier.
073abb4
to
dd373b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will keep an eye out for the issue. There are a number of neutrino-related issues that only really pop out with testnet or mainnet where sync can take actual time.
EDIT: just made that into an issue: #1934 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@buck54321 I hadn't updated the cache busters. You should be able to see the button now. |
c387299
to
34d186b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it was working well before the last changes. Not sure what happened but now unable to add peers.
The neutrino logs show:
2022-11-04 12:23:22.381 [DBG] NTRNO: Peer 127.0.0.1:28333 is banned for another 23h4m36.618483257s
2022-11-04 12:23:22.382 [DBG] NTRNO: Peer 127.0.0.1:28336 is banned for another 23h4m36.617422658s
2022-11-04 12:23:22.384 [DBG] NTRNO: Peer 127.0.0.1:28335 is banned for another 23h4m36.615500595s
2022-11-04 12:29:13.708 [DBG] NTRNO: Peer 127.0.0.1:28337 is banned for another 22h58m45.291805946s
And my bchd has logs that correspond with attempting to add:
2022/11/04 12:29:13 http: TLS handshake error from 127.0.0.1:32816: EOF
Also, removing peers shows an error, but also seems to work if I reload the page:
@JoeGruffins Seems like your node was banned by neutrino for some reason, maybe due to the node not serving the compact filters? I'll add a reconnect button on the screen that will unban and attempt to connect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like my suggestion of using s.ChainService.ConnectNode(addr, true)
is not working for me. It is not adding the node for some reason. I can look into it more.
client/asset/btc/spv_peer_manager.go
Outdated
// loadSavedPeersFromFile replaces the contents of dexc-peers.json. | ||
func (w *SPVPeerManager) writeSavedPeersToFile(peers map[string]peerSource) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadSavedPeersFromFile -> writeSavedPeersToFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bch issue does not appear to be related to these changes.
So is |
No. Well, yes for bitcoind <-> neturino, no for bchd <-> neutrino. But @martonp was looking into it and it looks like it is a communications issue. From @martonp: I think this warrants a separate issue. I am planning to make one after these changes are finalized, or marton can. |
I don't understand. The BCH SPV wallet has worked fine with the bchd harness in the past. Testnet too. Are you saying that it's now broken, or it's just broken when manually adding peers in this PR? |
I guess you were just talking about this PR. BCH SPV wallet still seems fine to me:
However, bchd still seems to try to discover other peers on regnet and I end up with it connecting to the same bitcoin-cash-node twice and seeing lots of apparently harmless DBG level errors. Silenced with: diff --git a/dex/testing/btc/base-harness.sh b/dex/testing/btc/base-harness.sh
index afa05287d..b9c565194 100755
--- a/dex/testing/btc/base-harness.sh
+++ b/dex/testing/btc/base-harness.sh
@@ -393,7 +393,7 @@ if [ ! -z "$GODAEMON" ]; then
OMEGA_RPC_PORT=21558
cat > "${NODE_CONF}" <<EOF
-addpeer=127.0.0.1:${ALPHA_LISTEN_PORT}
+connect=127.0.0.1:${ALPHA_LISTEN_PORT}
listen=:21577
rpcuser=user
rpcpass=pass
@@ -401,7 +401,7 @@ rpclisten=0.0.0.0:${OMEGA_RPC_PORT}
regtest=1
rpccert=${OMEGA_DIR}/rpc.cert
rpckey=${OMEGA_DIR}/rpc.key
-debuglevel=trace
+debuglevel=debug
EOF
cat > "${CLIENT_CONF}" <<EOF |
I don't think the bchd node should be trying to connect to the neutrino peer, only the other way around. I changed the omega node config from addpeer to connect, and created the following PR as well: gcash/bchd#519 |
I was able to connect to bchd on testnet as well. I removed the default peer now to avoid confusion.. the default peer was "localhost:28333" which attempted to connect to [::1]:28333, and would fail if bchd was set to listen to 127.0.0.1:2883. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review because I want to respond to latest comment from @martonp
} | ||
|
||
// Attempt to look up an IP address associated with the parsed host. | ||
ips, err := net.LookupIP(host) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the last few weeks we've been bit by localhost
not resolving on some systems, depending on build tags, OS config, and DNS provider. Two things we might want to do: change our pre-configured localhost
peers to 127.0.0.1, and put a manual resolution in this function to resolve localhost
to 127.0.0.1
because we can't always rely on net.LookupIP
to do it. If we really want to respect the possibility that the user has IPv6 loopback (::1), we could check if host
was localhost
in the err branch and only then fallback to 127.0.0.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't get any error if it resolves to the IPv6 address and there's nothing listening over there, neutrino will just keep retrying. So we'd have to separately make a connection to try it out. I think just manually resolving to 127.0.0.1
is fine.. do you think we should put a message on the screen stating that localhost
resolves to 127.0.0.1
, and users should type [::1]
if that's what they need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add messaging about how we resolve localhost
IMO. Resolvers are very inconsistent about localhost
, and given we won't get an error if it incorrectly resolves to IPv6 and neutrino will retry and fail forever, we should take steps to prevent that.
users should type [::1] if that's what they need?
Totally. If they really want [::1], they can type that.
client/asset/btc/spv_peer_manager.go
Outdated
// to the user. If a user previously added a peer that originally connected | ||
// but now the address cannot be resolved to an IP, it should be displayed | ||
// that the wallet was unable to connect to that peer. | ||
w.peers[addr] = &walletPeer{source: source, resolvedName: resolvedAddr} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't resolvedAddr
be an empty string here?
Made a relevant comment in my review here #1931 (comment) But ACK to removing the localhost peer, regardless of the resolution issues. Since BCH testnet4 at least has a DNS seeder (testnet4.imaginary.cash), unlike LTC testnet. It only returns 1 node unfortunately, itself apparently, but it's something. |
This change allows the user to see the peers the BTC SPV wallets are connected to, and add new peers. - `client/asset`: Adds a new `PeerManager` interface with three methods: `Peers`, `AddPeer` and `RemovePeer`. Wallets that implement this interface have the trait `WalletTraitPeerManager`. - `client/core`: Adds new methods that call the `PeerManager` methods. - `client/btc`: - A `SPVPeerManager` type is added which manages a key value db and a neutrino chain service. When the user adds a new peer, it is stored in the key value db, and will be automatically connected the next time the wallet is opened. - The `BTCWallet` interface now has three new functions, `Peers`, `AddPeer` and `RemovePeer`, and the BTC, BCH, and LTC SPV wallets each use a `SPVPeerManager` in order to handle these new functions.
BCH spv has never worked for me, besides one iteration of this pr. Could it be because I have ipv6 disabled on my computer? I see some discussion about that above. |
That's likely. Now that @martonp has it going to 127.0.0.1, I bet you'll be good now. |
Oh yeah! It is working now on sim and testnet. I guess that was it. |
There are a few unaddressed comments from other reviews, but otherwise good. Please circle back on the comments from @JoeGruffins and @buck54321, thanks. |
This change allows the user to see the peers the BTC SPV wallets are connected to, and add new peers.
client/asset
: Adds a newPeerManager
interface with three methods:Peers
,AddPeer
andRemovePeer
. Wallets that implement this interface have the traitWalletTraitPeerManager
.client/core
: Adds new methods that call thePeerManager
methods.client/btc
:SPVPeerManager
type is added which manages a key value db and a neutrino chain service. When the user adds a new peer, it is stored in the key value db, and will be automatically connected the next time the wallet is opened.BTCWallet
interface now has three new functions,Peers
,AddPeer
andRemovePeer
, and the BTC, BCH, and LTC SPV wallets each use aSPVPeerManager
in order to handle these new functions.app
: