feat(compile): compile taproot descriptor with randomized unspendable internal key by va-an · Pull Request #225 · bitcoindevkit/bdk-cli

va-an · 2025-11-19T11:55:26Z

Description

Part of #218
Depends on #203 (shorten() fix)

Implements randomization of the unspendable internal key for taproot descriptors. This is the first part of #218, which consists of two parts:

This PR: Randomize unspendable internal key for taproot descriptor
Follow-up: Add verification command to ensure internal key is derived from NUMS key

Split into separate PRs for easier review and iteration, and to allow independent discussion of the verification command implementation, as one of the possible approaches could introduce breaking changes.

Notes to the reviewers

The compile command now returns an additional r field for taproot descriptors (-t tr), containing the randomly generated internal key. Each compilation will produce a different internal key instead of using a fixed NUMS key.

Example output for taproot (first execution):

-> % bdk-cli compile "pk(A)" -t tr
{
  "descriptor": "tr(2dd09dd0355f4b2d5a4886de599786f3c0211652373221c87aba1cd1f7f1e593,pk(A))#anvu48aj",
  "r": "275a58827bd0ad459d6f92e083ddc3d99a03076155691680eb8f3b06380cdcfd"
}

Same descriptor compiled again produces different r and internal key:

-> % bdk-cli compile "pk(A)" -t tr
{
  "descriptor": "tr(801078f69dae7d95631723d4d13e6c32911633d227dcfc24c6b7e32e1e533e6c,pk(A))#f79rr82j",
  "r": "5e3ac63bb20d6a4bfff645279cc63a7472e18066da8826b13cbcb23aecb5c401"
}

Other descriptor types remain unchanged:

-> % bdk-cli compile "pk(A)" -t sh
{
  "descriptor": "sh(pk(A))#k80zhe7s"
}

Tests for compile command have been moved from handlers.rs to the tests directory. Since taproot descriptors now generate a random internal key on each invocation, the test for the compile command has been simplified. I plan to enhance this test in a follow-up PR once the verification command is implemented.

Changelog notice

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

coveralls · 2025-11-19T12:00:13Z

Pull Request Test Coverage Report for Build 21687075357

Details

21 of 31 (67.74%) changed or added relevant lines in 1 file are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-1.1%) to 9.663%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/handlers.rs	21	31	67.74%

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	4	10.03%

Totals
Change from base Build 21153868360:	-1.1%
Covered Lines:	238
Relevant Lines:	2463

💛 - Coveralls

tvpeter

Thanks for working on this @va-an
I have left a few comments.

110CodingP

Refactoring the tests to usestd::process::Command instead of calling the internal function handle_compile_subcommand make the tests vulnerable to the state of environment variables and makes debugging harder in my opinion. But I have no strong disagreement to the approach in the PR.

va-an · 2026-01-02T19:04:33Z

Refactoring the tests to usestd::process::Command instead of calling the internal function handle_compile_subcommand make the tests vulnerable to the state of environment variables and makes debugging harder in my opinion. But I have no strong disagreement to the approach in the PR.

My main idea is to have integration tests, not just unit tests, because that's the closest to how users actually interact with it.
We can have both: unit tests for functions and integration tests for CLI commands. Some duplication, but better coverage overall.

As for the environment variable state concern — we could remove related env vars to isolate integration tests, for example for compile tests:

    let output = Command::new("cargo")
        .args(args)
        .env_remove("NETWORK")
        .env_remove("DATADIR")
        .env_remove("POLICY")
        .env_remove("TYPE")
        .output()
        .unwrap();

This way we can isolate tests from the environment. Maintaining such tests might be a bit less convenient, but I promise to keep an eye on it :)
What do you think?

110CodingP · 2026-01-03T06:07:18Z

My main idea is to have integration tests, not just unit tests, because that's the closest to how users actually interact with it. We can have both: unit tests for functions and integration tests for CLI commands. Some duplication, but better coverage overall.

That definitely makes sense.

As for the environment variable state concern — we could remove related env vars to isolate integration tests, for example for compile tests:
    let output = Command::new("cargo")
        .args(args)
        .env_remove("NETWORK")
        .env_remove("DATADIR")
        .env_remove("POLICY")
        .env_remove("TYPE")
        .output()
        .unwrap();
