blockmanager: update in-memory chain tip before notification dispatch

lightninglabs / neutrino

Privacy-Preserving Bitcoin Light Client

MIT License

900 stars 183 forks source link

blockmanager: update in-memory chain tip before notification dispatch #200

Closed wpaulino closed 4 years ago

coveralls commented 4 years ago

Coverage decreased (-0.09%) to 72.065% when pulling 8da748b03c944b2e9cfe89ab33db1968b338c3a8 on wpaulino:best-height-historical-notifications into d84efcc5b276e6d55636708ad7846c7fd86c3723 on lightninglabs:master.

roeierez commented 4 years ago

Instead, we would inspect the filter header tip, which may not have been synced yet, resulting in a failure delivering backlogs of block header notifications.

@wpaulino I think I am struggling with an issue that this PR might solve. These are my logs:

unable to register block subscription: unable to retrieve blocks since height=601000: request with height 601000 is greater than best height known 599000

Is that what you saw that triggered this change? Or something else?

guggero commented 4 years ago

@roeierez yes, this is exactly the error this fixes. Or at least we got the same error message in our itests and they disappeared now with the fix.

roeierez commented 4 years ago

@guggero Thanks a lot!

halseth commented 4 years ago

I'm not sure this is correct. We do notify about blocks connected only when we have the filter, as only then it is safe for the caller to assume Neutrino "knows about" the block. Maybe there are wrong assumptions on the callsite instead?

halseth commented 4 years ago

Or maybe we are notifying about blocks before we have the filter

guggero commented 4 years ago

There is at least one more timing issue happening with Neutrino when the wallet recovery mode is activated. I've attached two logs from our itests, one where everything works as expected and one where the wallet starts to rescan too early.

good.log bad.log

In bad.log, the following line is missing completely: Seed birthday surpassed, starting recovery of wallet from height=1 hash=xxx with recovery-window=1000

I'm not sure if this is directly related to this issue. @halseth's mentioned offline that it could be what he mentioned in the previous comment above that we calculate the height from block headers but really should be using filter headers instead.

Roasbeef commented 4 years ago

Current state of this?

wpaulino commented 4 years ago

@roeierez could you provide more details for the issue you're running into?

roeierez commented 4 years ago

@roeierez could you provide more details for the issue you're running into?

@wpaulino not much as it was a while ago. I do remember it was after attempting to recover a wallet using the dropwtxmgr.