client: monitor wallet peers #1445

chappjc · 2022-01-24T23:45:50Z

This adds a PeersChange callback to client/asset.WalletConfig, similar to TipChange, for backends to report changes to the network peer count. The backends now implement a monitorPeers goroutine similar to monitorBlocks but checking and reporting on peer count changes. Any changes in peer count result in a wallet state note. Change in peer count from 0 to >0 also start a sync status monitoring goroutine, which runs until sync is restored.

This also updates each backend's SyncStatus method to ensure that the synced bool will never be true if there are peer count is 0.

Killing the "beta" node with the SPV beta wallet:

Starting up beta dcrd makes the signal-! icon change to a syncing arrow circle icon, which then goes away when it catches up with the network.

Killing "alpha" bitcoind with native btcwallet:

That also returns to normal after restarting alpha bitcoind.

Now the full-node wallets (still simnet):

wallet-peer-state-icons.mp4

2022-02-07 11:04:07.769 [ERR] CORE[dcr]: Failed to get peer count: rawrequest error: -9: wallet.NetworkBackend: Decred network is unreachable
2022-02-07 11:04:07.769 [WRN] CORE: Wallet for asset dcr has zero network peers!
2022-02-07 11:04:07.771 [WRN] CORE: notify: |WARNING| (walletconfig) Wallet network issue - Decred wallet has no network peers!
2022-02-07 11:04:07.771 [TRC] CORE: notify: |DATA| (walletstate)
2022-02-07 11:04:16.004 [TRC] CORE: notify: |DATA| (epoch) - Index: 109616897
2022-02-07 11:04:18.586 [ERR] CORE: btc wallet is reporting a failed state: failed to get best block hash from btc node
...
2022-02-07 11:04:22.586 [ERR] CORE[btc]: Failed to get peer count: rawrequest error: Post "http://127.0.0.1:20556/wallet/": dial tcp 127.0.0.1:20556: connect: connection refused
2022-02-07 11:04:22.586 [WRN] CORE: Wallet for asset btc has zero network peers!
2022-02-07 11:04:22.587 [ERR] CORE: btc wallet is reporting a failed state: failed to get best block hash from btc node
2022-02-07 11:04:22.589 [WRN] CORE: notify: |WARNING| (walletconfig) Wallet network issue - Bitcoin wallet has no network peers!
2022-02-07 11:04:22.589 [TRC] CORE: notify: |DATA| (walletstate)
2022-02-07 11:04:23.587 [ERR] CORE: btc wallet is reporting a failed state: failed to get best block hash from btc node
...
<start back up the alpha dcrd and bitcoind>
...
2022-02-07 11:05:42.769 [TRC] CORE: New peer count for asset dcr: 2
2022-02-07 11:05:42.769 [TRC] CORE: notify: |DATA| (walletstate)
2022-02-07 11:05:45.772 [TRC] CORE: notify: |DATA| (walletstate)
2022-02-07 11:05:45.779 [TRC] CORE: notify: |DATA| (balance) balance updated
2022-02-07 11:05:45.779 [INF] CORE: Wallet synced for asset dcr
2022-02-07 11:05:46.004 [TRC] CORE: notify: |DATA| (epoch) - Index: 109616903
...
2022-02-07 11:06:22.588 [TRC] CORE: New peer count for asset btc: 1
2022-02-07 11:06:22.588 [TRC] CORE: notify: |DATA| (walletstate)
2022-02-07 11:06:25.592 [TRC] CORE: notify: |DATA| (walletstate)
2022-02-07 11:06:25.596 [TRC] CORE: notify: |DATA| (balance) balance updated
2022-02-07 11:06:25.596 [INF] CORE: Wallet synced for asset btc

chappjc · 2022-01-25T21:31:28Z

client/asset/eth/eth.go

