util: Add “importfromcoldcard” command to bitcoin-wallet tool #23362

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:211025-cc changing 3 files +80 −4

hebasto commented at 11:27 am on October 26, 2021: member
Coldcard firmware v4.1.3+ supports wallet export via descriptors:
- https://github.com/Coldcard/firmware/commit/3151369e67951f68e86ba993ef1e7dfaaa39c13f
- Using Coldcard with Bitcoin Core
This PR adds a new command importfromcoldcard for the bitcoin-wallet tool which creates a new wallet and fills it with the provided descriptors.

Usage example:
0$ src/bitcoin-wallet -wallet=MyNewShinyCC -dumpfile=/home/hebasto/Coldcard/bitcoin-core.txt importfromcoldcard
To point to the “bitcoin-core.txt” file, the -dumpfile option is re-used.

Also the created wallet is forced for rescanning, to guarantee the access to all transactions even for wallets that were previously used some time before being imported into Bitcoin Core.

TODO (not here):
- add tests for the importfromcoldcard command to the tool_wallet.py functional test
- add the same functionality to the GUI
Based on bitcoin/bitcoin#23349.
util: Use FEATURE_LATEST for wallets created with bitcoin-wallet cf60e3a0d3
hebasto added the label Wallet on Oct 26, 2021
hebasto added the label Utils/log/libs on Oct 26, 2021
hebasto commented at 11:31 am on October 26, 2021: member

cc @achow101 @Sjors @meshcollider @doc-hex @nvk
util: Add "importfromcoldcard" command to bitcoin-wallet tool
Coldcard firmware v4.1.3+ supports wallet export via descriptors.
8076f8d4c2
hebasto force-pushed on Oct 26, 2021
doc-hex commented at 1:09 pm on October 26, 2021: none
- looks useful and simple enough
nvk commented at 1:12 pm on October 26, 2021: none

@hebasto thanks for this, nice to see HW <=> Core getting easier.
jamesob commented at 9:06 pm on October 26, 2021: member

Concept ACK
in src/wallet/wallettool.cpp:132 in 8076f8d4c2
127+ return false; 128+ } 129+ 130+ std::string line; 131+ while (std::getline(file, line)) { 132+ if (line.substr(0, 22) == "importdescriptors \'[{\"") break;
mjdietzx commented at 10:34 pm on October 26, 2021:

What are your thoughts on using regex from the standard library for this? https://www.cplusplus.com/reference/regex/

As someone not familiar w/ coldcard files, it would be much easier for me to understand this (ie the hardcoded string positions here, and at L138). It could possibly simplify this a bit and make it easier to read if we pulled the descriptor from the file w/ a regex
mjdietzx commented at 10:39 pm on October 26, 2021: contributor

Very cool‼️ One comment/question
achow101 commented at 10:53 pm on October 26, 2021: member

-0 on adding device specific code into Core.
Sjors commented at 9:57 am on October 27, 2021: member

The Coldcard produces a command that the user can copy-paste into the console to perform the import. This PR parses the text file with that command and does the import instead. That seems strange.

I would prefer to see something a bit more generic. E.g. ColdCard and other devices could produce a text file with just descriptors in it, and/or some other wallet metadata. And then we could import that.
fanquake commented at 2:31 pm on October 27, 2021: member

-0 on adding device specific code into Core. I would prefer to see something a bit more generic

I agree. Wouldn’t the HWI or a similar project be a better place for this? I don’t really think we should be adding vendor specific code/calls into Bitcoin Core utilities. Or, if it is going to be done, it should be done in some very generic way, otherwise we’re likely going to just end up with a bunch of importfrom* calls, and more interfaces / backwards compat / supported devices to have to worry about.
jamesob commented at 2:33 pm on October 27, 2021: member

I would prefer to see something a bit more generic. E.g. ColdCard and other devices could produce a text file with just descriptors in it, and/or some other wallet metadata. And then we could import that.

In hindsight, coldcard fan though I may be, I think @Sjors and @fanquake are right here. Burdening Core with parsing a potentially-unlimited number of vendor-specific data formats doesn’t seem right.
nvk commented at 2:39 pm on October 27, 2021: none

Selfishly all I want is easy with my preferred tool. But yes, a more vendor neutral approach makes sense. It seems that supporting core is not priority for most vendors, so I really have no good suggestions. Maybe the patch could be generalized to import a series of any commands and/or we could change CC to export comments (#) on all the other lines in the file, etc…
hebasto commented at 3:17 pm on October 27, 2021: member

Wouldn’t the HWI or a similar project be a better place for this?

No. It’s practically infeasible for a non-tech-savvy person to create a safe setup of “Bitcoin Core + python HWI” on a new Windows laptop. I tested this scenario many times IRL with different users ((

Also we should not recommend to such a type of users to use Bitcoin Core GUI console at all.

I would prefer to see something a bit more generic. E.g. ColdCard and other devices could produce a text file with just descriptors in it, and/or some other wallet metadata. And then we could import that.

Tend to agree.
luke-jr changes_requested
luke-jr commented at 5:05 pm on October 29, 2021: member

Suggest making a BIP for the format, and renaming this to something vendor-neutral.
lsilva01 commented at 9:39 pm on October 30, 2021: contributor

I agree.

Suggest making a BIP for the format, and renaming this to something vendor-neutral.
DrahtBot commented at 9:51 pm on November 1, 2021: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
laanwj commented at 6:02 pm on November 8, 2021: member

-0 on adding device specific code into Core.

Yes. I’m all for making the flows easy to use with hardware wallets, but supporting specific hardware seems to go a step too far. Also, we don’t have the test framework (e.g. device simulation) set up for use-cases like this.

No. It’s practically infeasible for a non-tech-savvy person to create a safe setup of “Bitcoin Core + python HWI” on a new Windows laptop. I tested this scenario many times IRL with different users ((

So then we need to make that process easier. We could even ship the distribution with HWI. This cannot be an argument for absorbing HWI’s functionality into this repository.
meshcollider commented at 2:32 am on November 22, 2021: contributor

Absolutely agree with the above comments, its definitely an excellent goal to improve integration with hardware wallets but not at the expense of burdening this repository with a bunch of case handling. Look forward to a vendor-independent proposal 👍
DrahtBot commented at 8:37 am on December 3, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
DrahtBot added the label Needs rebase on Dec 3, 2021
fanquake commented at 8:52 am on December 27, 2021: member

What’s the state of this PR? From the discussion the current approach is not going to be merged, so I think this should either be closed, or at least drafted, until the approach is changed.
hebasto marked this as a draft on Dec 27, 2021
hebasto commented at 8:56 am on December 27, 2021: member

What’s the state of this PR? From the discussion the current approach is not going to be merged, so I think this should either be closed, or at least drafted, until the approach is changed.

Drafted.
hebasto closed this on Mar 24, 2022
luke-jr referenced this in commit 2839a4c1e5 on May 21, 2022
DrahtBot locked this on Mar 24, 2023

Contributors
hebasto doc-hex nvk jamesob mjdietzx achow101 Sjors fanquake luke-jr lsilva01 DrahtBot laanwj meshcollider

Labels
Wallet Utils/log/libs Needs rebase

util: Add “importfromcoldcard” command to bitcoin-wallet tool #23362

Conflicts