Skip to content

wallet: remove outdated RewriteDB calls from SPKM & DBErrors::NEED_REWRITE enum value#34301

Merged
sedited merged 2 commits intobitcoin:masterfrom
rkrux:rewritedb
Mar 11, 2026
Merged

wallet: remove outdated RewriteDB calls from SPKM & DBErrors::NEED_REWRITE enum value#34301
sedited merged 2 commits intobitcoin:masterfrom
rkrux:rewritedb

Conversation

@rkrux
Copy link
Contributor

@rkrux rkrux commented Jan 15, 2026 •
edited
Loading

ISTM that there is no implementation left of the RewriteDB method in any of the SPKMs, and thus, its call sites can be removed safely.

Also remove DBErrors::NEED_REWRITE enum value as its usage is outdated now.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 15, 2026 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34301.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK achow101, furszy, sedited

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

@achow101
Copy link
Member

achow101 commented Jan 15, 2026 •
edited
Loading

Concept ACK

I think this can actually extend a bit futher into getting rid of DBErrors::NEED_REWRITE entirely. This only exists for wallets created in 0.4.x which had the bug that the database was not rewritten. However, given that BDB is no longer supported at all and migration is explicitly a rewrite anyways, there's actually no situation in which NEED_REWRITE being returned is ever doing anything useful.

The only places we should ever need WalletDatabase::Rewrite are in wallet encryption.

@rkrux rkrux changed the title wallet: remove unimplemented RewriteDB calls from SPKM wallet: remove outdated RewriteDB calls from SPKM & DBErrors::NEED_REWRITE enum value Jan 16, 2026
@rkrux
Copy link
Contributor Author

rkrux commented Jan 16, 2026

I think this can actually extend a bit futher into getting rid of DBErrors::NEED_REWRITE entirely.

Thanks, I have added a commit addressing this point.

This was referenced Jan 18, 2026
Merged
Draft
@achow101
Copy link
Member

achow101 commented Jan 19, 2026

ACK 5c802c7

@DrahtBot DrahtBot mentioned this pull request Feb 4, 2026
Open
rkrux added 2 commits February 5, 2026 18:01
@rkrux
wallet: remove unimplemented RewriteDB calls from SPKM
61039d7 
ISTM that there is no implementation left of the `RewriteDB` method
in any of the SPKMs, and thus, its call sites can be removed safely.
@rkrux
wallet: remove DBErrors::NEED_REWRITE enum value
c6a6435 
As highlighted in the PR review comment, this enum seems no longer
required as the specific issue it solves for involves BDB based wallets
that can't be loaded anymore outside the context of wallet migration,
which rewrites the database anyway.
@achow101
Copy link
Member

achow101 commented Feb 5, 2026

ACK c6a6435

furszy
furszy reviewed Mar 2, 2026
View reviewed changes
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.

ACK c6a6435

sedited
sedited approved these changes Mar 11, 2026
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK c6a6435

@sedited sedited merged commit 281c0cc into bitcoin:master Mar 11, 2026
27 checks passed
@hebasto hebasto mentioned this pull request Mar 11, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sedited sedited sedited approved these changes

+1 more reviewer

@furszy furszy furszy left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rkrux @DrahtBot @achow101 @furszy @sedited