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: Replace -zapwallettxes with wallet tool command #19700

Closed
wants to merge 7 commits into from

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Aug 12, 2020

Replaces the -zapwallettxes startup option with a zapwallettxes command in the wallet tool.

To preserve the transaction metadata, using zapwallettxes rewrites the transactions as a zaptx record and removes the original tx record. bestblock is also reset to null to trigger a rescan. On loading the wallet, a rescan will be triggered. If any zaptx records were found, then the metadata from those records is copied to the new tx. All zaptx records are removed after loading. If metadata is not kept (by using -keepmeta=0 in the wallet tool), then no zaptx records will be created.

Lastly -zapwallettxes is replaced with an error telling users to use the wallet tool command instead.

Alternative to #19653 and #19671

@laanwj
Copy link
Member

laanwj commented Aug 12, 2020

Concept ACK!

I think this is the best solution if we decide to not remove the functionality wholesale.

@meshcollider
Copy link
Contributor

As I just commented on #19671, I think it is better to just remove the option, so I prefer #19671

@jonasschnelli
Copy link
Contributor

Concept ACK and light core review ACK 5739465.

The questions in the past popped up if we have to bump the wallet version (not to mix with the minwalletversion) if we add a key/value type. I don't think so but can remember @luke-jr had some arguments for bumping it.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 20, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@fanquake
Copy link
Member

Seems the we have consensus to move forward in #19671, so I'm going to close this PR.

@fanquake fanquake closed this Aug 27, 2020
fanquake added a commit that referenced this pull request Sep 1, 2020
3340dba Remove -zapwallettxes (Andrew Chow)

Pull request description:

  It's not clear what use there is to keeping `-zapwallettxes` given that it's intended usage has been superseded by `abandontransaction`. So this removes it outright.

  Alternative to #19700

ACKs for top commit:
  meshcollider:
    utACK 3340dba
  fanquake:
    ACK 3340dba - remaining manpage references will get cleaned up pre-release.

Tree-SHA512: 3e58e1ef6f4f94894d012b93e88baba3fb9c2ad75b8349403f9ce95b80b50b0b4f443cb623cf76c355930db109f491b3442be3aa02972e841450ce52cf545fc8
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 3, 2020
3340dba Remove -zapwallettxes (Andrew Chow)

Pull request description:

  It's not clear what use there is to keeping `-zapwallettxes` given that it's intended usage has been superseded by `abandontransaction`. So this removes it outright.

  Alternative to bitcoin#19700

ACKs for top commit:
  meshcollider:
    utACK 3340dba
  fanquake:
    ACK 3340dba - remaining manpage references will get cleaned up pre-release.

Tree-SHA512: 3e58e1ef6f4f94894d012b93e88baba3fb9c2ad75b8349403f9ce95b80b50b0b4f443cb623cf76c355930db109f491b3442be3aa02972e841450ce52cf545fc8
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants