Remove the legacy wallet and BDB dependency #28710

pull achow101 wants to merge 8 commits into bitcoin:master from achow101:rm-legacy-wallet changing 83 files +212 −5682
  1. achow101 commented at 11:36 pm on October 23, 2023: member

    The final step of #20160.

    A bare minimum of legacy wallet code is kept in order to perform wallet migration. Migration of legacy wallets uses the independent BDB parser and a minimal LegacyDataSPKM that allows the legacy data to be loaded so that the migration can be completed.

    All tests which tested legacy wallet behavior have been removed. The --descriptors and --legacy-wallet options are removed from the functional tests.

    BDB has been removed as a dependency and documentation have been updated to reflect that.

  2. DrahtBot commented at 11:36 pm on October 23, 2023: contributor

    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/28710.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko
    Concept ACK Sjors

    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:

    • #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
    • #32238 (qt, wallet: Convert uint256 to Txid by marcofleon)
    • #32189 (refactor: Txid type safety (parent PR) by marcofleon)
    • #32086 (Shuffle depends instructions and recommend modern make for macOS by Sjors)
    • #31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
    • #31895 (doc: Improve dependencies.md by NicolaLS)
    • #31802 (Add bitcoin-{node,gui} to release binaries for IPC by Sjors)
    • #31679 (cmake: Move internal binaries from bin/ to libexec/ by ryanofsky)
    • #31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)
    • #31622 (psbt: add non-default sighash types to PSBTs and unify sighash type match checking by achow101)
    • #31453 (util: detect and warn when using exFAT on MacOS by willcl-ark)
    • #31404 (descriptors: inference process, do not return unparsable multisig descriptors by furszy)
    • #31398 (wallet: refactor: various master key encryption cleanups by theStack)
    • #31360 (depends: Avoid using helper variables in toolchain file by hebasto)
    • #31296 (wallet: Translate [default wallet] string in progress messages by ryanofsky)
    • #31275 (doc: corrected lockunspent rpc quoting by Talej)
    • #30595 (kernel: Introduce initial C header API by TheCharlatan)
    • #30437 (multiprocess: add bitcoin-mine test program by ryanofsky)
    • #30221 (wallet: Ensure best block matches wallet scan state by achow101)
    • #29675 (wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys by achow101)
    • #29652 (wallet: Avoid potentially writing incorrect best block locator by ryanofsky)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
    • #29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
    • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
    • #21283 (Implement BIP 370 PSBTv2 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.

  3. achow101 force-pushed on Oct 23, 2023
  4. DrahtBot added the label CI failed on Oct 23, 2023
  5. achow101 force-pushed on Oct 24, 2023
  6. achow101 force-pushed on Oct 24, 2023
  7. DrahtBot added the label Needs rebase on Oct 25, 2023
  8. achow101 force-pushed on Oct 25, 2023
  9. achow101 force-pushed on Oct 25, 2023
  10. DrahtBot removed the label Needs rebase on Oct 25, 2023
  11. DrahtBot added the label Needs rebase on Nov 6, 2023
  12. achow101 force-pushed on Nov 13, 2023
  13. DrahtBot removed the label Needs rebase on Nov 13, 2023
  14. DrahtBot added the label Needs rebase on Nov 16, 2023
  15. achow101 force-pushed on Nov 16, 2023
  16. DrahtBot removed the label Needs rebase on Nov 16, 2023
  17. DrahtBot added the label Needs rebase on Nov 22, 2023
  18. achow101 force-pushed on Nov 28, 2023
  19. DrahtBot removed the label Needs rebase on Nov 28, 2023
  20. DrahtBot added the label Needs rebase on Nov 28, 2023
  21. achow101 force-pushed on Nov 28, 2023
  22. DrahtBot removed the label Needs rebase on Nov 28, 2023
  23. DrahtBot added the label Needs rebase on Nov 30, 2023
  24. achow101 force-pushed on Dec 11, 2023
  25. achow101 force-pushed on Dec 11, 2023
  26. DrahtBot removed the label Needs rebase on Dec 11, 2023
  27. achow101 force-pushed on Dec 11, 2023
  28. DrahtBot added the label Needs rebase on Dec 13, 2023
  29. achow101 force-pushed on Dec 19, 2023
  30. DrahtBot removed the label Needs rebase on Dec 19, 2023
  31. achow101 force-pushed on Dec 19, 2023
  32. DrahtBot added the label Needs rebase on Jan 2, 2024
  33. fanquake referenced this in commit 04978c2e18 on Jan 5, 2024
  34. achow101 force-pushed on Jan 6, 2024
  35. DrahtBot removed the label Needs rebase on Jan 6, 2024
  36. DrahtBot added the label Needs rebase on Jan 11, 2024
  37. achow101 force-pushed on Jan 11, 2024
  38. DrahtBot removed the label Needs rebase on Jan 11, 2024
  39. DrahtBot added the label Needs rebase on Jan 16, 2024
  40. achow101 force-pushed on Jan 16, 2024
  41. DrahtBot removed the label Needs rebase on Jan 16, 2024
  42. DrahtBot added the label Needs rebase on Jan 17, 2024
  43. achow101 force-pushed on Jan 25, 2024
  44. achow101 force-pushed on Feb 1, 2024
  45. DrahtBot removed the label Needs rebase on Feb 1, 2024
  46. ryanofsky referenced this in commit 93e10cab5d on Feb 2, 2024
  47. DrahtBot added the label Needs rebase on Feb 3, 2024
  48. achow101 force-pushed on Feb 3, 2024
  49. DrahtBot removed the label Needs rebase on Feb 3, 2024
  50. DrahtBot added the label Needs rebase on Feb 6, 2024
  51. achow101 force-pushed on Feb 8, 2024
  52. DrahtBot removed the label Needs rebase on Feb 8, 2024
  53. DrahtBot added the label Needs rebase on Feb 10, 2024
  54. achow101 force-pushed on Feb 20, 2024
  55. DrahtBot removed the label Needs rebase on Feb 20, 2024
  56. DrahtBot added the label Needs rebase on Feb 26, 2024
  57. achow101 force-pushed on Feb 29, 2024
  58. DrahtBot removed the label Needs rebase on Feb 29, 2024
  59. jess2505 approved
  60. DrahtBot added the label Needs rebase on Mar 11, 2024
  61. achow101 force-pushed on Mar 11, 2024
  62. DrahtBot removed the label Needs rebase on Mar 11, 2024
  63. DrahtBot added the label Needs rebase on Mar 12, 2024
  64. achow101 force-pushed on Mar 12, 2024
  65. DrahtBot removed the label Needs rebase on Mar 12, 2024
  66. DrahtBot added the label Needs rebase on Mar 18, 2024
  67. achow101 force-pushed on Mar 29, 2024
  68. DrahtBot removed the label Needs rebase on Mar 29, 2024
  69. DrahtBot added the label Needs rebase on Apr 1, 2024
  70. achow101 force-pushed on Apr 1, 2024
  71. DrahtBot removed the label Needs rebase on Apr 1, 2024
  72. achow101 force-pushed on Apr 3, 2024
  73. DrahtBot added the label Needs rebase on Apr 8, 2024
  74. laanwj requested review from laanwj on Apr 9, 2024
  75. in src/wallet/migrate.cpp:46 in 7707db3ad5 outdated 
    41+enum class RecordType : uint8_t
