refactor: Removals after bdb removal #32438

pull maflcko wants to merge 8 commits into bitcoin:master from maflcko:2505-bdb-remove changing 20 files +6 −110
  1. maflcko commented at 3:29 pm on May 7, 2025: member
    This deletes some dead code
  2. DrahtBot commented at 3:29 pm on May 7, 2025: 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/32438.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK Sjors, rkrux
    Approach ACK w0xlt

    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:

    • #32345 (ipc: Handle unclean shutdowns better by ryanofsky)
    • #30221 (wallet: Ensure best block matches wallet scan state 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. DrahtBot added the label Refactoring on May 7, 2025
  4. maflcko force-pushed on May 7, 2025
  5. maflcko commented at 3:48 pm on May 7, 2025: member

    There are still some potential follow-ups for someone else:

    0git grep  -i 'legacy wallet' src/bitcoin-wallet.cpp src/wallet src/script/

    Also, (labelWatch* and fHaveWatch*) for the gui people.

  6. maflcko force-pushed on May 7, 2025
  8. w0xlt commented at 6:07 pm on May 7, 2025: contributor
    Approach ACK
  9. in test/functional/wallet_migration.py:43 in fadaa8baaf outdated 
    39@@ -40,7 +40,6 @@ def set_test_params(self):
40         self.setup_clean_chain = True
41         self.num_nodes = 2
42         self.supports_cli = False
43-        self.extra_args = [[], ["-deprecatedrpc=create_bdb"]]
    achow101 commented at 6:49 pm on May 7, 2025:

    In fadaa8baafc27fbe29721300569e58777643bb1f “test: remove unused extra_args”

    These args are used as the 28.0 node used in this test needs them to make the legacy wallets for testing.

    rkrux commented at 11:09 am on May 8, 2025:

    Indeed, the second node here is v28 and the wallet_migration test fails in the CI with this error:

    0test_framework.authproxy.JSONRPCException: 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. (-4)
    maflcko commented at 1:09 pm on May 9, 2025:
    I guess it could make sense to use a previous release where bdb wasn’t yet deprecated, but not sure if this is worth it, and it could be a follow-up. Removed this commit for now, thanks.
  10. achow101 referenced this in commit 1b1b9f32cf on May 7, 2025
  11. DrahtBot added the label CI failed on May 7, 2025
  12. laanwj added this to the milestone 30.0 on May 8, 2025
  13. vcpkg: Remove bdb fa5f3e62c8
  14. remove dead flush() 
    It is confusing that the chain client flush happens between
StopHTTPServer and StopMapPort. Also, it is unused code. Seems best to
just add it back properly when it is needed again.
    fa62a013a5
  15. remove NotifyWatchonlyChanged 
    The signal is never called.
    ffff949472
  16. Remove unused LegacyDataSPKM::DeleteRecords() fa7e5c15a7
  17. doc: fix typo in abortrescan rpc eeeef88d46
  18. doc: Remove note about bdb wallets fab5e2a094
  19. Remove unused IsSingleKey fa2125e7b8
  20. Remove create options from wallet tool 
    Just create descriptor wallets.
    fa061bfcdb
  21. maflcko force-pushed on May 9, 2025
  22. Sjors commented at 1:26 pm on May 9, 2025: member

    ACK fa061bfcdb0caea240fd15bcc309e7847132a4ff if CI is also happy

    I didn’t attempt a vcpkg build (fa5f3e62c8801cca80997cfb046c13983e0876e7), but I assume the Windows CI does that?

    I didn’t check for additional things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.

  23. DrahtBot requested review from w0xlt on May 9, 2025
  24. maflcko commented at 1:29 pm on May 9, 2025: member

    I didn’t check for additional things that could be removed. Though there is bunch of watch-only related code in the GUI that can be dropped.

    Yes, see #32438 (comment)

  25. rkrux commented at 2:34 pm on May 9, 2025: contributor

    Makes sense (and prefer) to remove the dead code right after the removal PR is merged, helpful for future readers of the codebase as it reduces the amount of code to navigate & process. I have not checked the history around the watch-only stuff but rest of the diff makes sense to me. Build and tests pass on my system.

    utACK fa061bf

  26. DrahtBot removed the label CI failed on May 9, 2025
  27. fanquake merged this on May 9, 2025
  28. fanquake closed this on May 9, 2025
  29. maflcko deleted the branch on May 12, 2025
  30. maflcko commented at 7:46 am on May 13, 2025: member

    The final possible bdb removals can be found via git grep -i 'legacy wallet' src/bitcoin-wallet.cpp src/wallet src/script/.

    Some may be fine to keep as historic comment and others are not a clean removal, some may need to replaced by a Assert/throw or util::Error.

    In any case, if the untested code isn’t removed, it should be tested:

    The upgradewallet RPC is currently completely untested.


maflcko DrahtBot pablomartin4btc w0xlt achow101 rkrux Sjors


w0xlt

Labels
Refactoring

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-05-30 00:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me