doc: external-signer, example 'createwallet' RPC call requires eight argument

cobratbq commented at 1:53 am on June 28, 2024: none

The eight argument ’true’ is necessary to invoke the external signer upon creating the wallet. The parameter defaults to ‘false’ otherwise.

DrahtBot commented at 1:53 am on June 28, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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

DrahtBot added the label Docs on Jun 28, 2024

in doc/external-signer.md:40 in f383c857ec outdated

36@@ -37,7 +37,7 @@ The master key fingerprint is used to identify a device.
37 Create a wallet, this automatically imports the public keys:
38 
39 ```sh
40-$ bitcoin-cli createwallet "hww" true true "" true true true
41+$ bitcoin-cli createwallet "hww" true true "" true true true true

maflcko commented at 6:38 am on June 28, 2024:

Could use named arguments?

cobratbq commented at 4:36 pm on June 28, 2024:

Done. I hope you like it … it’s become a bit convoluted, but it certainly is more descriptive.

cobratbq force-pushed on Jun 28, 2024

in doc/external-signer.md:40 in 487a7c0061 outdated

36@@ -37,7 +37,7 @@ The master key fingerprint is used to identify a device.
37 Create a wallet, this automatically imports the public keys:
38 
39 ```sh
40-$ bitcoin-cli createwallet "hww" true true "" true true true
41+$ bitcoin-cli -named createwallet wallet_name="hww" disable_private_keys=true blank=true passphrase="" avoid_reuse=true descriptors=true load_on_startup=true external_signer=true

Sjors commented at 8:26 am on July 2, 2024:

You should be able to drop most of these named arguments, except wallet_name and external_signer. Most are already defaults, except avoid_reuse but that’s not relevant to this tutorial.

cobratbq commented at 11:42 pm on July 7, 2024:

New version pushed with blank, avoid_reuse and load_on_startup removed.

Sjors commented at 8:27 am on July 2, 2024: member

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

cobratbq commented at 11:30 pm on July 7, 2024: none

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.

cobratbq force-pushed on Jul 7, 2024

DrahtBot added the label CI failed on Jul 8, 2024

achow101 commented at 10:35 pm on July 9, 2024: member

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.

cobratbq commented at 4:46 pm on July 10, 2024: none

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

DrahtBot removed the label CI failed on Jul 10, 2024

DrahtBot added the label CI failed on Jul 22, 2024

DrahtBot commented at 9:27 am on July 22, 2024: contributor

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

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Jul 22, 2024

fanquake requested review from Sjors on Aug 13, 2024

DrahtBot added the label CI failed on Aug 16, 2024

DrahtBot removed the label CI failed on Aug 19, 2024

doc: external-signer, createwallet RPC call requires eight argument

The eight argument 'true' is necessary to invoke the external signer upon
creating the wallet. The parameter defaults to 'false' otherwise.

cca20be8f1

cobratbq force-pushed on Aug 22, 2024

cobratbq commented at 11:19 pm on September 30, 2024: none

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.

DrahtBot added the label CI failed on Oct 23, 2024

DrahtBot removed the label CI failed on Oct 25, 2024

doc: external-signer, example ‘createwallet’ RPC call requires eight argument #30354

Code Coverage

Reviews