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

[va-an]

Description

Addresses #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:

1. This PR: Randomize unspendable internal key for taproot descriptor
2. 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:

• [x] I've signed all my commits
• [x] I followed the contribution guidelines
• [x] 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]

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%
<!--	Total:	21	31

Files with Coverage Reduction	New Missed Lines	%
src/handlers.rs	74	11.57%
<!--	Total:	74

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

---

💛 - Coveralls

[va-an]

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]

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]

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]

@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 — https://github.com/bitcoindevkit/bdk-cli/pull/225/commits/afc76aa8d44eb609548d0923c59cd55f6785a6bd.