19:01:21 <achow101> #startmeeting 
19:01:22 <core-meetingbot> Meeting started Fri Aug 12 19:01:21 2022 UTC.  The chair is achow101. Information about MeetBot at https://bitcoin.jonasschnelli.ch/ircmeetings.
19:01:22 <core-meetingbot> Available commands: action commands idea info link nick
19:01:32 <achow101> #bitcoin -core-dev Wallet Meeting: achow101 _aj_ amiti ariard BlueMatt cfields Chris_Stewart_5 darosior digi_james dongcarl elichai2 emilengler fanquake fjahr furszy gleb glozow gmaxwell gwillen hebasto instagibbs jamesob jarolrod jb55 jeremyrubin jl2012 jnewbery jonasschnelli jonatack josibake jtimon kallewoof kanzure kvaciral laanwj larryruane lightlike luke-jr maaku marcofalke meshcollider michagogo moneyball morcos Murch nehan NicolasDorier
19:01:32 <achow101> paveljanik petertodd phantomcircuit promag provoostenator ryanofsky sdaftuar S3RK sipa vasild
19:02:02 <furszy> hi
19:02:13 <achow101> There are no pre-proposed wallet meeting topics. Does anyone have any last minute topics to discuss?
19:03:44 <S3RK_> hi
19:04:06 <achow101> From yesterday's general meeting, it seems like we've decided to try to get #19602 for 24.0. The feature freeze is on Monday
19:04:08 <gribble> https://github.com/bitcoin/bitcoin/issues/19602 | wallet: Migrate legacy wallets to descriptor wallets by achow101 · Pull Request #19602 · bitcoin/bitcoin · GitHub
19:05:47 <S3RK_> I started reviewing this a long time ago, but lost all the context now. Need to start from scratch
19:05:59 <S3RK_> achow101 do you think we can get it reviewed by Mon?
19:06:29 <achow101> we may allow the feature freeze to slip a bit to get it in
19:06:46 <achow101> but probably not more than a week
19:07:54 <S3RK_> I'd imagine testing a big part of shipping this feature. What's the status with testing?
19:08:25 <achow101> I think reviewers have been testing manually
19:08:30 <achow101> there is also a functional test
19:08:40 <achow101> that tests several different scenarios
19:09:17 <achow101> the rpc is also marked as experimental
19:09:26 <S3RK_> I agree it'd be great to have it 24.0 can only blame myself for not paying more attention to it
19:11:17 <S3RK_> I can spend some time on it from Mon. Probably can do either review or some manual testing. What do you think would be more helpful?
19:11:39 <furszy> should be more a "what might be missing" work than actual testing
19:11:41 <achow101> I think review would be more helpful
19:11:57 <achow101> the main thing is that legacy wallets can have some weird script scenarios that I might have missed
19:12:05 <achow101> and manual testing probably won't get to those
19:12:11 <S3RK_> got it
19:14:44 <achow101> is there anything else to discuss?
19:15:02 <S3RK_> one thing
19:15:29 <S3RK_> I thought maybe it could be interesting to do coin selection simulations for each release
19:16:15 <S3RK_> we do them for some PR which we think could have an effect, but sometimes the consequences are hard to predict
19:16:51 <S3RK_> do you think it'll be beneficial to just do checkpoints at release time? or maybe at some other interval?
19:16:57 <achow101> that could be interesting
19:17:36 <achow101> it would probably make sense to do one on the first rc of each release?
19:17:54 <S3RK_> yep. that sounds reasonable
19:18:12 <S3RK_> is there a checklist or something where we can add a bullet point?
19:18:17 <furszy> goal is bench speed/performance or the selection results?
19:18:25 <instagibbs> or feature freeze, in case you want to get a head start on a regression fix :P
19:18:34 <instagibbs> unless it's just informational
19:18:49 <S3RK_> catching a regression is definitely a part of it
19:19:36 <achow101> there's a release checklist in doc/release-process.md
19:19:53 <achow101> but this seems more like something that a wallet contributor remembers to do around release time
19:20:05 <achow101> it would be nice if we had an automated way to do simulations
19:20:14 <S3RK_> furszy: not speed, but rather effectiveness in terms of fees/utxo set/changeless tx
19:21:10 <S3RK_> for starters: a step in release process could be just to poke wallet maintainer :D
19:21:49 <S3RK_> ofc it'll be great to automate things. What are the challenges today? I saw achow101's optimizations got merged
19:22:24 <achow101> I think it's just a matter of doing it
19:22:40 <achow101> I know that josibake has a project that kind of automates simulations
19:22:44 <achow101> https://github.com/josibake/bitsie
19:23:12 <S3RK_> yep, I used it on a VM. Do you imagine just running simulation as a CI for each PR?
19:24:14 <achow101> I think that would be useful, but would probably have to be separate from our actual CI
19:24:21 <achow101> since they can take a really long time
19:25:03 <S3RK_> wouldn't we hit some limits in terms of CPU/jobs or run time?
19:25:34 <S3RK_> sound heavy to run sims for all PRs
19:25:34 <achow101> yeah
19:25:57 <furszy> and there shouldn't be many PRs changing behavior there
19:26:06 <achow101> maybe something like the "guix build requested" label
19:26:19 <S3RK_> that's interesting
19:26:21 <instagibbs> yeah
19:26:53 <achow101> so we could just run it on the prs that would effect coin selection
19:27:11 <Murch> hi
19:29:29 <Murch> Yeah, that sounds interesting
19:30:27 <Murch> So far I've been running simulations manually, it would definitely be nice to get it automatically. How could we control how often it gets run though?
19:31:13 <Murch> Let's say if you spot a typo and forcepush to fix briefly after pushing, it shouldn't do the whole simulation twice.
19:31:32 <Murch> Perhaps if we leave a few hours until kicking off the simulation and only run it for the latest version?
19:32:14 <S3RK_> I like the idea with using a label to trigger the CI
19:32:21 <achow101> if we do it the same way that drahtbot guix builds work, then it's basically just triggered manually whenever the label is added
19:32:46 <achow101> the bot removes the label when it's done
19:35:13 <Murch> Who can add labels?
19:35:29 <achow101> anyone who has the permission
19:36:26 <S3RK_> hm.. is it a separate permission? I think I can't modify lables even for my PRs
19:37:04 <achow101> it's a separate permission, but I think we give it out pretty freely
19:38:10 <S3RK_> it's good that it's separate
19:40:21 <S3RK_> If nobody have more topics, I have one more thing about #25647
19:40:23 <gribble> https://github.com/bitcoin/bitcoin/issues/25647 | wallet: return change from SelectionResult by S3RK · Pull Request #25647 · bitcoin/bitcoin · GitHub
19:40:26 <achow101> go ahead
19:40:41 <S3RK_> Murch pointed out a small issue with that PR
19:40:58 <S3RK_> at high fee rates LOWER_CHANGE won't cover change_fee
19:41:22 <S3RK_> I think easy way to fix that is to make LOWER_CHANGE dynamic = max(50000sat, change_fee)
19:41:29 <S3RK_> any reasons why it's bad or won't work?
19:41:42 <S3RK_> sorry it's CHANGE_LOWER
19:41:59 <Murch> That sounds fine to me
19:42:15 <Murch> I don't think it'll be hit until well above 1000 s/vB anyway
19:42:31 <Murch> I'm not sure whether a change target that is exactly the fee makes sense, though
19:42:47 <Murch> maybe it should rather be fee+dust
19:42:53 <S3RK_> it's not change traget but rather CHANGE_LOWER
19:43:07 <S3RK_> then change target is calucalated using GenerateChangeTarget
19:43:24 <Murch> Ah right
19:43:41 <achow101> you could just change it to have that parameter include the fee?
19:43:55 <achow101> so m_min_change_target + m_change_fee
19:43:56 <Murch> Well, but it could still mean that if we roll a low value and match very closely, we would fall into the dust territory, right?
19:44:48 <S3RK_> I don't think it's possible. Given that dustRealy is 3sat/vb and we need to bump CHANGE_LOWER at fees about 1000sat/vb
19:44:54 <achow101> or rather GenerateChangeTarget should incorporate the change fee
19:46:03 <S3RK_> I like jsut adding m_min_change_target + m_change_fee
19:46:10 <S3RK_> that's also very clean and simple
19:46:43 <Murch> +1
19:47:53 <S3RK_> I'll add a commit to ensure change_target is always higher than change_fee using achow101 suggestion
19:47:56 <S3RK_> thanks
19:48:01 <achow101> cool
19:48:03 <achow101> anything else to discuss?
19:48:38 <S3RK_> not for my side
19:49:09 <Murch> I've spent a lot of time thinking about #24362 recently, I think that we could probably move it towards merging or closing
19:49:11 <gribble> https://github.com/bitcoin/bitcoin/issues/24362 | wallet: Do not match legacy addresses for change type by achow101 · Pull Request #24362 · bitcoin/bitcoin · GitHub
19:49:17 <Murch> I'm still on the fence
19:50:02 <Murch> right now fees are so low, I'd say close it and just match the legacy outputs, but in the long-term I do think that it would potentially create unexpected costs for users
19:50:16 <Murch> that is, not to merge it
19:50:42 <S3RK_> if you don't want to create legacy change outputs you shouldn't have a descriptor
19:50:51 <S3RK_> it's possible to deactivate a descriptor
19:50:59 <S3RK_> unless we're talking legacy wallets :D
19:51:06 <achow101> well legacy wallets are still a thing
19:51:36 <Murch> S3RK_: I think we have to operate under the assumption that most users will never change a config option or otherwise customize their wallet
19:52:23 <S3RK_> I can see why asking users to understand decsriptors could be too much
19:52:32 <Murch> And it might feel "more surprising" to a user that they suddenly sent themselves a legacy change output than the opposite
19:52:39 <S3RK_> but do you think it's too much for them to be aware what types their wallet supports?
19:54:33 <Murch> Yeah
19:54:43 <S3RK_> it feels to me adding more special cases like this make wallet behavior opaque and unpredictable in a way
19:54:46 <Murch> I think that most users would probably be aware w
19:54:54 <Murch> of the type of outputs they use for receivling only
19:55:13 <S3RK_> this is already good enough
19:55:27 <Murch> S3RK_: "Special case" as in not matching the recipient with the change just for legacy, or "special case" matching recipient outputs in the first place?
19:55:43 <S3RK_> the first
19:55:55 <achow101> I think right now it's not a problem, and perhaps we could revisit this if users start complaining or we see a noticeable increase in legacy usage?
19:56:17 <achow101> I would hope this becomes moot and legacy usage dies out
19:56:18 <Murch> achow101: Alright, then let's close it for now?
19:56:37 <achow101> sure
19:56:45 <Murch> Yeah, hopefully before feerates go up again, or at least when feerates start going up again…
19:57:32 <achow101> further proposals like josie[m]'s idea of using a different address type for change when the inputs match the recipients would also mitigate this somewhat
19:58:27 <achow101> anything else to talk about in the last minute of the meeting?
19:58:31 <Murch> I like the idea (but then, it was mine :p), but I think it should only happen when we would use the given input set already for cost reasons until we have a better way to score privacy benefits in a numeric fashion
20:00:01 <achow101> #endmeeting