feat(compile): compile taproot descriptor with randomized unspendable internal key #225

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

Description

Resolves #218 (part 1/2).

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

- Add basic tests validating that taproot results contain 'r' field while sh results do not - Fix test execution by enabling 'compiler' feature - Add error message validation for invalid cases Future iterations will include more comprehensive test coverage.

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

Pull Request Test Coverage Report for Build 20664781654

Details

17 of 25 (68.0%) changed or added relevant lines in 1 file are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-1.9%) to 6.004%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/handlers.rs	17	25	68.0%

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

Totals
Change from base Build 20377635666:	-1.9%
Covered Lines:	128
Relevant Lines:	2132

💛 - Coveralls

tvpeter

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

tvpeter · 2025-12-11T21:57:43Z

src/error.rs

+    #[error("Miniscript compiler error: {0}")]
+    MiniscriptCompilerError(#[from] bdk_wallet::miniscript::policy::compiler::CompilerError),
+


The above miniscript error wraps all the errors from miniscript including this one.

I added this error only for creating policies in a more concise way:
https://github.com/bitcoindevkit/bdk-cli/pull/225/files#diff-148e623ad4cdaab49b81901460099f4efe7900e8c57e4f7a0ff6778589be01f8L896-R905

Which way do you prefer more?

remove error, restore using err_map for policies

keep error and create policies without err_map

tvpeter · 2025-12-12T04:41:53Z

Cargo.toml

 compiler = []
+
+[dev-dependencies]
+shlex = "1.3.0"


shlex is an optional dependency, so instead of re-installing it under dev dependencies, it's better to make it a non-optional dependency.

shlex needs to remain both optional and in dev-dependencies:

Optional: it's only used by the repl feature, so other builds don't need it

Dev-dependencies: CI runs cargo test --no-default-features which excludes repl, but tests still need shlex to compile

Making it non-optional would unnecessarily add the dependency to all builds.

Alternatively, we could accept the dependency overhead but simplify the code and CI in return?

tvpeter · 2025-12-15T18:24:29Z

src/handlers.rs


-            let xonly_public_key = XOnlyPublicKey::from_str(NUMS_UNSPENDABLE_KEY_HEX)
-                .map_err(|e| Error::Generic(format!("Invalid NUMS key: {e}")))?;
+            let internal_key_point = nums_point.combine(&r_point)?;


Have you considered the add_exp_tweak method? I think it might be better suited for this use case because it knows one of the factors is a Generator point, whereas combine is for adding arbitrary public points.

Good point, thanks! Done.

…tion

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.

110CodingP · 2026-01-01T14:37:59Z

src/handlers.rs

        }
-    }?;
+    }?
+    .to_string();


nit: ig we do not need this to_string here because of the Display implementation of Descriptor<String>.

Fixed, thanks!

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 added 5 commits November 14, 2025 16:32

feat(compile): use random internal key for tr descriptor

a541122

chore(compile): simplify

6c3e071

test(compile): move invalid cases tests for compile cmd to new test file

09a381b

test(compile): delete obsoleted test for compile taproot descriptors

d763986

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

ci(dependencies): add shlex to dev-dependencies for running test on CI

df6ee82

tvpeter assigned va-an Dec 11, 2025

tvpeter requested changes Dec 15, 2025

View reviewed changes

va-an added 2 commits December 27, 2025 23:33

Merge branch 'master' into feat/tr-descriptor-with-random-point

791a775

refactor: use add_exp_tweak instead of combine for NUMS key randomiza…

7b9a84e

…tion

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

110CodingP reviewed Jan 1, 2026

View reviewed changes

chore(compile): remove unnecessary to_string()

fab2291

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

		#[error("Miniscript compiler error: {0}")]
		MiniscriptCompilerError(#[from] bdk_wallet::miniscript::policy::compiler::CompilerError),

feat(compile): compile taproot descriptor with randomized unspendable internal key #225

Are you sure you want to change the base?

feat(compile): compile taproot descriptor with randomized unspendable internal key #225

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 20664781654

Details

💛 - Coveralls

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

110CodingP 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

va-an commented Nov 19, 2025 •

edited

Loading

coveralls commented Nov 19, 2025 •

edited

Loading

110CodingP commented Jan 3, 2026 •

edited

Loading