add support for the BitBox02 hardware wallet #117

benma · 2024-08-16T22:17:23Z

Using the bitbox-api NPM package, which loads a WASM module.

Note about CJS:
Currently the bitbox-api package is an ESM package with a module: ... entrypoint, so it is not compatible with the cjs target of caravan-wallets. The only workaround that I could find where compilation succeeds and the package works in the browser is to mark bitbox-api as external in tsup.config.ts.

Note about signing tests:

The BitBox02 requires the previous transaction of each input to be
present in the PSBT (PSBT_IN_NON_WITNESS_UTXO), so it can verify the
input amount and avoid fee attacks. The signing test fixtures are
missing these, so they fail.
The BitBox02 uses the anti-klepto (anti-exfil) protocol to mitigate
covert nonce exfil attacks. This results in random signatures. The
unit test fixtures hardcode the expected signatures, assuming they are
always the same. As a result, also here the tests fail. To fix this,
the tests should rather verify the ECDSA signatures against the
transaction sighash for each input.

changeset-bot · 2024-08-16T22:17:27Z

🦋 Changeset detected

Latest commit: 25b399f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@caravan/wallets	Minor
caravan-coordinator	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2024-08-16T22:17:27Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
caravan-coordinator	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 8, 2024 10:11pm

packages/caravan-wallets/src/index.ts

packages/caravan-wallets/src/bitbox.ts

bucko13 · 2024-08-20T16:48:05Z

Note for when you're ready to take it out of draft that this will need a changeset in order to bump versions. Without having reviewed yet, I'm guessing minor version bump for @caravan/wallets.

benma · 2024-08-21T12:23:42Z

Note for when you're ready to take it out of draft that this will need a changeset in order to bump versions. Without having reviewed yet, I'm guessing minor version bump for @caravan/wallets.

Added the generated changeset file. Should I also bump the package.json version numbers manually?

benma · 2024-08-21T12:29:14Z

Ready for review 😄

bucko13 · 2024-08-22T03:58:02Z

Added the generated changeset file. Should I also bump the package.json version numbers manually?

@benma nope! The automation will take care of that

apps/coordinator/src/components/Wallet/ExtendedPublicKeyImporter.jsx

packages/caravan-wallets/src/index.ts

chadchapnick · 2024-09-07T21:29:50Z

packages/caravan-wallets/src/bitbox.ts

+      return {
+        address,
+        serializedPath: this.bip32Path,
+      };


Looks like this is mostly working—noticing we get back a testnet address when on regtest

