doc: consolidate legacy wallet documentation #23470

fanquake commented at 3:16 AM on November 9, 2021: member

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-wallet hasn't been passed.

fanquake added the label Docs on Nov 9, 2021

fanquake added the label Wallet on Nov 9, 2021

DrahtBot commented at 5:23 AM on November 9, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23954 (doc: remove CC_FOR_BUILD from OpenBSD build doc by fanquake)
#20610 (doc: update for NetBSD 9.1, add GUI Build Instructions by jarolrod)

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.

in doc/legacy-wallet.md:40 in 223a9c2c54 outdated

  35 | +### Fedora
  36 | +
  37 | +```bash
  38 | +dnf install libdb-devel libdb-cxx-devel
  39 | +```
  40 | +

MarcoFalke commented at 8:26 AM on November 9, 2021:

Missing freebsd?

fanquake commented at 10:21 AM on November 9, 2021:

Added

MarcoFalke commented at 8:43 AM on November 9, 2021: member

No opinion on the changes. Won't this go soon anyway?

fanquake force-pushed on Nov 9, 2021

in doc/build-osx.md:112 in 012acdd042 outdated

 121 | +Note: macOS includes a useable `sqlite` package, however it may be older than the
 122 | +one installed via brew.
 123 |  
 124 | -`sqlite` is required to enable support for descriptor wallets.
 125 | -Skip if you don't intend to use descriptor wallets.
 126 | +For information on the legacy wallet, see [legacy-wallet](/doc/leggacy-wallet.md)

katesalazar commented at 10:22 AM on November 9, 2021:

possible typo

katesalazar commented at 10:24 AM on November 9, 2021: contributor

Concept ACK.

fanquake commented at 10:24 AM on November 9, 2021: member

Won't this go soon anyway?

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.

fanquake force-pushed on Nov 9, 2021

laanwj commented at 12:41 PM on November 9, 2021: member

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.

fanquake commented at 1:12 PM on November 9, 2021: member

I don't think we should be in any rush to removing the BDB wallet.

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.

jnewbery commented at 1:17 PM on November 9, 2021: member

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.

laanwj commented at 2:21 PM on November 9, 2021: member

At this point, building with the legacy bdb wallet is a specialized option that most users won't need.

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).

lsilva01 approved

lsilva01 commented at 4:45 AM on November 10, 2021: contributor

ACK d526273

katesalazar commented at 2:41 PM on November 11, 2021: contributor

Concept ACK. At this point, building with the legacy bdb wallet is a specialized option that most users won't need.

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.

in doc/build-unix.md:247 in d526273859 outdated

 242 | @@ -282,12 +243,12 @@ Hardening enables the following features:
 243 |  
 244 |  Disable-wallet mode
 245 |  --------------------
 246 | -When the intention is to run only a P2P node without a wallet, Bitcoin Core may be compiled in
 247 | -disable-wallet mode with:
 248 | +When the intention is to run only a P2P node without a wallet, Bitcoin Core may
 249 | +be compiled without one using:

rajarshimaitra commented at 1:43 PM on November 13, 2021:

be compiled without one, using:

in doc/build-unix.md:45 in d526273859 outdated

  31 | @@ -42,7 +32,6 @@ Optional dependencies:
  32 |   ------------|------------------|----------------------
  33 |   miniupnpc   | UPnP Support     | Firewall-jumping support
  34 |   libnatpmp   | NAT-PMP Support  | Firewall-jumping support
  35 | - libdb4.8    | Berkeley DB      | Wallet storage (only needed when legacy wallet enabled)

rajarshimaitra commented at 1:48 PM on November 13, 2021:

Can this line stay? It's still a dep in case of legacy wallet usage.

in doc/build-unix.md:15 in d526273859 outdated

  10 | -For example, when specifying the path of the dependency:
  11 | -
  12 | -    ../dist/configure --enable-cxx --disable-shared --with-pic --prefix=$BDB_PREFIX
  13 | -
  14 | -Here BDB_PREFIX must be an absolute path - it is defined using $(pwd) which ensures
  15 | -the usage of the absolute path.

rajarshimaitra commented at 1:49 PM on November 13, 2021:

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.

