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 20901459542

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

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

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	74	11.57%

Totals
Change from base Build 20377635666:	-1.4%
Covered Lines:	138
Relevant Lines:	2138

💛 - Coveralls

tvpeter

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

src/error.rs

Cargo.toml

src/handlers.rs

…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.

src/handlers.rs

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

src/handlers.rs

-        .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

tests/compile.rs

+    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 · 2026-01-16T09:58:09Z

src/handlers.rs

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


Suggested change

let mut rows = vec![vec!["Descriptor".cell().bold(true), descriptor.cell()]];

let mut rows = vec![vec!["Descriptor".cell().bold(true), shorten(descriptor, 32, 29).cell()]];

you can remove: 908, 931-934, 946

you can also revert 951; it should size most screens comfortably.

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 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

src/error.rs Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

src/handlers.rs Outdated Show resolved Hide resolved

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

src/handlers.rs Outdated Show resolved Hide resolved

chore(compile): remove unnecessary to_string()

fab2291

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

110CodingP approved these changes Jan 4, 2026

View reviewed changes

test(compile): remove related env vars to avoid unexpected behavior

afc76aa

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 added 2 commits January 11, 2026 22:10

chore(deps): make shlex non-optional

c3da068

feat(compile): shorten internal key in tr descriptor for pretty output

787c701

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

tvpeter reviewed Jan 16, 2026

View reviewed changes

	let mut rows = vec![vec!["Descriptor".cell().bold(true), descriptor.cell()]];
	let mut rows = vec![vec!["Descriptor".cell().bold(true), shorten(descriptor, 32, 29).cell()]];

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 20901459542

Warning: This coverage report may be inaccurate.

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!

tvpeter Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

va-an Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

110CodingP Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

va-an Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

tvpeter Jan 16, 2026

Choose a reason for hiding this comment

Uh oh!

tvpeter left a comment

Choose a reason for hiding this comment

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