19:00:13 <meshcollider> #startmeeting
19:00:13 <lightningbot> Meeting started Fri Jan 31 19:00:13 2020 UTC.  The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:13 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:17 <provoostenator> hi
19:00:18 <kanzure> hi
19:00:19 <meshcollider> #bitcoin-core-dev Wallet Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball ariard digi_james amiti fjahr
19:00:19 <meshcollider> jeremyrubin emilengler jonatack hebasto jb55
19:00:33 <fjahr> hi
19:01:00 <achow101> hi
19:01:31 <meshcollider> So, wallet boxes merged this week, thanks to those who helped review it and get it in \o/
19:01:31 <sipa> hi
19:01:38 <meshcollider> Topics?
19:02:07 <achow101> does the descriptor wallets PR need to be broken up?
19:02:59 <provoostenator> It's not HUGE, but it's a lot of moving parts, so might be better to get some stuff in seperately. E.g. serialization.
19:03:23 <meshcollider> #16528
19:03:25 <gribble> https://github.com/bitcoin/bitcoin/issues/16528 | Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101 · Pull Request #16528 · bitcoin/bitcoin · GitHub
19:03:49 <jnewbery> hi
19:04:27 <meshcollider> Hmm yeah I think splitting it up at least a bit might help get it in quicker
19:04:39 <jonatack> hi
19:04:54 <provoostenator> If you do split however, it'll be more difficult to rely on the functional test suite to check everything. So you may need to add regular unit tests.
19:05:13 <provoostenator> Not necessarily a bad thing, but maybe unpleasant :-)
19:05:58 <meshcollider> Andrew has lots of time to add tests in between waiting for reviews ;)
19:06:14 <achow101> a lot of things in the test suite need to be changed to not rely on deprecated RPCs
19:06:34 <achow101> but those changes don't make sense (or work) without descriptor wallets already
19:08:40 <achow101> but threre aren't a lot of commits that can standalone. most of the commits are just implementing different functions
19:10:16 <provoostenator> I think the wallet encoding stuff can be done seperately, but it'll need unit tests.
19:10:56 <provoostenator> In other words, it wouldn't be reachable via RPC.
19:11:28 <meshcollider> You could probably do it in the kind of, implement first, then connect it to the existing code second, like you did with the first boxing PR
19:11:30 <achow101> you mean the WalletDescriptor class?
19:12:11 <provoostenator> Maybe the WalletDescriptor class and its WalletDB stuff.
19:12:41 <provoostenator> Not sure how easy it is to get that in without the whole ScriptpubkeyMan structure to support it.
19:13:33 <achow101> I'll try
19:14:42 <achow101> I won't be making changes until after next week, so feel free to leave your review comments next week before anything changes
19:15:49 <sipa> achow101: ?
19:16:04 <sipa> oh
19:16:08 <meshcollider> Sounds good
19:16:09 <sipa> i missed the "until"
19:16:15 <meshcollider> Anything else anyone wants to discuss?
19:16:33 <jonatack> meshcollider: 17585 might be RFM, has acks from jnewbery and wumpus
19:16:46 <provoostenator> Related to descriptor wallets...
19:16:50 <sipa> #17585
19:16:53 <gribble> https://github.com/bitcoin/bitcoin/issues/17585 | rpc: deprecate getaddressinfo label by jonatack · Pull Request #17585 · bitcoin/bitcoin · GitHub
19:17:02 <jonatack> sipa: thank you
19:17:10 <provoostenator> One thing I brought up in the review is if we really want to switch to BIP44/49/84 or stick to our hardened derivation thing.
19:17:24 <meshcollider> jonatack: sweet I'll review it too and merge later today
19:17:54 <achow101> he'll merge it soon(tm) :p
19:19:00 <achow101> provoostenator: maybe we should discuss that nnow
19:19:23 <achow101> I think the reason we didn't do public derivation before was because privkeys could be exported easily with dumpprivkey
19:19:59 <achow101> (and lots of things online casually mention that people can do this to check whether they have the private keys)
19:20:03 <provoostenator> We currently use m/0'/0'/i' derivation in the wallet
19:20:15 <provoostenator> Where the last 0' is 1' for change
19:20:22 <provoostenator> Every key is hardened
19:20:40 <sipa> i'm not opposed to having the option of having publicly derivable wallets
19:20:46 <sipa> i'm unsure if it should be default
19:21:26 <achow101> well it also means we don't need to unlock wallets to get more addresses
19:21:44 <achow101> so the "keypool" can be way smaller
19:21:50 <meshcollider> Yeah itd be quite nice to at least have the option there
19:21:59 <jonatack> +1
19:22:30 <achow101> descriptor wallets will always give you that option as you can import a descriptor with different derivs. the question is the setting for newly created wallets
19:22:40 <sipa> yeah
19:22:48 <sipa> achow101: the keypool (=precomputed pubkeys for descriptors?) still needs to be as large as the gap limit, right?
19:23:27 <achow101> yes, it still needs to be as large as the gap limit
19:23:35 <achow101> but that doesn't need to be 1000
19:23:40 <achow101> could go back to 100
19:23:40 <sipa> fair, possibly not
19:23:50 <provoostenator> What happens in the current wallet if you import and rescan a wallet that uses keys beyond the gap limit?
19:23:55 <sipa> the overhead of a large keypool would also be smaller, i think?
19:24:19 <sipa> as you don't get new key entries and whatever in the wallet, or do you?
19:24:34 <achow101> yes
19:24:46 <achow101> every pubkey is still precomputed and stored for the descriptor cache
19:24:55 <provoostenator> The serialized descriptor only needs to hold on to its public cache
19:25:07 <provoostenator> And we can stop storing the encrypted indivual private keys
19:25:13 <provoostenator> And just derive those when signing.
19:25:16 <sipa> yeah
19:25:21 <achow101> I don't think we store individual private keys
19:25:29 <sipa> achow101: we do now :p
19:25:31 <provoostenator> Maybe I misread that today
19:25:45 <achow101> sipa: in descriptor wallets :)
19:25:47 <sipa> or do you mean in descriptor wallets PR?
19:25:48 <provoostenator> Today we do that for backwards compatibility with legacy wallets
19:26:00 <sipa> right, that's what I assumed
19:26:12 <sipa> we store individual keys for legacy wallets but not for descriptor wallets
19:26:17 <provoostenator> The descriptor wallet PR currently maintains that behavior afaik, but doesn't have to.
19:26:32 <sipa> i hope it does not
19:26:47 <sipa> descriptor wallets can't be and shouldn't be compatible with old software
19:27:13 <meshcollider> Yeah that's what we are trying to escape with the redesign
19:27:14 <provoostenator> sipa: the descriptor wallet flag already prevents old software from loading
19:27:23 <achow101> at one point, an implementation did do that because it was easier. but I thought I changed that
19:27:42 <provoostenator> achow101: maybe you only kept the serialization code aorund; that's where I ended reviewing today
19:28:06 <achow101> provoostenator: maybe you are confusing the descriptor master private key storage?
19:28:51 <provoostenator> https://github.com/bitcoin/bitcoin/commit/d6f0c337f5bbf935cd93ed3884f5c10bcaa5d493
19:29:01 <provoostenator> AddKey and AddCryptedKey
19:29:24 <achow101> that's for the master private key
19:29:27 <achow101> just bad naming lol
19:29:39 <provoostenator> Aargh, not the first time I trip over that name.
19:29:57 <achow101> I'll rename it
19:30:20 <provoostenator> Ok, so assuming that's all great, we only need the password when expanding the (public) keypool.
19:30:25 <provoostenator> And when signing a transaction
19:30:31 <provoostenator> And it's not much burden.
19:30:43 <provoostenator> So that's not a reason to avoid hardened derivation by default
19:31:03 <achow101> it's the same thing you have to do today
19:33:02 <achow101> I suppose part of this is also that it seems like every other wallet has settled on a standardized derivation path scheme
19:33:23 <achow101> but at the same time, descriptors are completely unambiguous about the derivation to use
19:34:23 <provoostenator> https://walletsrecovery.org
19:34:47 <provoostenator> Reasonably strong BIP44/49/84 adoption
19:35:11 <provoostenator> But definately not universal
19:35:28 <provoostenator> See also https://github.com/bitcoin/bitcoin/issues/18043
19:35:29 <achow101> it's not a problem for people to import their stuff into our descriptor wallet as we can use any derivation path
19:35:46 <achow101> it's only a problem for people importing core to another wallet that may not allow custom derivation paths
19:36:43 <provoostenator> I wonder how common that use case is though, exporting keys to another wallet. Usually it's easier to just send them.
19:37:20 <achow101> Core -> other wallet is fairly common because people don't want to wait for IBD
19:37:41 <meshcollider> Yeah if you're restoring a backup or something and have keys but haven't synced to send tx
19:38:09 <meshcollider> It's probably fair to add a wallet creation option to be BIP 44/49/84 compliance
19:38:11 <achow101> threads on bitcointalk and questions on stackexchange about that show up pretty commonly
19:38:13 <provoostenator> AssumeUTXO to the rescue?
19:38:25 <provoostenator> meshcollider: as an option for sure
19:39:04 <provoostenator> Though without BIP39 mnemonic export that's still not practical
19:39:26 <achow101> that makes setup more annoying though, possibly need SetupGeneration to take more args
19:39:45 <provoostenator> achow101: it'll need more args for multisig too probably
19:40:00 <provoostenator> Actually no, those are still single descriptors.
19:40:30 <achow101> for multisigs, I would expect that to be imports rather than something we specifically allow in wallet creation
19:40:31 <provoostenator> If we were to add BIP39 support, then descriptors could contain the mnemonic.
19:41:19 <sipa> no
19:41:34 <sipa> the mnemonic is private information, and should be protected by the private key infrastructure
19:41:41 <sipa> descriptors are public
19:42:04 <achow101> sipa: descriptors can contain privkeys though
19:42:10 <achow101> that's supported for imports at least
19:42:12 <sipa> achow101: as syntactig sugar
19:42:16 <achow101> yes
19:42:21 <provoostenator> True, so inpractice you'd have a mnemonic in one place, and then have descriptors that just refer to the master key fingerprint
19:42:29 <sipa> for imports, supporting mnemonics is not a problem
19:42:34 <sipa> in descriptors
19:42:48 <sipa> but they'd be converted to a private key + descriptor with xpubs
19:43:00 <provoostenator> yes
19:43:18 <jonatack> "What happens in the current wallet if you import and rescan a wallet that uses keys beyond the gap limit?" -> #17719 just merged, related to this
19:43:21 <gribble> https://github.com/bitcoin/bitcoin/issues/17719 | Document better -keypool as a look-ahead safety mechanism by ariard · Pull Request #17719 · bitcoin/bitcoin · GitHub
19:43:25 <provoostenator> Or a descriptor without xpubs which assumes you have the matser key (doesn't work atm)
19:43:51 <sipa> provoostenator: hmm, no
19:44:04 <sipa> the descriptor needs to have enough information to generate future addresses
19:44:53 <provoostenator> OK, that's a reasonable constraint
19:45:23 <achow101> iirc, to be an "active" descriptor, we require IsSolvable() and IsRanged()
19:46:08 <achow101> although I think IsRanged() implies IsSolvable()
19:46:23 <provoostenator> (re multisig, I have an experimental createmultisigwallet RPC call in #16895 but I'm not convinced that's the way to go)
19:46:25 <gribble> https://github.com/bitcoin/bitcoin/issues/16895 | External signer multisig support by Sjors · Pull Request #16895 · bitcoin/bitcoin · GitHub
19:47:29 <provoostenator> The tricky part of multisig is that you need to collect information from multiple sources. Or one of the [devices] has to be responsible to generate an import incantation.
19:48:21 <provoostenator> Based on some info it gets from other devices.
19:48:38 <provoostenator> But maybe for some other time...
19:48:59 <achow101> i think we can think about multisig after descriptor wallets is merged
19:49:04 <achow101> (in like 6 months)
19:49:08 <provoostenator> 2 weeks
19:49:11 <meshcollider> lol
19:49:50 <achow101> back to the public deriv thing, what do we want for the default?
19:50:50 <meshcollider> Will there be a "dumpprivdescriptor" RPC or something, how is the user even going to extract it from their wallet to import?
19:51:22 <provoostenator> Not initially but hard to guarantee that'll never happen.
19:51:29 <achow101> not initially
19:51:38 <sipa> i don't see a big problem with that
19:51:55 <achow101> I think eventually there will be
19:52:02 <sipa> it's obviously scary, and needs big warnings
19:52:02 <meshcollider> Probably
19:52:12 <sipa> but it's in the same order of dangerous as dumpprivkey now
19:52:19 <meshcollider> Or dumpwallet
19:52:29 <sipa> yeah, dumpwallet is a better comparison
19:52:33 <achow101> dumpwallet is deprecated now I think
19:52:38 <achow101> with descriptor wallets
19:52:42 <sipa> good
19:52:42 <meshcollider> Good
19:52:43 <provoostenator> More like dumpwallet than dumpkey, because one compromised privkey can comprise the full wallet without hardned deriv
19:52:44 <sipa> it should be
19:53:06 <achow101> I think i'll add a dumppublicdescriptor
19:53:20 <achow101> so at least you can import watch only to another wallet, and then do psbt signing
19:53:37 <provoostenator> achow101: +1
19:53:44 <meshcollider> Yeah then with that, BIP derivation seems more sensible
19:53:52 <jonatack> Yes
19:54:48 <jonatack> will try to look at #16528 next week
19:54:51 <gribble> https://github.com/bitcoin/bitcoin/issues/16528 | Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101 · Pull Request #16528 · bitcoin/bitcoin · GitHub
19:55:11 <meshcollider> Alright any last topics?
19:55:33 <provoostenator> sipa: taproot wallet support in two weeks?
19:55:42 <fjahr> just a review beg for #17824, there was some discussion 4 weeks ago but no ACKs yet
19:55:45 <gribble> https://github.com/bitcoin/bitcoin/issues/17824 | wallet: Improve coin selection for destination groups >10 by fjahr · Pull Request #17824 · bitcoin/bitcoin · GitHub
19:55:46 <meshcollider> Yep, merge before SF ;)
19:57:28 <meshcollider> #endmeeting