42+{
43+    KEYDATA = 1,
44+    DUPLICATE = 2,
45+    OVERFLOW_DATA = 3,
46+    DELETE = 0x80, // Indicate this record is deleted. This is AND'd with the real type.
    laanwj commented at 10:26 am on April 9, 2024:
    OR’ed, i guess?
  76. achow101 force-pushed on Apr 25, 2024
  77. achow101 force-pushed on Apr 26, 2024
  78. achow101 force-pushed on May 21, 2024
  79. DrahtBot removed the label Needs rebase on May 21, 2024
  80. DrahtBot added the label Needs rebase on May 22, 2024
  81. achow101 force-pushed on May 22, 2024
  82. DrahtBot removed the label Needs rebase on May 22, 2024
  83. DrahtBot added the label Needs rebase on May 23, 2024
  84. achow101 force-pushed on May 29, 2024
  85. achow101 force-pushed on Jun 4, 2024
  86. DrahtBot removed the label Needs rebase on Jun 5, 2024
  87. DrahtBot added the label Needs rebase on Jun 5, 2024
  88. achow101 force-pushed on Jun 6, 2024
  89. DrahtBot removed the label Needs rebase on Jun 7, 2024
  90. achow101 force-pushed on Jun 7, 2024
  91. achow101 force-pushed on Jun 7, 2024
  92. achow101 force-pushed on Jun 7, 2024
  93. achow101 force-pushed on Jun 10, 2024
  94. achow101 force-pushed on Jun 10, 2024
  95. achow101 force-pushed on Jun 10, 2024
  96. DrahtBot removed the label CI failed on Jun 11, 2024
  97. DrahtBot added the label Needs rebase on Jun 11, 2024
  98. achow101 force-pushed on Jun 11, 2024
  99. DrahtBot removed the label Needs rebase on Jun 11, 2024
  100. DrahtBot added the label Needs rebase on Jun 12, 2024
  101. achow101 force-pushed on Jun 13, 2024
  102. DrahtBot removed the label Needs rebase on Jun 13, 2024
  103. DrahtBot added the label CI failed on Jun 14, 2024
  104. DrahtBot commented at 0:09 am on June 14, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/26201268639

  105. achow101 force-pushed on Jun 14, 2024
  106. DrahtBot removed the label CI failed on Jun 14, 2024
  107. DrahtBot added the label Needs rebase on Jun 17, 2024
  108. Sjors commented at 4:10 pm on June 27, 2024: member
    You can also drop the BerkeleyDatabaseSanityCheck related suppression in contrib/devtools/check-devs.sh
  109. achow101 force-pushed on Jun 27, 2024
  110. DrahtBot removed the label Needs rebase on Jun 27, 2024
  111. DrahtBot added the label CI failed on Jun 27, 2024
  112. DrahtBot commented at 8:59 pm on June 27, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/26774987403

  113. achow101 force-pushed on Jul 1, 2024
  114. DrahtBot removed the label CI failed on Jul 1, 2024
  115. DrahtBot added the label Needs rebase on Jul 3, 2024
  116. achow101 force-pushed on Jul 11, 2024
  117. DrahtBot removed the label Needs rebase on Jul 11, 2024
  118. DrahtBot added the label Needs rebase on Jul 16, 2024
  119. achow101 force-pushed on Jul 22, 2024
  120. DrahtBot removed the label Needs rebase on Jul 22, 2024
  121. DrahtBot added the label Needs rebase on Jul 25, 2024
  122. achow101 force-pushed on Aug 29, 2024
  123. DrahtBot removed the label Needs rebase on Aug 29, 2024
  124. DrahtBot added the label CI failed on Aug 30, 2024
  125. DrahtBot commented at 2:19 am on August 30, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29441840067

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  126. DrahtBot added the label Needs rebase on Aug 31, 2024
  127. achow101 force-pushed on Sep 3, 2024
  128. DrahtBot removed the label Needs rebase on Sep 3, 2024
  129. DrahtBot added the label Needs rebase on Sep 4, 2024
  130. achow101 force-pushed on Sep 10, 2024
  131. DrahtBot removed the label Needs rebase on Sep 10, 2024
  132. DrahtBot added the label Needs rebase on Sep 12, 2024
  133. achow101 force-pushed on Sep 17, 2024
  134. DrahtBot removed the label Needs rebase on Sep 17, 2024
  135. DrahtBot added the label Needs rebase on Sep 20, 2024
  136. achow101 force-pushed on Oct 4, 2024
  137. DrahtBot removed the label Needs rebase on Oct 4, 2024
  138. achow101 force-pushed on Oct 4, 2024
  139. DrahtBot added the label Needs rebase on Oct 5, 2024
  140. achow101 force-pushed on Oct 11, 2024
  141. DrahtBot removed the label Needs rebase on Oct 11, 2024
  142. DrahtBot added the label Needs rebase on Oct 24, 2024
  143. achow101 force-pushed on Oct 24, 2024
  144. DrahtBot removed the label Needs rebase on Oct 24, 2024
  145. DrahtBot removed the label CI failed on Oct 24, 2024
  146. DrahtBot added the label Needs rebase on Oct 25, 2024
  147. achow101 force-pushed on Oct 25, 2024
  148. DrahtBot removed the label Needs rebase on Oct 25, 2024
  149. DrahtBot added the label Needs rebase on Oct 28, 2024
  150. achow101 force-pushed on Oct 28, 2024
  151. DrahtBot removed the label Needs rebase on Oct 28, 2024
  152. DrahtBot added the label Needs rebase on Oct 29, 2024
  153. achow101 force-pushed on Oct 29, 2024
  154. DrahtBot removed the label Needs rebase on Oct 29, 2024
  155. DrahtBot added the label Needs rebase on Nov 1, 2024
  156. achow101 force-pushed on Nov 1, 2024
  157. DrahtBot removed the label Needs rebase on Nov 1, 2024
  158. murchandamus commented at 9:43 pm on November 5, 2024: contributor
    It looks like all three dependencies got merged, is this ready for review?
  159. achow101 commented at 10:48 pm on November 5, 2024: member

    It looks like all three dependencies got merged, is this ready for review?

    Currently it is still dependent on #30328 but I suppose it doesn’t have to be.

  160. DrahtBot added the label Needs rebase on Nov 6, 2024
  161. achow101 force-pushed on Nov 6, 2024
  162. DrahtBot added the label CI failed on Nov 6, 2024
  163. DrahtBot removed the label Needs rebase on Nov 6, 2024
  164. achow101 force-pushed on Nov 7, 2024
  165. achow101 force-pushed on Nov 7, 2024
  166. achow101 force-pushed on Nov 7, 2024
  167. DrahtBot added the label Needs rebase on Nov 11, 2024
  168. fanquake referenced this in commit 2b33322169 on Nov 12, 2024
  169. achow101 force-pushed on Nov 13, 2024
  170. DrahtBot removed the label Needs rebase on Nov 13, 2024
  171. DrahtBot added the label Needs rebase on Nov 15, 2024
  172. in src/wallet/wallet.cpp:3484 in 323bc8982a outdated 
    3487@@ -3684,9 +3488,8 @@ void CWallet::SetupLegacyScriptPubKeyMan()
3488         return;
3489     }
3490 
3491-    std::unique_ptr<ScriptPubKeyMan> spk_manager = m_database->Format() == "bdb_ro" ?
3492-        std::make_unique<LegacyDataSPKM>(*this) :
3493-        std::make_unique<LegacyScriptPubKeyMan>(*this, m_keypool_size);
3494+    Assert(m_database->Format() == "bdb_ro");
3495+    std::unique_ptr<ScriptPubKeyMan> spk_manager = std::make_unique<LegacyDataSPKM>(*this);
    furszy commented at 4:00 pm on November 23, 2024:

    In order to continue testing this without manually writing the bdb file, we need to allow the ‘mock’ db format here:

    0const std::string& db_format{m_database->Format()};
