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