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.

Sign up for GitHub

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

Jump to bottom

wallet: No BDB creation, unless -deprecatedrpc=create_bdb #28597

Merged
merged 1 commit into from
Oct 5, 2023
Merged

wallet: No BDB creation, unless -deprecatedrpc=create_bdb #28597

merged 1 commit into from
Oct 5, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Oct 5, 2023

With BDB being removed soon, it seems confusing and harmful to allow users to create fresh BDB wallets going forward, as it would load them with an additional burden of having to migrate them soon after.

Also, it would be good to allow for one release for test (and external) scripts to adapt.

Fix all issues by introducing the -deprecatedrpc=create_bdb setting.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 5, 2023 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK Sjors, furszy, achow101
Concept ACK hebasto

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27788 (rpc: Be able to access RPC parameters by name by achow101)

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 DrahtBot added the Wallet label Oct 5, 2023
@maflcko maflcko added this to the 26.0 milestone Oct 5, 2023
@furszy furszy mentioned this pull request Oct 5, 2023
Closed
furszy
furszy reviewed Oct 5, 2023
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.

Concept ACK.

Maybe add a small test creating a legacy wallet without the "create_bdb" flag? asserting the error is being properly thrown.

@maflcko
Copy link
Member Author

maflcko commented Oct 5, 2023

Maybe add a small test creating a legacy wallet without the "create_bdb" flag? asserting the error is being properly thrown.

Yes, happy to push a commit or patch, if someone writes one. Otherwise, it should be possible to test locally:

$ ./src/bitcoin-cli -datadir=/tmp -regtest createwallet name false false "" false false
error code: -4
error message:
BDB wallet creation is deprecated and will be removed in a future release. In this release it can be re-enabled temporarily with the -deprecatedrpc=create_bdb setting.

@Sjors
Copy link
Member

Sjors commented Oct 5, 2023

Concept ACK

@hebasto
Copy link
Member

hebasto commented Oct 5, 2023

Concept ACK.

@Sjors
Copy link
Member

Sjors commented Oct 5, 2023

tACK fa071ae

@fanquake fanquake requested a review from achow101 October 5, 2023 16:16
furszy
furszy reviewed Oct 5, 2023
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.

utACK fa071ae

@furszy
Copy link
Member

furszy commented Oct 5, 2023 •
edited
Loading

On a second thought, probably better to call it "create_legacy_wallet" instead of "create_bdb". Users might not be aware of the legacy wallet db engine name.

@maflcko
Copy link
Member Author

maflcko commented Oct 5, 2023

Users might not be aware of the legacy wallet db engine name.

I think BDB is mentioned in all compile docs, as well as the GUI. Maybe not in the RPC interface, but it is explained in this pull.

Though, happy to change.

@maflcko maflcko mentioned this pull request Oct 5, 2023
Merged
@achow101
Copy link
Member

achow101 commented Oct 5, 2023

ACK fa071ae

@DrahtBot DrahtBot removed the request for review from achow101 October 5, 2023 19:29
@achow101 achow101 merged commit cf553e3 into bitcoin:master Oct 5, 2023
@DrahtBot DrahtBot mentioned this pull request Oct 5, 2023
Closed
@maflcko maflcko deleted the 2310-less-bdb- branch October 6, 2023 13:19
pablomartin4btc
pablomartin4btc reviewed Oct 6, 2023
View reviewed changes
Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

post-merge ACK fa071ae

@kristapsk kristapsk mentioned this pull request Oct 7, 2023
Open
@achow101 achow101 mentioned this pull request Oct 20, 2023
Open
46 tasks
@kristapsk
Copy link
Contributor

This should have had release notes.

@willcl-ark willcl-ark mentioned this pull request Dec 7, 2023
Merged
@maflcko
Copy link
Member Author

maflcko commented Dec 7, 2023

This should have had release notes.

A label was added after merge, but it looks like no one went through those. Currently there are 4 tagged: https://github.com/bitcoin/bitcoin/issues?q=label%3A%22Needs+release+note%22+is%3Aclosed

@maflcko maflcko mentioned this pull request Dec 7, 2023
Merged
@fanquake
Copy link
Member

fanquake commented Dec 7, 2023

Release note being added in #29023.

fanquake added a commit that referenced this pull request Dec 8, 2023
@fanquake
Merge #29025: doc: Add link to needs-release-notes label
14d1732 
fa88953 doc: Add link to needs-release-notes label (MarcoFalke)

Pull request description:

  This makes it easier to spot and not forget. C.f. #28597 (comment)

ACKs for top commit:
  kristapsk:
    ACK fa88953
  TheCharlatan:
    ACK fa88953

Tree-SHA512: 28336cde36d62622d1b6627497291cbd4644bf1e4e6f17dc9cde39f254e7094dd02484da754e45558e59facb20941dd0c049ce7b33dcc62bfec6c26c16516cdf
gentoo-repo-qa-bot pushed a commit to gentoo-mirror/bitcoin that referenced this pull request Dec 10, 2023
@whitslack
net-p2p/joinmarket: fix tests with >=net-p2p/bitcoin-core-26.0
e0fe0b7 
See: bitcoin/bitcoin#28597
@bitcoin bitcoin locked and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pablomartin4btc pablomartin4btc pablomartin4btc left review comments

@furszy furszy furszy left review comments

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
26.0
Development

Successfully merging this pull request may close these issues.
9 participants
@maflcko @DrahtBot @Sjors @hebasto @furszy @achow101 @kristapsk @fanquake @pablomartin4btc