DigiByte-Core / digibyte

DigiByte Core 7.17.3 - CURRENT (5-12-2021) - 8.22.0 Development
https://digibyte.org
MIT License
98 stars 61 forks source link

Stop creating bdb ( QT ) && Warn if the loaded wallet is legacy #227

Closed ghost closed 6 months ago

ghost commented 6 months ago

Stop creating new legacy-bdb wallets in Digibyte QT. * ( creating new legacy-bdb with rpc still works. )

Screenshot from 2024-04-03 17-55-18


Legacy wallets are being deprecated, warn if the loaded wallet is legacy

File >> Open Wallet

Screenshot from 2024-04-03 22-11-16


Legacy bdb vs New wallet SQLite.

Screencast from 2024-04-03 22-27-30.webm

JaredTate commented 6 months ago

I need to think on this one a bit more. This is just disabled in the GUI correct, not core protocol? I know BTC may be doing it, but from experience that's not always best. Platforms can be so slow to upgrade their system,

Thinking through possible issues, one would be if we roll this out and take it completely out it would not be backwards compatible for any exchange or service relying on legacy wallets. They may be inclined to de-list DGB vs rewriting their trading platform. Just a thought for now.

I agree it's good to push people towards descriptors, but not sure completely disabling it is necessary right now. Perhaps we should delay this till 2025 or 2026? Just my thoughts would love to hear others' perspectives on this. We also probably need to break down for people why descriptors are better.

Edit: One additional thing I realized today is descriptors might actually be causing some of our possible dandelion/mempool issues. Need to work through that.

Edit 2: Just re-read first line "creating new legacy-bdb with rpc still works". So we 100% need to keep that capability. Just thinking if any service might somehow rely on the GUI for something.

Edit 3: It may be useful to keep the GUI functionality for future ease of testing. If we for some reason need to quickly generate legacy wallets. Just thinking out loud.

ghost commented 6 months ago

I need to think on this one a bit more. This is just disabled in the GUI correct, not core protocol? I know BTC may be doing it, but from experience that's not always best. Platforms can be so slow to upgrade their system,

Correct this pr remove the option to create legacy wallets in the gui. I think if you look at their issue list and ours. That they are making a good choice here.

Thinking through possible issues, one would be if we roll this out and take it completely out it would not be backwards compatible for any exchange or service relying on legacy wallets. They may be inclined to de-list DGB vs rewriting their trading platform. Just a thought for now.

It stays fully backwards compatible. Only prevent new users from creating a legacy wallet. Not exchange or service. because they use rpc. But also for them it would be smart to upgrade.

I agree it's good to push people towards descriptors, but not sure completely disabling it is necessary right now. Perhaps we should delay this till 2025 or 2026? Just my thoughts would love to hear others' perspectives on this. We also probably need to break down for people why descriptors are better.

I think Digibyte and Bitcoin repo got enough issues to show why SQLite(2024) and not Berkeley DB 4.8.30 (2010)

https://github.com/orgs/DigiByte-Core/discussions/223

Edit: One additional thing I realized today is descriptors might actually be causing some of our possible dandelion/mempool issues. Need to work through that.

??

Edit 2: Just re-read first line "creating new legacy-bdb with rpc still works". So we 100% need to keep that capability. Just thinking if any service might somehow rely on the GUI for something.

If this is true. Don't know how. they still can use the legacy wallet. Yet it is still smart for them to upgrade.

Edit 3: It may be useful to keep the GUI functionality for future ease of testing. If we for some reason need to quickly generate legacy wallets. Just thinking out loud.

I think DigiByte 8 final is not for testing. We can easy create them ( if needed ) with rpc. And prevent normal users to make a legacy wallet ( Berkeley DB is not designed to be used as an application data file. The Legacy Wallet has several hacks to get round this as a result and Berkeley DB wallet files can easily be corrupted. )

saltedlolly commented 6 months ago

I think removing the ability to create legacy wallets in QT is a good thing, as long as it is still possible in the core protocol itself i.e. via RPC (which it is with this PR). This will prevent most users from creating legacy wallets, but keep the feature available to developers if needed. We obviously still want the ability to open existing legacy wallets, but to strongly discourage their use in future in favour of descriptor wallets.

ghost commented 6 months ago

I am closing this. We can reopen it in the future.

JaredTate commented 6 months ago

I am closing this. We can reopen it in the future.

Lol Jan I was just compiling this and waiting to run through all the tests to ACK it!

My final thoughts are there are many issues with legacy wallets, including:

Typically legacy wallet backups have consisted of solely the private keys, nowadays primarily in the form of BIP 39 mnemonics. However, this backup solution is insufficient, especially since the introduction of Segregated Witness which added new output types. Given just the private keys, it is not possible for restored wallets to know which kinds of output scripts and addresses to produce. This has led to incompatibilities between wallets when restoring a backup or exporting data for a watch-only wallet.

As long as rpc generation of legacy wallets is still possible for backward compatibility I think moving GUI forward without them is ok. We don't want to break server-side services... like for exchanges that will be resistant to change.

ghost commented 6 months ago

Haha it is open.