+	timeDiff := time.Now().Unix() - int64(bh.Time)
+	if timeDiff > dexeth.MaxBlockInterval && eth.net != dex.Simnet {


@JoeGruffins I noticed this was dividing now in seconds by a thousand before comparing with the header time in seconds, so in practice this check was almost impossible to fail. I'm pretty sure 180 seconds since last block in our simnet workflows is gonna be too tight so I'm excluding simnet here. Note that the unit tests leave net as 0 (mainnet), so we're still testing the conditions.

client/core/wallet.go

client/core/core.go

chappjc · 2022-02-09T14:55:27Z

client/core/core.go

+			select {
+			case <-ticker.C:
+				if c.walletCheckAndNotify(wallet) {
+					return


Not new behavior (see the old connectWallet method), but in this case the canceler goroutine above is stuck on Wait until the wallet shuts down even though the sync status loop is done. Arguably a minor goroutine leak, but really just excessive lifetime. No issue IMO, but worth noting.

buck54321

Iffy about the pattern used in loadWallet, but otherwise g2g.

buck54321 · 2022-02-09T11:31:27Z

client/core/core.go

@@ -1871,7 +1860,7 @@ func (c *Core) loadWallet(dbWallet *db.Wallet) (*xcWallet, error) {

 	// Construct the unconnected xcWallet.
 	contractLockedAmt, orderLockedAmt := c.lockedAmounts(assetID)
-	return &xcWallet{
+	wallet = &xcWallet{ // captured by the PeersChange closure


// captured by the PeersChange closure

Assumption is that a wallet will never call its PeersChange callback in the constructor itself, otherwise we'd panic in (*Core).peerChange.

buck54321 · 2022-02-09T11:39:07Z

client/asset/interface.go

+	// PeersChange is a function that will be called when the number of
+	// wallet/node peers changes, or the wallet fails to get the count.
+	PeersChange func(uint32)


As per other comment, it's important that PeersChange is not called in the constructor (actually a little fuzzier than that). It's presumably not an issue since we wouldn't have a peer change until Connect, but we're relying on the wallet to get this right.

If you wanna roll as-is, please comment here on the importance of not calling this method during initialization.

buck54321 · 2022-02-09T21:24:19Z

client/core/core.go

+	go func() {
+		if wallet.connector.On() {
+			wallet.connector.Wait()
+		}
+		cancel()
+	}()


This makes me want a Done() <-chan struct{} on the ConnectionMaster.

Testing a Done() method in #1474

client/core/core_test.go

chappjc · 2022-02-09T22:42:10Z

client/core/core.go

@@ -1860,6 +1846,9 @@ func (c *Core) loadWallet(dbWallet *db.Wallet) (*xcWallet, error) {
 			// of deadlocking a Core method that calls Wallet.Disconnect.
 			go c.tipChange(assetID, err)
 		},
+		PeersChange: func(numPeers uint32) {
+			go c.peerChange(wallet, numPeers)


@buck54321 I can put a wallet != nil check in here.
If the check is hit that would mean there's a data race though..

Can you just pass an assetID and pull the wallet from the map?

Had actually started that way. Will see why I veered away. Might be fine.

Oh right, because it's not in the wallets map until after successful connect and a variety of other operations depending on whos calling loadWallet. :(
I recall now being mystified why peerChanges sometimes didn't work.

potentially:

diff --git a/client/core/core.go b/client/core/core.go index f81f9895..45ca809d 100644 --- a/client/core/core.go +++ b/client/core/core.go @@ -1834,6 +1834,7 @@ func (c *Core) assetDataDirectory(assetID uint32) string { // wallet. The returned wallet is running but not connected. func (c *Core) loadWallet(dbWallet *db.Wallet) (*xcWallet, error) { var wallet *xcWallet + var ready uint32 // in case asset.Open tries to call PeersChange before actually starting // Create the client/asset.Wallet. assetID := dbWallet.AssetID walletCfg := &asset.WalletConfig{ @@ -1847,6 +1848,9 @@ func (c *Core) loadWallet(dbWallet *db.Wallet) (*xcWallet, error) { go c.tipChange(assetID, err) }, PeersChange: func(numPeers uint32) { + if atomic.LoadUint32(&ready) == 0 { + return + } go c.peerChange(wallet, numPeers) }, DataDir: c.assetDataDirectory(assetID), @@ -1875,6 +1879,7 @@ func (c *Core) loadWallet(dbWallet *db.Wallet) (*xcWallet, error) { walletType: dbWallet.Type, traits: asset.DetermineWalletTraits(w), } + atomic.StoreUint32(&ready, 1) return wallet, nil }

but... 🤮

chappjc force-pushed the monitor-peers branch 2 times, most recently from 0b49f8d to e231a12 Compare January 25, 2022 21:26

chappjc commented Jan 25, 2022

View reviewed changes

client/core/wallet.go Show resolved Hide resolved

chappjc commented Jan 25, 2022

View reviewed changes

client/core/core.go Show resolved Hide resolved

chappjc force-pushed the monitor-peers branch from e231a12 to 9a4db97 Compare January 25, 2022 22:07

chappjc force-pushed the monitor-peers branch from 9a4db97 to 07f1858 Compare February 4, 2022 01:57

chappjc added this to the 0.4.1 milestone Feb 7, 2022

This was linked to issues Feb 7, 2022

need for constant node/wallet connectivity checks #1435

Closed

wallet health check on each order placement #1436

Closed

chappjc force-pushed the monitor-peers branch from 07f1858 to 5aefc72 Compare February 7, 2022 16:54

chappjc marked this pull request as ready for review February 7, 2022 17:02

chappjc commented Feb 9, 2022

View reviewed changes

buck54321 requested changes Feb 9, 2022

View reviewed changes

chappjc commented Feb 9, 2022

View reviewed changes

chappjc force-pushed the monitor-peers branch from 5aefc72 to ea72e3d Compare February 10, 2022 16:44

chappjc mentioned this pull request Feb 10, 2022

dex: redesign ConnectionMaster #1474

Merged

client: monitor wallet peers

0ca5b52

chappjc force-pushed the monitor-peers branch from ea72e3d to 0ca5b52 Compare February 11, 2022 18:45

chappjc merged commit 14f1b48 into decred:master Feb 11, 2022

chappjc deleted the monitor-peers branch February 11, 2022 18:52

chappjc mentioned this pull request Jul 15, 2022

monitor getnetworkinfo -> "connections" #808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: monitor wallet peers #1445

client: monitor wallet peers #1445

chappjc commented Jan 24, 2022 •

edited

Loading

chappjc Jan 25, 2022

chappjc Feb 9, 2022 •

edited

Loading

buck54321 left a comment

buck54321 Feb 9, 2022

buck54321 Feb 9, 2022

buck54321 Feb 9, 2022

chappjc Feb 10, 2022

chappjc Feb 9, 2022 •

edited

Loading

buck54321 Feb 9, 2022

chappjc Feb 9, 2022

chappjc Feb 9, 2022 •

edited

Loading

chappjc Feb 9, 2022 •

edited

Loading

		timeDiff := time.Now().Unix() - int64(bh.Time)
		if timeDiff > dexeth.MaxBlockInterval && eth.net != dex.Simnet {

client: monitor wallet peers #1445

client: monitor wallet peers #1445

Conversation

chappjc commented Jan 24, 2022 • edited Loading

chappjc Jan 25, 2022

Choose a reason for hiding this comment

chappjc Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

buck54321 left a comment

Choose a reason for hiding this comment

buck54321 Feb 9, 2022

Choose a reason for hiding this comment

buck54321 Feb 9, 2022

Choose a reason for hiding this comment

buck54321 Feb 9, 2022

Choose a reason for hiding this comment

chappjc Feb 10, 2022

Choose a reason for hiding this comment

chappjc Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

buck54321 Feb 9, 2022

Choose a reason for hiding this comment

chappjc Feb 9, 2022

Choose a reason for hiding this comment

chappjc Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

chappjc commented Jan 24, 2022 •

edited

Loading

chappjc Feb 9, 2022 •

edited

Loading

chappjc Feb 9, 2022 •

edited

Loading

chappjc Feb 9, 2022 •

edited

Loading

chappjc Feb 9, 2022 •

edited

Loading