[RPC] Expand help text for importmulti changes #15122

Expands the RPC help text for changes to the importmulti RPC method.

Requested here: #14565 (comment)

cc @Sjors @instagibbs

Thanks for the quick review @instagibbs . Pushed a fixup commit 6b55e629b22843ac53e8e37be4995a0dc3d6c0c1 to address your comments.

Spurious travis failure here: https://travis-ci.org/bitcoin/bitcoin/jobs/476515100

Kicking travis.

utACK 1c8e7a6

utACK after squash

Squashed. @harding - do you mind taking a quick look?

This would be great documentation to add to importmulti help, but it's a bad release note because it tells you almost nothing about how the new behavior is different than the old behavior, and it sounds like everything described is new when in reality most of it is unchanged, and most existing uses of importmulti should be unaffected.

A better release note would be a simple bulleted list of changes like: #14565 (comment)

I agree that the release notes aren't necessarily the perfect place for this, but I also don't think the solvability/spendability definition should go in the importmulti help text, since those concepts are more widely applicable. I added these to the release notes because I think it's better to have them there than nowhere at all, but if you have a suggestion for improving how the docs are structured and a better place for his should live, I'd be very happy to make those changes.

I'm mildly opposed to this approach. I would really like to see the existing bullet points kept, as they quickly describe the user-visible changes in the section where readers would expect to learn about those changes.

If you then remove the corresponding changes from the new section, you have a section that describes the long-standing behavior of Bitcoin Core---which is not something that belongs in a new release note. And, while I agree that it's better to have documentation somewhere than nowhere, I don't think understanding this concept is important enough for most users of Bitcoin Core (who don't use it as a waching-only or multisig wallet) that we should subject them to learning about it in the release notes we want all users to read before upgrading.

Although solvability and spendability are mostly wallet concepts, the only way the user can really interface with those concepts right now is through the RPC, so I think it would not be inappropriate to add that documentation to the RPC overview: https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md If that is done, than a link can be added to the release notes pointing to that documentation (e.g. one of the getaddressinfo bullet points in the current release notes describes solvability; a link can be added to the end of it for "Learn more").

As for the actual text, I'd suggest rearranging the first two bullet points so that you can describe the second in terms of differences from the first:

• A script is spendable if the wallet can construct the transaction input to spend outputs to the script and produce all required signatures or witness data.

• If you can do everything except produce the required signatures or witness data, then a script is solvable.

Agree with @harding, but I actually don't know why users need to think about solvability and spendability in these terms unless they are importing external keys, so it seems ideal to me to have these definitions in the importmulti help text. But defining them in the JSON-RPC markdown documentation instead as suggested, or in another markdown file and then referencing them from the RPC help also seems fine.

I like @harding's suggestion of moving the detailed explanation to the (painfully short) doc/JSON-RPC-interface.md. In addition to pointing to it from the release notes, the RPC help could also point to it (as we do with descriptors.md).

I don't think understanding this concept is important enough for most users of Bitcoin Core (who don't use it as a waching-only or multisig wallet) that we should subject them to learning about it in the release notes we want all users to read before upgrading.

Yes, I think that's reasonable. I've changed this branch to only add text about the watchonly flag to the rpc help text.

I think the information about solvability and spendability is useful, but I don't think it fits naturally in the JSON-RPC-interface.md doc. For now, I'm just including it in my own wallet design notes gist. If people think there's something there of wider use, then I can perhaps add a docs/wallet_design.md file to this repo.

I'm very receptive to feedback on this. If people don't think the added text in the RPC help is useful, then I'm fine to close this PR (and apologise for the noise).

@jnewbery

I've changed this branch to only add text about the watchonly flag to the rpc help text.

I think maybe you didn't push; I don't see that.

I think maybe you didn't push

Oops. Sorry. Pushed!

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14987 (RPCHelpMan: Pass through Result and Examples by MarcoFalke)
• #14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)

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.

Partially tested ACK 319047318b1c06d286f7dfcd37262d1e2a10890d (looked at rendered help message; did not check modified warning message). Thanks!

utACK 319047318b1c06d286f7dfcd37262d1e2a10890d

Addressed @ryanofsky's comments.

utACK a79fcd667a1d47c46864068d7a30ef58d1002b9d

I'm so confused what happened

On Tue, Jan 15, 2019, 10:56 John Newbery <notifications@github.com wrote:

@jnewbery commented on this pull request.

In src/wallet/rpcdump.cpp https://github.com/bitcoin/bitcoin/pull/15122#discussion_r247949302:

@@ -1148,7 +1148,9 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) if (mainRequest.fHelp || mainRequest.params.size() < 1 || mainRequest.params.size() > 2) throw std::runtime_error( RPCHelpMan{"importmulti",

•            "\nImport addresses/scripts (with private or public keys, redeem script (P2SH)), rescanning all addresses in one-shot-only (rescan can be disabled via options). Requires a new wallet backup.\n",

•            "\nImport addresses/scripts (with private or public keys, redeem script (P2SH)), optionally rescanning the blockchain from the earliest creation time of the imported scripts. Requires a new wallet backup.\n"

•            "If an address/script is imported without all of the private keys required to spend from that address, it will be watchonly. The 'watchonly' option must be set to True or a warning will be returned.\n"

•            "Conversely, if all the private keys are provided and the address/script is spendable, the watchonly option must be set to False, or a warning will be returned.\n",

That's a fair point, and was discussed in the original PR: #14565 (comment) https://github.com/bitcoin/bitcoin/pull/14565#discussion_r238422419.

This PR simply updates the help text to better explain the current functionality. Any discussion of changes to that functionality should be in a separate PR/issue.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/15122#discussion_r247949302, or mute the thread https://github.com/notifications/unsubscribe-auth/AsfTHlK65mIOE-Ajpxsz9fFxry6oNueJks5vDfoogaJpZM4Zz4f1 .

Damn I'm sorry I dont know ow I copied the wrong mnemonic.. I'm completely lost now. I apologize if I made more work for any1 or caused issues