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

bitcoin core v23 creates descriptor wallets by default #1192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LiranCohen
Copy link
Member

Bitcoin Core v23 has descriptor wallets as the default for the createwallet RPC.

Since SideTree isn't currently supporting descriptor wallets, the easiest path forward is to set descriptors parameter to false.

Legacy wallets require Berkeley DB in order to work. I usually build from source and include BDB so I am leaving this in draft mode until I can test against the built release binaries.

I will do some more testing and update the ION Installation documentation to reflect the requirement of Berkeley DB, as well as potentially update the provided script to pull in v23 of bitcoin core binary.

@LiranCohen
Copy link
Member Author

Thanks @coffnix!

So yeah it seems like we would need to migrate away from legacy wallets before v26 (late 2023).

Setting createwallet descriptor parameter to false should be fine for the time being, but I'll see if I can start working on a separate PR for supporting descriptor wallets within SideTree/ION.

@LiranCohen
Copy link
Member Author

I've done some more testing and the binary comes compiled with BDB which would allow for descriptors to be set to false and work.

However this change is not backward compatible with anything before v0.21.0 when the descriptors option was added.

Do we want to force a newer release of ION to be only compatible with v0.21.0+ or do we want to check for a version and based on that include the descriptors parameter in the walletcreate rpc?

I will also make an update to the bitcoin core installation script that is provided within the ION Installation guide to check that it installs bitcoin core v23 correctly.

Opening this PR for review and discussion.

@LiranCohen LiranCohen marked this pull request as ready for review June 20, 2022 19:54
@coffnix
Copy link

coffnix commented Jun 20, 2022

if possible we could already make the new version compatible with Sqlite

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

Not sure its possible to do so given the current project structure, but this seems like a PR for the ION repo, not Sidetree.

@LiranCohen
Copy link
Member Author

if possible we could already make the new version compatible with Sqlite

That would take modifying to support descriptor wallets. I've put together a placeholder Issue for researching what it would take and tracking any progress:
decentralized-identity/ion#304

Not sure its possible to do so given the current project structure, but this seems like a PR for the ION repo, not Sidetree.

@OR13 The code for the bitcoin core RPCs unfortunately live within the Sidetree repo, I was surprised to find that myself when debugging this. That could be also a separate Issue/PR to try and accomplish.

@thehenrytsai
Copy link
Collaborator

Just providing an update that I have not forgotten about this PR. I'll be looking into this soon, as soon as I get the improvement to docker support work done!

@thehenrytsai
Copy link
Collaborator

@LiranCohen, my ION docker PR finally got merged in. I will be therefore finally be looking into testing this PR out in the next few days!

I am in the camp that it is entirely fine to break compatibility for any pre v23.0 versions as it can act as a forcing function to not use a to-be-obsolete version anyway. But based on what you said, your change is compatible with v21+, so it could be as simple as:

  1. updating the docker image to use v23; and
  2. initialize the DB using legacy Berkley DB

But your insights in decentralized-identity/ion#275 tells me it might be way more complicated that that. Will report my progress here.

@LiranCohen
Copy link
Member Author

@thehenrytsai Thanks!

I think it should be pretty straight forward like you mention, let me know if you need any help testing or are having any issues.

@decentralgabe
Copy link
Member

this should be moved to the new impl repo https://github.com/decentralized-identity/sidetree-reference-impl

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.

5 participants