The BitBox02 itself does not actually support regtest right now. Adding support for it would be easy, and in fact I made a PR for it now (BitBoxSwiss/bitbox02-firmware#1302). It would take a little while until it is released and can be used here though.

Another solution here would be to convert the address to the regtest format inside Caravan after the BitBox02 returns it in the testnet format.

What do you think is the best course of action?

In favor for keeping it elegant without the regtest hack for P2WSH! Nested segwit works perfectly and that's likely sufficient for now. If your firmware PR makes it in, it will puzzle in nicely.

Thank you sharing and for suggesting the address conversion, it does work well

import { decodeSegwitAddress, encodeSegwitAddress } from "./vendor/bech32"; if (this.walletConfig.network == "regtest") { if (address.startsWith("tb1q")) { try { const decoded = decodeSegwitAddress("tb", address); if (!decoded) { throw new Error("Could not decode address"); } const resp = encodeSegwitAddress( "bcrt", decoded.version, decoded.program ); return { address: resp, serializedPath: this.bip32Path, }; } catch (err) { console.log("error is", err); } } }

@benma any sense of when the updated firmware might be released?

It was released a few weeks ago - if you update your BitBox02 to v9.21.0 it will show the proper regtest address prefix.

You can update to this firmware most easily via BitBoxApp v4.45+.

I also added a commit here to make use of it.

bucko13 · 2024-09-22T17:49:34Z

I'm surprised you were able to get this to run. I had to add the following to the coordinator's vite config:

export default defineConfig({
    ...
    build: {
        target: "esnext", //browsers can handle the latest ES features
         ...
    },
}

Without that, I get these failures:

caravan-coordinator:build: error during build:
caravan-coordinator:build: Error: Transform failed with 1 error:
caravan-coordinator:build: assets/bitbox_api-!~{004}~.js:1371:27: ERROR: Top-level await is not available in the configured target environment ("chrome87", "edge88", "es2020", "firefox78", "safari14" + 3 overrides)

There are also a couple of linting errors that need to be fixed.

benma · 2024-09-24T07:33:01Z

I'm surprised you were able to get this to run. I had to add the following to the coordinator's vite config:
export default defineConfig({
    ...
    build: {
        target: "esnext", //browsers can handle the latest ES features
         ...
    },
}

I only developed using turbo run dev, where this error never appeared. I can see it however when running turbo run build. I added the above line.

There are also a couple of linting errors that need to be fixed.

Fixed 👍

bucko13

This looks great! Just a couple of small things that maybe aren't even blockers.

I'd like us to add unit tests for the @caravan/wallets code, but since we do have the test suite at least, we don't need to block this going in.

It would also be nice to have the regtest firmware update in so we can do more rigorous testing, but otherwise I'm excited to have this merged in soon!

bucko13 · 2024-10-18T21:22:11Z

packages/caravan-wallets/src/bitbox.ts

+  }
+}
+
+async function convertMultisig(pairedBitBox: PairedBitBox, walletConfig: MultisigWalletConfig): Promise<{ scriptConfig: BtcScriptConfig; keypathAccount: string; }> {


It feels like this function is doing two things and it might cleaner to separate them. Correct me if I'm wrong but what it's doing is:

verifying that the devices root fingerprint is in a given wallet config

converting the caravan style wallet config to bitbox style

What if the verification was a method on the interaction base class where you know you'll have a paired bitbox you can interact with and then this was kept as a simple conversion utility function?

Having the verification on the interaction would allow it to be used in other interactions as well. could even retrieve the root fingerprint presumably on startup and store it as a property on the class or something and then use it as needed. Not sure if that's strictly correct or necessary, but I did find this a little hard to grok the way it is.

It feels like this function is doing two things and it might cleaner to separate them. Correct me if I'm wrong but what it's doing is:

verifying that the devices root fingerprint is in a given wallet config

converting the caravan style wallet config to bitbox style

It is not doing the first one. It is merely trying to identify which of the xpubs belongs to the device, as that is a required parameter in the script config passed to the BitBox (ourXpubIndex).

bucko13 · 2024-10-18T21:25:06Z

packages/caravan-wallets/src/bitbox.ts

+      return {
+        address,
+        serializedPath: this.bip32Path,
+      };


@benma any sense of when the updated firmware might be released?

bucko13 · 2024-10-18T21:27:53Z

packages/caravan-wallets/src/bitbox.ts

+  }
+
+  async maybeRegisterMultisig(pairedBitBox: PairedBitBox, walletConfig: MultisigWalletConfig): Promise<{ scriptConfig: BtcScriptConfig, keypathAccount: string; }> {
+    const { scriptConfig, keypathAccount } = await convertMultisig(pairedBitBox, walletConfig);


Could do something like:

Suggested change

const { scriptConfig, keypathAccount } = await convertMultisig(pairedBitBox, walletConfig);

const rootFingerprint = await pairedBitBox.rootFingerprint()

this.verifyConfig(rootFingerprint, walletConfig)

// or this.verifyConfig(pairedBitBox, walletConfig) which could do the fingerprint retrieval

const { scriptConfig, keypathAccount } = convertMultisig(walletConfig);

bucko13 · 2024-10-18T21:29:33Z

packages/caravan-wallets/src/bitbox.ts

+    );
+    // No name means the user inputs it on the device.
+    // eslint-disable-next-line no-undefined
+    const name = undefined;


There is a name that comes in from the wallet config actually so maybe it should just use that (same is done for ledger and coldcard)

There might be trouble as the BitBox02 has some requirements on the name - max 30 chars, only printalble ASCII chars (including space), etc. If entered on the device it cannot fail.

Using the `bitbox-api` NPM package, which loads a WASM module. Note about CJS: Currently the `bitbox-api` package is an ESM package with a `module: ...` entrypoint, so it is not compatible with the `cjs` target of caravan-wallets. The only workaround that I could find where compilation succeeds and the package works in the browser is to mark bitbox-api as external in tsup.config.ts. Note about signing tests: - The BitBox02 requires the previous transaction of each input to be present in the PSBT (`PSBT_IN_NON_WITNESS_UTXO`), so it can verify the input amount and avoid fee attacks. The signing test fixtures are missing these, so they fail. - The BitBox02 uses the anti-klepto (anti-exfil) protocol to mitigate covert nonce exfil attacks. This results in random signatures. The unit test fixtures hardcode the expected signatures, assuming they are always the same. As a result, also here the tests fail. To fix this, the tests should rather verify the ECDSA signatures against the transaction sighash for each input.

BitBox02 does not support legacy P2SH.

The BitBox, if not paired yet, will show a pairing code for confirmation. This can happen in any BitBox interaction. This commit adds a `showPairingCode` param to all BitBox interactions. If not provided, a default implementation is used which shows the pairing code in a browser popup. The current `messages()` system is not a good fit, as the client does not know when to call `messagesFor()` to display it. Having a separate UI button to pair the BitBox is not good UX (why should the user be bothered to click a "pair" button first? What if the user doesn't) and also fragile (a re-pairing could be needed at any time).

benma · 2024-10-31T08:44:51Z

This looks great! Just a couple of small things that maybe aren't even blockers.

I'd like us to add unit tests for the @caravan/wallets code, but since we do have the test suite at least, we don't need to block this going in.

It would also be nice to have the regtest firmware update in so we can do more rigorous testing, but otherwise I'm excited to have this merged in soon!

Thanks! The firmware with regtest has been released and I integrated the change in a new commit, please check it out. See #117 (comment)

benma commented Aug 16, 2024

View reviewed changes

packages/caravan-wallets/src/index.ts Outdated Show resolved Hide resolved

vercel bot had a problem deploying to Preview August 16, 2024 22:17 Failure

benma commented Aug 16, 2024

View reviewed changes

packages/caravan-wallets/src/bitbox.ts Show resolved Hide resolved

benma force-pushed the bitbox branch from 821bab0 to c71f157 Compare August 16, 2024 22:22

vercel bot had a problem deploying to Preview August 16, 2024 22:23 Failure

benma force-pushed the bitbox branch from c71f157 to 3386984 Compare August 21, 2024 12:23

vercel bot had a problem deploying to Preview August 21, 2024 12:26 Failure

benma marked this pull request as ready for review August 21, 2024 12:26

chadchapnick reviewed Sep 1, 2024

View reviewed changes

apps/coordinator/src/components/Wallet/ExtendedPublicKeyImporter.jsx Outdated Show resolved Hide resolved

chadchapnick reviewed Sep 1, 2024

View reviewed changes

packages/caravan-wallets/src/index.ts Show resolved Hide resolved

benma force-pushed the bitbox branch from 3386984 to 7116ca7 Compare September 2, 2024 09:14

vercel bot had a problem deploying to Preview September 2, 2024 09:18 Failure

chadchapnick reviewed Sep 7, 2024

View reviewed changes

benma force-pushed the bitbox branch from 7116ca7 to 292dca5 Compare September 24, 2024 07:31

vercel bot deployed to Preview September 24, 2024 07:35 View deployment

vercel bot deployed to Preview September 24, 2024 10:04 View deployment

bucko13 requested changes Oct 18, 2024

View reviewed changes

benma added 4 commits October 31, 2024 09:28

hide BitBox02 menu item for P2SH

4efb8d9

BitBox02 does not support legacy P2SH.

add regtest support for BitBox02

7e212ed

benma force-pushed the bitbox branch from e0a7301 to 7e212ed Compare October 31, 2024 08:34

vercel bot deployed to Preview October 31, 2024 08:38 View deployment

Merge branch 'main' into bitbox

25b399f

bucko13 approved these changes Nov 8, 2024

View reviewed changes

vercel bot deployed to Preview November 8, 2024 22:11 View deployment

bucko13 merged commit d0e08e8 into caravan-bitcoin:main Nov 8, 2024
3 checks passed

github-actions bot mentioned this pull request Nov 8, 2024

Version Packages #145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for the BitBox02 hardware wallet #117

add support for the BitBox02 hardware wallet #117

benma commented Aug 16, 2024 •

edited

Loading

changeset-bot bot commented Aug 16, 2024 •

edited

Loading

vercel bot commented Aug 16, 2024 •

edited

Loading

bucko13 commented Aug 20, 2024

benma commented Aug 21, 2024

benma commented Aug 21, 2024

bucko13 commented Aug 22, 2024

chadchapnick Sep 7, 2024

benma Sep 7, 2024

chadchapnick Sep 8, 2024

bucko13 Oct 18, 2024

benma Oct 31, 2024

bucko13 commented Sep 22, 2024

benma commented Sep 24, 2024

bucko13 left a comment

bucko13 Oct 18, 2024

benma Oct 31, 2024

bucko13 Oct 18, 2024

bucko13 Oct 18, 2024

bucko13 Oct 18, 2024

benma Oct 31, 2024

benma commented Oct 31, 2024

-    const { scriptConfig, keypathAccount } = await convertMultisig(pairedBitBox, walletConfig);
+    const rootFingerprint = await pairedBitBox.rootFingerprint()
+    this.verifyConfig(rootFingerprint, walletConfig)
+    // or this.verifyConfig(pairedBitBox, walletConfig) which could do the fingerprint retrieval
+    const { scriptConfig, keypathAccount } = convertMultisig(walletConfig);

add support for the BitBox02 hardware wallet #117

add support for the BitBox02 hardware wallet #117

Conversation

benma commented Aug 16, 2024 • edited Loading

changeset-bot bot commented Aug 16, 2024 • edited Loading

🦋 Changeset detected

vercel bot commented Aug 16, 2024 • edited Loading

bucko13 commented Aug 20, 2024

benma commented Aug 21, 2024

benma commented Aug 21, 2024

bucko13 commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bucko13 commented Sep 22, 2024

benma commented Sep 24, 2024

bucko13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma commented Oct 31, 2024

benma commented Aug 16, 2024 •

edited

Loading

changeset-bot bot commented Aug 16, 2024 •

edited

Loading

vercel bot commented Aug 16, 2024 •

edited

Loading