This way we can isolate tests from the environment. Maintaining such tests might be a bit less convenient, but I promise to keep an eye on it :) What do you think?

Thanks! I did not know that we could remove them.

I agree this could be helpful. Do you think it should be done in a separate PR though? maybe one that also refactors tests/integration.rs or is it better to keep it here?

va-an · 2026-01-04T11:13:53Z

I agree this could be helpful. Do you think it should be done in a separate PR though? maybe one that also refactors tests/integration.rs or is it better to keep it here?

I would prefer to keep this PR as is, since @tvpeter has already looked at these changes.
Let's create a new issue to discuss follow-up changes - I find it easier to coordinate when discussion and implementation are separated.

va-an · 2026-01-06T12:18:56Z

@110CodingP I decided to add the code for removing env vars in tests, as we discussed, so we don't forget to do this in future PRs.

I verified locally that this works as expected — the command process will not receive these env vars from the parent process.

use std::{env, process::Command};

fn main() {
    unsafe {
        env::set_var("FOO", "1");
        env::set_var("BAR", "2");
        env::set_var("BAZ", "3");
    }

    let cmd = Command::new("printenv")
        .env_remove("FOO")
        .env_remove("BAR")
        .output()
        .unwrap();

    let stdout = String::from_utf8_lossy(&cmd.stdout);

    assert!(!stdout.contains("FOO"), "FOO should be removed");
    assert!(!stdout.contains("BAR"), "BAR should be removed");
    assert!(stdout.contains("BAZ"), "BAZ should still be present");

    println!("it's all good man");
}

Please take a look — afc76aa.

tvpeter

Thank you for updating the PR @va-an

Once you address the little comments, I will do a final test.

tvpeter · 2026-01-08T11:48:25Z

-        .table()
-        .display()
-        .map_err(|e| Error::Generic(e.to_string()))?;
+        let mut rows = vec![vec!["Descriptor".cell().bold(true), descriptor.cell()]];


