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

Remove -rescan startup parameter #23123

Closed
wants to merge 4 commits into from
Closed

Remove -rescan startup parameter #23123

wants to merge 4 commits into from

Conversation

meshcollider
Copy link
Contributor

Remove the -rescan startup parameter.

Rescans can be run with the rescanblockchain RPC.

Rescans are still done on wallet-load if needed due to corruption, for example.

src/wallet/wallettool.cpp Outdated Show resolved Hide resolved
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Concept ACK. See also #13044

Copy link
Member

@maflcko maflcko 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 256314b

Didn't review closely

@fanquake
Copy link
Member

Concept ACK

@Sjors
Copy link
Member

Sjors commented Sep 29, 2021

utACK 256314b

@laanwj
Copy link
Member

laanwj commented Sep 29, 2021

Concept and code review ACK 256314b

src/wallet/wallet.h Outdated Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 29, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22928 (refactor: Remove gArgs from wallet.h and wallet.cpp (2) by kiminuo)
  • #22868 (wallet: Call load handlers without cs_wallet locked by promag)
  • #22805 (refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack)
  • #22787 (refactor: actual immutable pointing by kallewoof)
  • #22235 (script: add script to generate example bitcoin.conf by josibake)
  • #21726 (Improve Indices on pruned nodes via prune blockers by fjahr)
  • #15719 (Wallet passive startup by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@meshcollider
Copy link
Contributor Author

meshcollider commented Sep 29, 2021

Modified second commit to use DBErrors::RESCAN_REQUIRED instead of an out bool, as suggested by @achow101

@achow101
Copy link
Member

ACK dc3ec74

@laanwj
Copy link
Member

laanwj commented Sep 30, 2021

re-ACK dc3ec74

laanwj added a commit to bitcoin-core/gui that referenced this pull request Sep 30, 2021
dc3ec74 Add rescan removal release note (Samuel Dobson)
bccd1d9 Remove -rescan startup parameter (Samuel Dobson)
f963b0f Corrupt wallet tx shouldn't trigger rescan of all wallets (Samuel Dobson)
6c00649 Remove outdated dummy wallet -salvagewallet arg (Samuel Dobson)

Pull request description:

  Remove the `-rescan` startup parameter.

  Rescans can be run with the `rescanblockchain` RPC.

  Rescans are still done on wallet-load if needed due to corruption, for example.

ACKs for top commit:
  achow101:
    ACK dc3ec74
  laanwj:
    re-ACK dc3ec74

Tree-SHA512: 608360d0e7d73737fd3ef408b01b33d97a75eebccd70c6d1b47a32fecb99b9105b520b111b225beb10611c09aa840a2b6d2b6e6e54be5d0362829e757289de5c
Copy link
Contributor

@ryanofsky ryanofsky 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 dc3ec74. Looks great!

@@ -48,7 +48,8 @@ enum class DBErrors
NONCRITICAL_ERROR,
TOO_NEW,
LOAD_FAIL,
NEED_REWRITE
NEED_REWRITE,
RESCAN_REQUIRED
Copy link
Contributor

Choose a reason for hiding this comment

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

In commit "Corrupt wallet tx shouldn't trigger rescan of all wallets" (f963b0f)

Minor: Might call this NEED_RESCAN to be consistent with NEED_REWRITE

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good suggestion to be more consistent. Maybe in a follow-up PR.

@meshcollider
Copy link
Contributor Author

Merged here: 571bb94

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 1, 2021
fanquake added a commit that referenced this pull request Oct 1, 2021
8615507 scripted-diff: rename DBErrors::RESCAN_REQUIRED to NEED_RESCAN (Samuel Dobson)

Pull request description:

  Suggested here as a trivial follow-up: #23123 (comment)

  Makes RESCAN_REQUIRED consistent with NEED_REWRITE

ACKs for top commit:
  achow101:
    ACK 8615507
  jonatack:
    ACK 8615507

Tree-SHA512: 82d057c45e192cd6dd8a47675b52699e6cbc82272609a971e6e5d6796aad14a941a70e40d3913dbb611f79c8eadff8030c60ea6f203f2edc3720c0e78c166b97
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 1, 2021
…ED_RESCAN

8615507 scripted-diff: rename DBErrors::RESCAN_REQUIRED to NEED_RESCAN (Samuel Dobson)

Pull request description:

  Suggested here as a trivial follow-up: bitcoin#23123 (comment)

  Makes RESCAN_REQUIRED consistent with NEED_REWRITE

ACKs for top commit:
  achow101:
    ACK 8615507
  jonatack:
    ACK 8615507

Tree-SHA512: 82d057c45e192cd6dd8a47675b52699e6cbc82272609a971e6e5d6796aad14a941a70e40d3913dbb611f79c8eadff8030c60ea6f203f2edc3720c0e78c166b97
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants