Proposed Timeline for Legacy Wallet and BDB removal

achow101 commented at 10:17 pm on October 15, 2020: member

What to review next: #30328

Here is a proposed timeline for the move from legacy wallets to descriptor wallets, and at the same time, bdb to sqlite, culminating with the removal of both legacy wallets and bdb. Even though the wallet type and the wallet storage format are orthogonal to each other, as noted in #19077, I would like to tie them together for the sake of simplicity when it comes to how we make new wallets and the migration of one type to another.

The 4 combinations of type and format are: legacy-bdb, legacy-sqlite, descriptor-bdb, and descriptor-sqlite. While all 4 types will be supported, we will primarily deal with legacy-bdb and descriptor-sqlite. Setting descriptors=True in createwallet (or checking the Descriptors option in the GUI) will always make a descriptor-sqlite. Setting that to False will always make a legacy-bdb.

0.21 (late 2020)
- Initial release of descriptor type and sqlite format. (#16528, #19077)
- Descriptor type is experimental with a warning as such (#16528)
- Only legacy-bdb and descriptor-sqlite will be created (#19077)
22.0 (mid 2021)
- Implement descriptor exports (#20226)
- Legacy type remains default (no pr needed)
- Introduce dump and createfromdump wallet tool commands with an additional -format option to specify the wallet type (#19137)
  - Users may use dump and then createfromdump in order to make legacy-sqlite from legacy-bdb
  - Users may use dump and then createfromdump in order to make descriptor-bdb from descriptor-sqlite
- If a wallet specified in conf or command line doesn’t exist, don’t make a wallet and instead warn (#15454)
23.0 (early 2022)
- Change descriptor type to non-experimental (#23002)
- Change createwallet descriptors default to True (#23002)
- Change wallet tool create -descriptors default to True (#23002)
24.0 (late 2022)
- Introduce migratewallet command to migrate a legacy-bdb to descriptor-sqlite (#19602)
- For newly created legacy-bdb wallets, warn that they are deprecated and will soon be unsupported (#24505)
26.0 (late 2023)
- When loading legacy-bdb, descriptor-bdb, or legacy-sqlite wallets, warn that the legacy type and bdb format will soon be unsupported, as applicable. (#27869)
- Deprecate creating new legacy-bdb wallets #28597
- migratewallet bug fixes
  - #28609
  - #28546
27.0 (early 2024)
- address book performance improvement #26836
- bulk deletion of txs performance improvement #28987
- bulk deletion of records performance improvement #29403
- Caching to improve performance of migrated non-HD descriptor wallets (also perf improvement in general) #26008
- Remove migratewallet experimental warning #28037
28.0 (late 2024)
- Improve performance of migratewallet #28574
- Reimplement legacy-bdb to descriptor-sqlite migration (maybe as a separate command or in the wallet tool) to be independent of LegacyScriptPubKeyMan and BDB (#26596)
- For dump, implement an independent BDB file loader (this should probably be shared with the previous) (#26606)
- GUI: Migrate wallets that are not loaded (https://github.com/bitcoin-core/gui/pull/824)
- Fix default wallet migration naming (#29586)
- Fix listwalletdir of migrated default wallets and generated backups (#30265)
29.0 (early 2025)
- Remove IsMine from migration (#30328)
- Rework wallet_migration.py (#31248)
- Add combinerawtransaction test that doesn’t need legacy wallet (#31249)
- Remove legacy wallet from migration benchmark (#31241)
- Stop loading legacy-bdb, descriptor-bdb, legacy-sqlite wallet entirely. Inform users how to migrate (#31250)
- Remove LegacyScriptPubKeyMan except for the independent migration stuff (#28710)
- Remove BDB except for independent loading for migration stuff (#28710)

Additional improvements that have no target deadline:

Backwards compatibility tests fixes for descriptor wallets #28027

The migration code will probably be around for a long time/forever.

Note that we expect users who go through the effort to make a legacy-sqlite or descriptor-bdb wallet to suck it up and deal with the consequences of doing something that isn’t really supported. Such users should be able to figure out for themselves how to migrate their wallets. (Maybe migratewallet can migrate legacy-sqlite to descriptor-sqlite. bdb to sqlite is way easier than legacy to descriptor).

achow101 added the label Feature on Oct 15, 2020

fanquake added the label Wallet on Oct 16, 2020

jonasschnelli commented at 3:36 pm on October 20, 2020: contributor

Look good except 0.27 (late 2023) (Stop loading legacy-bdb [...]). AFAIK this would be the first time one couldn’t load an historical wallet.dat file in Core (which is IMO a great feature). Disabling the possibility of loading wallets that have been created just two years ago seems to be too strict.

Since the wallet files are precious data sources and could lead to stress when failing to load, I suggest to continue support for BDB wallets indefinitely. Support could also mean an automatic migration to sqlite with an internal dependency free converter.

achow101 commented at 4:01 pm on October 20, 2020: member

I suggest to continue support for BDB wallets indefinitely. Support could also mean an automatic migration to sqlite with an internal dependency free converter.

This is almost what the last step is. There will be a dependency free migration tool that users can use to move legacy-bdb wallets to descriptor-sqlite. I would be hesitant to make this automatic though.

ChristopherA commented at 2:07 am on October 23, 2020: none

I’m hoping not for indefinite support of BDB wallets. It is easy enough to have an external tool do the conversion post 0.27.

maflcko commented at 1:35 pm on November 23, 2020: member

0.23 Introduce migratewallet command to migrate a legacy-bdb to descriptor-sqlite

Any reason this can’t be done in 22.0 already?

achow101 commented at 5:14 pm on November 23, 2020: member

0.23 Introduce migratewallet command to migrate a legacy-bdb to descriptor-sqlite

Any reason this can’t be done in 22.0 already?

No, I just don’t think it will make it.

S3RK commented at 12:09 pm on January 1, 2021: contributor

23.0 (late 2021)

Change createwallet descriptors default to True

I think it’s good to add wallet tool here as well to not forget to change default wallet type there as well

achow101 commented at 9:28 pm on October 11, 2021: member

Something that is not on this list, but is going to need to be done at some point prior to removing the legacy wallet is updating all of the unit tests and benchmarks to use descriptor wallets. Currently a bunch of them us a LegacyScriptPubKeyMan because it was convenient. Those will need to be changed.

fanquake commented at 10:32 pm on October 11, 2021: member

I’d suggest we move:

024.0 (late 2022)
1    For newly created legacy-bdb wallets, warn that they are deprecated and will soon be unsupported (not yet started)

into the 0.23 milestone, unless there is a particular reason to delay warning? I don’t think we need to wait an extra 6 months to start warning about this.

maflcko referenced this in commit 02feae54a5 on Oct 22, 2021

sidhujag referenced this in commit 47c21fec08 on Oct 22, 2021

jnewbery commented at 9:32 am on October 25, 2021: contributor

@achow101 do you mind if I update the description to include checkboxes so it’s easier to see progress at a glance:

I’d suggest we move:

24.0 (late 2022) For newly created legacy-bdb wallets, warn that they are deprecated and will soon be unsupported (not yet started)

into the 0.23 milestone,

I agree with this, and think that the other deprecation warning can be moved into 0.23:

When loading legacy-bdb, descriptor-bdb, or legacy-sqlite wallets, warn that the legacy type and bdb format will soon be unsupported, as applicable. (not yet started)

and then everything else bumped forward by one release.

The migration code will probably be around for a long time/forever.

I don’t agree with this. The migration path could be “Download Bitcoin Core 25.x (or whatever) and use the migration tool”.

maflcko commented at 9:38 am on October 25, 2021: member

I don’t agree with this. The migration path could be “Download Bitcoin Core 25.x (or whatever) and use the migration tool”.

This might cause issues when the user’s platform was unsupported as of 25.x and self-compiling old releases might not be possible either due to compiler bugs or stale dependencies.

achow101 commented at 4:12 pm on October 25, 2021: member

@jnewbery Go ahead.

I’m also fine with moving first deprecation warning into 23.0 and everything else forward by one release.

jnewbery commented at 4:17 pm on October 25, 2021: contributor

@jnewbery Go ahead.

I’m also fine with moving first deprecation warning into 23.0 and everything else forward by one release.

Done and done :+1:

achow101 commented at 1:09 pm on March 8, 2022: member

Missed a few things for 23.0, so everything else is going to be pushed back one release.

luke-jr commented at 6:40 pm on March 8, 2022: member

Concept NACK on removal. Users shouldn’t be forced to upgrade their wallets (especially not on such a short timeline).

fanquake commented at 8:48 am on March 14, 2022: member

Updated OP now that #24505 has been merged.

michaelfolkson commented at 7:58 pm on March 24, 2022: contributor

Concept ACK on latest updated timetable. I think it addresses @luke-jr timeline concerns and supporting the legacy wallet, BDB for the rest of Core’s existence doesn’t make sense to me.

(Of course it just signals an intention rather than a rigid timetable that is guaranteed to be followed.)

luke-jr commented at 8:31 pm on March 24, 2022: member

2024 is very soon. Keep in mind we have literally never removed support for older wallets before.

Sjors commented at 8:10 am on May 20, 2022: member

I’ve noticed at least two projects now that tripped over the default change for new wallets. Although it’s easy to work around, if they rely on dependencies (e.g. https://github.com/bitcoindevkit/bdk/issues/598#issuecomment-1131250261) it can get tricky. Maybe we should add a wallet_create_legacy_default config option (with a clear warning that it will get deprecated).

Rspigler commented at 0:04 am on August 17, 2022: contributor

I too am concerned about how quickly we would be losing support for older wallets for the first time (less than two years from when the first warning appears about legacy/BDB being deprecated; 24.0-27.0). I think this should at least be pushed back to 28.0, but perhaps even more.

I know the PR implementing the migration isn’t finished yet, but I think the warning given and the documentation around it will be very important as well.

luke-jr commented at 8:03 pm on September 4, 2022: member

In light of descriptor wallets not having an upgrade path for non-HD wallets, I don’t think legacy wallet support can be reasonably removed.

sipa commented at 10:29 pm on September 12, 2022: member

I find the proposed timeline in general somewhat aggressive here, and would prefer to let old wallets to just keep working for a few years longer. Historically speaking, we’ve never dropped support for an old wallet format, and how frequently I see reports of people trying to access old wallets, I fear this will worsen it.

In light of descriptor wallets not having an upgrade path for non-HD wallets, I don’t think legacy wallet support can be reasonably removed.

Why specifically do you see that as a problem? You already need regular backups of non-HD wallets anyway, needing a one-time new backup after migration to HD doesn’t seem too big of a deal to me. As said above, I do prefer a few more years before forcing people to migrate, but if one accepts that a migration is going to happen, I don’t see the problem with restricting it to use HD key derivation. If one is concerned about the specifics of BIP32’s unhardened derivation, it should be possible to choose hardened derivation descriptors instead, or by then some other deterministic key derivation mechanism may be supported by descriptors.

luke-jr commented at 0:57 am on September 13, 2022: member

Why specifically do you see that as a problem?

Mainly performance issues (a SPKM for every existing key) and lack of builtin seed rotation.

But it’s also a security change, which users probably shouldn’t be forced into. Even hardened keys are endangered by a leaked seed.

S3RK commented at 7:56 am on November 16, 2022: contributor

I propose to also include #26511 As part of the timeline

michaelfolkson commented at 10:27 am on November 28, 2022: contributor

The timeline needs updating for what was left out of 24.0 and what will be included in 25.0 (obviously no rush)

ghost commented at 11:45 am on November 28, 2022: none

I find the proposed timeline in general somewhat aggressive here, and would prefer to let old wallets to just keep working for a few years longer. Historically speaking, we’ve never dropped support for an old wallet format, and how frequently I see reports of people trying to access old wallets, I fear this will worsen it.

Agree. Also everything doesn’t work with descriptor wallets.

Example: I have used legacy wallets in this proof of concept which will be used to build a p2p exchange

achow101 commented at 3:56 pm on November 28, 2022: member

The timeline needs updating for what was left out of 24.0 and what will be included in 25.0 (obviously no rush)

It was updated a while ago, and should be up to date.

Example: I have used legacy wallets in this proof of concept which will be used to build a p2p exchange

There’s nothing in that example that I can see that requires the use of legacy wallets. I can see why it doesn’t currently work with descriptor wallets, but that’s easily solved by having a watchonly wallet contain the multisig descriptor that processes the psbt first so that the multisig keys are actually included in the psbt before it goes to the signers.

ghost commented at 4:46 pm on November 28, 2022: none

There’s nothing in that example that I can see that requires the use of legacy wallets. I can see why it doesn’t currently work with descriptor wallets, but that’s easily solved by having a watchonly wallet contain the multisig descriptor that processes the psbt first so that the multisig keys are actually included in the psbt before it goes to the signers.

Sorry I do not understand this and legacy wallet UX makes it easy for buyer and seller. Please let me know if I am doing something wrong:

https://github.com/1440000bytes/p2p/issues/1

Alice has a wallet with some private keys using for some bitcoin tx Bob has a wallet with some private keys using for some bitcoin tx

We do not want users to create another wallet to use the exchange.

michaelfolkson commented at 11:06 am on November 29, 2022: contributor

@1440000bytes: Please ask on StackExchange or IRC. This issue is for tracking a timeline for the Core wallet, it is hard to do that if the discussion wanders off to other topics. I am already discussing this with you on the linked issue in your repo.

Sjors commented at 3:10 pm on November 29, 2022: member

#26596 (draft) allows users to migrate a legacy wallet without the need for BDB. There are some (corruption) scenario’s it can’t handle. For that users would have to download an old version of Bitcoin Core.

Recent versions contain a separate bitcoin-wallet binary, which means they don’t have to run a potentially outdated Bitcoin Core node just to migrate their wallet. We’d have to make sure that standalone tool is feature complete enough. I don’t have much experience with fixing corrupt wallets. See #13044 for things that might be missing (e.g. only available as an bitcoind RPC call or startup argument).

In addition we’d need a way to migrate a non-hd legacy wallet (see #20160 (comment)). IIUC in particular we need it to not generate a fresh HD seed in that case. And maybe benchmark the performance to see if the use of individual SKPM’s is acceptable, and if not, design a new one that can have multiple keys. Such ancient wallets may be a very small number of users, but probably not very small amounts of BTC. It makes sense to make any migration for them as limited as possible (they can create a fresh wallet and move coins over when they feel comfortable). In particular this deprecation should not be a barrier to upgrade their node.

Having a read-only BDB wallet (which #26596 enables) could be useful for such users too. It lets them see the coins are still there.

That removes any objections I have about dropping BDB from the codebase and dependencies entirely. However, not too soon. We can probably wait until the ancient 2010 code becomes a problem for (using) modern compilers. As long as building from source and running (most of) the test suite doesn’t require BDB, it’s not a huge burden to keep it around a while longer.

ghost commented at 4:39 pm on November 29, 2022: none

@1440000bytes: Please ask on StackExchange or IRC. This issue is for tracking a timeline for the Core wallet, it is hard to do that if the discussion wanders off to other topics. I am already discussing this with you on the linked issue in your repo.

I was thinking about asking a question on stackexchange although I changed my mind and would instead ACK/NACK related pull requests with reasons when I need to.

Thanks for letting me know what this issue is about though.

maflcko commented at 3:52 pm on April 28, 2023: member

I think the milestones will need to be edited in OP to be moved back by one release?

fanquake commented at 1:38 pm on May 1, 2023: member

I think the milestones will need to be edited in OP to be moved back by one release?

Moved all the remaining milestones back by a release.

fanquake commented at 9:52 am on July 6, 2023: member

Now that #27869 has been merged. It might be time to no-longer mark the migratewallet RPC as “EXPERIMENTAL”.

maflcko commented at 10:04 am on July 6, 2023: member

@fanquake Does it not need the performance fix for bdb wallets with a lot of keys first?

fanquake commented at 10:07 am on July 6, 2023: member

@fanquake Does it not need the performance fix for bdb wallets with a lot of keys first?

I’m not sure. However if we are going to ship this warning that instructs users to use migratewallet, I don’t think we can leave it marked as experimental.

maflcko added this to the milestone 26.0 on Jul 6, 2023

maflcko removed this from the milestone 26.0 on Sep 5, 2023

maflcko added this to the milestone 27.0 on Sep 5, 2023

maflcko removed this from the milestone 27.0 on Sep 5, 2023

achow101 commented at 9:03 pm on February 7, 2024: member

Seems unlikely that the bdb parser stuff will make it into 27.0, so pushed those back another release.

bitcoin deleted a comment on Mar 20, 2024

brunoerg commented at 12:57 pm on March 25, 2024: contributor

fyi: #29694 (adds a fuzz target for spkm migration)

vijaydasmp referenced this in commit f2404a0349 on Sep 6, 2024

vijaydasmp referenced this in commit f5153b1659 on Sep 7, 2024

vijaydasmp referenced this in commit 79ad739d56 on Sep 10, 2024

vijaydasmp referenced this in commit 721d525dd8 on Sep 10, 2024

vijaydasmp referenced this in commit 98e885f727 on Sep 10, 2024

vijaydasmp referenced this in commit bd221adb05 on Sep 10, 2024

vijaydasmp referenced this in commit a8ecd750db on Sep 11, 2024

vijaydasmp referenced this in commit 56cfafa9d6 on Sep 14, 2024

vijaydasmp referenced this in commit 0cb3d6bbf0 on Sep 14, 2024

vijaydasmp referenced this in commit 3a404270fa on Oct 5, 2024

vijaydasmp referenced this in commit 9a71755f33 on Oct 5, 2024

vijaydasmp referenced this in commit fceb8e3e14 on Oct 17, 2024

Sjors commented at 1:12 pm on November 16, 2024: member

I wrote two years ago:

However, not too soon.

Ok, now is fine :-)

It seems #30328 and #31248 is next on the review list and then actual removal in #28710?

Proposed Timeline for Legacy Wallet and BDB removal #20160