doc: consolidate legacy wallet documentation #23470
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
maflcko
left a comment
There was a problem hiding this comment.
No opinion on the changes. Won't this go soon anyway?
223a9c2 to
012acdd
Compare
|
Concept ACK. |
I don't know. According to #20160 we'll be supporting BDB wallets in some capacity for a few more years at least, I assume that means having some BDB documentation available? If we can delete it now I'm happy to do that, otherwise, I think consolidating it into a single, not front-and-center place is much better than having it spread out over the build docs, being misleading etc. |
012acdd to
d526273
Compare
|
I don't think we should be in any rush to removing the BDB wallet. Definitely not before there is a feasible, well tested upgrade path for old wallets. I think it's scaring some people. |
Sure. However I think consolidating the legacy documentation is a worthwhile interim step, and will likely help with the migration towards sql based wallets, by moving the BDB documentation out of front and center. As is, the build docs for master don't do the best explanations for a first time builder / user. |
|
Concept ACK. At this point, building with the legacy bdb wallet is a specialized option that most users won't need. Consolidating all of the documentation for that option into one place rather than having it spread across multiple documents makes sense to me. |
I'm not convinced about this. Descriptor wallets were only introduced (as experimental) in 0.21.0, released at the start of this year, right? There is no released version that creates them by default (that will be 23.0). There will still be many BerkeleyDB wallets in use. Also some software using bitcoin core cannot cope with descriptor wallets yet (such as joinmarket, but there is a PR for this). |
At this point, building with the legacy bdb wallet is a specialized option that most new users won't need (not even all new users), and, soon, most users won't need. People are just afraid that soon is a few years. Sadly, they are right. |
rajarshimaitra
left a comment
There was a problem hiding this comment.
Concept ACK. It is indeed confusing. And I actually didn't know we don't need BDB for wallet support anymore.
Below are few nits and comments.
doc/build-unix.md
Outdated
There was a problem hiding this comment.
| be compiled without one using: | |
| be compiled without one, using: |
doc/build-unix.md
Outdated
There was a problem hiding this comment.
I am wondering if a version of this note can be kept (without the wallet refs). Its useful information that all paths needs to be absolute in configure.
doc/build-osx.md
Outdated
There was a problem hiding this comment.
if qt is installed, the gui would be built by default, right? So instead of specifying how to explictly enable gui maybe its better to specify ho to explicitly disable gui? And then add note similar to sqlite lines above, saying when gui will be enabled by default?
My only point is the explicit gui disable step was there before few lines ago, and now its removed.
Agree with this. Descriptors are still new and wallets as well. It is interesting and will be used more in future however I don't think legacy wallets should/will be removed soon. I will quote something from a discussion during our internship in which one project used descriptors: yeah it is confusing at first, it's funny that Andrew Chow say descriptors are not humans readable but engineers readable Few differences and trade-offs including possibility of bugs: https://bitcoin.stackexchange.com/questions/107926/fuzzing-descriptor-wallet-rpcs |
d526273 to
c5650a5
Compare
|
I've made the changes here less aggressive, but still tried to cleanup the build / install instructions. Updated the PR description accordingly. |
rajarshimaitra
left a comment
There was a problem hiding this comment.
reACK c5650a5
I am still unsure on the use of wallet and legacy wallet, and thinking having descriptor wallet and legacy wallet terms is more clearer. It kinda implicitly indicates the difference between the two.
doc/build-freebsd.md
Outdated
There was a problem hiding this comment.
I am going a little back and forth on the terms legacy wallet and wallet. It indicates that there are two different kind of wallets in the context, but doesn't explain what's the difference. So may be instead we could use legacy wallet and descriptor wallet in the doc everywhere? In that way I feel it would be more clear to new readers. Because it is not a well known fact outside of the dev community that a sqlite based default wallet is descriptor based and its not same as regular BDB wallets.
doc/build-openbsd.md
Outdated
There was a problem hiding this comment.
Same as netbsd, don't we have to mention somewhere that, to enable descriptor wallet sqlite3 needs to be installed?
c5650a5 to
b323971
Compare
|
🐙 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". |
|
I also agree that it would be best to use the terms |
|
Moved this to draft, and proposing some other changes. i.e #24585. |
Tries to cleanup the wallet build / install documentation. Similar to #23446.
Always mention how to build for the wallet (sqlite) first, then legacy wallets (bdb).
Where possible, just use "wallet", rather than "descriptor wallet". Non-technical
or causal users don't care about whether the wallet is a descriptor wallet or not,
or what it's backed by they just want to build (and or download) and use Bitcoin Core,
create a wallet, and accept a payment. Using "descriptor wallet" could also imply
that there is some other not legacy but also not a descriptor wallet alternative,
which there is not.
Using 2 terms (wallet and legacy wallet), rather than 5 terms (wallet,
legacy wallet, bdb wallet, sqlite wallet, descriptor wallet) for the same two things
is also much less confusing.
Note that most builders now likely have to do nothing to get "wallet support" as
there's a decent probability they will already have sqlite installed on their system,
and configure no-longer fails if BDB isn't present and
--disable-wallethasn't been passed.