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

Add deriveaddresses RPC util method #14667

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
@Sjors
Copy link
Member

commented Nov 6, 2018

Usage:

bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/0)"
[
  "bc1qg6ucjz7kgdedam7v5yarecy54uqw82yym06z3q"
] // part of the BIP32 test vector

Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with scantxoutset as a poor mans wallet. Might be useful to test more complicated future descriptors.

To keep it as simple as possible it only supports descriptors that result in a single address, so no combo() and ranges.

As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR.

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

I know, this comes up every time—see for example #14476 (comment) —wasn't the idea to not add more pure-utility RPC calls?
We really need a clear stance on this.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

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

Conflicts

No conflicts as of last run.

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

I created an issue to discuss a tool that could replace pure utility RPC calls: #14671

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

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch Nov 7, 2018

@meshcollider
Copy link
Member

left a comment

This looks good, and after the discussion in the meeting this morning I think its worth adding.
utACK 9a55d5f

src/rpc/misc.cpp Outdated
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor"));
}
if (desc->IsRange()){
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Descriptor range not supported"));

This comment has been minimized.

Copy link
@meshcollider

meshcollider Nov 9, 2018

Member

It would be easy enough to add support for picking a specific index from a ranged descriptor, is there a reason you chose to disallow it? Just add a second parameter which specifies the index.
EDIT: thinking about it, it would be just as easy to not use a ranged descriptor if we're only using a single key, don't worry.

This comment has been minimized.

Copy link
@sipa

sipa Nov 10, 2018

Member

It seems a weird anti-feature to me. I don't understand why you're adding extra complexity to the function just to prevent a range. If you only want one, you can still do so.

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 10, 2018

Author Member

I initially did that to prevent extra complexity, but I've actually encountered a situation where allowing ranges would be very useful. Will change this to return an array.

This comment has been minimized.

Copy link
@meshcollider

meshcollider Nov 10, 2018

Member

@Sjors I assume that also includes allowing combo right? Sounds good 👍

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 10, 2018

Author Member

@meshcollider yes, no reason anymore to exclude combo()

doc/release-notes-13152.md Outdated
@@ -3,3 +3,4 @@ New RPC methods

- `getnodeaddresses` returns peer addresses known to this node. It may be used to connect to nodes over TCP without using the DNS seeds.
- `listwalletdir` returns a list of wallets in the wallet directory which is configured with `-walletdir` parameter.
- `deriveaddress` returns the address corresponding to an [output descriptor](/doc/descriptors.md).

This comment has been minimized.

Copy link
@sipa

sipa Nov 11, 2018

Member

This doesn't seem to belong in this file (the file is specific per PR).

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 11, 2018

Author Member

listwalletdir was already hijacking that file, so I joined, but I can add a new one.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 14, 2018

Member

In which case we could add them to the main file in the first place.

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 19, 2018

Author Member

I thought the point of these extra files was mainly to prevent constant rebasing, which seemed mostly a result of different sections interfering, and less when you're just adding one line an existing entry. Though I'm not sure exactly what trips up git.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 19, 2018

Member

This file was removed in master. Also, if another patch added a line in the meantime, git(hub) would also report a merge conflict.

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Rebased. Result is now an array of addresses, so combo() and ranges can be used. Added range arguments.

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch Nov 19, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 19, 2018

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch Nov 19, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Getting a rather cryptic linter error: @MarcoFalke?
schermafbeelding 2018-11-19 om 20 11 42

Update: #14718 was merged right before I pushed this. I rebased again and now I'm able to reproduce the linter error. It happens on master too.

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch 2 times, most recently Nov 19, 2018

@meshcollider
Copy link
Member

left a comment

Looking good

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

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch 2 times, most recently Nov 20, 2018

@Sjors Sjors changed the title Add deriveaddress RPC util method Add deriveaddresses RPC util method Nov 20, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

Rebased so CI works again. Renamed to plural deriveaddresses. Simplified the range arguments (must provide start and end).

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch Nov 20, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

utACK 82d2a8f

One last nit, I would add a comment about the default behavior of 0 for ranged descriptors if start and end aren't provided.

EDIT: Actually, I think the helptext needs to be switched to RPCHelpMan

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

@meshcollider alternatively I could make the range argument mandatory for ranged descriptors.

What do you mean with RPCHelpMan?

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch Nov 27, 2018

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

Rebased due to #14726 and using RPCHelpMan now.

Range argument is now mandatory for ranged descriptors (typing 0 instead of * is trivial).

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Concept ACK

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

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

utACK with a small nit.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Needs non conflict rebase to fix the new RPCHelpMan() argument section (default and comments).

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

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch 2 times, most recently Jan 9, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

Rebased for RPCHelpMan and addressed some of the nits. Will continue later.

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch to f8f115d Jan 15, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

Rebased for no particular reason, all feedback should be addressed now.

@FrancisPouliot

This comment has been minimized.

Copy link

commented Jan 22, 2019

Concept ack. Many web services (including mine) derive receiving addresses from external xpub and need to run some other utility like pycoin to do this, and then import adresses in Bitcoin Core to watch them.

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

utACK f8f115d

@meshcollider

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

utACK f8f115d

@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

testing f8f115d. bitcoind will abort if you pass a range begin < 0;

i.e src/bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)" -1 0

