wallet: Do not download cfilters before birthday. #2506
Conversation
|
Needs some tests. I did not add any checks to chain/sync but I suppose it makes sense there too. Is looking for the cfilters when rescanning good enough for imported keys? More info about this should be added to docs as well. |
JoeGruffins
force-pushed
the
birthdaycfilters
branch
6 times, most recently
from
092584c to
abbe34a
Compare
wallet/rescan.go
Outdated
| // not have the cfilters needed. Set birthday to the rescan | ||
| // height and download the filters. This may take some time | ||
| // depending on network conditions and amount of filters missing. | ||
| if int32(bs.Height) > startHeight { |
There was a problem hiding this comment.
you could drop an indent by using if bs != nil && int32(bs.Height) > startHeight { in the outer if block, but I wonder if you are missing a check here for date-based birthdays.
There was a problem hiding this comment.
I was working off the assumption that we wouldn't request a rescan before the birthday is found. The birthday will be found if we have completed initial sync. Is it safe to assume we have synced to the best chain before trying to rescan?
There was a problem hiding this comment.
Dropping the extra indent anyway.
wallet/rescan.go
Outdated
| return errors.E(op, err) | ||
| } | ||
| if fetchMissing { | ||
| if err := w.FetchMissingCFilters(ctx, n); err != nil { |
There was a problem hiding this comment.
This will fetch all cfilters from the beginning of the chain, not from the point being rescanned. (which could still be fine i guess, but it's weird to throw out this optimization).
There was a problem hiding this comment.
I meant to change FetchMissingCFilters to only download from the birthday, which is being set above. Will double check to make sure that is the case, but I don't think it should be rescanning from zero unless that is the birthday.
| // SetMissingMainChainCFilters sets whether we have all of the main chain | ||
| // cfilters. Should be used to set missing to false if the wallet birthday is | ||
| // moved back in time. | ||
| func (s *Store) SetMissingMainChainCFilters(dbtx walletdb.ReadWriteTx, have bool) error { |
There was a problem hiding this comment.
instead of a boolean value, we could just record the earliest block height that we have saved cfilters from. If that's 0, it's the same as this recording true. If nonzero, it's the same as false but gives us additional information to work with (and return in the function just below this).
There was a problem hiding this comment.
Ok, all current wallets are 0 so this will not need a db upgrade. Working on it.
There was a problem hiding this comment.
I think maybe we still want to set whether we have all filters. If we do a rescan and the wallet is shut off before all of the filters are downloaded, we need to know that when starting up.
There was a problem hiding this comment.
Sorry, do you mean replace the boolean with a block number? I guess it would need an upgrade in the case of false.
There was a problem hiding this comment.
I looked into doing this but unsure if it simplifies the process. If we only save a number, and zero has the meaning that we have them all, then we loose the value of having the number except while downloading. I think the current code handles that well enough though, and it is only when changing the birthday. It may also require a new db version to replace the current boolean if I understand correctly.
There was a problem hiding this comment.
The birthday height would have the same meaning as a new db value, I believe.
There was a problem hiding this comment.
true, i think two fields should be used, the height/block of the first cfilter, and the last cfilter
There was a problem hiding this comment.
Am I understanding correctly in that we should replace the bool or add more. I'll tinker some more.
There was a problem hiding this comment.
The birthday would be where we want to save from, but it could be earlier. So another db value for where the actual first cfilter should be, and where the last one is. The last one will be the head block filter once we are synced, will we update that every block?
There was a problem hiding this comment.
It still looks like more code for no gain to me unless I am mistaking something. The problem is that if we set the birthday back a second time there's a new gap, so we either need to save more variables or look through everything similarly to what we are doing now.
JoeGruffins
force-pushed
the
birthdaycfilters
branch
3 times, most recently
from
e80e4d6 to
b589183
Compare
Especially for spv wallets with a birthday, cfilters for blocks before the birthday are not used for anything as there can be no transactions before the wallet existed. Do not download them but check if we need them whenever doing a rescan from height.
JoeGruffins
force-pushed
the
birthdaycfilters
branch
from
b589183 to
d4f50d7
Compare
2 participants
Especially for spv wallets with a birthday, cfilters for blocks before the birthday are not used for anything as there can be no transactions before the wallet existed. Do not download them but check if we need them whenever doing a rescan from height.
closes #2498