in doc/build-osx.md:231 in d526273859 outdated

 241 |  If `sqlite` is not installed, then wallet functionality will be disabled.
 242 |  
 243 |  ``` bash
 244 |  ./autogen.sh
 245 | -./configure --without-bdb --with-gui=yes
 246 | +./configure --with-gui=yes

rajarshimaitra commented at 2:01 PM on November 13, 2021:

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.

rajarshimaitra commented at 2:07 PM on November 13, 2021: contributor

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.

ghost commented at 2:24 PM on November 13, 2021: none

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).

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/107905/retrieve-the-xpub-and-private-key-of-a-given-path-bitcoin-core/

https://bitcoin.stackexchange.com/questions/107926/fuzzing-descriptor-wallet-rpcs

DrahtBot added the label Needs rebase on Nov 15, 2021

fanquake force-pushed on Nov 22, 2021

fanquake commented at 11:12 AM on November 22, 2021: member

I've made the changes here less aggressive, but still tried to cleanup the build / install instructions. Updated the PR description accordingly.

in doc/build-netbsd.md:33 in c5650a5466 outdated

  29 | @@ -29,8 +30,7 @@ See [dependencies.md](dependencies.md) for a complete overview.
  30 |  
  31 |  ### Building BerkeleyDB
  32 |  
  33 | -BerkeleyDB is only necessary for the wallet functionality. To skip this, pass
  34 | -`--disable-wallet` to `./configure` and skip to the next section.
  35 | +BerkeleyDB is only necessary for legacy wallet functionality.

katesalazar commented at 1:01 PM on November 22, 2021:

Nit: I think it's Berkeley DB, with a space. 🤔

rajarshimaitra commented at 10:26 AM on November 23, 2021:

In the netbsd doc we are not mentioning anywhere the thing about "sqlite3 enables descriptor based wallet". Maybe that line could be added somewhere?

fanquake commented at 5:27 AM on December 2, 2021:

Nit: I think it's Berkeley DB, with a space. 🤔

Done in next push.

DrahtBot removed the label Needs rebase on Nov 22, 2021

in doc/build-freebsd.md:58 in c5650a5466 outdated

  58 | -`db5` is required to enable support for legacy wallets. Skip if you don't intend to use legacy wallets
  59 | +###### Wallet
  60 |  
  61 | -```bash
  62 | -pkg install db5
  63 | +`sqlite3` is used for the ([descriptor based](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md)) wallet.

meshcollider commented at 7:05 AM on November 23, 2021:

nit: used for descriptor based wallets

fanquake commented at 5:27 AM on December 2, 2021:

Done in next push.

in doc/build-osx.md:102 in c5650a5466 outdated

  98 | @@ -99,30 +99,22 @@ git clone https://github.com/bitcoin/bitcoin.git
  99 |  
 100 |  #### Wallet Dependencies
 101 |  
 102 | -It is not necessary to build wallet functionality to run `bitcoind` or  `bitcoin-qt`.

meshcollider commented at 7:13 AM on November 23, 2021:

IMO a comment like this is worth having still (here and in the other files), I'm not convinced the new format clearly conveys that the two types of wallet are both optional.

fanquake commented at 8:47 AM on November 23, 2021:

I was hoping the fact that this exists in the section for optional dependencies would convey that both are optional. We could add back a line, but I'm trying to trim this down so it's not so overly verbose/repetitive.

meshcollider commented at 1:09 AM on November 29, 2021:

@fanquake fair enough, agreed!

meshcollider commented at 7:17 AM on November 23, 2021: contributor

Concept ACK

in doc/build-freebsd.md:101 in c5650a5466 outdated

  96 | @@ -98,8 +97,8 @@ pkg install python3
  97 |  ### 1. Configuration
  98 |  
  99 |  There are many ways to configure Bitcoin Core, here are a few common examples:
 100 | -##### Wallet (BDB + SQlite) Support, No GUI:
 101 | -This explicitly enables legacy wallet support and disables the GUI. If `sqlite3` is installed, then descriptor wallet support will be built.
 102 | +##### Wallet & Legacy Wallet Support, No GUI:
 103 | +This explicitly enables legacy wallet support and disables the GUI. If `sqlite3` is installed, then wallet support will also be built.

rajarshimaitra commented at 10:23 AM on November 23, 2021:

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.

in doc/build-openbsd.md:81 in c5650a5466 outdated

  75 | @@ -77,24 +76,23 @@ and preprocessor defines like `waitid()` and `WEXITED` that are not available.)
  76 |  To configure with wallet:
  77 |  ```bash
  78 |  ./configure --with-gui=no --disable-external-signer CC=cc CXX=c++ \
  79 | -    BDB_LIBS="-L${BDB_PREFIX}/lib -ldb_cxx-4.8" \
  80 | -    BDB_CFLAGS="-I${BDB_PREFIX}/include" \

rajarshimaitra commented at 10:28 AM on November 23, 2021:

Same as netbsd, don't we have to mention somewhere that, to enable descriptor wallet sqlite3 needs to be installed?

rajarshimaitra commented at 10:32 AM on November 23, 2021: contributor

reACK https://github.com/bitcoin/bitcoin/pull/23470/commits/c5650a5466eaa2a1f5779ce797c2857ba8399d4b

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: cleanup wallet build docs b32397167a

fanquake force-pushed on Dec 2, 2021

DrahtBot added the label Needs rebase on Jan 4, 2022

DrahtBot commented at 4:05 PM on January 4, 2022: member

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

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

Rspigler commented at 3:45 AM on January 6, 2022: contributor

I also agree that it would be best to use the terms legacy wallet and descriptor wallet, rather than just legacy/wallet. The user should know what type of wallet it is (and its not like there isn't other technical terms in other docs or even in the GUI itself). There is also descriptors.md if the user is confused.

fanquake marked this as a draft on Mar 17, 2022

fanquake commented at 10:28 AM on March 17, 2022: member

Moved this to draft, and proposing some other changes. i.e #24585.

fanquake commented at 10:47 AM on March 18, 2022: member

Further changes in #24600 and #24608.

fanquake commented at 10:55 AM on March 24, 2022: member

Closing this now that changes have either been merged or split out (#24652, #24658).

fanquake closed this on Mar 24, 2022

fanquake deleted the branch on Mar 24, 2022

DrahtBot locked this on Mar 24, 2023