Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Skip rescanning if wallet is more recent than tip #26679

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Dec 9, 2022

If a wallet has key birthdates that are more recent than the currrent chain tip, or a bestblock height higher than the current tip, we should not attempt to rescan as there is nothing to scan for.

Fixes #26655

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 9, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK ishaanam, w0xlt, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Wallet label Dec 9, 2022
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK

Copy link
Contributor

@ishaanam ishaanam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 57f8f61

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
If a wallet has key birthdates that are more recent than the currrent
chain tip, or a bestblock height higher than the current tip, we should
not attempt to rescan as there is nothing to scan for.
@ishaanam
Copy link
Contributor

re-utACK 3784009

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 3784009

Ci error seems unrelated.

if (!time_first_key || time < *time_first_key) time_first_key = time;
}
if (time_first_key) {
FoundBlock found = FoundBlock().height(rescan_height);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason behind the height set here? It doesn't seems to be used anywhere inside findFirstBlockWithTimeAndHeight nor below.

Copy link
Member

@maflcko maflcko Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in ScanForWalletTransactions, below?

Copy link
Member

@furszy furszy Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah ok, digested it now. thanks. Was expecting FoundBlock to be an inputs-only container when it's actually the outputs container.

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 3784009
A test covering the existent failure would be nice too.

@achow101 achow101 merged commit 4586ae2 into bitcoin:master Jan 11, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 11, 2023
… than tip

3784009 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow)

Pull request description:

  If a wallet has key birthdates that are more recent than the currrent chain tip, or a bestblock height higher than the current tip, we should not attempt to rescan as there is nothing to scan for.

  Fixes bitcoin#26655

ACKs for top commit:
  ishaanam:
    re-utACK 3784009
  w0xlt:
    utACK bitcoin@3784009
  furszy:
    Code review ACK 3784009

Tree-SHA512: f0d90b62940d97d50f21e1e01fa6dcb54409fad819cea4283612825c4d93d733df323cd92787fed43956b0a8e386a5bf88218f1f5749c913398667a5c8f54470
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 12, 2023
If a wallet has key birthdates that are more recent than the currrent
chain tip, or a bestblock height higher than the current tip, we should
not attempt to rescan as there is nothing to scan for.

Github-Pull: bitcoin#26679
Rebased-From: 3784009
@fanquake fanquake mentioned this pull request Jan 12, 2023
@fanquake
Copy link
Member

Backported in #26878.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 12, 2023
If a wallet has key birthdates that are more recent than the currrent
chain tip, or a bestblock height higher than the current tip, we should
not attempt to rescan as there is nothing to scan for.

Github-Pull: bitcoin#26679
Rebased-From: 3784009
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Feb 22, 2023
If a wallet has key birthdates that are more recent than the currrent
chain tip, or a bestblock height higher than the current tip, we should
not attempt to rescan as there is nothing to scan for.

Github-Pull: bitcoin#26679
Rebased-From: 3784009
glozow added a commit that referenced this pull request Feb 27, 2023
784a754 wallet, rpc: Update migratewallet help text for encrypted wallets (Andrew Chow)
debcfe3 tests: Tests for migrating wallets by name, and providing passphrase (Andrew Chow)
ccc72fe wallet: Be able to unlock the wallet for migration (Andrew Chow)
50dd8b1 rpc: Allow users to specify wallet name for migratewallet (Andrew Chow)
648b062 wallet: Allow MigrateLegacyToDescriptor to take a wallet name (Andrew Chow)
ab3bd45 i2p: use consistent number of tunnels with i2pd and Java I2P (Vasil Dimov)
29cdf42 i2p: lower the number of tunnels for transient sessions (Vasil Dimov)
5027e93 i2p: reuse created I2P sessions if not used (Vasil Dimov)
a62c541 wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
64e7db6 Zero out wallet master key upon lock (John Moffett)
b7e242e Correctly limit overview transaction list (John Moffett)
cff6718 depends: fix systemtap download URL (fanquake)
7cf73df Add missing includes to fix gcc-13 compile error (MarcoFalke)
07397cd addrdb: Only call Serialize() once (Martin Zumsande)
91f83db hash: add HashedSourceWriter (Martin Zumsande)
5c824ac For feebump, ignore abandoned descendant spends (John Moffett)
428dcd5 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow)
cbcdafa test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner)
342abfb wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner)

Pull request description:

  Backports:
  * #26595
  * #26675
  * #26679
  * #26761
  * #26837
  * #26909
  * #26924
  * #26944
  * bitcoin-core/gui#704
  * #27053
  * #27080

ACKs for top commit:
  instagibbs:
    ACK 784a754
  achow101:
    ACK 784a754
  hebasto:
    ACK 784a754, I've made backporting locally and got a diff between my branch and this PR as follows:

Tree-SHA512: 8ea84aa02d7907ff1e202e1302b441ce9ed2198bf383620ad40056a5d7e8ea88e1047abef0b92d85648016bf9b3195c974be3806ccebd85bef4f85c326869e43
@bitcoin bitcoin locked and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Pruned node unable to start when a wallet is present
7 participants