2019-01-27T03:47:50Z UpdateTip: new best=0000000000000000002d802cf5fdbbfa94926be7f03b40be75eb6c3c13cbc8e4 height=560180 version=0x20000000 log2_work=90.279438 tx=376715337 date='2019-01-26T12:02:01Z' progress=0.999639 cache=17.7MiB(131993txo) warning='25 of last 100 blocks have unexpected version'
Assertion failed: ((nChild >> 31) == 0), function Derive, file pubkey.cpp, line 229.
Process 25118 stopped
* thread #2, name = 'bitcoin-httpworker', stop reason = signal SIGABRT
    frame #0: 0x00007fff5ba9523e libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff5ba9523e <+10>: jae    0x7fff5ba95248            ; <+20>
    0x7fff5ba95240 <+12>: movq   %rax, %rdi
    0x7fff5ba95243 <+15>: jmp    0x7fff5ba8f3b7            ; cerror_nocancel
    0x7fff5ba95248 <+20>: retq   
Target 0: (bitcoind) stopped.
(lldb) bt
* thread #2, name = 'bitcoin-httpworker', stop reason = signal SIGABRT
  * frame #0: 0x00007fff5ba9523e libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff5bb4bc1c libsystem_pthread.dylib`pthread_kill + 285
    frame #2: 0x00007fff5b9fe1c9 libsystem_c.dylib`abort + 127
    frame #3: 0x00007fff5b9c6868 libsystem_c.dylib`__assert_rtn + 320
    frame #4: 0x00000001003cfee5 bitcoind`CPubKey::Derive(this=0x000070000305501c, pubkeyChild=0x000070000305501c, ccChild=0x0000700003054ffc, nChild=<unavailable>, cc=<unavailable>) const at pubkey.cpp:229 [opt]
    frame #5: 0x00000001003d01c9 bitcoind`CExtPubKey::Derive(this=<unavailable>, out=<unavailable>, _nChild=4294967295) const at pubkey.cpp:271 [opt]
    frame #6: 0x000000010037cdaf bitcoind`(anonymous namespace)::BIP32PubkeyProvider::GetPubKey(this=0x0000000133aa7420, pos=-1, arg=<unavailable>, key=<unavailable>, info=0x0000000176f57888) const at descriptor.cpp:173 [opt]
    frame #7: 0x000000010037dc09 bitcoind`(anonymous namespace)::OriginPubkeyProvider::GetPubKey(this=0x0000000125778480, pos=<unavailable>, arg=<unavailable>, key=<unavailable>, info=0x0000000176f57888) const at descriptor.cpp:73 [opt]
    frame #8: 0x000000010037f6f9 bitcoind`(anonymous namespace)::DescriptorImpl::ExpandHelper(this=<unavailable>, pos=-1, arg=0x0000700003055630, cache_read=0x0000000000000000, output_scripts=size=0, out=0x0000700003055630, cache_write=0x0000000000000000) const at descriptor.cpp:298 [opt]
    frame #9: 0x000000010037efdd bitcoind`(anonymous namespace)::DescriptorImpl::Expand(this=<unavailable>, pos=<unavailable>, provider=<unavailable>, output_scripts=<unavailable>, out=<unavailable>, cache=<unavailable>) const at descriptor.cpp:343 [opt]
    frame #10: 0x000000010011b02c bitcoind`deriveaddresses(request=<unavailable>) at misc.cpp:207 [opt]

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch from f8f115d to 71eef5a Jan 29, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

bitcoind will abort if you pass a range begin < 0;

Pff, don't do that! :-) Fixed (with test).

@MarcoFalke maybe we can expand RPCTypeCheck to also enforce constraints like minimum value?

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

Indeed, we could pass a lambda into the RPCHelpMan to validate the value and use the passed in type to validate the type (and thus get rid of RPCTypeCheck completely)

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch from 71eef5a to 7f78582 Jan 29, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Rebased due to somewhat inexplicable Travis failure.

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

@Sjors Sjors force-pushed the Sjors:2018/11/deriveaddress branch from 7f78582 to 5952838 Jan 29, 2019

@instagibbs
Copy link
Member

left a comment

Couple of non-blocking nits, tACK 5952838

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

bare_multisig_descriptor = "multi(1, tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/0, tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1)"
assert_raises_rpc_error(-5, "Descriptor does not have a corresponding address", self.nodes[0].deriveaddresses, bare_multisig_descriptor)

This comment has been minimized.

Copy link
@instagibbs

instagibbs Feb 6, 2019

Member

I'd add a wallet roundtrip test like:

wpkh_info = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress("", "bech32"))
# Trim the witness part of descriptor off, check pkh entry in wallet, match descriptor
pkh_desc = wpkh_info["desc"][1:]
pkh_addr = self.nodes[0].deriveaddresses(pkh_desc)
pkh_info = self.nodes[0].getaddressinfo(pkh_addr)
assert_equal(wpkh_info["hdkeypath"], pkh_info["hdkeypath"])
assert_equal(wpkh_info["pubkey"], pkh_info["pubkey"])

@meshcollider meshcollider merged commit 5952838 into bitcoin:master Feb 7, 2019

2 checks passed

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

meshcollider added a commit that referenced this pull request Feb 7, 2019

Merge #14667: Add deriveaddresses RPC util method
5952838 [rpc] util: add deriveaddresses method (Sjors Provoost)

Pull request description:

  Usage:

  ```sh
  bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/0)"
  [
    "bc1qg6ucjz7kgdedam7v5yarecy54uqw82yym06z3q"
  ] // part of the BIP32 test vector
  ```

  Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with `scantxoutset` as a poor mans wallet. Might be useful to test more complicated future descriptors.

  ~To keep it as simple as possible it only supports descriptors that result in a single address, so no `combo()` and ranges.~

  As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR.

Tree-SHA512: b8e53db11a8fd87638cc98766270cc3be9adc4b3e5085798a6a4e2e6ad252bf6d2189346bbb2da72d04d13f7f1e80b5cb88e8039653bea1f150602a876ef7f34

@Sjors Sjors deleted the Sjors:2018/11/deriveaddress branch Feb 8, 2019

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