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

Reviews

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #33765 (doc: update interface, --stdin flag, signtx (Docs: "External signer" documentation is outdated. (plz close if unwanted request) #31005) by cobratbq)
• #33112 (wallet: relax external_signer flag constraints by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Do you happen to know which pull request added a new positional argument in the middle? Or was this instruction always broken? cc @achow101

Do you happen to know which pull request added a new positional argument in the middle? Or was this instruction always broken? cc @achow101

I don't know. I'm new to this. However, in working with this documentation, it seems that there are more discrepancies or inaccuracies. It seems now the account-ID in getdescriptors is provided using --account 0, for example.

I'm currently going through the bitcoin-core sources, to figure out where my other mistakes are, because the external-signer documentation is not sufficient. (Not so much meant as complaint, but rather a heads-up on the status.)

Another thing is the use of descriptors. I am not well informed, but it seems that a certain path is required. I think it follows the idea outlined in BIP-43, which for legacy is defined in BIP-44. However, the docs also say:

A future extension could add an optional return field with device capabilities. Perhaps a descriptor with wildcards. For example: ["pkh("44'/0'/$'/{0,1}/"), sh(wpkh("49'/0'/$'/{0,1}/")), wpkh("84'/0'/$'/{0,1}/*")]. This would [..]

The docs on descriptors (descriptors.md) also show examples, such as pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8), and I have no idea whether this is supposed to be supported or not.

Do you happen to know which pull request added a new positional argument in the middle? Or was this instruction always broken? cc @achow101

Highly likely it was always broken. I'd be surprised if a PR that added a positional argument in the middle would make it through code review.

A future extension could add an optional return field with device capabilities. Perhaps a descriptor with wildcards. For example: ["pkh("44'/0'/$'/{0,1}/"), sh(wpkh("49'/0'/$'/{0,1}/")), wpkh("84'/0'/$'/{0,1}/*")]. This would [..]

The docs on descriptors (descriptors.md) also show examples, such as pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8), and I have no idea whether this is supposed to be supported or not.

That suggestion is neither implemented nor specified.

Highly likely it was always broken. I'd be surprised if a PR that added a positional argument in the middle would make it through code review.

Okay, that's clear. Well we can fix it now, and add the named arguments.

The docs on descriptors (descriptors.md) also show examples, such as pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8), and I have no idea whether this is supposed to be supported or not.

That suggestion is neither implemented nor specified.

Okay, so if I understand correctly: getdescriptors expects results in the forms specified by BIP-44, BIP-49 and BIP-84 only. (All of which seem to follow the idea of BIP-43.)

So I guess we should update the getdescriptors section to make this explicit. Does this mean that getdescriptors also is not optional, but instead required? I.e. the wallet requires descriptors to be useful, does it not? (That's what the RPC commands suggest.)

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/27138140194}

In addition: documentation is completely lacking for flags --stdin and which stdin-commands to expect, including their format. Additionally, if we passing in to stdin directly, why wouldn't we pass the PSBT in binary form? Just have signtx prefix then the binary data. (I guess this is too late now, but there is no reasoning or documentation about this anywhere in the spec.)
Furthermore, --account and --chain parameters.

So: --account, --chain, --stdin and in case of stdin whatever is supported.

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

cc @Sjors

Instead of bitcoin-cli -named let's recommend bitcoin rpc (introduced in #31375 and shipped as of v30, it automatically sets -named).