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