1Assert(db_format == "bdb_ro" || db_format == "mock");
  173. ryanofsky referenced this in commit 2eccb8bc5e on Dec 5, 2024
  174. achow101 force-pushed on Dec 9, 2024
  175. DrahtBot removed the label Needs rebase on Dec 9, 2024
  176. DrahtBot removed the label CI failed on Dec 10, 2024
  177. achow101 force-pushed on Dec 13, 2024
  178. achow101 force-pushed on Dec 16, 2024
  179. achow101 force-pushed on Dec 16, 2024
  180. in src/wallet/scriptpubkeyman.cpp:84 in e6ccd17275 outdated 
    80@@ -79,7 +81,7 @@ bool HaveKeys(const std::vector<valtype>& pubkeys, const LegacyDataSPKM& keystor
81 //!                            scripts or simply treat any script that has been
82 //!                            stored in the keystore as spendable
83 // NOLINTNEXTLINE(misc-no-recursion)
84-IsMineResult IsMineInner(const LegacyDataSPKM& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
85+IsMineResult LegacyWalletIsMineInnerDONOTUSE(const LegacyDataSPKM& keystore, const CScript& scriptPubKey, IsMineSigVersion sigversion, bool recurse_scripthash=true)
    Sjors commented at 2:20 pm on January 13, 2025:

    e6ccd17275e06e6278473587257f80c2d9bd8834: maybe move the legacy wallet migration code to a separate file? And mark it read-only :-)

    e.g. https://github.com/Sjors/bitcoin/commit/364088a306f8de9ece2c5201dbe48a252c39ea03

  181. DrahtBot added the label Needs rebase on Jan 17, 2025
  182. achow101 force-pushed on Jan 20, 2025
  183. DrahtBot removed the label Needs rebase on Jan 20, 2025
  184. DrahtBot added the label CI failed on Jan 21, 2025
  185. DrahtBot added the label Needs rebase on Jan 21, 2025
  186. achow101 force-pushed on Jan 21, 2025
  187. DrahtBot removed the label Needs rebase on Jan 21, 2025
  188. DrahtBot removed the label CI failed on Jan 21, 2025
  189. DrahtBot added the label Needs rebase on Jan 23, 2025
  190. achow101 force-pushed on Feb 1, 2025
  191. DrahtBot added the label CI failed on Feb 1, 2025
  192. DrahtBot commented at 3:36 am on February 1, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36513189138

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  193. DrahtBot removed the label Needs rebase on Feb 4, 2025
  194. bitcoin deleted a comment on Feb 9, 2025
  195. achow101 force-pushed on Feb 10, 2025
  196. DrahtBot removed the label CI failed on Feb 10, 2025
  197. glozow referenced this in commit 96d30ed4f9 on Feb 13, 2025
  198. DrahtBot added the label Needs rebase on Feb 13, 2025
  199. achow101 force-pushed on Feb 13, 2025
  200. DrahtBot removed the label Needs rebase on Feb 13, 2025
  201. DrahtBot added the label Needs rebase on Feb 14, 2025
  202. achow101 force-pushed on Feb 14, 2025
  203. DrahtBot removed the label Needs rebase on Feb 14, 2025
  204. achow101 force-pushed on Feb 14, 2025
  205. achow101 force-pushed on Feb 19, 2025
  206. achow101 force-pushed on Feb 19, 2025
  207. DrahtBot added the label Needs rebase on Feb 20, 2025
  208. achow101 force-pushed on Mar 5, 2025
  209. achow101 force-pushed on Mar 5, 2025
  210. DrahtBot commented at 8:28 pm on March 5, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38265463467

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  211. DrahtBot added the label CI failed on Mar 5, 2025
  212. DrahtBot removed the label Needs rebase on Mar 5, 2025
  213. DrahtBot removed the label CI failed on Mar 5, 2025
  214. DrahtBot added the label Needs rebase on Mar 7, 2025
  215. achow101 force-pushed on Mar 7, 2025
  216. DrahtBot removed the label Needs rebase on Mar 7, 2025
  217. DrahtBot added the label Needs rebase on Mar 14, 2025
  218. achow101 force-pushed on Mar 14, 2025
  219. DrahtBot removed the label Needs rebase on Mar 14, 2025
  220. DrahtBot added the label Needs rebase on Mar 16, 2025
  221. achow101 force-pushed on Apr 10, 2025
  222. achow101 force-pushed on Apr 10, 2025
  223. achow101 force-pushed on Apr 10, 2025
  224. DrahtBot added the label CI failed on Apr 10, 2025
  225. DrahtBot commented at 4:21 am on April 10, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40297294016

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  226. achow101 force-pushed on Apr 10, 2025
  227. DrahtBot removed the label Needs rebase on Apr 10, 2025
  228. achow101 force-pushed on Apr 10, 2025
  229. DrahtBot removed the label CI failed on Apr 10, 2025
  230. achow101 force-pushed on Apr 10, 2025
  231. achow101 force-pushed on Apr 10, 2025
  232. DrahtBot added the label Needs rebase on Apr 11, 2025
  233. achow101 force-pushed on Apr 11, 2025
  234. achow101 commented at 7:00 pm on April 11, 2025: member
    The test each commit task fails because the HEAD of this PR has bdb removed from that task’s install list, but the earlier commits in this PR still needs BDB. I guess we can leave it there for now and remove it later?
  235. DrahtBot removed the label Needs rebase on Apr 11, 2025
  236. achow101 force-pushed on Apr 12, 2025
  237. DrahtBot added the label Needs rebase on Apr 14, 2025
  238. achow101 force-pushed on Apr 14, 2025
  239. DrahtBot removed the label Needs rebase on Apr 15, 2025
  240. DrahtBot added the label Needs rebase on Apr 17, 2025
  241. achow101 force-pushed on Apr 17, 2025
  242. DrahtBot removed the label Needs rebase on Apr 17, 2025
  243. DrahtBot added the label Needs rebase on Apr 21, 2025
  244. achow101 force-pushed on Apr 21, 2025
  245. DrahtBot removed the label Needs rebase on Apr 21, 2025
  246. DrahtBot added the label Needs rebase on Apr 23, 2025
  247. achow101 force-pushed on Apr 23, 2025
  248. DrahtBot removed the label Needs rebase on Apr 23, 2025
  249. achow101 force-pushed on Apr 23, 2025
  250. DrahtBot added the label CI failed on Apr 23, 2025
  251. DrahtBot commented at 7:12 pm on April 23, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41034814129

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  252. achow101 force-pushed on Apr 23, 2025
  253. DrahtBot removed the label CI failed on Apr 24, 2025
  254. in CMakePresets.json:82 in a594c904ed outdated 
    79@@ -80,7 +80,6 @@
80         "ENABLE_HARDENING": "ON",
81         "ENABLE_WALLET": "ON",
82         "WARN_INCOMPATIBLE_BDB": "OFF",
    pablomartin4btc commented at 1:03 pm on April 24, 2025:

    nit: shouldn’t this one be removed too?

    achow101 commented at 9:57 pm on April 24, 2025:
    Done.
  255. in doc/files.md:118 in a594c904ed outdated 
    113@@ -123,6 +114,15 @@ Path           | Description | Repository notes
114 `addr.dat`     | Peer IP address BDB database; replaced by `peers.dat` in [0.7.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.7.0.md) | [PR #1198](https://github.com/bitcoin/bitcoin/pull/1198), [`928d3a01`](https://github.com/bitcoin/bitcoin/commit/928d3a011cc66c7f907c4d053f674ea77dc611cc)
115 `onion_private_key` | Cached Tor onion service private key for `-listenonion` option. Was used for Tor v2 services; replaced by `onion_v3_private_key` in [0.21.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.21.0.md) | [PR #19954](https://github.com/bitcoin/bitcoin/pull/19954)
116 
117+### Berkeley DB database based wallets
118+
    pablomartin4btc commented at 2:22 pm on April 24, 2025:

    nit: something like this just for reference, if you agree ofc…

    0
1These wallets, Legacy Wallets, are not longer supported (since v3x), can't be created nor loaded but migrated into SQLite (Descriptor Wallets).
    achow101 commented at 9:57 pm on April 24, 2025:
    I think it’s fine to leave as is, it’s under the “Legacy subdirectories and Files” heading. This document discusses the files that Bitcoin Core creates and uses, and legacy wallets are neither created nor really used after this PR.
  256. achow101 force-pushed on Apr 24, 2025
  257. fanquake referenced this in commit 80e6ad9e30 on Apr 25, 2025
  258. DrahtBot added the label Needs rebase on Apr 25, 2025
  259. achow101 force-pushed on Apr 25, 2025
  260. achow101 commented at 4:10 pm on April 25, 2025: member

    Rebased and added a couple commits for followups from #31250

    This is now ready for review.

  261. achow101 marked this as ready for review on Apr 25, 2025
  262. in depends/packages/bdb.mk:19 in ec1b63648f outdated 
    13-$(package)_cppflags_netbsd=-D_XOPEN_SOURCE=600
14-$(package)_cppflags_mingw32=-DUNICODE -D_UNICODE
15-endef
16-
17-define $(package)_preprocess_cmds
18-  patch -p1 < $($(package)_patch_dir)/clang_cxx_11.patch && \
    fanquake commented at 4:14 pm on April 25, 2025:
    clang_cxx_11.patch also needs deleting.
    achow101 commented at 7:30 pm on April 25, 2025:
    Removed
  263. fanquake commented at 4:17 pm on April 25, 2025: member
    There’s also a libdb++-dev instance in workflows/ci.yml & deadlock:libdb / BerkeleyBatch etc in tsan suppressions. Berkeley* in walletdb.h. BerkeleyEnvironment::Salvage in utils_tests.cpp.
  264. DrahtBot removed the label Needs rebase on Apr 25, 2025
  265. achow101 force-pushed on Apr 25, 2025
  266. achow101 commented at 7:31 pm on April 25, 2025: member

    There’s also a libdb++-dev instance in workflows/ci.yml & deadlock:libdb / BerkeleyBatch etc in tsan suppressions. Berkeley* in walletdb.h. BerkeleyEnvironment::Salvage in utils_tests.cpp.

    Removed

  267. achow101 added this to the milestone 30.0 on Apr 25, 2025
  268. build, wallet, doc: Remove BDB 8e2d14de5c
  269. test: Remove unused options and variables, correct comments 9d1a9565ca
  270. test: Run multisig script limit test 
    This test was mistakenly disabled.
    8de844b955
  271. test: rpcs disabled for descriptor wallets will be removed 235dfce217
  272. wallet, rpc: Remove legacy wallet only RPCs 09c9f706f5
  273. wallet: Delete LegacySPKM 
    Deletes LegacyScriptPubKeyMan and related tests
    dc7bf5fd6a
  274. wallet: Remove unused db functions 
    SOme db functions were for BDB, these are no longer needed.
    cc3c3b9958
  275. legacy spkm: Make IsMine() and CanProvide() private and migration only bc3f07e384
  276. achow101 force-pushed on Apr 25, 2025
  277. maflcko commented at 6:36 am on April 28, 2025: member

    All tests which tested legacy wallet behavior have been removed. The --descriptors and --legacy-wallet options are removed from the functional tests.

    nit in OP, I don’t think this is true. This was already done in https://github.com/bitcoin/bitcoin/pull/31250

  278. in src/test/util_tests.cpp:185 in 8e2d14de5c outdated 
    181@@ -182,7 +182,7 @@ BOOST_AUTO_TEST_CASE(parse_hex)
182     result = TryParseHex<uint8_t>("12 34 56 78").value();
183     BOOST_CHECK_EQUAL_COLLECTIONS(result.begin(), result.end(), expected.begin(), expected.end());
184 
185-    // Leading space must be supported (used in BerkeleyEnvironment::Salvage)
186+    // Leading space must be supported
    maflcko commented at 7:01 am on April 28, 2025:
    As a follow-up idea, someone could investigate if this is still needed or if it can be cleaned up
  279. in src/qt/overviewpage.cpp:1 in dc7bf5fd6a outdated
    maflcko commented at 9:02 am on April 28, 2025:
    in dc7bf5fd6a320c4528a28cef2a565366bbab3877: Are labelWatch* still needed?
  280. in src/wallet/interfaces.cpp:180 in dc7bf5fd6a outdated 
    176@@ -177,11 +177,7 @@ class WalletImpl : public Wallet
177     }
178     bool haveWatchOnly() override
179     {
180-        auto spk_man = m_wallet->GetLegacyScriptPubKeyMan();
181-        if (spk_man) {
182-            return spk_man->HaveWatchOnly();
183-        }
184-        return false;
185+        return m_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS);
    maflcko commented at 9:06 am on April 28, 2025:
    in https://github.com/bitcoin/bitcoin/commit/dc7bf5fd6a320c4528a28cef2a565366bbab3877: Does not look right?
    maflcko commented at 9:13 am on April 28, 2025:

    Also, the comment Legacy wallets are being deprecated, warn if the loaded wallet is legacy in the code should be removed?

    0$ git grep 'Legacy wallets are being deprecated, warn if the loaded wallet is legacy' bc3f07e384c2e145d6d14cfa3ad65b976233b538
1bc3f07e384c2e145d6d14cfa3ad65b976233b538:src/wallet/wallet.cpp:        // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
    Sjors commented at 9:14 am on April 28, 2025:
    In dc7bf5fd6a320c4528a28cef2a565366bbab3877 “wallet: Delete LegacySPKM”: why doesn’t this just return false like before? And if so, can’t the entire helper be dropped?
    Sjors commented at 10:19 am on April 28, 2025:
    It causes a bug in the GUI, see (1) here: #28710#pullrequestreview-2798630677
  281. in src/wallet/rpc/coins.cpp:479 in dc7bf5fd6a outdated 
    478@@ -479,15 +479,6 @@ RPCHelpMan getbalances()
479         }
    maflcko commented at 9:14 am on April 28, 2025:
    dc7bf5fd6a320c4528a28cef2a565366bbab3877: Needs to update the RPC help, to remove the field?
  282. in src/wallet/rpc/wallet.cpp:114 in dc7bf5fd6a outdated 
    106@@ -107,14 +107,6 @@ static RPCHelpMan getwalletinfo()
107     }
108     obj.pushKV("keypoolsize", (int64_t)kpExternalSize);
109 
110-    LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
111-    if (spk_man) {
112-        CKeyID seed_id = spk_man->GetHDChain().seed_id;
113-        if (!seed_id.IsNull()) {
114-            obj.pushKV("hdseedid", seed_id.GetHex());
    maflcko commented at 9:15 am on April 28, 2025:
    Same?
    maflcko commented at 9:26 am on April 28, 2025:

    Also, some more stuff:

     0$ git grep -i 'legacy wallets' src/script/ src/wallet doc/*.md
 1doc/build-netbsd.md:`db4` is required to enable support for legacy wallets.
 2doc/managing-wallets.md:## Migrating Legacy Wallets to Descriptor Wallets
 3doc/managing-wallets.md:Legacy wallets (traditional non-descriptor wallets) can be migrated to become Descriptor wallets
 4doc/managing-wallets.md:Legacy wallets can know about, be watching for, and be able to sign for, `migratewallet` only
 5doc/psbt.md:If you are using legacy wallets feel free to continue with the example provided here.
 6src/script/descriptor.h:     *  TODO: Remove this method once legacy wallets are removed as it is only necessary for importmulti.
 7src/wallet/rpc/addresses.cpp:                // In legacy wallets hdkeypath has always used an apostrophe for
 8src/wallet/rpc/wallet.cpp:                        {RPCResult::Type::NUM_TIME, "keypoololdest", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest pre-generated key in the key pool. Legacy wallets only."},
 9src/wallet/scriptpubkeyman.cpp:    // Legacy wallets can also contain scripts whose P2SH, P2WSH, or P2SH-P2WSH it is not watching for
10src/wallet/scriptpubkeyman.h:/** struct containing information needed for migrating legacy wallets to descriptor wallets */
11src/wallet/wallet.cpp:        // Legacy wallets are being deprecated, warn if the loaded wallet is legacy
12src/wallet/wallet.cpp:            warnings.emplace_back(_("Wallet loaded successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future. Legacy wallets can be migrated to a descriptor wallet with migratewallet."));
13src/wallet/wallet.cpp:        error = Untranslated("Legacy wallets can no longer be created");
14src/wallet/wallet.cpp:    // Legacy wallets are being deprecated, warn if a newly created wallet is legacy
15src/wallet/wallet.cpp:        warnings.emplace_back(_("Wallet created successfully. The legacy wallet type is being deprecated and support for creating and opening legacy wallets will be removed in the future."));
16src/wallet/wallet.cpp:                // Legacy wallets need SetupGeneration here.
17src/wallet/wallet.cpp:    // Activating ScriptPubKeyManager for a given output and change type is incompatible with legacy wallets.
18src/wallet/wallet.cpp:    // Legacy wallets have only one ScriptPubKeyManager and it's active for all output and change types.
  283. in src/wallet/test/CMakeLists.txt:16 in dc7bf5fd6a outdated 
    12@@ -13,9 +13,7 @@ target_sources(test_bitcoin
13     feebumper_tests.cpp
14     group_outputs_tests.cpp
15     init_tests.cpp
16-    ismine_tests.cpp
    Sjors commented at 9:28 am on April 28, 2025:
    In dc7bf5fd6a320c4528a28cef2a565366bbab3877 “wallet: Delete LegacySPKM”: this test (file) and the one below still exist.
  284. Sjors changes_requested
  285. Sjors commented at 9:35 am on April 28, 2025: member

    Concept ACK.

    bc3f07e384c2e145d6d14cfa3ad65b976233b538 looks good, except:

    1. The GUI now shows a watch-only balance for descriptor wallets. See inline for the culprit.

    2. In that same commit, the churn in wallet/scriptpubkeyman.cpp is hard to follow, both here on Github as well as a git show --color-moved=dimmed-zebra. I’m guessing you’re moving stuff around, deleting and modifying? Update: use --histogram, it’s just deletes.

    And a few details:

    • there’s one more -DWITH_BDB=ON in test-each-commit-exec.sh
    • test_framework/bdb.py seems unused now
    • fuzz/wallet_dbd_parser checks for the presence of USE_BDB which is dropped.
    • DEFAULT_FLUSHWALLET is unused
    • wallet_createwallet.py has a comment referring to sethdseed
    • importprivkey and friends are still in bitcoin-cli.bash

    Also it might be good to rename the importprivkey test helper to e.g. importkey, so it’s not confused with the disappeared RPC method.

    As I mentioned in an earlier review https://github.com/bitcoin/bitcoin/pull/28710/commits/bc3f07e384c2e145d6d14cfa3ad65b976233b538#r1913267051, rather than naming functions DONOTUSE you could also move the migration code into a new file. But that could be a nice followup, along with changes to remove any legacy wallet weirdness from scriptpubkeyman.h

  286. in src/wallet/wallet.cpp:2779 in dc7bf5fd6a outdated 
    2666@@ -2776,68 +2667,6 @@ void CWallet::ListLockedCoins(std::vector<COutPoint>& vOutpts) const
2667     }
2668 }
2669 
2670-/** @} */ // end of Actions
    maflcko commented at 9:39 am on April 28, 2025:
    (same commit): Should remove both comments, or none?
  287. in src/wallet/walletdb.cpp:755 in dc7bf5fd6a outdated 
    735-        int64_t nIndex;
736-        key >> nIndex;
737-        CKeyPool keypool;
738-        value >> keypool;
739-        pwallet->GetOrCreateLegacyDataSPKM()->LoadKeyPool(nIndex, keypool);
740-        return DBErrors::LOAD_OK;
    maflcko commented at 9:45 am on April 28, 2025:
    (same commit): I wonder if this is fine to remove, given that someone could have a non-hd backup where some keys in the keypool have been used. For migration, it could make sense to take those as well, or document that they will not be taken?
  288. in src/wallet/interfaces.cpp:590 in cc3c3b9958 outdated 
    586@@ -587,7 +587,7 @@ class WalletLoaderImpl : public WalletLoader
587         m_context.scheduler = &scheduler;
588         return StartWallets(m_context);
589     }
590-    void flush() override { return FlushWallets(m_context); }
591+    void flush() override {}
    maflcko commented at 9:49 am on April 28, 2025:
    cc3c3b9958b31ddfc03905ed969c4da0a3bf4409: could probably fully remove this, given that it would be trivial to re-add, if ever needed.
  289. in src/wallet/wallet.cpp:238 in cc3c3b9958 outdated 
    234@@ -235,8 +235,7 @@ static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_
