< sipa> bare script transactions are nonstandard to begin with
< sipa> except p2sh/p2pk/p2pkh/multisig
< jeremyrubin> Ah fair, I guess if I make a new standard type I can set whatever rules I want anyways
< sipa> anything but <constant> OP_CTV as standard seems crazy
< jeremyrubin> not planning on doing that, just more out of intellectual curiosity
< bitcoin-git> [bitcoin] achow101 opened pull request #18034: Get the OutputType for a descriptor (master...desc-getoutputtype) https://github.com/bitcoin/bitcoin/pull/18034
< bitcoin-git> [bitcoin] promag opened pull request #18035: gui: Drop BanTableModel dependency to ClientModel (master...2020-01-bantablemodel) https://github.com/bitcoin/bitcoin/pull/18035
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/1d1f8bbf5711...d176aeafde29
< bitcoin-git> bitcoin/master 085423b Aaron Clauson: Remove GitHub Actions CI workflow.
< bitcoin-git> bitcoin/master d176aea fanquake: Merge #18031: Remove GitHub Actions CI workflow.
< bitcoin-git> [bitcoin] fanquake merged pull request #18031: Remove GitHub Actions CI workflow. (master...remove-ghaction) https://github.com/bitcoin/bitcoin/pull/18031
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/d176aeafde29...3e1bf71064cc
< bitcoin-git> bitcoin/master c7ec9a1 Andrew Toth: Add missing supported rpcs to doc/descriptors.md
< bitcoin-git> bitcoin/master 3e1bf71 fanquake: Merge #18025: doc: Add missing supported rpcs to doc/descriptors.md
< bitcoin-git> [bitcoin] fanquake merged pull request #18025: doc: Add missing supported rpcs to doc/descriptors.md (master...patch-1) https://github.com/bitcoin/bitcoin/pull/18025
< bitcoin-git> [bitcoin] promag opened pull request #18036: gui: Drop ShutdownWindow dependency to BitcoinGUI (master...2020-01-utilitydialog) https://github.com/bitcoin/bitcoin/pull/18036
< bitcoin-git> [bitcoin] amitiuttarwar opened pull request #18037: Util: Allow scheduler to be mocked (master...2020-01-mock-scheduler) https://github.com/bitcoin/bitcoin/pull/18037
< bitcoin-git> [bitcoin] amitiuttarwar opened pull request #18038: P2P: Mempool tracks locally submitted transactions to improve privacy (master...2020-01-unbroadcast) https://github.com/bitcoin/bitcoin/pull/18038
< bitcoin-git> [bitcoin] promag closed pull request #18035: gui: Drop BanTableModel dependency to ClientModel (master...2020-01-bantablemodel) https://github.com/bitcoin/bitcoin/pull/18035
< jonasschnelli> Anyone know why the macos build failes on the buitcoinbuilds site? https://bitcoinbuilds.org/index.php?ansilog=e9054288-51c8-4e70-923f-84dea2dfcd15.log#l1752
< jonasschnelli> It should compile Qt from depends...
< fanquake> jonasschnelli: that looks like a Linux build, if it's compiling dbus and freedesktop notifications
< fanquake> Neither are used on macOS
< jonasschnelli> thats a point. :/
< jonasschnelli> though: /home/ubuntu/src/depends/x86_64-apple-darwin14/native/bin/clang -target x86_64-apple-darwin14 -mmacosx-version-min=10.12 --sysroot /home/ubuntu/src/depends/SDKs/MacOSX10.11.sdk
< jonasschnelli> But yes,.. it passes the #ifdef USE_DBUS macro
< jonasschnelli> fanquake: but USE_DBUS is enabled on macOS
< fanquake> jonasschnelli: if you're compiling qt from depends, we explicitly disable the dbus module when targeting macos, so when configure checks for QtDbus, it shouldn't find it, and USE_DBUS shouldn't get set.
< jonasschnelli> Thanks. Maybe my cache is invalid...
< fanquake> If you blow everything away and still seeing issues let me know and I'll have a look
< sdaftuar> wumpus: fyi I think #17951 is ready for merge
< gribble> https://github.com/bitcoin/bitcoin/issues/17951 | Use rolling bloom filter of recent block txs for AlreadyHave() check by sdaftuar . Pull Request #17951 . bitcoin/bitcoin . GitHub
< bitcoin-git> [bitcoin] jonasschnelli pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/3e1bf71064cc...d104aa0ace36
< bitcoin-git> bitcoin/master a029e18 Suhas Daftuar: Use rolling bloom filter of recent block tx's for AlreadyHave() check
< bitcoin-git> bitcoin/master d104aa0 Jonas Schnelli: Merge #17951: Use rolling bloom filter of recent block txs for AlreadyHave...
< bitcoin-git> [bitcoin] jonasschnelli merged pull request #17951: Use rolling bloom filter of recent block txs for AlreadyHave() check (master...2020-01-improve-alreadyhave) https://github.com/bitcoin/bitcoin/pull/17951
< jonasschnelli> fanquake: hmm.. still facing the macOS cross compile DBUS error
< jonasschnelli> fanquake: https://bitcoinbuilds.org/logs/b8458fba-f290-4431-b5ab-4cebb700b2f6.log (a normal macOS depends build with x86_64-apple-darwin16 and MacOS SDK 10.11)
< jonasschnelli> -no-dbus gets passed into the depends qt builds
< provoostenator> fanquake: the macOS Travis #16392 build is failing, but it's marked as high priority for review. Should I try it anyway?
< gribble> https://github.com/bitcoin/bitcoin/issues/16392 | build: macOS toolchain update by fanquake . Pull Request #16392 . bitcoin/bitcoin . GitHub
< jonasschnelli> feature_block.py fails (reproducible) on bitcoinbuilds: https://bitcoinbuilds.org/index.php?ansilog=b682c112-b8f3-4ad8-859b-d0916e5b3a85.log#l6123
< jonasschnelli> fanquake: found more about the DBus issue. Our qt m4 detects QT_DBUS even if it has not been compiled via depends. Probably detects it if it has been installed locally as non crosscompile host shared library
< bitcoin-git> [bitcoin] jonasschnelli opened pull request #18042: Don't check for DBUS notification on macOS (master...2020/01/mac_dbus) https://github.com/bitcoin/bitcoin/pull/18042
< promag> jonasschnelli: imo these are good to go #17937 #18036
< gribble> https://github.com/bitcoin/bitcoin/issues/17937 | gui: Remove WalletView and BitcoinGUI circular dependency by promag . Pull Request #17937 . bitcoin/bitcoin . GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/18036 | gui: Break trivial circular dependencies by promag . Pull Request #18036 . bitcoin/bitcoin . GitHub
< bitcoin-git> [bitcoin] sdaftuar opened pull request #18044: Use wtxid for transaction relay (master...2020-01-wtxid-inv) https://github.com/bitcoin/bitcoin/pull/18044
< bitcoin-git> [bitcoin] hebasto opened pull request #18045: build: Do not use QtDBus for macOS builds (master...20200131-macos-qdbus) https://github.com/bitcoin/bitcoin/pull/18045
< hebasto> jonasschnelli: mind looking ^
< provoostenator> Wallet meeting in 1 hour? Or is the schedule different?
< achow101> sounds about right
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #18042: Don't check for DBUS notification on macOS (master...2020/01/mac_dbus) https://github.com/bitcoin/bitcoin/pull/18042
< meshcollider> #startmeeting
< lightningbot> Meeting started Fri Jan 31 19:00:13 2020 UTC. The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< provoostenator> hi
< kanzure> hi
< 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
< meshcollider> jeremyrubin emilengler jonatack hebasto jb55
< fjahr> hi
< achow101> hi
< meshcollider> So, wallet boxes merged this week, thanks to those who helped review it and get it in \o/
< sipa> hi
< meshcollider> Topics?
< achow101> does the descriptor wallets PR need to be broken up?
< 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.
< meshcollider> #16528
< gribble> https://github.com/bitcoin/bitcoin/issues/16528 | Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101 . Pull Request #16528 . bitcoin/bitcoin . GitHub
< jnewbery> hi
< meshcollider> Hmm yeah I think splitting it up at least a bit might help get it in quicker
< jonatack> hi
< 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.
< provoostenator> Not necessarily a bad thing, but maybe unpleasant :-)
< meshcollider> Andrew has lots of time to add tests in between waiting for reviews ;)
< achow101> a lot of things in the test suite need to be changed to not rely on deprecated RPCs
< achow101> but those changes don't make sense (or work) without descriptor wallets already
< achow101> but threre aren't a lot of commits that can standalone. most of the commits are just implementing different functions
< provoostenator> I think the wallet encoding stuff can be done seperately, but it'll need unit tests.
< provoostenator> In other words, it wouldn't be reachable via RPC.
< 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
< achow101> you mean the WalletDescriptor class?
< provoostenator> Maybe the WalletDescriptor class and its WalletDB stuff.
< provoostenator> Not sure how easy it is to get that in without the whole ScriptpubkeyMan structure to support it.
< achow101> I'll try
< achow101> I won't be making changes until after next week, so feel free to leave your review comments next week before anything changes
< sipa> achow101: ?
< sipa> oh
< meshcollider> Sounds good
< sipa> i missed the "until"
< meshcollider> Anything else anyone wants to discuss?
< jonatack> meshcollider: 17585 might be RFM, has acks from jnewbery and wumpus
< provoostenator> Related to descriptor wallets...
< sipa> #17585
< gribble> https://github.com/bitcoin/bitcoin/issues/17585 | rpc: deprecate getaddressinfo label by jonatack . Pull Request #17585 . bitcoin/bitcoin . GitHub
< jonatack> sipa: thank you
< 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.
< meshcollider> jonatack: sweet I'll review it too and merge later today
< achow101> he'll merge it soon(tm) :p
< achow101> provoostenator: maybe we should discuss that nnow
< achow101> I think the reason we didn't do public derivation before was because privkeys could be exported easily with dumpprivkey
< achow101> (and lots of things online casually mention that people can do this to check whether they have the private keys)
< provoostenator> We currently use m/0'/0'/i' derivation in the wallet
< provoostenator> Where the last 0' is 1' for change
< provoostenator> Every key is hardened
< sipa> i'm not opposed to having the option of having publicly derivable wallets
< sipa> i'm unsure if it should be default
< achow101> well it also means we don't need to unlock wallets to get more addresses
< achow101> so the "keypool" can be way smaller
< meshcollider> Yeah itd be quite nice to at least have the option there
< jonatack> +1
< 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
< sipa> yeah
< sipa> achow101: the keypool (=precomputed pubkeys for descriptors?) still needs to be as large as the gap limit, right?
< achow101> yes, it still needs to be as large as the gap limit
< achow101> but that doesn't need to be 1000
< achow101> could go back to 100
< sipa> fair, possibly not
< provoostenator> What happens in the current wallet if you import and rescan a wallet that uses keys beyond the gap limit?
< sipa> the overhead of a large keypool would also be smaller, i think?
< sipa> as you don't get new key entries and whatever in the wallet, or do you?
< achow101> yes
< achow101> every pubkey is still precomputed and stored for the descriptor cache
< provoostenator> The serialized descriptor only needs to hold on to its public cache
< provoostenator> And we can stop storing the encrypted indivual private keys
< provoostenator> And just derive those when signing.
< sipa> yeah
< achow101> I don't think we store individual private keys
< sipa> achow101: we do now :p
< provoostenator> Maybe I misread that today
< achow101> sipa: in descriptor wallets :)
< sipa> or do you mean in descriptor wallets PR?
< provoostenator> Today we do that for backwards compatibility with legacy wallets
< sipa> right, that's what I assumed
< sipa> we store individual keys for legacy wallets but not for descriptor wallets
< provoostenator> The descriptor wallet PR currently maintains that behavior afaik, but doesn't have to.
< sipa> i hope it does not
< sipa> descriptor wallets can't be and shouldn't be compatible with old software
< meshcollider> Yeah that's what we are trying to escape with the redesign
< provoostenator> sipa: the descriptor wallet flag already prevents old software from loading
< achow101> at one point, an implementation did do that because it was easier. but I thought I changed that
< provoostenator> achow101: maybe you only kept the serialization code aorund; that's where I ended reviewing today
< achow101> provoostenator: maybe you are confusing the descriptor master private key storage?
< provoostenator> AddKey and AddCryptedKey
< achow101> that's for the master private key
< achow101> just bad naming lol
< provoostenator> Aargh, not the first time I trip over that name.
< achow101> I'll rename it
< provoostenator> Ok, so assuming that's all great, we only need the password when expanding the (public) keypool.
< provoostenator> And when signing a transaction
< provoostenator> And it's not much burden.
< provoostenator> So that's not a reason to avoid hardened derivation by default
< achow101> it's the same thing you have to do today
< achow101> I suppose part of this is also that it seems like every other wallet has settled on a standardized derivation path scheme
< achow101> but at the same time, descriptors are completely unambiguous about the derivation to use
< provoostenator> Reasonably strong BIP44/49/84 adoption
< provoostenator> But definately not universal
< achow101> it's not a problem for people to import their stuff into our descriptor wallet as we can use any derivation path
< achow101> it's only a problem for people importing core to another wallet that may not allow custom derivation paths
< provoostenator> I wonder how common that use case is though, exporting keys to another wallet. Usually it's easier to just send them.
< achow101> Core -> other wallet is fairly common because people don't want to wait for IBD
< meshcollider> Yeah if you're restoring a backup or something and have keys but haven't synced to send tx
< meshcollider> It's probably fair to add a wallet creation option to be BIP 44/49/84 compliance
< achow101> threads on bitcointalk and questions on stackexchange about that show up pretty commonly
< provoostenator> AssumeUTXO to the rescue?
< provoostenator> meshcollider: as an option for sure
< provoostenator> Though without BIP39 mnemonic export that's still not practical
< achow101> that makes setup more annoying though, possibly need SetupGeneration to take more args
< provoostenator> achow101: it'll need more args for multisig too probably
< provoostenator> Actually no, those are still single descriptors.
< achow101> for multisigs, I would expect that to be imports rather than something we specifically allow in wallet creation
< provoostenator> If we were to add BIP39 support, then descriptors could contain the mnemonic.
< sipa> no
< sipa> the mnemonic is private information, and should be protected by the private key infrastructure
< sipa> descriptors are public
< achow101> sipa: descriptors can contain privkeys though
< achow101> that's supported for imports at least
< sipa> achow101: as syntactig sugar
< achow101> yes
< provoostenator> True, so inpractice you'd have a mnemonic in one place, and then have descriptors that just refer to the master key fingerprint
< sipa> for imports, supporting mnemonics is not a problem
< sipa> in descriptors
< sipa> but they'd be converted to a private key + descriptor with xpubs
< provoostenator> yes
< 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
< 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
< provoostenator> Or a descriptor without xpubs which assumes you have the matser key (doesn't work atm)
< sipa> provoostenator: hmm, no
< sipa> the descriptor needs to have enough information to generate future addresses
< provoostenator> OK, that's a reasonable constraint
< achow101> iirc, to be an "active" descriptor, we require IsSolvable() and IsRanged()
< achow101> although I think IsRanged() implies IsSolvable()
< provoostenator> (re multisig, I have an experimental createmultisigwallet RPC call in #16895 but I'm not convinced that's the way to go)
< gribble> https://github.com/bitcoin/bitcoin/issues/16895 | External signer multisig support by Sjors . Pull Request #16895 . bitcoin/bitcoin . GitHub
< 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.
< provoostenator> Based on some info it gets from other devices.
< provoostenator> But maybe for some other time...
< achow101> i think we can think about multisig after descriptor wallets is merged
< achow101> (in like 6 months)
< provoostenator> 2 weeks
< meshcollider> lol
< achow101> back to the public deriv thing, what do we want for the default?
< meshcollider> Will there be a "dumpprivdescriptor" RPC or something, how is the user even going to extract it from their wallet to import?
< provoostenator> Not initially but hard to guarantee that'll never happen.
< achow101> not initially
< sipa> i don't see a big problem with that
< achow101> I think eventually there will be
< sipa> it's obviously scary, and needs big warnings
< meshcollider> Probably
< sipa> but it's in the same order of dangerous as dumpprivkey now
< meshcollider> Or dumpwallet
< sipa> yeah, dumpwallet is a better comparison
< achow101> dumpwallet is deprecated now I think
< achow101> with descriptor wallets
< sipa> good
< meshcollider> Good
< provoostenator> More like dumpwallet than dumpkey, because one compromised privkey can comprise the full wallet without hardned deriv
< sipa> it should be
< achow101> I think i'll add a dumppublicdescriptor
< achow101> so at least you can import watch only to another wallet, and then do psbt signing
< provoostenator> achow101: +1
< meshcollider> Yeah then with that, BIP derivation seems more sensible
< jonatack> Yes
< jonatack> will try to look at #16528 next week
< gribble> https://github.com/bitcoin/bitcoin/issues/16528 | Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101 . Pull Request #16528 . bitcoin/bitcoin . GitHub
< meshcollider> Alright any last topics?
< provoostenator> sipa: taproot wallet support in two weeks?
< fjahr> just a review beg for #17824, there was some discussion 4 weeks ago but no ACKs yet
< gribble> https://github.com/bitcoin/bitcoin/issues/17824 | wallet: Improve coin selection for destination groups >10 by fjahr . Pull Request #17824 . bitcoin/bitcoin . GitHub
< meshcollider> Yep, merge before SF ;)
< meshcollider> #endmeeting
< lightningbot> Meeting ended Fri Jan 31 19:57:28 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< jonatack> fjahr: thanks
< meshcollider> fjahr: it's always hard to get coin selection review haha
< fjahr> meshcollider: hehe, I will try to be patient :)
< hebasto> jonasschnelli: is it feasible to point the latest successful feature_block.py on bitcoinbuild?
< jonasschnelli> hebasto: I found the issue (why the tests fail)... its out-of-memory
< jonasschnelli> Somehow running test_runner.py with the sanitizers requires more then 5GB of ram...
< jonasschnelli> 1603 ubuntu 20 0 1233224 1.071g 9028 S 2.0 22.1 0:24.26 python3
< jonasschnelli> Some tests require up to 1GB of ram (python part)
< jonasschnelli> running with -j 1 or -j 2 (default are 4 jobs)
< jonasschnelli> ... should reduce the mem footprint
< hebasto> jonasschnelli: thanks for info
< jonatack> Could someone kindly restart travis for #17812 please
< gribble> https://github.com/bitcoin/bitcoin/issues/17812 | config, test: asmap functional tests and feature refinements by jonatack . Pull Request #17812 . bitcoin/bitcoin . GitHub