descriptor: fix comments in descriptor.cpp::DescriptorImpl #33384

pull pythcoiner wants to merge 1 commits into bitcoin:master from pythcoiner:fix_comments_DescriptorImpl changing 1 files +2 −2

pythcoiner commented at 12:40 PM on September 13, 2025: contributor

P2TR was missing in m_pubkey_args description and it was a typo in doc/descriptors.md
descriptor: fix comments in descriptor.cpp::DescriptorImpl 768eeafbda
DrahtBot added the label Descriptors on Sep 13, 2025
DrahtBot commented at 12:40 PM on September 13, 2025: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33384.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



LLM Linter (✨ experimental)

Possible typos and grammar issues:
- In doc/descriptors.md this is referred to as SCRIPT expressions sh(SCRIPT) -> In doc/descriptors.md, this is referred to as SCRIPT expressions: sh(SCRIPT) [Missing comma after the filename and missing colon before the example make the phrase harder to parse; adding them clarifies that sh(SCRIPT) is an example of "SCRIPT expressions".]
No other typos were found.

<sup>drahtbot_id_5_m</sup>
fanquake requested review from Sjors on Sep 23, 2025
Sjors commented at 9:57 AM on September 26, 2025: member

cc @achow101
in src/script/descriptor.cpp:796 in 768eeafbda
```
 792 | @@ -793,13 +793,13 @@ class MuSigPubkeyProvider final : public PubkeyProvider
 793 |  class DescriptorImpl : public Descriptor
 794 |  {
 795 |  protected:
 796 | -    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size for WSH and Multisig).
 797 | +    //! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH; any size for WSH, P2TR, Multisig).
```
rkrux commented at 3:06 PM on November 6, 2025:

This doesn't seem to be correct. I debugged the unit tests & the listdescriptors RPC, and found that m_pubkey_args.size() remains 1 for tr() descriptors as well. It's the m_subdescriptor_args.size() that increases if script path(s) is(are) present. Following points support this:

The TRDescriptor constructor accepts a single PubkeyProvider that's treated as the Taproot internal key: https://github.com/bitcoin/bitcoin/blob/5c5704e730796c6f31e2d7891bf6334674a04219/src/script/descriptor.cpp#L1407-L1408

The following older commit also hints at it https://github.com/bitcoin/bitcoin/commit/4441c6f3c046c0b28ce3f0ca6d938af71d797586:

Make DescriptorImpl support multiple subscripts So far, no descriptor exists that supports more than one sub-script descriptor. This will change with taproot, so prepare for this by changing the m_subdescriptor_arg from a unique_ptr to a vector of unique_ptr's.
Dmenec commented at 7:10 PM on January 29, 2026:

+1 to what @rkrux found.

Checked the code as well and m_pubkey_args.size() remains 1 for TR descriptors. The internal key is passed via Vector(std::move(internal_key)), which constructs a single-element vector.

I would suggest updating the comment to:

//! Public key arguments for this descriptor (size 1 for PK, PKH, WPKH, TR; any size for Multisig).

Also, it looks like the only real “any size” case for m_pubkey_args is multisig, since it is the one that passes a vector of multiple providers directly into DescriptorImpl.

MultisigDescriptor(int threshold, std::vector<std::unique_ptr<PubkeyProvider>> providers, bool sorted = false) : DescriptorImpl(std::move(providers), sorted ? "sortedmulti" : "multi"), m_threshold(threshold), m_sorted(sorted) {}

SHDescriptor don’t populate m_pubkey_args at all, it initializes it as an empty vector and stores the wrapped descriptor under m_subdescriptor_args:

SHDescriptor(std::unique_ptr<DescriptorImpl> desc) : DescriptorImpl({}, std::move(desc), "sh") {}

It might be worth updating the m_subdescriptor_args comment:

https://github.com/bitcoin/bitcoin/blob/5c5704e730796c6f31e2d7891bf6334674a04219/src/script/descriptor.cpp#L795-L798

As TRDescriptors are using m_subdescriptor_args.
rkrux commented at 3:06 PM on November 6, 2025: contributor

CR 768eeafbda3b36428d90239c92ddfb41402cfd0c
sedited commented at 11:05 AM on March 13, 2026: contributor

There has been an unaddressed review comment on this PR for many months already. Since this is a trivial documentation change, and the open review seems to indicate that this change is not correct, I'm closing this PR at this point. Since the comments do seem a bit confusing, another PR can be submitted with a proper correction if so desired.
sedited closed this on Mar 13, 2026