235 static void FlushAndDeleteWallet(CWallet* wallet)
236 {
237     const std::string name = wallet->GetName();
238-    wallet->WalletLogPrintf("Releasing wallet %s..\n", name);
239-    wallet->Flush();
240+    wallet->WalletLogPrintf("Releasing wallet\n");
    maflcko commented at 9:50 am on April 28, 2025:
    cc3c3b9958b31ddfc03905ed969c4da0a3bf4409: stray , unrelated change?
  290. maflcko approved
  291. maflcko commented at 9:53 am on April 28, 2025: member

    lgtm ACK bc3f07e384c2e145d6d14cfa3ad65b976233b538 💇

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK bc3f07e384c2e145d6d14cfa3ad65b976233b538 💇
3eF2eywtBy1XVEkBKNQpSjNzCFy6rQijB2u0oSJ5ekqHuSEbmLrtrlwP1Y1QQ9QHaog9QUeesH7zr4jfYiCYqAg==
  292. DrahtBot requested review from Sjors on Apr 28, 2025
  293. maflcko commented at 9:57 am on April 28, 2025: member

    2. In that same commit, the churn in wallet/scriptpubkeyman.cpp is hard to follow, both here on Github as well as a git show --color-moved=dimmed-zebra. I’m guessing you’re moving stuff around, deleting and modifying?

    You can use --patience. Maybe add this to the commit message?

  294. Sjors commented at 10:22 am on April 28, 2025: member
    @maflcko --patience did the trick. So does the newer histogram. It can be made default too if your computer is fast enough: git config --global diff.algorithm histogram


achow101 DrahtBot jess2505 laanwj Sjors murchandamus furszy pablomartin4btc fanquake maflcko


Sjors

Milestone
30.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-04-28 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me