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

Resolves #218 (part 1/2).
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.

src/error.rs

Cargo.toml

src/handlers.rs

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.

Ok, I’ll add these changes when I do the squash.

I made the changes, and only then remembered why I did it this way — if we apply shorten globally (not just for tr), we'll get an error for other descriptors because we're trying to shorten an already too short string.

Here's an example of how to reproduce this (if you apply your suggested changes locally):

-> % cargo run --all-features -- compile "pk(ABC)" -t tr --pretty +------------+------------------------------------------------------------------+ | Descriptor | tr(cf07afc46391a3ce22c1024a682ca...fc9486809e6,pk(ABC))#3qlg4fwz | +------------+------------------------------------------------------------------+ | r | ee1c3591351b7c879fd415c0fbe3871bf61c752d7d35ed2ed94f0a278de9073d | +------------+------------------------------------------------------------------+ -> % cargo run --all-features -- compile "pk(ABC)" -t wsh --pretty thread 'main' (1258654) panicked at src/utils.rs:383:39: byte index 32 is out of bounds of `wsh(pk(ABC))#8gmtcnaw` note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Also, I'm not sure we should successfully compile a script like "pk(ABC)" at all, but we can discuss this in a separate issue.

Yeah, I noticed this bug and fixed it in #203 here. We may have to wait for the PR to get merged.

Also, I'm not sure we should successfully compile a script like "pk(ABC)" at all, but we can discuss this in a separate issue.

Well, policy compilation is actually from the upstream source but I have tested policies that failed. I will try to see if I can know what made this one to pass.

Also, I'm not sure we should successfully compile a script like "pk(ABC)" at all, but we can discuss this in a separate issue.

Well, policy compilation is actually from the upstream source but I have tested policies that failed. I will try to see if I can know what made this one to pass.

I just noticed that bdk-cli can create a descriptor that bitcoin-cli doesn't accept as valid:

-> % bdk-cli compile "pk(ABC)" { "descriptor": "wsh(pk(ABC))#8gmtcnaw" } -> % bitcoin-cli deriveaddresses "wsh(pk(ABC))#8gmtcnaw" error code: -5 error message: pk(): key 'ABC' is not valid

This might confuse some users.

As a simple fix, we could explicitly state in the compile command documentation that we don't validate keys at compile time.

Yeah, I noticed this bug and fixed it in #203 here. We may have to wait for the PR to get merged.

Nice! Let's wait for #203 to merge then.
I've added "Depends on" to the PR description.
If we have a label like "blocked" or "do-not-merge", please add it to this PR.

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.

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

src/error.rs Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

src/handlers.rs Outdated Show resolved Hide resolved

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

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

va-an added 2 commits January 18, 2026 20:52

test(compile): move and edit tests for compile command

fbf263e

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

b5dac76

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

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

6bcf948

	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()]];

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!

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!

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!

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