rpc: add require_checksum flag to deriveaddresses #24162

kallewoof · 2022-01-26T05:58:34Z

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

Alternative to #24161.

src/rpc/misc.cpp

shaavan

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

kallewoof · 2022-01-26T09:32:56Z

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.

shaavan · 2022-01-26T11:46:31Z

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.

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.

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?

kallewoof · 2022-01-26T12:31:16Z

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.

DrahtBot · 2022-01-26T23:40:09Z

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

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.

src/rpc/misc.cpp

Sjors · 2022-01-27T17:58:47Z

Concept ACK

Sjors

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.

src/rpc/misc.cpp

Sjors · 2022-01-28T09:16:18Z

re-utACK e34c8e1b63628986d496b5f7bb29addac50e459a

src/rpc/misc.cpp

shaavan

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:

Sjors · 2022-01-31T10:22:08Z

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

kallewoof · 2022-01-31T10:31:22Z

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

shaavan · 2022-01-31T11:25:02Z

Both of these methods worked correctly:

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

kallewoof · 2022-01-31T11:43:35Z

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

Sjors · 2022-01-31T14:35:22Z

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

There's #15448

shaavan

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:

@Sjors

There's #15448

Let me take a look at it.

kallewoof · 2022-02-07T05:05:25Z

"checked" → "verified".

Sjors · 2022-03-16T10:12:26Z

re-utACK 1345e8d817acd316b71f23315f0996580253ade3

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

Sjors · 2022-05-10T16:20:50Z

tACK 97a69e2

CI failures seem spurious.

Might be worth adding a test.

Maybe @achow101 can bless this PR too?

kallewoof · 2022-05-11T09:08:19Z

CI failures seem spurious.

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

shaavan

reACK 97a69e2

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.

This allows a user to derive the addresses for some privkey without first having to derive the descriptor checksum. Github-Pull: bitcoin#24162 Rebased-From: 97a69e2

w0xlt

Code Review ACK 97a69e2

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

kallewoof · 2022-06-16T03:31:32Z

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

DrahtBot · 2022-10-13T16:08:06Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2023-01-11T01:04:30Z

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.

DrahtBot · 2023-04-11T01:27:58Z

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.

This allows a user to derive the addresses for some privkey without first having to derive the descriptor checksum. Github-Pull: bitcoin#24162 Rebased-From: 97a69e2

kallewoof mentioned this pull request Jan 26, 2022

rpc/doc: describe using combo(privkey) to get checksum and then list … #24161

Closed

maflcko reviewed Jan 26, 2022

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from 12300e9 to 48e9935 Compare January 26, 2022 06:37

DrahtBot added the RPC/REST/ZMQ label Jan 26, 2022

shaavan reviewed Jan 26, 2022

View reviewed changes

This was referenced Jan 27, 2022

Add scanblocks RPC call (attempt 2) #23549

Merged

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

Sjors reviewed Jan 27, 2022

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from 48e9935 to 8d3c91d Compare January 28, 2022 01:17

Sjors reviewed Jan 28, 2022

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

src/rpc/misc.cpp Outdated Show resolved Hide resolved

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from 8d3c91d to e34c8e1 Compare January 28, 2022 09:12

luke-jr suggested changes Jan 31, 2022

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch 2 times, most recently from 4068e78 to eea9178 Compare January 31, 2022 05:50

shaavan reviewed Jan 31, 2022

View reviewed changes

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from eea9178 to 1375e9f Compare January 31, 2022 11:43

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from 1375e9f to 5a9587c Compare February 1, 2022 06:33

shaavan approved these changes Feb 1, 2022

View reviewed changes

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from 5a9587c to 04dfe24 Compare February 4, 2022 05:42

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from 29b896b to 1345e8d Compare February 7, 2022 11:10

achow101 requested a review from Sjors March 11, 2022 16:58

DrahtBot mentioned this pull request Apr 25, 2022

Remove not needed clang-format off comments #24959

Merged

DrahtBot mentioned this pull request May 3, 2022

rpc: Move output script RPCs to separate file, rename misc.cpp #25058

Merged

DrahtBot added the Needs rebase label May 4, 2022

rpc: add require_checksum flag to deriveaddresses

97a69e2

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

kallewoof force-pushed the 202201-deriveaddr-nochecksum branch from 1345e8d to 97a69e2 Compare May 7, 2022 14:38

DrahtBot removed the Needs rebase label May 7, 2022

DrahtBot mentioned this pull request May 12, 2022

tidy: use modernize-use-raw-string-literal #25110

Closed

Sjors approved these changes May 12, 2022

View reviewed changes

shaavan approved these changes May 13, 2022

View reviewed changes

DrahtBot mentioned this pull request Jun 15, 2022

util, rpc: Add parameter to deriveaddresses to display address information #25366

Closed

w0xlt approved these changes Jun 15, 2022

View reviewed changes

DrahtBot mentioned this pull request Sep 8, 2022

refactor: Run type check against RPCArgs (1/2) #26039

Merged

DrahtBot mentioned this pull request Oct 3, 2022

clang-tidy: fixup named argument comments #26238

Merged

DrahtBot added the Needs rebase label Oct 13, 2022

kallewoof closed this Apr 11, 2023

kallewoof deleted the 202201-deriveaddr-nochecksum branch April 11, 2023 04:43

bitcoin locked and limited conversation to collaborators Apr 10, 2024

rpc: add require_checksum flag to deriveaddresses #24162

rpc: add require_checksum flag to deriveaddresses #24162

Uh oh!

Conversation

kallewoof commented Jan 26, 2022

Uh oh!

Uh oh!

shaavan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan commented Jan 26, 2022

Uh oh!

kallewoof commented Jan 26, 2022

Uh oh!

DrahtBot commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Sjors commented Jan 27, 2022

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jan 28, 2022

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jan 31, 2022

Uh oh!

kallewoof commented Jan 31, 2022

Uh oh!

shaavan commented Jan 31, 2022

Uh oh!

kallewoof commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jan 31, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Feb 7, 2022

Uh oh!

Sjors commented Mar 16, 2022

Uh oh!

Sjors commented May 10, 2022

Uh oh!

kallewoof commented May 11, 2022

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jun 16, 2022

Uh oh!

DrahtBot commented Oct 13, 2022

Uh oh!

DrahtBot commented Jan 11, 2023

Uh oh!

DrahtBot commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

shaavan left a comment •

edited

Loading

kallewoof commented Jan 26, 2022 •

edited

Loading

DrahtBot commented Jan 26, 2022 •

edited

Loading

Sjors left a comment •

edited

Loading

kallewoof commented Jan 31, 2022 •

edited

Loading