Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from Apr 5, 2024
Merged

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

merged 6 commits into from Apr 5, 2024

Conversation

ghost
Copy link

@ghost ghost commented Apr 3, 2024

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

  • Bitcoin 26.0 (late 2023) Deprecate creating new legacy-bdb wallets
  • Bitcoin 29.0 (early 2025) Stop loading legacy-bdb, descriptor-bdb.

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

@ghost ghost changed the title Stop creating new legacy-bdb wallets in QT Stop creating new bdb wallets in QT Apr 3, 2024
@ghost ghost changed the title Stop creating new bdb wallets in QT Stop creating bdb ( QT ) && Warn if the loaded wallet is legacy Apr 3, 2024
@JaredTate
Copy link

JaredTate commented Apr 4, 2024

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
Copy link
Author

ghost commented Apr 4, 2024

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
Copy link

saltedlolly commented Apr 4, 2024

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
Copy link
Author

ghost commented Apr 5, 2024

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

@ghost ghost closed this Apr 5, 2024
@JaredTate
Copy link

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:

  • Berkley DB based, stuck on a 14-year-old piece of c++ database software no longer maintained or able to upgrade
  • issues with legacy wallet backups being voided after 100 new private keys are generated. non-detemrinistic (HD)
  • Watch only wallet issues with legacy wallets
  • Legacy wallet backups can't determine output scripts... like with segwit txs

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 ghost reopened this Apr 5, 2024
@ghost
Copy link
Author

ghost commented Apr 5, 2024

Haha it is open.

Copy link

@JaredTate JaredTate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

Copy link
Member

@ycagel ycagel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK.

Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@gto90 gto90 merged commit 99ed14a into DigiByte-Core:develop Apr 5, 2024
@ghost ghost deleted the descriptor77 branch April 5, 2024 23:00
gto90 added a commit that referenced this pull request Dec 15, 2024
- Removed the unused descriptor checkbox from the wallet creation dialog.

This change cleans up the UI and resolves compiler errors due to missing the `descriptor_checkbox` member by removing an unused element, improving the user experience.

References:
- #213
- #227
- #239 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants