client/asset/btc: syncStatus considers wallet/addrmgr sync #1271
Conversation
chappjc
force-pushed
the
two-stage-btc-spv-sync
branch
from
9f3e6cb
to
3827b15
Compare
client/asset/btc/spv.go
Outdated
| DataDir: w.netDir, | ||
| Database: w.neutrinoDB, | ||
| ChainParams: *w.chainParams, | ||
| // AddPeers: []string{"localhost"}, // worth a shot |
There was a problem hiding this comment.
Almost 4 minutes exactly with a hand-picked remote mainnet peer. This can be painful if you get a poor peer, and neutrino doesn't do multipeer, at least not for getcfilters, so I put up a node at 45.76.69.19 and got the 4 minute sync result. I think we can hard code this (or maybe a host name) as an addpeer to make it less likely a user will get a terrible sync node for their compact filters.
|
The sync progress indicator now reflects wallet sync instead of just the chain service, and livetest/regnet tests passing along with the other unit tests, so making r4r. |
client/asset/btc/spv.go
Outdated
| var synced bool | ||
| var blk *block | ||
| // Wallet address manager sync height. | ||
| if currentHeight >= target { |
There was a problem hiding this comment.
Note that this condition intentionally prevents SyncStatus from reporting synced = true before the wallet address manager says so. So even if progress may gets close to 1 in the few seconds while the chain service syncs before consulting the address manager, it will never tell the consumer that it's done.
The ultimate solution might be to get the wallet birthday block from the Wallet instance somehow, and when w.cl.BestBlock() is at the birthday block, switch to the address manager's status at that point. But all I want from this fix is to only report 1.0 and synced=true when the address manager says it has reached the target. There is easy access to the birthday time from Wallet. We should look into getting the block however.
There was a problem hiding this comment.
The db approach to pulling the birthday block failed because it's not set until the wallet address manager hits the birthday, which makes perfect sense.
All we have in terms of birthday is a time.Time. We should be able to compare the BlockStamp.Timestamp with the birthday time to know when to switch over, but the chain service's BestBlock does not set Timestamp. This may be resolved in lightninglabs/neutrino#235. We should circle back on SyncStatus when/if that happens.
chappjc
force-pushed
the
two-stage-btc-spv-sync
branch
3 times, most recently
from
c6e5dfc
to
30bdc3b
Compare
| @@ -1769,7 +1770,7 @@ func (c *Core) CreateWallet(appPW, walletPW []byte, form *WalletForm) error { | |||
| if err != nil { | |||
| return initErr("error getting wallet balance for %s: %w", symbol, err) | |||
| } | |||
| wallet.balance = balances // update xcWallet's WalletBalance | |||
| wallet.setBalance(balances) // update xcWallet's WalletBalance | |||
There was a problem hiding this comment.
Unrelated change here. The syncstatus goroutine started by connect above actually accesses wallet.balance (under lock). It's pretty much never gonna happen with a long ticker interval in that goroutine, but I dropped it to 0.5 seconds while testing and some stuff popped out.
chappjc
force-pushed
the
two-stage-btc-spv-sync
branch
2 times, most recently
from
98da5e9
to
33e6afd
Compare
| var addPeers []string | ||
| switch w.chainParams.Net { | ||
| case wire.MainNet: | ||
| addPeers = []string{"cfilters.ssgen.io"} |
There was a problem hiding this comment.
I wonder whether this could result in bottlenecks. What are the chances that this node is picked for the filter source? Is it random, or since this is in addpeer, does it result in this node being chosen more often or even always? Hard-coding a mainnet node also feels pretty icky, though I admit I've considered it as a solution for various problems. I would hope that if we do have "sanctioned" nodes, we would make them opt-out at the very least.
There was a problem hiding this comment.
Good questions. The "addpeer" nodes are only suggestions, so they get mixed into the other nodes from DNS seeders, so neutrino ends up connecting to a bunch of other nodes too. I had the same icky feeling at first, but when you consider we are told what nodes to use in part from the Bitcoin Core devs, some of whom were opposed to compact filters, it's quite fair that we at least throw one in the mix.
I'm not sure opting out provides much benefit considering that we cannot opt out of the DNS seed nodes either. This could be prevented however if we expose the connect peers, which are exclusive and prevent discovery. I'm extremely concerned about preventing discovery though because your chain service can quite quickly become without a sync peer.
Regarding if the addpeer takes some priority or gets chosen more readily, I cannot tell for sure, but in my observations it does seem to take some precedence. Probably just because DNS seed discovery is a slower than what's already in memory via this setting.
FWIW, my SOP with dcrd deployments is to supply a couple addpeers, and dcrd always initiates more outbound connections, but the addpeer nodes do seem to happen more quickly.
There is actually a two-stage sync with btcwallet-neutrino sync, When the chain service reaches its target height, we have to start watching the address manager's sync height. We could just track the address manager's sync height and ignore the chain service height, but then it could appear to be stuck at zero percent for a while.
chappjc
force-pushed
the
two-stage-btc-spv-sync
branch
from
7bdf221
to
63cd6c2
Compare
|
For posterity, here's a command I use to tail the neutrino+wallet log for the most informative sync info:
|
There is actually a two-stage sync with btcwallet-neutrino sync. When the chain service reaches its target height, we have to start watching the address manager's sync height.
We could just track the address manager's sync height and ignore the chain service height, but then it could appear to be stuck at zero percent for a while. That actually might be fine as the chain service seems to sync headers consistently quickly while the address manager with cfilters fetching can do it quite slowly depending on the peer.