btc spv: give wallet precedence for block notifications #1250
Conversation
|
The btcd fix for the |
| type tipNotifier interface { | ||
| tipFeed() <-chan *block |
There was a problem hiding this comment.
If we wanted to do totally idiomatic Go, we'd make this tipFeeder I suppose, but meh.
| prevTip := btc.currentTip | ||
| btc.currentTip = newTip |
There was a problem hiding this comment.
The sameTip check in run/watchBlocks previously was good enough, but given the logic race with Stopping the queuedBlock's timer, it's possible although unlikely that the same tip could be reported twice. So maybe we either check here like:
prevTip := btc.currentTip
if prevTip.hash == newTip.hash {
return // already reported
}
btc.currentTip = newTipor in watchBlocks like
case walletTip := <-walletBlock:
if queuedBlock != nil && walletTip.height >= queuedBlock.height {
if !queuedBlock.queue.Stop() && walletTip.hash == queuedBlock.hash {
continue
}
queuedBlock = nil
}I think it's clearer and just as good here in reportTip.
There was a problem hiding this comment.
I added the reportTip solution in the latest commit, but I think I'm going to go with your second one instead. If a wallet tip comes through after the queued block is sent, we'd still want to notify the user to trigger a balance check.
buck54321
force-pushed
the
smarttip
branch
2 times, most recently
from
e7b10d3
to
6aa954c
Compare
| if err != nil { | ||
| btc.log.Errorf("Error retreiving sync status before queuing polled block: %v", err) | ||
| } else if syncStatus.Syncing { | ||
| blockAllowance *= 10 |
There was a problem hiding this comment.
Even with the a 10 second allowance, I was seeing warnings during testnet syncing.
When a wallet is able to provide block notifications, allow wallet notifications to take precedence. Queue polled new tips for 10 seconds, canceling their broadcast and sending the notification earlier if the wallet notification comes during this window. If the wallet note doesn't come by 10 seconds, send the queued notification and log a warning. SPV tip updates are provided by the FilteredBlockConnected notification, which can come after neutrino stores the block header(s). Care is taken not to spam the caller with notifications, since they are not limited by the polling frequency. The same mechanism for limiting spam is used to solve the non-existent wallet error messages for *Core though, so you get two for the price of one with that solution.
| if walletBlock == nil { | ||
| btc.reportNewTip(ctx, newTip) | ||
| } else { |
There was a problem hiding this comment.
Looks like you could continue rather than using the else indention. No difference really though.
| @@ -284,9 +285,13 @@ type spvWallet struct { | |||
|
|
|||
| log dex.Logger | |||
| loader *wallet.Loader | |||
|
|
|||
| tipChan chan *block | |||
| syncTarget int32 | |||
There was a problem hiding this comment.
Could you leave a comment about what syncTarget is? I guess it is the current highest known block number?
| if atomic.SwapInt32(&w.syncTarget, target) == 0 && target > 0 { | ||
| w.tipChan <- &block{ | ||
| hash: blk.Hash, | ||
| height: int64(blk.Height), | ||
| } | ||
| } |
There was a problem hiding this comment.
// Setting highest known block number if not initiated?
| if syncTarget == 0 || (lastBlock.Height < syncTarget && lastBlock.Height%10_000 != 0) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
what does lastBlock.Height%10_000 != 0 do? this will be true at at 5001 - 9999 and then from 10001?
| if syncTarget == 0 || (lastBlock.Height < syncTarget && lastBlock.Height%10_000 != 0) { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
Could I please get the following here?
for ib := range note.AttachedBlocks {
for _, nt := range note.AttachedBlocks[ib].Transactions {
w.log.Debugf("Block %d contains wallet transaction %v", note.AttachedBlocks[ib].Height, nt.Hash)
}
}
Which change in this PR was to address this? I don't think that's resolved. I'm working on trade_simnet_test.go, and I get that error message, but I don't see a change to how |
|
This solves a couple of issues with bitcoin SPV syncing.
balanceUpdaterequested byCorecan come before the wallet has seen new funds. This was obvious during testnet syncing when outputs from even 1000s of blocks ago were not seen (block headers are stored in batches during initial sync) until an additional block was mined after sync.ExchangeWalletbefore thexcWalletwas added to thewalletsmap in*Core, resulting in error messages sayingnon-existent 0 wallet should exist.This solution gives wallet notifications precedence by queuing polled new tips temporarily to allow the wallet notifications to come through. This is important, because neutrino stores the block headers before requesting filters, and doesn't send the
FilteredBlockConnectednotification until after the filters are received and processed, allowing ample time for a poll to sneak in and grab a header whose corresponding filter hasn't been checked yet.