the descriptor value is quite long, can you apply the [shorten](https://github.com/bitcoindevkit/bdk-cli/blob/fb7f6c60ca4ac1ad80750391435bbafd7346c714/src/utils.rs#L381) fn so it displays better?

Done! I also used shorten() for r.
I extracted shorten description into a variable to shorten only the internal key and leave the rest of the descriptor as is.

without pretty:

-> % cargo run --all-features -- compile "pk(ABC)" -t tr { "descriptor": "tr(1985c2a86e01bbcdb1b5806422531a73c77e568ecfdd6d2863c158a76628ea50,pk(ABC))#6qsjh5q3", "r": "e38caa40d0db7cae7037336c982278a7fc6b770e7a80f8bbce0d79976a14d5e1" }

with pretty:

-> % cargo run --all-features -- compile "pk(ABC)" -t tr --pretty +------------+----------------------------------+ | Descriptor | tr(ccb4...b1d0,pk(ABC))#qrk923py | +------------+----------------------------------+ | r | a129...7a14 | +------------+----------------------------------+

110CodingP · 2026-01-09T03:33:59Z

+    let output = Command::new("cargo")
+        .args(args)
+        .env_remove("NETWORK")
+        .env_remove("DATADIR")


nit: ig removing DATADIR env variable is not required? Since the compile command does not use it?

You're right, DATADIR isn't used in compile command.
I'm planning to reuse this command execution code in other tests, so I preemptively cleared all env vars.

tvpeter

Hi @va-an,

Could you please squash your commits into two or three commits and fix the shortening approach?

Aside from that, it looks good to go. Thank you.

va-an · 2026-01-18T17:03:16Z

Hi @va-an,

Could you please squash your commits into two or three commits and fix the shortening approach?

Aside from that, it looks good to go. Thank you.

Squashed into 2 commits, but without the shortening approach changes - I've replied about that in the discussion above.

va-an · 2026-02-04T20:29:55Z

@tvpeter

Hey! I noticed that #203 was merged. I merged master into my branch and verified that the shorten() bug no longer reproduces.

-> % cargo run --all-features -- compile "pk(ABC)" -t tr --pretty
+------------+----------------------------------+
| Descriptor | tr(e753...c650,pk(ABC))#quuf6809 |
+------------+----------------------------------+
| r          | d527...f9c9                      |
+------------+----------------------------------+
-> % cargo run --all-features -- compile "pk(ABC)" -t wsh --pretty
+------------+-----------------------+
| Descriptor | wsh(pk(ABC))#8gmtcnaw |
+------------+-----------------------+

Please take a look.

notmandatory

Overall the code looks good, my only concern is with running shell commands as part of the testing, which I'd like to see removed.

Ultimately I'd like to figure out how to do the TR optimizatoin for the pk(A) policy so that it creates the descriptor tr(A) but I don't know if/how to do it or if something would need to be changed upstream.

notmandatory · 2026-03-19T21:11:01Z

+    let full_cmd = format!("run --features compiler -- {}", cmd);
+    let args = shlex::split(&full_cmd).unwrap();
+
+    let output = Command::new("cargo")


I don't see any reason to test the running of these commands. I know some older tests also do that but it's a pattern I don't want to continue and would like to remove. As far as I can tell these tests only verify the clap library is doing its job right, which we do not need to re-test.

I'd rather see the tests only verify the handlers functions return what is expected.

Although not directly related to this PR's changes, during review we agreed to make shlex non-optional since it's used by the default `repl` feature and the package is under 20 KiB. See: bitcoindevkit#225 (comment)

codecov · 2026-03-24T18:08:46Z

Codecov Report

❌ Patch coverage is 89.02439% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 12.34%. Comparing base (7c33b33) to head (053073f).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
src/handlers.rs	89.02%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   10.96%   12.34%   +1.37%     
==========================================
  Files           8        8              
  Lines        2526     2568      +42     
==========================================
+ Hits          277      317      +40     
- Misses       2249     2251       +2

Flag	Coverage Δ
rust	`12.34% <89.02%> (+1.37%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

va-an · 2026-03-25T07:49:45Z

+
+            let secp = Secp256k1::new();
+            let r_secret = SecretKey::new(&mut rand::thread_rng());
+            r = Some(r_secret.display_secret().to_string());
+
+            let nums_key = XOnlyPublicKey::from_str(NUMS_UNSPENDABLE_KEY_HEX)?;
+            let nums_point = PublicKey::from_x_only_public_key(nums_key, Parity::Even);
+
+            let internal_key_point = nums_point.add_exp_tweak(&secp, &Scalar::from(r_secret))?;
+            let (xonly_internal_key, _) = internal_key_point.x_only_public_key();

-            let xonly_public_key = XOnlyPublicKey::from_str(NUMS_UNSPENDABLE_KEY_HEX)
-                .map_err(|e| Error::Generic(format!("Invalid NUMS key: {e}")))?;
+            let tree = TapTree::Leaf(Arc::new(policy.compile::<Tap>()?));


I'm planning to replace compile() with compile_tr() for the tr match branch, and moving the compile step inside the match is preparation for that.

See: #244

Nope, this won't happen - #244 (comment).

va-an · 2026-03-25T22:07:41Z

Ultimately I'd like to figure out how to do the TR optimizatoin for the pk(A) policy so that it creates the descriptor tr(A) but I don't know if/how to do it or if something would need to be changed upstream.

This is exactly what compile_tr does - it extracts the most probable key as the internal key, so pk(A) would become tr(A).

But as I noted in #244 (comment), compile_tr always prioritizes a real key over the provided unspendable_key, so we can't guarantee our NUMS (H + rG) as the internal key. One option would be to add a --nums flag - with it we use our NUMS + return r, without it we use compile_tr for optimal key-path spending. But that adds some complexity for user and for our code, so I'd prefer keeping the current simpler approach with NUMS by default.

va-an · 2026-03-29T20:48:41Z


 [dependencies]
-bdk_wallet = { version = "2.1.0", features = ["rusqlite", "keys-bip39", "compiler", "std"] }
+bdk_wallet = { version = "=2.1.0", features = ["rusqlite", "keys-bip39", "compiler", "std"] }


Pls revert because of this

va-an · 2026-05-04T19:00:46Z

Hey @tvpeter, is there anything else you'd like me to do on my end? Should I squash the follow-up commits down again (a few have piled up since the last squash)?

Thanks!

tvpeter · 2026-05-04T19:05:53Z

Hey @tvpeter, is there anything else you'd like me to do on my end? Should I squash the follow-up commits down again (a few have piled up since the last squash)?

Hi @va-an, apologies for the delay. I will give it a final check shortly.

tvpeter

hi @va-an , I have reviewed again and left one minor nit below.

Also, kindly remove these lines because the gating does not apply to compiler feature tests, and there is no way to run the compile feature tests except all-features are compiled.

Please, once you have effected these changes, tag me to approve.

Thank you.

tvpeter · 2026-05-06T23:02:54Z


 [dependencies]
-bdk_wallet = { version = "2.1.0", features = ["rusqlite", "keys-bip39", "compiler", "std"] }
+bdk_wallet = { version = "=2.1.0", features = ["rusqlite", "keys-bip39", "compiler", "std"] }


Pls revert because of this

va-an · 2026-05-07T21:00:31Z

hi @va-an , I have reviewed again and left one minor nit below.

Also, kindly remove these lines because the gating does not apply to compiler feature tests, and there is no way to run the compile feature tests except all-features are compiled.

Please, once you have effected these changes, tag me to approve.

Thank you.

@tvpeter done, pls have a look.
Should I squash commits into one after your review before merge?

tvpeter

Thank you @va-an.

While it is not mandatory, you can squash 05a248c,
1ed41d2 and bc8135d.

Also, please revert the bdK_wallet version to 2.1, as some features in versions above 2.1 have not yet been implemented in the CLI.

Thank you

Although not directly related to this PR's changes, during review we agreed to make shlex non-optional since it's used by the default `repl` feature and the package is under 20 KiB. See: bitcoindevkit#225 (comment)

Instead of using a fixed NUMS key as the internal key for taproot descriptors, generate a randomized unspendable key (H + rG) for each compilation. This improves privacy by preventing observers from determining whether key path spending is disabled. The randomness factor `r` is included in the output so the user can verify that the internal key is derived from the NUMS point. Also applies `shorten()` globally in pretty mode and uses `?` operator via dedicated error variants instead of `map_err`.

Add `claims` dev-dependency for ergonomic `assert_ok!`/`assert_err!` macros. Adapt compile tests to verify randomized key behavior: - descriptor structure instead of exact match (key is now random) - presence of `r` field for taproot, absence for other types - uniqueness of `r` across compilations

Move miniscript compilation inside match arms to avoid compiling for all script contexts when only one is needed. Use `tap::Pipe` for concise method chaining in sh/wsh/sh-wsh branches.

- Remove MSRV mention as the project no longer enforces it - Add Conventional Commits and GPG signing links to align with other repos

Rebased onto master to drop the merge commit, which left Cargo.lock with uncommitted changes. The cleaner fix would be to squash these into the commits that introduced the dependencies, but I went with the simpler approach.

va-an · 2026-05-08T16:25:32Z

Thank you @va-an.

While it is not mandatory, you can squash 05a248c, 1ed41d2 and bc8135d.

Also, please revert the bdK_wallet version to 2.1, as some features in versions above 2.1 have not yet been implemented in the CLI.

Thank you

@tvpeter phew, did it:

rebased on master, dropped the merge commit and both bdk_wallet pin/bump commits
added one cleanup commit on top to sync Cargo.lock with the new Cargo.toml entries

bdk_wallet is back on 2.1.0 in both Cargo.toml and Cargo.lock.

github-project-automation Bot added this to BDK-CLI Nov 19, 2025

tvpeter assigned va-an Dec 11, 2025

tvpeter requested changes Dec 15, 2025

View reviewed changes

Comment thread src/error.rs

Comment thread Cargo.toml Outdated

Comment thread src/handlers.rs Outdated

va-an requested a review from tvpeter December 27, 2025 20:46

110CodingP reviewed Jan 1, 2026

View reviewed changes

Comment thread src/handlers.rs Outdated

va-an requested a review from 110CodingP January 2, 2026 20:27

110CodingP approved these changes Jan 4, 2026

View reviewed changes

va-an requested a review from 110CodingP January 6, 2026 12:00

tvpeter reviewed Jan 8, 2026

View reviewed changes

110CodingP approved these changes Jan 9, 2026

View reviewed changes

va-an requested a review from tvpeter January 11, 2026 20:53

tvpeter reviewed Jan 16, 2026

View reviewed changes

Comment thread src/handlers.rs Outdated

tvpeter reviewed Jan 16, 2026

View reviewed changes

va-an force-pushed the feat/tr-descriptor-with-random-point branch from 787c701 to b5dac76 Compare January 18, 2026 16:57

va-an requested a review from tvpeter January 18, 2026 17:05

notmandatory moved this to Ready to Review in BDK-CLI Feb 17, 2026

tvpeter added this to the CLI 3.0.0 milestone Feb 26, 2026

va-an mentioned this pull request Feb 27, 2026

NUMS_UNSPENDABLE_KEY_HEX is not supposed to be used as is (privacy concerns) #218

Open

notmandatory reviewed Mar 19, 2026

View reviewed changes

tvpeter moved this from Ready to Review to In Progress in BDK-CLI Mar 20, 2026

va-an force-pushed the feat/tr-descriptor-with-random-point branch from 6bcf948 to 741e672 Compare March 24, 2026 18:07

va-an requested a review from notmandatory March 24, 2026 18:12

tvpeter removed this from the CLI 3.0.0 milestone Mar 25, 2026

va-an commented Mar 25, 2026

View reviewed changes

va-an commented Mar 29, 2026

View reviewed changes

va-an mentioned this pull request Apr 3, 2026

Create integration test with OPRETURN #120

Open

tvpeter added this to the CLI 4.0.0 milestone May 6, 2026

tvpeter reviewed May 6, 2026

View reviewed changes

tvpeter approved these changes May 7, 2026

View reviewed changes

va-an and others added 8 commits May 8, 2026 20:00

chore(deps): make shlex a non-optional dependency

74303bc

Although not directly related to this PR's changes, during review we agreed to make shlex non-optional since it's used by the default `repl` feature and the package is under 20 KiB. See: bitcoindevkit#225 (comment)

refactor(compile): lazy compilation and use pipe for readability

87d8863

Move miniscript compilation inside match arms to avoid compiling for all script contexts when only one is needed. Use `tap::Pipe` for concise method chaining in sh/wsh/sh-wsh branches.

docs(contributing): update CONTRIBUTING.md

d633268

- Remove MSRV mention as the project no longer enforces it - Add Conventional Commits and GPG signing links to align with other repos

docs: add pre-push recipe description to README

a84473f

test(handlers): drop redundant cfg gate on test module

39d799d

chore: sync Cargo.lock

053073f

Rebased onto master to drop the merge commit, which left Cargo.lock with uncommitted changes. The cleaner fix would be to squash these into the commits that introduced the dependencies, but I went with the simpler approach.

va-an force-pushed the feat/tr-descriptor-with-random-point branch from 5f0acd7 to 053073f Compare May 8, 2026 16:16

Conversation

va-an commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

coveralls commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 21687075357

Details

💛 - Coveralls

Uh oh!

tvpeter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

110CodingP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

va-an commented Jan 2, 2026

Uh oh!

110CodingP commented Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

va-an commented Jan 4, 2026

Uh oh!

va-an commented Jan 6, 2026

Uh oh!

tvpeter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tvpeter left a comment

Choose a reason for hiding this comment

Uh oh!

va-an commented Jan 18, 2026

Uh oh!

va-an commented Feb 4, 2026

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

va-an commented Mar 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

va-an commented May 4, 2026

Uh oh!

tvpeter commented May 4, 2026

Uh oh!

tvpeter left a comment

Choose a reason for hiding this comment

Uh oh!

va-an commented Nov 19, 2025 •

edited

Loading

coveralls commented Nov 19, 2025 •

edited

Loading

110CodingP commented Jan 3, 2026 •

edited

Loading

codecov Bot commented Mar 24, 2026 •

edited

Loading