rpc: add require_checksum flag to deriveaddresses #24162

pull kallewoof wants to merge 1 commits into bitcoin:master from kallewoof:202201-deriveaddr-nochecksum changing 2 files +29 −6
  1. kallewoof commented at 5:58 AM on January 26, 2022: member

    This allows a user to derive the addresses for some privkey without first having to derive the descriptor checksum.

    Alternative to #24161.

  2. in src/rpc/misc.cpp:265 in 12300e9b43 outdated 
     263 |          [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 264 |  {
 265 | -    RPCTypeCheck(request.params, {UniValue::VSTR, UniValueType()}); // Range argument is checked later
 266 | +    RPCTypeCheck(request.params, {UniValue::VSTR, UniValueType(), UniValue::VBOOL}); // Range argument is checked later
 267 |      const std::string desc_str = request.params[0].get_str();
 268 | +    const bool require_checksum = request.params[2].isNull() || request.params[2].get_bool();
    maflcko commented at 6:21 AM on January 26, 2022: 
        const bool require_checksum{request.params[2].isNull() ? true : request.params[2].get_bool()};

    nit: I prefer to print the default value as literal. This makes it easier to change the value without changing the logic.

    kallewoof commented at 6:37 AM on January 26, 2022:

    Makes sense, incorporated.

  3. kallewoof force-pushed on Jan 26, 2022
  4. DrahtBot added the label RPC/REST/ZMQ on Jan 26, 2022
  5. shaavan commented at 8:46 AM on January 26, 2022: contributor

    ~Concept NACK~

    The purpose of checksum is to ensure the integrity of data and ensure that the data has not been wrongly entered or tampered with.

    • Say you use a misspelled private key and used it to generate an address. Any bitcoin received on that address would potentially be unrecoverable and hence burned, which is a potentially very devastating situation.

    Though I understand this feature can be helpful for someone regularly transacting with the same key, and hence is familiar with the risk and how to ensure the integrity of the key. Still, the potential risk involved makes this feature not worth the risk.

  6. kallewoof commented at 9:32 AM on January 26, 2022: member

    The purpose of checksum is to ensure the integrity of data and ensure that the data has not been wrongly entered or tampered with.

    The descriptor checksum, yes. The WIF format (private key) already has a built-in checksum.

    See the alternative, where the descriptor checksum is derived using getdescriptorinfo first, before being passed to deriveaddresses. The scenario is identical, but it requires an extra step due to the checksum requirement.

    • Say you use a misspelled private key and used it to generate an address. Any bitcoin received on that address would potentially be unrecoverable and hence burned, which is a potentially very devastating situation.

    It would fail. The WIF format includes a checksum. Besides, a WIF privkey that had a typo and still passed the WIF checksum would be a valid private key, and all derived addresses would thus be valid recipients for whoever held that private key.

  7. shaavan commented at 11:46 AM on January 26, 2022: contributor

    It would fail. The WIF format includes a checksum.

    I checked it, and it seems like it is correct.

    I took the private key present in the example to derive addresses on this PR. It worked correctly as expected. Screenshot from 2022-01-26 17-06-00

    However, when I made the following change to the private key to simulate a typo: (The tenth digit has been changed from 8 -> 2)

    - cPsQTSmMZ8e3AEUWGjS73f5R364yJxH6RxcgnwbHjbKbFPUP2Dtu
+ cPsQTSmMZ2e3AEUWGjS73f5R364yJxH6RxcgnwbHjbKbFPUP2Dtu

    The expression failed, showing that the private key was invalid.

    Screenshot from 2022-01-26 17-08-11

    I have retracted my NACK from the last review. Let me do some further reviewing before forming a decision on this PR.

    I was curious about one thing, though. I went to some forums and saw WIF had been around for a long time now. Why was this redundant 2-step behavior of deriveaddresses was not addressed earlier?

  8. kallewoof commented at 12:31 PM on January 26, 2022: member

    I was curious about one thing, though. I went to some forums and saw WIF had been around for a long time now. Why was this redundant 2-step behavior of deriveaddresses was not addressed earlier?

    I believe deriveaddresses is fairly new, and the "find all addresses" use case, while useful, was probably not high priority.

  9. DrahtBot commented at 11:40 PM on January 26, 2022: contributor

    <!--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:

    • #26039 (rpc: Run type check against RPCArgs (1/2) by MarcoFalke)
    • #25366 (util, rpc: Add parameter to deriveaddresses to display address information by w0xlt)
    • #24161 (rpc/doc: describe using combo(privkey) to get checksum and then list … by kallewoof)
    • #23549 (Add scanblocks RPC call (attempt 2) by jamesob)
    • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

    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.

  10. in src/rpc/misc.cpp:246 in 48e99357da outdated 
     242 | @@ -243,6 +243,7 @@ static RPCHelpMan deriveaddresses()
 243 |              {
 244 |                  {"descriptor", RPCArg::Type::STR, RPCArg::Optional::NO, "The descriptor."},
 245 |                  {"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED_NAMED_ARG, "If a ranged descriptor is used, this specifies the end or the range (in [begin,end] notation) to derive."},
 246 | +                {"require_checksum", RPCArg::Type::BOOL, RPCArg::Default{true}, "Fail if the descriptor checksum is invalid or missing."},
    Sjors commented at 5:57 PM on January 27, 2022:

    Let's always fail if it's invalid. So if the user doesn't provide one and require_checksum is false we're happy, but if they provide a checksum, is must be valid.

    kallewoof commented at 1:16 AM on January 28, 2022:

    That is already the case.

    $ ./bitcoin-cli -regtest getdescriptorinfo "combo(cTsWy7DsaN2uKgVueRHcUKAP3mCNzX5r77GsiLWw9C9zapjiK8tQ)"
{
  "descriptor": "combo(02fdbfbbfc35d0748ec58f628988090fce16985087e89639bccc4bfec4056323dc)#lc57gg0x",
  "checksum": "y9v74p5f",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": true
}
$ ./bitcoin-cli -regtest deriveaddresses "combo(02fdbfbbfc35d0748ec58f628988090fce16985087e89639bccc4bfec4056323dc)#lc57gg0x"
[
  "mtrCy8QYfDU7tjPnkpWwWdQomuRfjptFy2",
  "mtrCy8QYfDU7tjPnkpWwWdQomuRfjptFy2",
  "bcrt1qjg7un9j86eld6y3sg0u0scmxedv6hued2z9m89",
  "2NDtkyew2s7qD1HYDXkNZTi1HY7nT7ZhB72"
]
$ ./bitcoin-cli -regtest deriveaddresses "combo(02fdbfbbfc35d0748ec58f628988090fce16985087e89639bccc4bfec4056323dc)#lc57gg0x" null false
[
  "mtrCy8QYfDU7tjPnkpWwWdQomuRfjptFy2",
  "mtrCy8QYfDU7tjPnkpWwWdQomuRfjptFy2",
  "bcrt1qjg7un9j86eld6y3sg0u0scmxedv6hued2z9m89",
  "2NDtkyew2s7qD1HYDXkNZTi1HY7nT7ZhB72"
]
$ ./bitcoin-cli -regtest deriveaddresses "combo(02fdbfbbfc35d0748ec58f628988090fce16985087e89639bccc4bfec4056323dc)#lc57gg1x" null false
error code: -5
error message:
Provided checksum 'lc57gg1x' does not match computed checksum 'lc57gg0x'

    I've updated the description to reflect this.

  11. Sjors commented at 5:58 PM on January 27, 2022: member

    Concept ACK

  12. kallewoof force-pushed on Jan 28, 2022
  13. in src/rpc/misc.cpp:246 in 8d3c91d049 outdated 
     242 | @@ -243,6 +243,7 @@ static RPCHelpMan deriveaddresses()
 243 |              {
 244 |                  {"descriptor", RPCArg::Type::STR, RPCArg::Optional::NO, "The descriptor."},
 245 |                  {"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED_NAMED_ARG, "If a ranged descriptor is used, this specifies the end or the range (in [begin,end] notation) to derive."},
 246 | +                {"require_checksum", RPCArg::Type::BOOL, RPCArg::Default{true}, "Fail even if the descriptor checksum is missing. (Always fails if a checksum is present, but invalid.)"},
    Sjors commented at 8:42 AM on January 28, 2022:

    "Require a checksum. If a checksum is provided it will be checked regardless of this parameter."

  14. in src/rpc/misc.cpp:258 in 8d3c91d049 outdated 
     251 | @@ -251,14 +252,17 @@ static RPCHelpMan deriveaddresses()
 252 |                  }
 253 |              },
 254 |              RPCExamples{
 255 | -                "First three native segwit receive addresses\n" +
 256 | +                "First three native segwit receive addresses:\n" +
 257 |                  HelpExampleCli("deriveaddresses", "\"" + EXAMPLE_DESCRIPTOR + "\" \"[0,2]\"") +
 258 | -                HelpExampleRpc("deriveaddresses", "\"" + EXAMPLE_DESCRIPTOR + "\", \"[0,2]\"")
 259 | +                HelpExampleRpc("deriveaddresses", "\"" + EXAMPLE_DESCRIPTOR + "\", \"[0,2]\"") +
 260 | +                "Derive all addresses from a privkey cPsQTSmMZ8e3AEUWGjS73f5R364yJxH6RxcgnwbHjbKbFPUP2Dtu:\n" +
    Sjors commented at 8:46 AM on January 28, 2022:

    "Derive all addresses from a WIF, which has a built-in checksum:\n"

    kallewoof commented at 9:11 AM on January 28, 2022:

    "a WIF privkey" seems more correct to me, but going with your suggestion.

  15. Sjors commented at 8:48 AM on January 28, 2022: member

    utACK 8d3c91d04931c9a2ad44ca04175944c617fe149e

    Note that there are several things we typically put in a descriptor that already have a checksum, namely the xpriv and xpub. The main scenario where it doesn't make sense to use a descriptor checksum, is when you have to generate the descriptor manually. That's clearly the case if you import a WIF from some older software or a paper backup. But there may be watch-only scenario's too, as many wallet software supports the export of a single xpub. That's why I removed the emphasis on "private key" in the help text below.

  16. kallewoof force-pushed on Jan 28, 2022
  17. Sjors commented at 9:16 AM on January 28, 2022: member

    re-utACK e34c8e1b63628986d496b5f7bb29addac50e459a

  18. in src/rpc/misc.cpp:246 in e34c8e1b63 outdated 
     242 | @@ -243,6 +243,7 @@ static RPCHelpMan deriveaddresses()
 243 |              {
 244 |                  {"descriptor", RPCArg::Type::STR, RPCArg::Optional::NO, "The descriptor."},
 245 |                  {"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED_NAMED_ARG, "If a ranged descriptor is used, this specifies the end or the range (in [begin,end] notation) to derive."},
 246 | +                {"require_checksum", RPCArg::Type::BOOL, RPCArg::Default{true}, "Require a checksum. If a checksum is provided it will be checked regardless of this parameter."},
    luke-jr commented at 2:29 AM on January 31, 2022:

    This wouldn't ordinarily be needed, and booleans in general aren't very clear as positional arguments, so it should use an options Object instead.

    kallewoof commented at 3:47 AM on January 31, 2022:

    Switched to an options object.

  19. luke-jr changes_requested
  20. kallewoof force-pushed on Jan 31, 2022
  21. kallewoof force-pushed on Jan 31, 2022
  22. shaavan commented at 8:59 AM on January 31, 2022: contributor

    Concept ACK

    Changes since my last review:

    • require_checksum has been moved from a positional argument to an option. This is done according to the reasoning given by @luke-jr in the above comment, with which I agree.
    • Slight rewording of the require_checksum help message. I prefer the new wordings as it better explains the working if the checksum is provided.

    I agree with the idea of keeping the checksum as an optional part for deriving address than as a mandatory part. Since the WIF already has an in-built checksum, it doesn't necessarily need the redundant checksum part. These changes allow the user to remove the extra step of getting the checksum before deriving the address. This PR also preserves the old behavior if a user prefers to take the traditional approach of deriving addresses.

    I was NOT able to test the working of this RPC successfully. I used the string given in the example to test deriveaddresses, but it failed with the following error.

    error: Error parsing JSON: {require_checksum:

    Screenshot:

    Screenshot from 2022-01-31 14-28-34

  23. Sjors commented at 10:22 AM on January 31, 2022: member

    @shaavan the JSON object needs to be wrapped with single quotes: '{....}'. This is what makes option objects a bit annoying to use.

  24. kallewoof commented at 10:31 AM on January 31, 2022: member

    Alternatively you can do "{\"require_checksum\": false}".

  25. shaavan commented at 11:25 AM on January 31, 2022: contributor

    Both of these methods worked correctly:

    Screenshot from 2022-01-31 16-51-42 @kallewoof I think the help message should be updated to indicate one of these methods:

    • '{"require_checksum": false}'
    • "{\"require_checksum\": false}" @Sjors

    This is what makes option objects a bit annoying to use.

    Do we have any alternatives to the options object? Or some way to bypass this annoying behavior?

  26. kallewoof commented at 11:43 AM on January 31, 2022: member

    @shaavan Good point, I've updated the help text to use single quotes.

    Do we have any alternatives to the options object? Or some way to bypass this annoying behavior?

    Ultimately having an options object rather than stacking on new parameters as we go turns out to be a generally better and more easy to use approach. I think you'll get used to it eventually. :)

  27. kallewoof force-pushed on Jan 31, 2022
  28. Sjors commented at 2:35 PM on January 31, 2022: member

    Do we have any alternatives to the options object? Or some way to bypass this annoying behavior?

    There's #15448

  29. kallewoof force-pushed on Feb 1, 2022
  30. shaavan approved
  31. shaavan commented at 11:58 AM on February 1, 2022: contributor

    ACK 5a9587c5701a4df51c11cea075128ca6e6898681

    Changes since my last review:

    • Updated RPC help message example. The example command now works correctly.
    • Rebased over master.

    I was able to compile the updated PR on Ubuntu 20.04 successfully. The example is working perfectly as well.

    Screenshot: Screenshot from 2022-02-01 17-23-42 @Sjors

    There's #15448

    Let me take a look at it.

  32. kallewoof force-pushed on Feb 4, 2022
  33. Sjors commented at 9:13 AM on February 4, 2022: member

    tACK 04dfe24609fb4dde16c8ba4c314fe9653ef65741

  34. in src/rpc/misc.cpp:249 in 04dfe24609 outdated 
     242 | @@ -243,6 +243,13 @@ static RPCHelpMan deriveaddresses()
 243 |              {
 244 |                  {"descriptor", RPCArg::Type::STR, RPCArg::Optional::NO, "The descriptor."},
 245 |                  {"range", RPCArg::Type::RANGE, RPCArg::Optional::OMITTED_NAMED_ARG, "If a ranged descriptor is used, this specifies the end or the range (in [begin,end] notation) to derive."},
 246 | +
 247 | +                {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
 248 | +                    {
 249 | +                        {"require_checksum", RPCArg::Type::BOOL, RPCArg::Default{true}, "Require a checksum. If a checksum is provided it will be checked regardless of this parameter."},
    luke-jr commented at 11:32 PM on February 5, 2022:

    nit

                            {"require_checksum", RPCArg::Type::BOOL, RPCArg::Default{true}, "Require a checksum. If a checksum is provided it will be verified regardless of this parameter."},
    kallewoof commented at 5:05 AM on February 7, 2022:

    Fixed.

  35. in src/rpc/misc.cpp:264 in 04dfe24609 outdated 
     257 | @@ -251,14 +258,30 @@ static RPCHelpMan deriveaddresses()
 258 |                  }
 259 |              },
 260 |              RPCExamples{
 261 | -                "First three native segwit receive addresses\n" +
 262 | +                "First three native segwit receive addresses:\n" +
 263 |                  HelpExampleCli("deriveaddresses", "\"" + EXAMPLE_DESCRIPTOR + "\" \"[0,2]\"") +
 264 | -                HelpExampleRpc("deriveaddresses", "\"" + EXAMPLE_DESCRIPTOR + "\", \"[0,2]\"")
 265 | +                HelpExampleRpc("deriveaddresses", "\"" + EXAMPLE_DESCRIPTOR + "\", \"[0,2]\"") +
 266 | +                "Derive all addresses from a WIF, which has a built-in checksum:\n" +
    luke-jr commented at 11:33 PM on February 5, 2022:

    I don't think we should have examples without a sane use case. Couldn't this example just as well derive only a single address?

    Sjors commented at 4:04 PM on February 6, 2022:

    Is it not sane though? A user might not remember which address type they used, which is what combo is for. Though I guess it's likely to have been pkh()

    kallewoof commented at 5:04 AM on February 7, 2022:

    It's literally the use case I needed to figure out the various permutations for the test case I was writing. I can agree that it's probably unusual but still sane.

    luke-jr commented at 5:29 AM on February 7, 2022:

    I thought combo was for backward compatibility with the undesirable behaviour of legacy wallets being unable to differentiate?

    In any case, having it as an example is effectively encouraging it or suggesting it, which really doesn't make sense.

    kallewoof commented at 6:10 AM on February 7, 2022:

    I think a frequently asked question is "how do I get the bitcoin address(es) from a given private key", actually, so providing an answer to that question in the documentation doesn't seem off to me, even if it's not exactly best practice to do that on the regular. I personally had no idea about combo and ended up wasting my time writing #24160 for no good reason.

    Sjors commented at 9:33 AM on February 7, 2022:

    If somebody dumped a private key from an old core wallet, rather than make a regular backup, they might be in this situation where they used the SegWit address. There might also be other wallets out there that use WIF keys without specifying the address (though I suspect most adopted BIP39 before SegWit).

    That said, it might be still be better to recommend that users specify which address type they want to derive, which you could illustrate with two examples, e.g. pkh(WIF) and sh(wpkh(WIF).

    kallewoof commented at 9:42 AM on February 7, 2022:

    I'm fine with that (I think pkh example is sufficient though), but I don't know if that addresses @luke-jr's concerns.

    Sjors commented at 11:04 AM on February 7, 2022:

    pkh would indeed "derive only a single address" (and it's the most likely use case for anyone with a pre-BIP39 paper wallet, e.g. from 2011-2014)

    (if exchanges dedicated an FAQ to it, it's probably a thing: https://bitonic.nl/en/faq/26/i-have-created-a-bitcoin-address-on-bitaddressorg-how-can-i-sell-my-bitcoins)

    kallewoof commented at 11:10 AM on February 7, 2022:

    Switched to pkh() example.

  36. luke-jr changes_requested
  37. kallewoof force-pushed on Feb 7, 2022
  38. kallewoof commented at 5:05 AM on February 7, 2022: member

    "checked" → "verified".

  39. kallewoof force-pushed on Feb 7, 2022
  40. achow101 requested review from Sjors on Mar 11, 2022
  41. Sjors commented at 10:12 AM on March 16, 2022: member

    re-utACK 1345e8d817acd316b71f23315f0996580253ade3

  42. DrahtBot added the label Needs rebase on May 4, 2022
  43. rpc: add require_checksum flag to deriveaddresses
    This allows a user to derive the addresses for some privkey without first having to derive the descriptor checksum.
    97a69e232b
  44. kallewoof force-pushed on May 7, 2022
  45. DrahtBot removed the label Needs rebase on May 7, 2022
  46. Sjors commented at 4:20 PM on May 10, 2022: member

    tACK 97a69e232be707768b6fec3539bdb825235d8d26

    CI failures seem spurious.

    Might be worth adding a test.

    Maybe @achow101 can bless this PR too?

  47. kallewoof commented at 9:08 AM on May 11, 2022: member

    CI failures seem spurious.

    Thanks, didn't realize there were failures. Re-ran → passed.

  48. Sjors approved
  49. shaavan approved
  50. shaavan commented at 12:07 PM on May 13, 2022: contributor

    reACK 97a69e232be707768b6fec3539bdb825235d8d26

    Changes since my last review:

    • “checked” → “verified”. In the context of checksum, “verify” seems like a better choice of words.
    • “all addresses” → “the PKH address”, as well as used a pkh() example instead of combo(). I think that is the right approach as this encourages the user to specify which type of address they would like to derive.
  51. luke-jr referenced this in commit 7813bfd12c on May 21, 2022
  52. w0xlt approved
  53. w0xlt commented at 7:09 PM on June 15, 2022: contributor

    Code Review ACK https://github.com/bitcoin/bitcoin/pull/24162/commits/97a69e232be707768b6fec3539bdb825235d8d26

    Perhaps require_checksum could be a parameter by itself instead of being the only item in options.

  54. kallewoof commented at 3:31 AM on June 16, 2022: member

    Thanks for the review!

    Perhaps require_checksum could be a parameter by itself instead of being the only item in options.

    The idea is that any new parameters in the future will also go into options. That turns out to be easier when you e.g. want to deprecate a parameter (see e.g. the first argument to getbalance which had to be converted into a 'dummy' argument, to avoid having to change the indices of arguments).

  55. DrahtBot added the label Needs rebase on Oct 13, 2022
  56. DrahtBot commented at 4:08 PM on October 13, 2022: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  57. DrahtBot commented at 1:04 AM on January 11, 2023: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->

    There hasn't been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
  58. DrahtBot commented at 1:27 AM on April 11, 2023: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->

    There hasn't been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
  59. kallewoof closed this on Apr 11, 2023
  60. kallewoof deleted the branch on Apr 11, 2023
  61. luke-jr referenced this in commit 79adb4194c on Aug 16, 2023
  62. Retropex referenced this in commit eaa28ac054 on Mar 28, 2024
  63. bitcoin locked this on Apr 10, 2024
Contributors
kallewoofmaflckoshaavanDrahtBotSjorsluke-jrw0xlt
Labels
RPC/REST/ZMQNeeds rebase
Linked (view graph)
#24161 rpc/doc: describe using combo(privkey) to get checksum and then list …
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me