-
Notifications
You must be signed in to change notification settings - Fork 22
chore: migrate the watch-only Bitcoin Core wallet to use descriptors #177
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
base: master
Are you sure you want to change the base?
Conversation
218d892 to
1a9f400
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates JoinMarket to commit ce32baf and modernizes the installation infrastructure to support Python 3.12/3.13 and Bitcoin Core's descriptor wallets instead of legacy BDB wallets.
Changes:
- Updated JoinMarket from tagged release v0.9.11 to commit ce32baf with enhanced verification logic for commit-based installations
- Extended Python version support to include 3.12 and 3.13, prioritizing newer versions
- Migrated Bitcoin Core wallet configuration from legacy
wallet.datto descriptor-basedwatch-only-descriptor-wallet
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.joinmarket.sh | Supports commit-based JoinMarket installations with PGP verification via centralized verify.git.sh |
| scripts/menu.update.sh | Displays commit hash when no tag is available |
| build_joininbox.sh | Adds Python 3.12/3.13 detection prioritizing newer versions |
| scripts/_functions.bitcoincore.sh | Updates wallet references and removes deprecated BDB configuration checks |
| scripts/_functions.sh | Updates wallet configuration and adds dialog terminal compatibility fixes |
| scripts/install.bitcoincore.sh | Creates descriptor wallets for signet with proper parameters |
| scripts/standalone/_functions.standalone.sh | Updates wallet path check and creation to use descriptor wallets |
| scripts/menu.wallet.sh | Updates user-facing message about watch-only wallets |
| ci/amd64/debian/build.amd64-debian.pkr.hcl | New HCL-format Packer configuration with UEFI support and Debian 13.3.0 |
| ci/amd64/packer.build.amd64-debian.sh | Modernized to use packer init and new HCL configuration |
| ci/amd64/debian/http/preseed.cfg | New preseed configuration for Debian 13 |
| prepare_remote_node.md | Fixed typo: "numbes" → "numbers" |
| scripts/.dialogrc | Changed border color from black to cyan |
| .editorconfig | Added new editor configuration file |
| release_notes.md, README.md, FAQ.md | Updated documentation references to descriptor wallets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
openoms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Summary
This is a well-structured PR that modernizes JoininBox's infrastructure with several important updates. The changes are logically grouped and maintain backward compatibility where needed.
Strengths
1. JoinMarket Commit-Based Installation
- Good approach to support both tagged releases and commit hashes
- The verification logic correctly differentiates between tags (starting with
v) and commit hashes - Reusing
verify.git.shfor signature verification reduces code duplication
2. Descriptor Wallet Migration
- Properly switches from legacy BDB wallets to descriptor wallets with
disable_private_keys=true - Correctly removes the
deprecatedrpc=create_bdbworkaround that's no longer needed - Consistent naming convention (
watch-only-descriptor-wallet) across all scripts and docs
3. Python Version Support
- Prioritizing newer Python versions (3.13 > 3.12 > ...) is the right approach for forward compatibility
- Maintains fallback to older versions for compatibility with existing systems
4. CI/Build Infrastructure
- Migrating from JSON to HCL format for Packer is a good modernization
- UEFI boot support alongside legacy BIOS is well implemented
- The
packer init -upgradeapproach for plugin management is cleaner
5. Code Quality Improvements
.editorconfigaddition helps maintain consistent code style- Dialog fixes for unknown terminal types (xterm-ghostty fallback) improve robustness
NCURSES_NO_UTF8_ACS=1export fixes box drawing character issues
Minor Observations
-
_functions.standalone.sh:293: The wallet directory check path changed from/home/bitcoin/.bitcoin/mainnet/wallets/to/home/bitcoin/.bitcoin/wallets/- verify this is intentional for the mainnet case. -
install.joinmarket.sh: ThetestedJMversionis commented out whiletestedJMcommitis active. This is correct for using the commit, but consider adding a comment explaining when to switch back to a tagged release. -
Preseed references: The new preseed.cfg references
httpredir.debian.orgwhich is the modern redirect service - good choice over the oldftp.de.debian.org.
Testing Recommendations
Before merging, consider testing:
- Fresh installation on Debian 13 with Python 3.12/3.13
- Wallet creation with the new descriptor wallet format
- Signet mode activation/deactivation
- Packer build with UEFI boot mode
- Dialog display with various terminal types
Overall, this is a solid update that prepares JoininBox for newer Python versions, modern Bitcoin Core wallet formats, and updated Debian releases. Looks good!
f8e1d31 to
e3f70d4
Compare
30db791 to
5ef2e78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
scripts/install.joinmarket.sh:135
- Duplicate code block: Lines 124-129 are identical to lines 130-135. The version variable initialization and GitHub release check are being performed twice with the same logic. Remove the duplicate block.
# Use tag if set, otherwise use commit hash
: "${version:=${testedJMversion:-$testedJMcommit}}"
# Only check GitHub releases if version looks like a tag (starts with 'v')
if [[ "${version}" == v* ]]; then
curl -s "https://github.com/JoinMarket-Org/joinmarket-clientserver/release/tag/${version}" | grep -q "\"message\": \"Version not found\"" && error_msg "'There is no: https://github.com/JoinMarket-Org/joinmarket-clientserver/release/tag/${version}'"
fi
# Use tag if set, otherwise use commit hash
: "${version:=${testedJMversion:-$testedJMcommit}}"
# Only check GitHub releases if version looks like a tag (starts with 'v')
if [[ "${version}" == v* ]]; then
curl -s "https://github.com/JoinMarket-Org/joinmarket-clientserver/release/tag/${version}" | grep -q "\"message\": \"Version not found\"" && error_msg "'There is no: https://github.com/JoinMarket-Org/joinmarket-clientserver/release/tag/${version}'"
fi
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5ef2e78 to
67b0eac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bitcoin Core Descriptor Wallet Migration
wallet.datwithwatch-only-descriptor-walletusingdescriptors=trueanddisable_private_keys=truedeprecatedrpc=create_bdbconfiguration check since BDB wallets are no longer createdcheckWalletMigrationfunction that detects existingwallet.datand guides users through the migration process with step-by-step instructionswalletMigrationDone=trueinjoinin.confto show migration notice only oncewallet.dattowatch-only-descriptor-walletacross:_functions.bitcoincore.sh_functions.sh_functions.standalone.shinstall.bitcoincore.shCI/Build Infrastructure
joininbox-amd64-debian.jsonwithbuild.amd64-debian.pkr.hclovmfpackage to enable UEFI boot in QEMUpacker init: Build script now usespacker init -upgradefor automatic plugin managementDocumentation
Migration Guide for Existing Users
Existing users with the legacy
wallet.datwill see a migration notice on first run after updating. The steps are:WALLET->DISPLAYto import addresses into the new descriptor walletWALLET->RESCANwith blockheight481824(or earlier if needed)See the updated FAQ for detailed instructions.