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

hebasto · 2021-10-26T11:27:22Z

Coldcard firmware v4.1.3+ supports wallet export via descriptors:

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:

$ 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 #23349.

hebasto · 2021-10-26T11:31:57Z

cc @achow101 @Sjors @meshcollider @doc-hex @nvk

Coldcard firmware v4.1.3+ supports wallet export via descriptors.

doc-hex · 2021-10-26T13:09:20Z

looks useful and simple enough

nvk · 2021-10-26T13:12:15Z

@hebasto thanks for this, nice to see HW <=> Core getting easier.

jamesob · 2021-10-26T21:06:09Z

Concept ACK

mjdietzx

Very cool‼️ One comment/question

mjdietzx · 2021-10-26T22:34:58Z

src/wallet/wallettool.cpp

+
+    std::string line;
+    while (std::getline(file, line)) {
+        if (line.substr(0, 22) == "importdescriptors \'[{\"") break;


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

achow101 · 2021-10-26T22:53:06Z

-0 on adding device specific code into Core.

Sjors · 2021-10-27T09:57:38Z

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 · 2021-10-27T14:31:57Z

-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 · 2021-10-27T14:33:58Z

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 · 2021-10-27T14:39:17Z

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 · 2021-10-27T15:17:18Z

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

Suggest making a BIP for the format, and renaming this to something vendor-neutral.

lsilva01 · 2021-10-30T21:39:30Z

I agree.

Suggest making a BIP for the format, and renaming this to something vendor-neutral.

DrahtBot · 2021-11-01T21:51:02Z

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 · 2021-11-08T18:02:30Z

-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 · 2021-11-22T02:32:43Z

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 · 2021-12-03T08:37:44Z

🐙 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".}

fanquake · 2021-12-27T08:52:15Z

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 · 2021-12-27T08:56:44Z

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.

Coldcard firmware v4.1.3+ supports wallet export via descriptors. Github-Pull: bitcoin#23362 Rebased-From: 8076f8d

util: Use FEATURE_LATEST for wallets created with bitcoin-wallet

cf60e3a

hebasto added Wallet Utils/log/libs labels Oct 26, 2021

util: Add "importfromcoldcard" command to bitcoin-wallet tool

8076f8d

Coldcard firmware v4.1.3+ supports wallet export via descriptors.

hebasto force-pushed the 211025-cc branch from b2fb479 to 8076f8d Compare October 26, 2021 11:38

mjdietzx reviewed Oct 26, 2021

View reviewed changes

luke-jr suggested changes Oct 29, 2021

View reviewed changes

This was referenced Nov 2, 2021

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

DrahtBot mentioned this pull request Nov 13, 2021

Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces #23497

Merged

DrahtBot added the Needs rebase label Dec 3, 2021

hebasto marked this pull request as draft December 27, 2021 08:56

Sjors mentioned this pull request Jan 14, 2022

build: explicitly disable support for external signing on Windows #24065

Merged

hebasto closed this Mar 24, 2022

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

util: Add "importfromcoldcard" command to bitcoin-wallet tool

2839a4c

Coldcard firmware v4.1.3+ supports wallet export via descriptors. Github-Pull: bitcoin#23362 Rebased-From: 8076f8d

bitcoin locked and limited conversation to collaborators Mar 24, 2023

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

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

Uh oh!

Conversation

hebasto commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 26, 2021

Uh oh!

doc-hex commented Oct 26, 2021

Uh oh!

nvk commented Oct 26, 2021

Uh oh!

jamesob commented Oct 26, 2021

Uh oh!

mjdietzx left a comment

Choose a reason for hiding this comment

Uh oh!

mjdietzx Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 26, 2021

Uh oh!

Sjors commented Oct 27, 2021

Uh oh!

fanquake commented Oct 27, 2021

Uh oh!

jamesob commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvk commented Oct 27, 2021

Uh oh!

hebasto commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

lsilva01 commented Oct 30, 2021

Uh oh!

DrahtBot commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Nov 22, 2021

Uh oh!

DrahtBot commented Dec 3, 2021

Uh oh!

fanquake commented Dec 27, 2021

Uh oh!

hebasto commented Dec 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

hebasto commented Oct 26, 2021 •

edited

Loading

jamesob commented Oct 27, 2021 •

edited

Loading

hebasto commented Oct 27, 2021 •

edited

Loading

DrahtBot commented Nov 1, 2021 •

edited

Loading

laanwj commented Nov 8, 2021 •

edited

Loading