19:00:04 <meshcollider> #startmeeting 19:00:04 <lightningbot> Meeting started Fri Apr 12 19:00:04 2019 UTC. The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:00:04 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic. 19:00:07 <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 19:00:11 <jnewbery> hi 19:01:10 <achow101> hi 19:01:35 <meshcollider> #topic : Upgrade wallet (#15761) (jnewbery) 19:01:37 <gribble> https://github.com/bitcoin/bitcoin/issues/15761 | Replace -upgradewallet startup option with upgradewallet RPC by achow101 · Pull Request #15761 · bitcoin/bitcoin · GitHub 19:02:14 <kanzure> hi 19:02:26 <jnewbery> This PR is to replace the -upgradewallet startup option, which is great news 19:03:01 <jnewbery> achow is proposing to replace it with an RPC. There were a couple of other suggestions on what to replace it with in the PR and I wanted to poll for opinions 19:03:10 <achow101> I think the primary question is RPC, wallet-tool command, or both 19:03:30 <achow101> I prefer both 19:03:47 <jnewbery> I'm slightly concerned about having an RPC that can upgrade at any time. I just feel that it might introduce subtle corner cases if the wallet is doing something else at the time 19:04:16 <jnewbery> but I might be wrong. Perhaps it's fine 19:04:44 <achow101> the whole operation is locked, and any operations on the wallet are also locked, so I don't think that's really a problem 19:04:47 <jnewbery> (I'm definitely a big concept ACK on moving away from startup option) 19:05:28 <kanzure> does upgradewallet replace the wallet or does it create an upgraded wallet only? 19:05:40 <achow101> kanzure: it just upgrades the wallet file 19:05:52 <luke-jr> could be only allowed when loading 19:06:03 <meshcollider> I'm in favour of both too 19:06:50 <jnewbery> luke-jr: that's one suggestion in the PR. Another option would be to only allow it on wallets that aren't currently loaded. 19:07:11 <jnewbery> (ie separate upgrade from running entirely) 19:07:13 <achow101> jnewbery: what if upgradewallet unloads it, upgrades, then reloads it 19:07:29 <achow101> that would disconnect all of the signals and stuff that would cause issues 19:07:34 <kanzure> need lock during upgrade 19:07:38 <kanzure> so that they don't reopen 19:07:52 <kanzure> in achow101's flow. 19:08:16 <jnewbery> Should we encourage users to backup before they upgrade? 19:08:30 <meshcollider> kanzure: I think thats the current behavior anyway 19:08:53 <kanzure> meshcollider: ok, i thought the lock only applies to loaded wallets. nevermind. 19:09:29 <achow101> kanzure: meshcollider: in the idea I just proposed, you would have to lock something to prevent loading the wallet during upgarde 19:09:35 <luke-jr> jnewbery: at least after 19:09:58 <harding> Wallet files are generally small. If you think a backup is important, it's probably better to just make one automatically and stuff it somewhere in ~/.bitcoin/ 19:10:07 <achow101> jnewbery: probably. the help text for the RPC does say that backups after are required 19:10:15 <achow101> I can change it to also say before too 19:10:32 <meshcollider> backup before can be done automatically like harding suggests 19:10:41 <meshcollider> Make a wallet.old or something 19:10:47 <harding> (Backup after is a different thing, since that's talking about off-disk backups.) 19:11:14 <luke-jr> achow101: well, only after is *required*.. 19:12:58 <achow101> right 19:14:17 <achow101> I think it's possible to make both an upgradewallet RPC and wallet-tool command work safely 19:15:58 <meshcollider> It doesn't seem like theres any real objection to that 19:16:56 <meshcollider> jnewbery: anything else to discuss on this topic? 19:17:12 <jnewbery> nothing from me 19:17:31 <meshcollider> #topic native descriptor wallets (achow101) 19:18:01 <achow101> So i finished up an implementation of native descriptor wallets last weekend: #15764 19:18:03 <gribble> https://github.com/bitcoin/bitcoin/issues/15764 | Native descriptor wallets by achow101 · Pull Request #15764 · bitcoin/bitcoin · GitHub 19:18:26 <achow101> the only question I really have is what to do with the tests for the old wallet stuff 19:18:50 <jnewbery> I think it's pretty important to keep those tests 19:18:58 <achow101> currently I have a bunch of tests removed or changed because they don't work with descriptor wallets 19:19:17 <jnewbery> Most users will still be using non-descriptor wallets for some time. We can't just stop testing those and hope there are no regressions 19:19:45 <ryanofsky> you can add descriptors as an optional feature, there's no need to remove old code or old tests 19:22:31 <achow101> ryanofsky: that's an option, but I feel like descriptors and its new definitions is such a departure from current wallet stuff that it should have better distinction than just a wallet flag 19:23:57 <ryanofsky> you are referring to the practical downside of having to keep more code around? 19:23:58 <achow101> e.g. it's possible for someone to write code which accidentally unsets the wallet flag. it's much harder to accidentally downgrade the version 19:24:01 <meshcollider> Does your PR remove the ability to generate new "old" wallets 19:24:15 <achow101> meshcollider: yes 19:24:22 <achow101> (it's a wallet version bump) 19:26:02 <sipa> achow101: i think, even for just testing purposes, we'll need to retain the ability to create old wallets 19:26:15 <jnewbery> The main downside of it being a flag rather than a version is that it becomes combinatorially more difficult to test everything 19:26:24 <sipa> as this is a very invasive change, and i don't think we want to lose the ability to test old logic 19:26:51 <jnewbery> I think that the default should be that newly created wallets are old-style, and users need to explicitly upgrade 19:26:51 <bitcoin-git> [13bitcoin] 15jamesob opened pull request #15809: gitignore: plist and dat (06master...062019-04-gitignore) 02https://github.com/bitcoin/bitcoin/pull/15809 19:27:09 <achow101> jnewbery: how come? 19:27:45 <achow101> I think an explicit upgrade is actually far more dangerous than creating a new descriptor wallet 19:27:45 <jnewbery> because like you say, it's a big departure from the current wallet design 19:28:10 <jnewbery> we need to support both for some time to come, so why not take the conservative approach for now 19:28:21 <meshcollider> I think thats sensible for now, old wallet generation can be "deprecated" later on 19:29:17 <ryanofsky> yeah, i just think you don't need to add descriptors as this big one time change 19:29:50 <jnewbery> > I think an explicit upgrade is actually far more dangerous... 19:29:51 <ryanofsky> you can add new functionality alongside existing functionality, you will get better review better testing 19:30:07 <jnewbery> We definitely shouldn't be releasing wallet code that we think is *in any way* dangerous 19:30:32 <achow101> ok 19:31:03 <achow101> jnewbery: the upgrade stuff I think will be inherently dangerous. shoehorning the old ismine logic into the new ismine logic is not trivial and they are incompatible in many different ways 19:31:35 <ryanofsky> achow101, you don't even need to add upgrade in the initial pr 19:32:10 <ryanofsky> we can add support for just creating new descriptor wallets, or just importing descriptors first 19:32:28 <jnewbery> i don't agree that it's inherently dangerous. We just need to do lots of testing until we're satisfied that it's no longer dangerous 19:32:30 <meshcollider> provoostenator is having IRC issues and can't send messages here atm but would like to point out he also has a slightly different (and perhaps less complete) PR open: https://github.com/bitcoin/bitcoin/pull/15487 19:32:45 <jnewbery> and roll it out slowly, with warnings to backup, etc 19:32:49 <achow101> ryanofsky: right 19:33:36 <jnewbery> Everything ryanofsky is suggesting is still possible by using wallet versions and not flags 19:33:37 <achow101> ryanofsky: we shouldn't allow people to create mixed descriptor and non-descriptor wllets though. so no importing descriptors 19:34:05 <achow101> jnewbery: making it optional is not 19:34:19 <achow101> that should only be done with flags 19:34:29 <jnewbery> why not? Add a new parameter to createwallet 19:34:50 <ryanofsky> yeah, i'm not sure you actually need a version or a flag 19:35:05 <jnewbery> if descriptorwallet=false create an old-style wallet, if true create a descriptor wallet 19:35:17 <achow101> the last time we did optional version was hd wallet and that was a headache to reconcile with upgrades in the future. I would rather not go through that excercise again 19:35:18 <jnewbery> default to false 19:35:37 <jnewbery> we need to handle upgrades anyway. You already have code to do that 19:36:05 <provoostenator> hi? 19:36:09 <harding> provoostenator: hi 19:36:12 <provoostenator> YES! 19:36:18 <meshcollider> provoostenator: hi \o/ 19:36:23 <jnewbery> hi sjors! 19:36:24 <provoostenator> That was weird, I've been talking into a void for a day or so :-) 19:37:03 <provoostenator> In my version of descriptor wallets it's a feature flag and opt-in. Obviously this sort of thing is easy to tweak. 19:37:16 <achow101> jnewbery: I think we should maintain the separation of wallet flags for optional, wallet version for mandatory 19:37:17 <provoostenator> I'll do a more thorough comparison later. 19:37:58 <jnewbery> wallet version is optional. Upgrading from an old version to new is optional 19:37:59 <meshcollider> achow101: but why not just allow the wallet to be created with either version number 19:38:06 <jnewbery> right 19:38:14 <ryanofsky> jnewbery, i think version numbers are just confusing and should never be used again 19:38:52 <jnewbery> They are confusing, but they at least cut down on the number of combinations of options 19:38:52 <ryanofsky> your concern about testing and flag combinations is easily addressed by just refusing to load / support wallets with whatever combinations of flags you want to rule out 19:39:26 <jnewbery> I think that's effectively the same thing, no? 19:39:33 <meshcollider> IIRC in lightning, when exchanging feature lists, there are some optional features and some mandatory features based on bit positions 19:39:36 <ryanofsky> yes, exactly 19:39:49 <achow101> meshcollider: jnewbery: the problem with optional wallet versions is that any future wallet version becomes optional as well. You can't e.g. skip descriptor wallets and go to the version after that which introduces something completely unrelated to descriptors 19:39:56 <ryanofsky> anything you want to do with version numbers is possible with flags, but flags are more readable and easier to think about 19:40:00 <achow101> reconciling the two becomes a pain, as it was for hd wallets 19:40:17 <jnewbery> achow101 that's always the case with version numbers 19:40:24 <meshcollider> We dont have to add any new features to old style wallets from now on 19:40:35 <meshcollider> Force a descriptor upgrade before any other upgrade 19:40:45 <ryanofsky> because you don't have to mentally load the whole project history to figure out what conditions are possible, it's just written explicitly in code 19:41:08 <provoostenator> I'd rather not force upgrades anytime soon. That most likely will lead to endless delays in shipping. 19:41:11 <jnewbery> even if you don't make the version 'optional', there are still users of old wallets who would have to upgrade through descriptor wallets if they wanted a later version 19:41:34 <jnewbery> provoostenator: definitely. No forced upgrades 19:42:12 <achow101> i agree with ryanofsky 19:42:25 <achow101> anyways, this is bikeshedding 19:42:32 <jnewbery> ? 19:42:51 <meshcollider> I think this is important for the approach 19:43:20 <provoostenator> What's more important I think is to decide if we want to support a hybrid with descriptors and regular stuff (I prefer not). 19:43:34 <achow101> provoostenator: definitely no 19:43:38 <meshcollider> No, we shouldnt 19:44:54 <provoostenator> Ok, so that can be supported both with versioning and with feature flags I think. We can revisit that later? 19:45:17 <provoostenator> Or is there something where this choice does matter more urgently? 19:45:29 <achow101> meshcollider: jnewbery: consider the case where we introduce descriptor wallets as a new optional version. Sometime later down the road, we introduce a new wallet version because a new field is introduced in the wallet that is inherently not backwards compatible so it needs a version bump to prevent old software from loading, e.g. several more wallet flags 19:45:44 <meshcollider> I think it just affects how to deal with transitioning tests 19:46:24 <achow101> in order for users to upgrade to that version with more wallet flags, they first have to upgrade to descriptors, which they may not want to. that new version provides new functionality that does not require descriptors but is still applicable to old wallets. now this needs to be reconciled, and that's rather difficult 19:46:37 <ryanofsky> yeah, i don't see why flags aren't just obviously better in every case. you can still prevent combinations of features, you are just forced to write down which combinations are possible 19:46:48 <jnewbery> achow101 are you arguing that this should be a flag? 19:47:04 <meshcollider> Flags alone dont prevent old nodes from trying to open though do they? 19:47:08 <achow101> jnewbery: yes. if it is going to be an optional feature, it should be a flag 19:47:23 <ryanofsky> meshcollider, mandatory flags do 19:47:49 <jnewbery> Of course it's an optional feature. We don't force people to upgrade! 19:48:02 <meshcollider> Ok yes I agree with making it a mandatory flag rather than version 19:48:28 <jnewbery> I'm fine with it being a flag. Like ryanofsky has said, you can do everything with flags that you can with versions (and more) 19:49:14 <jnewbery> As long as the valid combinations are all documented and enforced in one place 19:49:17 <provoostenator> A mandatory flag also means we can reduce complexity. 19:49:40 <provoostenator> Only supporting upgrades for basic cases. 19:49:51 <achow101> alright, i'll change the pr to use a flag, restore the tests, and drop the upgrading logic for now 19:50:02 <meshcollider> Just because its a flag doesn't mean we have to allow all possible combinations of flags 19:50:43 <meshcollider> achow101: +1 19:50:56 <meshcollider> Anything else for the last 10 minutes? 19:51:22 <meshcollider> Anything else for backport to 0.18.0rc4 or high priority requests? 19:51:50 <jnewbery> Wallet high priority PRs are #15006 #14447 #15741 19:51:54 <gribble> https://github.com/bitcoin/bitcoin/issues/15006 | Add option to create an encrypted wallet by achow101 · Pull Request #15006 · bitcoin/bitcoin · GitHub 19:51:55 <gribble> https://github.com/bitcoin/bitcoin/issues/14447 | Armory 0.96.4 causies BitcoinCore 0.17 crash sometimes · Issue #14447 · bitcoin/bitcoin · GitHub 19:51:57 <gribble> https://github.com/bitcoin/bitcoin/issues/15741 | Batch write imported stuff in importmulti by achow101 · Pull Request #15741 · bitcoin/bitcoin · GitHub 19:52:02 <jnewbery> sorry #15557 19:52:06 <gribble> https://github.com/bitcoin/bitcoin/issues/15557 | Enhance `bumpfee` to include inputs when targeting a feerate by instagibbs · Pull Request #15557 · bitcoin/bitcoin · GitHub 19:52:33 <jnewbery> I think 15557 is almost ready for merge. It's a really nice feature and quite easy to review 19:53:06 <meshcollider> Maybe kallewoof would like to have that debit/credit caching one on there 19:53:17 <meshcollider> #15780 19:53:20 <gribble> https://github.com/bitcoin/bitcoin/issues/15780 | wallet: add cachable accounts for caching credit/debit values by kallewoof · Pull Request #15780 · bitcoin/bitcoin · GitHub 19:54:04 <jnewbery> I think that can go on. It blocks his other PR 19:54:26 <jnewbery> (which has been open for 8 months) 19:55:12 <meshcollider> Yes, and he is very proactive with rebasing, he deserves some acceleration lol 19:55:30 <jnewbery> #action review more kallewoof PRs! 19:56:11 <meshcollider> Ok seems thats all for the meeting today then :) 19:56:16 <meshcollider> #endmeeting