19:00:29 <meshcollider> #startmeeting 
19:00:30 <core-meetingbot> Meeting started Fri Sep 24 19:00:29 2021 UTC.  The chair is meshcollider. Information about MeetBot at https://bitcoin.jonasschnelli.ch/ircmeetings.
19:00:30 <core-meetingbot> Available commands: action commands idea info link nick
19:00:34 <meshcollider> #bitcoin -core-dev Wallet Meeting: achow101 aj amiti ariard bluematt cfields Chris_Stewart_5 digi_james dongcarl elichai2 emilengler fanquake fjahr gleb glozow gmaxwell gwillen hebasto instagibbs jamesob jb55 jeremyrubin jl2012 jnewbery jonasschnelli jonatack jtimon kallewoof kanzure kvaciral lightlike luke-jr maaku marcofalke meshcollider michagogo moneyball morcos nehan NicolasDorier paveljanik petertodd phantomcircuit promag
19:00:34 <meshcollider> provoostenator ryanofsky sdaftuar sipa vasild wumpus
19:00:44 <achow101> hi
19:00:49 <michaelfolkson> hi
19:01:32 <sipa> hi
19:01:36 <meshcollider> I don't think there are any proposed topics, last minute proposals?
19:02:21 <michaelfolkson> I'll ask a couple of questions on achow101's recent Twitch streams/PRs that he's been working on if nothing else
19:03:28 <meshcollider> After another user earlier this week asking about upgrading an old wallet and why they're still only getting legacy addresses, I was wondering if we should add an easier way to fully flush the keypool after an upgrade
19:04:14 <meshcollider> achow101: what's the reason we keep using the old pre-HD keypool after upgrade until it has run out? Why not just use the new HD keys immediately?
19:04:23 <achow101> it wouldn't be particularly hard to add a function like "newkeypool"
19:05:10 <meshcollider> Yeah it'd be super easy, could overload keypoolrefill too, but I wanted to discuss the rationale first
19:05:19 <sipa> fwiw, when upgrading from unencrypted to encypted wallet, this already happens
19:05:24 <sipa> so i'm sure that code exists
19:05:45 <achow101> meshcollider: I don't remember, would have to check the original PR
19:06:07 <achow101> sipa: there's a convenient NewKeypool() function :)
19:07:34 <prayank> I had one question
19:07:38 <meshcollider> Hmm was it #12560
19:07:41 <gribble> https://github.com/bitcoin/bitcoin/issues/12560 | [wallet] Upgrade path for non-HD wallets to HD by achow101 · Pull Request #12560 · bitcoin/bitcoin · GitHub
19:08:44 <achow101> I think so
19:10:16 <meshcollider> Strange, the OP says "When it is used for such an upgrade, the keypool will be regenerated."
19:10:26 <meshcollider> So you must have changed it for some reason through the reviews
19:10:34 <achow101> meshcollider: I remember now. It's because -upgradewallet would do the wallet before syncing
19:11:08 <achow101> I think it had something to do with the wallet being upgraded and then losing coins due to sync?
19:11:16 <achow101> maybe not
19:11:32 <achow101> I'm pretty sure it was implemented that way because of this comment: https://github.com/bitcoin/bitcoin/pull/12560#issuecomment-377567454
19:11:49 <meshcollider> Interesting
19:12:11 <meshcollider> So I feel like we should change it now to flush the keypool on upgrade from non-HD to HD
19:12:37 <meshcollider> Or even after every upgrade?
19:13:21 <achow101> oh, I think part of it was that it was basically impossible to show a warning to users that they had to make a new backup after -upgradewallet
19:13:26 <achow101> but that's no longer relevant with the upgradewallet RPC
19:13:51 <achow101> I think it would be reasonable to have upgradewallet flush after upgrading to HD
19:13:54 <meshcollider> Cool, I'll open a PR after the meeting then
19:14:00 <achow101> as well as a dedicated flush keypool RPC
19:14:12 <meshcollider> prayank: Sure, what was your question :)
19:14:17 <prayank> I wanted to start working on things explained in this issue and create a draft PR but don't want to waste time and end up with NACKs in few minutes. So what are your thoughts on it?
19:14:21 <prayank> https://github.com/bitcoin/bitcoin/issues/22424
19:15:23 <achow101> can you briefly explain the idea?
19:15:32 <prayank> Yes
19:15:42 <michaelfolkson> Dirty coins are dumped into a subwallet and can't be spent in the original wallet?
19:15:54 <prayank> Right
19:16:14 <prayank> This was suggested in a discussion by harding
19:16:19 <prayank> Let me find the link
19:16:35 <meshcollider> Its on the issue, https://github.com/bitcoin/bitcoin/issues/22018#issuecomment-850803134
19:17:08 <michaelfolkson> Always upsides and downsides but one downside appears to be reduction in the success rate of BnB
19:17:17 <michaelfolkson> Less UTXOs at its disposal
19:18:16 <prayank> Let me explain with one example
19:18:27 <achow101> I would generally avoid automatically creating new wallet files when certain behavior occurs
19:19:03 <achow101> a "sub wallet" should just be an internal structure
19:19:18 <prayank> Alice creates a new address. Bob sends multiple times to it. Becomes dirty. Detected by Core and moved to New wallet with keys and all meta data.
19:20:03 <michaelfolkson> achow101: Can a subwallet with UTXOs that can't be spent by the original wallet fit into that "internal structure"? I'm guessing not
19:20:22 <michaelfolkson> The divide is pretty clear
19:20:26 <achow101> michaelfolkson: it's a program, you can make anything work :)
19:20:50 <michaelfolkson> Ha ok, I don't know what you mean by an internal structure then ;)
19:21:27 <prayank> michaelfolkson: Maybe not show user a new wallet is created
19:21:47 <achow101> with meshcollider's persistent utxo locks, it seems like it would be fairly easy to detect these "dirty utxos" and apply a utxo lock on them
19:22:26 <meshcollider> Doesn't avoid_reuse basically do that anyway
19:22:34 <achow101> later users can go in, manually unlock those utxos, and do whatever with them. I think that approximately achieves what #22424 describes
19:22:34 <gribble> https://github.com/bitcoin/bitcoin/issues/22424 | Sub wallet for dirty coins · Issue #22424 · bitcoin/bitcoin · GitHub
19:23:11 <meshcollider> Could possibly make it easier to do so with an RPC "consolidatedirty" or something
19:23:19 <michaelfolkson> meshcollider: Isn't avoid_reuse avoidance rather than making it impossible?
19:23:59 <achow101> but in general, I don't particularly like the idea of outright disabling users from spending certain UTXOs just because they are reused
19:24:06 <achow101> it is trivial to lock people out of funds by dust spam
19:24:13 <achow101> s/is/becomes
19:24:33 <michaelfolkson> To me avoidance seems fine. Literally dumping the private key so that the original wallet can't spend a UTXO in any circumstances seems overkill
19:24:37 <prayank> meshcollider: avoid_reuse has issues
19:25:21 <prayank> achow101: Won't be default. Only for user who want this level of privacy can use it.
19:25:34 <achow101> prayank: perhaps improve avoid_reuse rather than throwing it out?
19:26:15 <michaelfolkson> prayank: What are the issues?
19:26:20 <meshcollider> I like achow101's suggestion of simply locking the UTXOs, but I think then there should be some way to distinguish these types of UTXO for when you do want to consolidate (e.g. red on GUI as suggested)
19:26:40 <prayank> achow101: well 2 issues that exist are 1. Unable to spend unconfirmed UTXO 2. OUTPUT_GROUP_MAX_ENTRIES = 100 and I was told both will get NACKs if I try to fix them
19:26:57 <achow101> meshcollider: we could add a "lock reason"
19:27:28 <achow101> prayank: that's avoidpartialspends, not avoid_reuse
19:28:31 <achow101> there is a subtle difference IIRC
19:28:32 <prayank> Okay they looked similar things maybe I misunderstood. avoid_reuse is a just a flag right in wallet?
19:28:42 <meshcollider> Yes
19:28:54 <meshcollider> WALLET_FLAG_AVOID_REUSE
19:30:14 <jonatack> hi
19:30:28 <achow101> avoid_reuse will cause coin selection to exclude UTXOs that have already been spent previously
19:30:41 <prayank> jonatack: any opinion on this?
19:31:07 <jonatack> prayank: maybe have a look at src/wallet-init.cpp:47 regarding avoidpartialspends, and grep for it in the codebase
19:31:36 <achow101> so it seems like having a wallet with avoid_reuse already solves the problem?
19:31:45 <meshcollider> I think a consolidatedirty RPC could still be useful
19:32:13 <meshcollider> No actually that would basically be impossible to do well
19:32:51 <meshcollider> Is there an easy way to list the dirty UTXOs being avoided by avoid_reuse?
19:33:21 <achow101> meshcollider: listunspent has a "reused" value
19:34:01 <meshcollider> True, okay, in that case it should still be possible to manually run them through a mixer or something if desired
19:34:38 <achow101> in any case, I think prayank needs to have a look at avoid_reuse in combination with avoidpartialspends first
19:34:57 <achow101> before we have any further discussions with changes to make
19:35:05 <prayank> Okay I will check these things in detail
19:35:20 <prayank> Thanks everyone
19:35:23 <meshcollider> prayank: thanks! Glad you have some good ideas for this though :)
19:35:30 <meshcollider> Any other topics?
19:35:41 <michaelfolkson> A couple of quick ones
19:36:02 <meshcollider> michaelfolkson: Go ahead
19:36:18 <michaelfolkson> One, any way to speed up the Proposed Timeline for Legacy Wallet and BDB removal?
19:36:30 <jonatack> don't worry prayank, it takes time, gathering and slowly accumulating context helps
19:36:37 <michaelfolkson> 27.0 and 2024 seems long
19:36:46 <michaelfolkson> https://github.com/bitcoin/bitcoin/issues/20160
19:37:04 <michaelfolkson> Given there is migration code
19:37:19 <achow101> maybe, but it's intentionally slow and drawn out
19:37:32 <michaelfolkson> I just wonder if it is too drawn out :)
19:37:43 <achow101> the point is to ease the transition, especially since the migration might not migrate everything
19:37:45 <prayank> jonatack: True :)
19:38:02 <achow101> (it is difficult to replicate everything that ismine would match on)
19:38:12 <michaelfolkson> achow101: Oh the migration might not... ok I didn't consider that
19:38:23 <michaelfolkson> Ok that is a reason for being extra cautious
19:38:47 <meshcollider> michaelfolkson: why would you like it brought forward? Just code legacy code maintaince reasons?
19:39:04 <achow101> the idea is that there will be time to migrate while all of the legacy wallet stuff is still there so that if there are any issues, a backup can be used and the user could just make a new descriptor wallet and move coins
19:39:11 <sipa> also, we regularly see people on stackexchange trying to recover funds from wallet.dat files from 2013 or older
19:39:33 <achow101> also, 2024 is not as far away as you think it is :p
19:39:47 <michaelfolkson> Ok fair enough, leave as is
19:39:48 <sipa> it's closer to today than segwit's activation
19:39:50 <meshcollider> Scary thought
19:40:22 <michaelfolkson> And second question, what was the point of the coin selection simulations? Just see how often the different algorithms were used?
19:40:53 <achow101> the coin selection simulations are to compare how the algorithms perform
19:41:18 <achow101> with perform being somewhat subjective hence all of the data output by the simulation
19:41:43 <michaelfolkson> To check that the preference for BnB makes sense? And check that Knapsack and SRD are equal second preference?
19:42:19 <michaelfolkson> The conclusion was SRD wasn't quite as good as we thought but not materially enough to change that preference ordering?
19:42:38 <achow101> not really check anything specific
19:42:52 <meshcollider> There is no preferred order after the new waste metric approach is there
19:42:55 <achow101> but moreso see what the effects are in general, e.g. how the utxo set is affected, how fees are affected, etc.
19:43:01 <michaelfolkson> I guess if SRD became third preference it would never get used. Knapsack never fails right?
19:43:12 <achow101> with the waste metric, there's no explicit preference
19:43:16 <sipa> don't forget that these algorithms affect eachother
19:43:31 <sipa> introducing one means your wallet's UTXO composition changes
19:43:38 <sipa> which may affect the behavior of the other algorithms
19:43:56 <michaelfolkson> Hmm ok interesting
19:44:23 <achow101> the point of the simulations is to compare the different strategies we could use and see how they behave with a common dataset and common metrics
19:44:32 <achow101> and then somewhat try to rationalize why we see certain behaviors
19:44:53 <michaelfolkson> Ok cool, makes sense
19:44:59 <michaelfolkson> Ok I'm done, thanks
19:45:21 <achow101> I have some old gists with other simulation results from a variety of combinations of BnB + knpsack and other things that were proposed in the past
19:45:41 <achow101> with some things like different min change targets, different feerates, etc.
19:46:06 <michaelfolkson> I knew it wasn't pointless :) Just didn't know what the major objective(s) was
19:46:23 <meshcollider> Just briefly, re: #23065, do people think all unlocks should remove both persistent and non-persistent locks, or should it be possible to temporarily unlock a persistent lock
19:46:25 <gribble> https://github.com/bitcoin/bitcoin/issues/23065 | Allow UTXO locks to be written to wallet DB by meshcollider · Pull Request #23065 · bitcoin/bitcoin · GitHub
19:46:57 <achow101> I think it should remove both
19:47:27 <meshcollider> Ok
19:47:47 <meshcollider> That's all then, no more topics?
19:48:17 <achow101> just a review beg for #17526
19:48:19 <gribble> https://github.com/bitcoin/bitcoin/issues/17526 | Add Single Random Draw as an additional coin selection algorithm by achow101 · Pull Request #17526 · bitcoin/bitcoin · GitHub
19:48:35 <meshcollider> Topical :)
19:48:37 <meshcollider> #endmeeting