Closes #29552
Add better descriptions to help string for all binaries. Use format which is correctly detected by help2man when generating manpages.
Examples:
Before:
After:
Demonstration using bitcoin-cli also highlights removal of inline usage explanations which were being incorrectly formatted by help2man. This results in the following changed format to bitcoin-cli --help:
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29686.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | tdb3, rkrux, maflcko, achow101 |
| Concept ACK | edilmedeiros |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
140 | @@ -141,7 +141,13 @@ static bool ProcessInitCommands(ArgsManager& args) 141 | if (args.IsArgSet("-version")) { 142 | strUsage += FormatParagraph(LicenseInfo()); 143 | } else { 144 | - strUsage += "\nUsage: bitcoind [options] Start " PACKAGE_NAME "\n" 145 | + strUsage += "\n" 146 | + "The Bitcoin Core daemon (bitcoind) is a headless program that connects to the Bitcoin network to validate and relay transactions and blocks, as well as relaying addresses.\n\n"
PACKAGE_NAME should be used here
done
Why no bitcoin-qt?
158 | + "The bitcoin-cli utility provides a command line interface to interact with a bitcoind/bitcoin-qt RPC server.\n" 159 | + "\nIt can be used to query network information, manage wallets, create or broadcast transactions, and control the bitcoind server.\n" 160 | + "\nUse the \"help\" command to list all commands. Use \"help <command>\" to show help for that command.\n" 161 | + "Using the -named option allows specifying params using key=value style, and therefore doesn't require passing unused params.\n" 162 | + "\nUsage: bitcoin-cli [options] <command> [params]\n" 163 | + "or: bitcoin-cli [options] -named <command> [name=value]...\n"
Should we be keeping the spaces to line them up?
Re-added the spacing
150 | @@ -151,10 +151,15 @@ static int AppInitRPC(int argc, char* argv[]) 151 | strUsage += FormatParagraph(LicenseInfo()); 152 | } else { 153 | strUsage += "\n" 154 | - "Usage: bitcoin-cli [options] <command> [params] Send command to " PACKAGE_NAME "\n" 155 | - "or: bitcoin-cli [options] -named <command> [name=value]... Send command to " PACKAGE_NAME " (with named arguments)\n" 156 | - "or: bitcoin-cli [options] help List commands\n" 157 | - "or: bitcoin-cli [options] help <command> Get help for a command\n";
I think we might be losing some clarity by splitting the descriptions up from the usage lines
Yeah I did think about this, but I did add the same information to the paragraph above. Also, the current format is somewhat "broken" when genrating manpages currently (see first image in OP, all sorts of messy underlines).
I also checked a load of man pages and --help docs from other popular utils, and it seems that our current format is the outlier, in terms of including explanatory text inline with examples/usage strings.
Why no bitcoin-qt?
Dunno, think I somehow dropped it during a final rebase/tidy up. It's back in now
150 | @@ -151,10 +151,15 @@ static int AppInitRPC(int argc, char* argv[]) 151 | strUsage += FormatParagraph(LicenseInfo()); 152 | } else { 153 | strUsage += "\n" 154 | - "Usage: bitcoin-cli [options] <command> [params] Send command to " PACKAGE_NAME "\n" 155 | - "or: bitcoin-cli [options] -named <command> [name=value]... Send command to " PACKAGE_NAME " (with named arguments)\n" 156 | - "or: bitcoin-cli [options] help List commands\n" 157 | - "or: bitcoin-cli [options] help <command> Get help for a command\n"; 158 | + "The bitcoin-cli utility provides a command line interface to interact with a " PACKAGE_NAME " RPC server.\n"
This was better without PACKAGE_NAME here (since it isn't tied to a specific program)
Kind of not agree since the tool is intended to be used with Bitcoin Core RPC and maintained for that only purpose. On the other hand, I understand that knots (and other forks) is compatible in this regard with Core and the bitcoin-cli tool will work anyhow without discriminating the server.
I also don't agree? This tool is designed to work with Bitcoin Core's RPC server.
56 | @@ -57,9 +57,12 @@ HelpMessageDialog::HelpMessageDialog(QWidget *parent, bool about) : 57 | ui->aboutMessage->setWordWrap(true); 58 | ui->helpMessage->setVisible(false); 59 | } else { 60 | - setWindowTitle(tr("Command-line options")); 61 | - QString header = "Usage: bitcoin-qt [command-line options] [URI]\n\n" 62 | - "Optional URI is a Bitcoin address in BIP21 URI format.\n"; 63 | + setWindowTitle(tr("options"));
This seems strange
Correct, I thought this had been interferring with the manpage section title, but it's not. Reverted.
62 | - "Optional URI is a Bitcoin address in BIP21 URI format.\n"; 63 | + setWindowTitle(tr("options")); 64 | + QString header = "The bitcoin-qt application provides a graphical interface for interacting with " PACKAGE_NAME ".\n\n" 65 | + "It combines the core functionalities of bitcoind with a user-friendly interface for wallet management, transaction history, and network statistics.\n\n" 66 | + "It is suitable for desktop users preferring a graphical over a command-line interface.\n\n" 67 | + "You can optionally specify a [BIP21URI] using the BIP21 URI format.\n\n"
We should eliminate unnecessary specific BIP21 references in favour of just URI or maybe "payment URI" (see discussion of a new URI BIP on the horizon).
I suppose you mean https://delvingbitcoin.org/t/revisiting-bip21/630 ? Whilst I am in favour of many parts of that discussion, istm that we should probably update this to include the new format, after a new format is specified?
I'm going to take the suggestion to just use [URI], but will still give BIP21 as an example, so folks reading the manpage can find the format of a (the only) valid URI format accepted (for now).
ACK for b680c1c6ffdb63a3f345d3e8ff80be9a547ca0e1
Nice work. Seems like an improvement for users and better aligns with common manpages.
Pulled the PR branch, built, ran all unit and functional tests (all passed). Following contrib/devtools/README.md, ran gen-manpages.py
contrib/devtools/gen-manpages.py
and checked the output of each of the pages with man -l doc/man/<file>.
bitcoin-qt was not inspected.
All appeared as expected to align with the style presented in the opening post.
59 | @@ -60,7 +60,9 @@ static int AppInitUtil(ArgsManager& args, int argc, char* argv[]) 60 | strUsage += FormatParagraph(LicenseInfo()); 61 | } else { 62 | strUsage += "\n" 63 | - "Usage: bitcoin-util [options] [commands] Do stuff\n"; 64 | + "The bitcoin-util tool is used to grind proof of work on a hex-formatted bitcoin block header.\n"
I don't think this is accurate. The stuff it does depends on the command that is passed to the utility.
I think I see your point, in that we may add more commands to it in the future, so this should be made more generic now?
Yes, otherwise, the executable would have been called bitcoin-grind, no?
"The bitcoin-util tool provides bitcoin related functionality that does not rely on the ability to access a running node.\n"
Why not use the description from #19937 which introduced the tool?
Done
146 | + "The " PACKAGE_NAME " daemon (bitcoind) is a headless program that connects to the Bitcoin network to validate and relay transactions and blocks, as well as relaying addresses.\n\n" 147 | + "It provides the backbone of the Bitcoin network and its RPC, REST and ZMQ services can provide various transaction, block and address-related services.\n\n" 148 | + "There is an optional wallet component which provides cutting-edge transaction services.\n\n" 149 | + "It can be used in a headless environment or as part of a server setup.\n" 150 | + "\n" 151 | + "Usage: bitcoind [options]\n"
"Usage: bitcoind [options] Start " PACKAGE_NAME "\n"
109 | @@ -110,8 +110,11 @@ static int AppInitRawTx(int argc, char* argv[]) 110 | strUsage += FormatParagraph(LicenseInfo()); 111 | } else { 112 | strUsage += "\n" 113 | - "Usage: bitcoin-tx [options] <hex-tx> [commands] Update hex-encoded bitcoin transaction\n" 114 | - "or: bitcoin-tx [options] -create [commands] Create hex-encoded bitcoin transaction\n" 115 | + "The bitcoin-tx tool is used for creating and modifying bitcoin transactions.\n\n" 116 | + "bitcoin-tx can be used with \"<hex-tx> [commands]\" to update a hex-encoded bitcoin transaction, or with \"-create [commands]\" to create a hex-encoded bitcoin transaction.\n" 117 | + "\n" 118 | + "Usage: bitcoin-tx [options] <hex-tx> [commands]\n"
"Usage: bitcoin-tx [options] <hex-tx> [commands] Update hex-encoded bitcoin transaction\n"
114 | - "or: bitcoin-tx [options] -create [commands] Create hex-encoded bitcoin transaction\n" 115 | + "The bitcoin-tx tool is used for creating and modifying bitcoin transactions.\n\n" 116 | + "bitcoin-tx can be used with \"<hex-tx> [commands]\" to update a hex-encoded bitcoin transaction, or with \"-create [commands]\" to create a hex-encoded bitcoin transaction.\n" 117 | + "\n" 118 | + "Usage: bitcoin-tx [options] <hex-tx> [commands]\n" 119 | + "or: bitcoin-tx [options] -create [commands]\n"
"or: bitcoin-tx [options] -create [commands] Create hex-encoded bitcoin transaction\n"
This is not rendering nicely in the terminal:
Usage: bitcoin-tx [options] <hex-tx> [commands]
or: bitcoin-tx [options] -create [commands]
When it should be
Usage: bitcoin-tx [options] <hex-tx> [commands]
or: bitcoin-tx [options] -create [commands]
fixed in 61e73920d8
160 | + "\nUse the \"help\" command to list all commands. Use \"help <command>\" to show help for that command.\n" 161 | + "Using the -named option allows specifying params using key=value style, and therefore doesn't require passing unused params.\n" 162 | + "\nUsage: bitcoin-cli [options] <command> [params]\n" 163 | + "or: bitcoin-cli [options] -named <command> [name=value]...\n" 164 | + "or: bitcoin-cli [options] help\n" 165 | + "or: bitcoin-cli [options] help <command>\n"
"\nUsage: bitcoin-cli [options] <command> [params] Send command to " PACKAGE_NAME "\n"
"or: bitcoin-cli [options] -named <command> [name=value]... Send command to " PACKAGE_NAME " (with named arguments)\n"
"or: bitcoin-cli [options] help List commands\n"
"or: bitcoin-cli [options] help <command> Get help for a command\n"
Unfortunately these types of descriptions pretty much break help2man's detection algorithms, see images in initial post.
Concept ACK
I would keep the old comment for each command even in face of the additional information provided (see review comments). I tend to rely a lot on those to quickly remember what the commands do (but will not oppose taking them out if this is the consensus). I understand that they may break the formatting of the manual pages, so feel free to discard them.
For the bitcoin-util tool, I suggest taking the description from the PR that introduced the tool (see comment).
I would keep the old comment for each command even in face of the additional information provided (see review comments). I tend to rely a lot on those to quickly remember what the commands do (but will not oppose taking them out if this is the consensus). I understand that they may break the formatting of the manual pages, so feel free to discard them.
Yes, these extra, "inline" descriptions do get in the way of automatic manpage generation unfortunately.
59 | @@ -60,7 +60,10 @@ static int AppInitUtil(ArgsManager& args, int argc, char* argv[]) 60 | strUsage += FormatParagraph(LicenseInfo()); 61 | } else { 62 | strUsage += "\n" 63 | - "Usage: bitcoin-util [options] [commands] Do stuff\n"; 64 | + "The bitcoin-util tool provides bitcoin related functionality that does not rely on the ability to access a running node. Available [commands] are listed below.\n" 65 | + "\n" 66 | + "Usage: bitcoin-util [options] [command]\n" 67 | + "or: bitcoin-util [options] grind <hex-block-header>\n";
I just realized that bitcoin-util does not have a help command like bitcoin-cli. Looks silly now as it has only one command, but if the tool is to increase, this seems to be a fairly easy improvement someone else can work on. In case there's another PR for that, let's try to make this more close to the bitcoin-cli message mentioning the help command only.
What about, instead of this, we just remove it altogether in favor of the Commands section that's already there? That way, it will follow the same pattern as bitcoin-wallet.
I think it's helpful to detail the only command.
156 | - "or: bitcoin-cli [options] help List commands\n" 157 | - "or: bitcoin-cli [options] help <command> Get help for a command\n"; 158 | + "The bitcoin-cli utility provides a command line interface to interact with a " PACKAGE_NAME " RPC server.\n" 159 | + "\nIt can be used to query network information, manage wallets, create or broadcast transactions, and control the " PACKAGE_NAME " server.\n" 160 | + "\nUse the \"help\" command to list all commands. Use \"help <command>\" to show help for that command.\n" 161 | + "Using the -named option allows specifying params using key=value style, and therefore doesn't require passing unused params.\n"
"The -named option allows specifying parameters using key=value instead of positional style.\n"
IMO the former provides more information (that unused params aren't needed)
What about
"The -named option allows specifying parameters using key=value style, and therefore doesn't require passing unused params.\n"
Starting with the verb seem to be an odd style compared to all the other strings this PR touches.
Re-worded in 4aff4ea92c96f4f66e0159329ad8d01d4cf7bce7
57 | @@ -58,8 +58,11 @@ HelpMessageDialog::HelpMessageDialog(QWidget *parent, bool about) : 58 | ui->helpMessage->setVisible(false); 59 | } else { 60 | setWindowTitle(tr("Command-line options")); 61 | - QString header = "Usage: bitcoin-qt [command-line options] [URI]\n\n" 62 | - "Optional URI is a Bitcoin address in BIP21 URI format.\n"; 63 | + QString header = "The bitcoin-qt application provides a graphical interface for interacting with " PACKAGE_NAME ".\n\n" 64 | + "It combines the core functionalities of bitcoind with a user-friendly interface for wallet management, transaction history, and network statistics.\n\n" 65 | + "It is suitable for desktop users preferring a graphical over a command-line interface.\n\n"
"It is suitable for desktop users who prefer a graphical over a command-line interface.\n\n"
Taken in 61e73920d8
Other than those comments, I compiled everything and ran the tests. Man pages seem fine too.
re ACK 47f50c7af5572520fd986b313a63a44a76d3c859
Ran
BUILDDIR=$PWD/build contrib/devtools/gen-manpages.py
and checked the output of each of the pages with man -l doc/man/<file>.
Also displayed "Command-line options" in the GUI.
tACK 47f50c7af5572520fd986b313a63a44a76d3c859
Successful make and functional tests. Certainly an improvement for all the executables we have. I generated the man pages for all 6 of them and verified the contents using man ./doc/man/<executable-name>.1. Also, compiled and ran the GUI & checked the help section over there. PFB the screenshots from my setup.
<img width="789" alt="Screenshot 2024-11-08 at 12 22 09 PM" src="https://github.com/user-attachments/assets/0db4fa0c-5be0-4f70-b7ca-683e48137125"> <img width="1680" alt="Screenshot 2024-11-08 at 12 23 03 PM" src="https://github.com/user-attachments/assets/22a2cdb1-c8a8-4972-949f-f5458545df00">
@willcl-ark I just realised it would be great to commit the newly generated man pages here? https://github.com/bitcoin/bitcoin/tree/master/doc/man
There are already outdated man pages in that directory, might as well update them now.
73 | - "To change the target wallet, use the -datadir, -wallet and -regtest/-signet/-testnet/-testnet4 arguments.\n\n" 74 | - "Usage:\n" 75 | - " bitcoin-wallet [options] <command>\n"; 76 | + "bitcoin-wallet is an offline tool for creating and interacting with " CLIENT_NAME " wallet files.\n\n" 77 | + "By default bitcoin-wallet will act on wallets in the default mainnet wallet directory in the datadir.\n\n" 78 | + "To change the target wallet, use the -datadir, -wallet and -regtest / -signet / -testnet arguments.\n"
nit: Why remove testnet4? Makes sense to not duplicate everything here, but leaving the others seems inconsistent. I'd suggest to just say: "..., or the (test)chain selection arguments.\n"
I just realised it would be great to commit the newly generated man pages here? https://github.com/bitcoin/bitcoin/tree/master/doc/man
There are already outdated man pages in that directory, might as well update them now.
I don't understand. How are the placeholders outdated and how should they be updated?
ACK 47f50c7af5572520fd986b313a63a44a76d3c859 ðŸ“
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 47f50c7af5572520fd986b313a63a44a76d3c859 ðŸ“
sXhLu7skycKS+J3E/I54n61bnjM10TUKeni4UU2rKwLj5EOx9m6J8P5CTKenz+7wrLc7Hq/ZxW6V4AgWPV2qCg==
</details>
I just realised it would be great to commit the newly generated man pages here? https://github.com/bitcoin/bitcoin/tree/master/doc/man There are already outdated man pages in that directory, might as well update them now.
I don't understand. How are the placeholders outdated and how should they be updated?
Oh my bad, I forgot they are placeholders.
ACK 47f50c7af5572520fd986b313a63a44a76d3c859
@willcl-ark Thanks!
Aoproved