Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RPC] Expand help text for importmulti changes #15122

Merged
merged 1 commit into from Jan 16, 2019

Conversation

Projects
None yet
@jnewbery
Copy link
Member

commented Jan 7, 2019

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

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Requested here: #14565 (comment)

cc @Sjors @instagibbs

Show resolved Hide resolved doc/release-notes.md Outdated
Show resolved Hide resolved doc/release-notes.md Outdated
Show resolved Hide resolved doc/release-notes.md Outdated
Show resolved Hide resolved doc/release-notes.md Outdated
@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

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

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

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

Kicking travis.

@fanquake
Copy link
Member

left a comment

utACK 1c8e7a6

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

utACK after squash

@jnewbery jnewbery force-pushed the jnewbery:pr14565_release_notes branch Jan 8, 2019

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

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)

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2019

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.

@harding

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

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.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

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

doc/release-notes.md Outdated

Imported scripts/addresses are categorised by solvability and spendability:

- A script is *solvable* if the the wallet can construct the transaction input

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 9, 2019

Member

Accidental repeating of "the" :-)

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

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

@harding

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

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

@jnewbery jnewbery force-pushed the jnewbery:pr14565_release_notes branch Jan 11, 2019

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

I think maybe you didn't push

Oops. Sorry. Pushed!

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2019

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

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.

@harding

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

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

src/wallet/rpcdump.cpp Outdated
"\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",

This comment has been minimized.

Copy link
@promag

promag Jan 15, 2019

Member

I think in this case it should fail instead of warn, and a separate PR should fix that. It's easy to call again with watchonly=false but it's not so easy to undo the import.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 15, 2019

Author Member

That's a fair point, and was discussed in the original PR: #14565 (comment).

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.

@jnewbery jnewbery changed the title [docs] Expand release notes for importmulti changes [RPC] Expand help text for importmulti changes Jan 15, 2019

@ryanofsky
Copy link
Contributor

left a comment

utACK 319047318b1c06d286f7dfcd37262d1e2a10890d

src/wallet/rpcdump.cpp Outdated
@@ -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"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

Would s/True/true in this case/ and s/False/false/. True and False are only capitalized in python, not javascript, and without qualifying True, the sentence doesn't sound connected to the previous text.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 15, 2019

Author Member

fixed

src/wallet/rpcdump.cpp Outdated
@@ -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"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

Maybe /spend from that address/spend it/

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 15, 2019

Author Member

I think 'spend from that address' is more precise. Addresses aren't spent, UTXOs are.

@jnewbery jnewbery force-pushed the jnewbery:pr14565_release_notes branch Jan 15, 2019

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

Addressed @ryanofsky's comments.

@ryanofsky
Copy link
Contributor

left a comment

utACK a79fcd667a1d47c46864068d7a30ef58d1002b9d

src/wallet/rpcdump.cpp Outdated
@@ -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"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 15, 2019

Contributor

Again would say "must be set to true in this case"

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 15, 2019

Author Member

ah, sorry. Missed that last time.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jan 15, 2019

Author Member

now fixed

@inkedup369

This comment has been minimized.

Copy link

commented Jan 15, 2019

@jnewbery jnewbery force-pushed the jnewbery:pr14565_release_notes branch to b745e14 Jan 15, 2019

@laanwj laanwj merged commit b745e14 into bitcoin:master Jan 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 16, 2019

Merge #15122: [RPC] Expand help text for importmulti changes
b745e14 [docs] Expand help text for importmulti changes (John Newbery)

Pull request description:

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

Tree-SHA512: e90e5abf66bba3863e7519b5f79c26d18a4d624e6e7878293bdd4ebb57f1a01c67de52e4a5621901a8cb87fb3516264b3b1a826997c7c3c17b11216f1f1a3db0

@jnewbery jnewbery deleted the jnewbery:pr14565_release_notes branch Jan 16, 2019

@inkedup369

This comment has been minimized.

Copy link

commented Jan 16, 2019

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

@bitcoin bitcoin locked and limited conversation to collaborators Jan 16, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.