19:00:21 <meshcollider> #startmeeting
19:00:21 <lightningbot> Meeting started Fri Oct 25 19:00:21 2019 UTC.  The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:21 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:23 <provoostenator> wallet meeting?
19:00:23 <jnewbery> hi
19:00:26 <provoostenator> hi
19:00:27 <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 moneyball
19:00:46 <fanquake> hi
19:00:47 <instagibbs> hi
19:00:56 <meshcollider> Topics?
19:00:58 <digi_james> hi
19:01:40 <achow101> hi
19:01:44 <provoostenator> Not really. Lots of review work out there, just do it :-)
19:01:44 <BlueMatt> <suggestor> now that bitcoin core is defaulting to segwit more, I wonder if it would make sense to change the default on spending unconfirmed change to not spend non-segwit unconfirmed change due to malleability.
19:02:12 <instagibbs> BlueMatt, hmm
19:02:40 <instagibbs> achow101, you mentioned something about wanting to work on coin selection again, if you want to mention it here? (I don't know what you were thinking)
19:02:44 <provoostenator> Coin selection already prefers confirmed coins, so that seems a bit overkill.
19:02:59 <instagibbs> true, it heavily tries deeply-then-shallow-confirmed
19:02:59 <achow101> instagibbs: yes, planning on doing some coin selection stuff and reviving SRD
19:03:23 <meshcollider> Thanks to everyone who has been reviewing #16341, it's getting very close now which is awesome :)
19:03:26 <gribble> https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
19:03:29 <provoostenator> I went down the coin selection rabbit hole yesterday. Happy to review attemps by others :-)
19:03:43 <instagibbs> Sorry I haven't had time to re-review SPKM PR :/
19:04:30 <instagibbs> Honestly knapsack has to die
19:04:32 <achow101> BlueMatt: would there even be non-segwit unconfirmed change unless the wallet is configured to addresstype=legacy?
19:05:01 <instagibbs> Previously I had tried working on the wallet again, and threw my hands up in dispair because of the knapsack loop behavior
19:05:06 <achow101> errr changetype=legacy.. I don't think we make non-segwit change anymore?
19:05:20 <BlueMatt> achow101: no, you need to avoid spending any change that came from a transaction with non-segwit inputs
19:05:41 <BlueMatt> cause that underlying tx is malleable
19:05:46 <achow101> oh right
19:06:11 <instagibbs> achow101, so personally speaking, I'd strong concept ACK killing off knapsack if the UTXO simulation story doesn't look awful
19:06:43 <instagibbs> imo it's limped along by fear of removing the current UTXO vacuum(that doesn't benefit small wallets anyways!)
19:08:27 <bitcoin-git> [13bitcoin] 15fanquake pushed 2 commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/693e40090ae7...25d7e2e78137
19:08:28 <bitcoin-git> 13bitcoin/06master 1404dbdd6 15Sjors Provoost: [net] SocketHandler: log peer id for close and disconnect
19:08:28 <bitcoin-git> 13bitcoin/06master 1425d7e2e 15fanquake: Merge #17251: net: SocketHandler logs peer id for close and disconnect
19:08:32 <provoostenator> Yeah that loop is horrible. It doesnt' even really start at the top: splitting the fee amongst outputs is done at the top, based on the last iteration.
19:08:47 <bitcoin-git> [13bitcoin] 15fanquake merged pull request #17251: net: SocketHandler logs peer id for close and disconnect (06master...062019/10/net-socket-peer) 02https://github.com/bitcoin/bitcoin/pull/17251
19:08:49 <provoostenator> ^ yolo merge, before Travis ready :-)
19:08:54 <instagibbs> It makes improvements to the wallet basically impossible :(
19:08:59 <instagibbs> some improvements*
19:09:07 <bitcoin-git> [13bitcoin] 15adamjonas opened pull request #17258: Fix issue with conflicted mempool tx in listsinceblock (06master...06listsinceblock-filter-conflicts) 02https://github.com/bitcoin/bitcoin/pull/17258
19:09:12 <instagibbs> anyways, stepping off soapbox
19:09:42 <instagibbs> f.e., having the wallet implicitly CPFP
19:10:02 <provoostenator> instagibbs: you'll like #17246
19:10:04 <gribble> https://github.com/bitcoin/bitcoin/issues/17246 | wallet: avoid knapsack when theres no change by Sjors · Pull Request #17246 · bitcoin/bitcoin · GitHub
19:10:05 <achow101> BlueMatt: I suppose that idea is reasonable. but, on the topic of unconfirmed change, how do we handle fee bumping a tx from which we've spent the unconfirmed change?
19:10:12 <provoostenator> (or some variant thereof)
19:10:17 <instagibbs> provoostenator, right, CPFP would have worked for me... only for CPFP case :)
19:10:18 <BlueMatt> you dont
19:10:21 <BlueMatt> you bump the next one up
19:10:24 <BlueMatt> cpfp :)
19:10:25 <instagibbs> but I felt that wasn't worth it
19:11:14 <jnewbery> topic request: #16341
19:11:16 <gribble> https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
19:12:06 <achow101> BlueMatt: true. I was about to suggest that maybe we shouldn't allow unconfirmed change at all
19:12:50 <achow101> And with upcoming more complex scripts, maybe we shouldn't, because we aren't necessarily the sole owner of those outputs
19:13:58 <provoostenator> Instead of spending unconfirmed change, we could RBF and add an output?
19:14:09 <provoostenator> That would be a nice fee saving UX improvment.
19:14:47 <provoostenator> (though non trivial if the pre-RBF transactions gets mined)
19:14:57 <instagibbs> that's the sticky part
19:15:25 <provoostenator> You can sign two variants and start broadcasting the fallback if needed.
19:15:38 <provoostenator> But that's less of a set-and-forget than we have now.
19:15:44 <instagibbs> our wallet is very "transactional" i nthe wrong sense currently
19:15:51 <provoostenator> set-and-quit-and-forget I mean
19:15:57 <instagibbs> transaction as in CTransaction rather than logical transaction :)
19:18:14 <achow101> next topic?
19:18:19 <meshcollider> #topic SPKM PR #16341 (jnewbery)
19:18:22 <gribble> https://github.com/bitcoin/bitcoin/issues/16341 | Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
19:18:33 <achow101> ack 'n merge pls
19:18:59 <meshcollider> Go and bug sipa first, he said he wanted to review it
19:19:00 <jnewbery> I'd like to help but there's no way I have time to review the PR in the way that it's structured
19:19:27 <provoostenator> jnewbery: did you also look at ryanofsky's variant, with the same end result?
19:19:30 <jnewbery> I'm particularly concerned about ryanofsky's comment here: https://github.com/bitcoin/bitcoin/pull/16341#issuecomment-541330425
19:19:36 <achow101> jnewbery: do you want me to use ryanofsky's structure?
19:19:43 <provoostenator> I found it useful to review achow101's version first, and then ryanofsky's version
19:19:57 <jnewbery> provoostenator achow101: I haven't looked at Russ's version yet
19:20:21 <jnewbery> it's mostly about the volume of changes
19:20:37 <provoostenator> But I wouldn't either is ideal. Russ' version does a _lot_ of stuff in big increments in the beginning. But his later commits are perhaps better.
19:20:56 <jnewbery> I know it'd take me at least a week to review so much code and satisfy myself that there aren't bugs
19:21:19 <provoostenator> It's a huge PR indeed. But it's not obvious how to refactor the giant ball of spaghetti in multiple steps
19:21:46 <ryanofsky> just catching up but my branch has two big commits at the beginning. one is 100% moveonly. one is a bunch of renames
19:21:56 <ryanofsky> every other commit is small
19:22:17 <meshcollider> Yeah this has come up in discussion a few times, noone has come up with an alternative to either this or more massive commits
19:22:38 <jnewbery> ryanofsky: would only merging those first two commits leave us in a bad state (would it be possible to slice those off in their own PR)?
19:23:00 <jnewbery> I can definitely review move-only/rename commits
19:23:06 <ryanofsky> merging those two commits would be fine, as far as i know
19:23:27 <jnewbery> achow101: what do you think about that approach?
19:24:19 <meshcollider> If we are going to split this PR up a bit, maybe we should feature-freeze the wallet until all the parts are in to avoid things getting messy half way
19:24:46 <achow101> jnewbery: I haven't looked at russ's branch in detail
19:24:51 <provoostenator> Maybe the "Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes" can split into one commit that introduces the class, and one that moves the methods over?
19:24:53 <instagibbs> that's the other thing true, you're going to force people to rebase 2+ times
19:24:59 <instagibbs> in serious ways
19:25:25 <meshcollider> Well they can just wait til it's all in ^
19:25:26 <ryanofsky> just to clarify my github comment from a few weeks ago, i'm fine if achow's pr is merged as is, i just wouldn't want my ack to be a deciding factor
19:26:12 <ryanofsky> it's too many scattered changes that i don't fully understand to be confident about all of them
19:26:35 <jnewbery> That's my main concern (separate from not being able to review it myself):I'm worried that due to the size of this PR, other people who have reviewed it haven't been able to do so in sufficient detail to avoid merging bugs
19:26:37 <achow101> ryanofsky: I think the only reason acks came in quickly after yours is because they were just waiting for you to be done asking for changes before commenting
19:26:43 <meshcollider> Yeah it is enormous, I'm feeling the same way
19:27:15 <ryanofsky> achow101, yes you're right i probably misinterpreted the timing
19:27:25 <meshcollider> That's why I want to merge it early on in the 0.20 release cycle so we have plenty of time to identify bugs before we get close to a release
19:27:53 <instagibbs> Yes, I had reviewed once, then waited for velocity to stop, then reviewed the diff between the version I'd reviewed and tip. That said, it deserves more eyes
19:28:29 <meshcollider> Smaller PRs would definitely encourage more reviews too
19:28:48 <ryanofsky> meshcollider, i don't think this has to be an all or nothing thing merged early in the release cycle. you could merge the moves early, and then let all the little behavior changes get reviewed as normal
19:28:50 <provoostenator> For release cycle it doesn't matter much if we wait another week or two for proof-of-newbery
19:29:15 <jnewbery> I'm raising it because I'm not getting the sense from any of the reviewers who have ACKed that they're super confident
19:29:40 <achow101> I can try to break it up further based on ryanofsky's branch as it seems like people are okay with some big moveonlys and renames
19:30:18 <jnewbery> achow101: thanks! I'll also try to take a look at ryanofsky's version next week
19:30:23 <achow101> if we want to do it in smaller prs, then I think we should feature freeze the wallet
19:30:34 <sipa> (speaking as someone who has not looked at the changes in detail, but plans to)  big moveonlys are still fairly mechanically verifiable, regardless of size
19:31:19 <ryanofsky> yeah i think the first big commit which is completely moveonly is basically trivial
19:31:40 <achow101> (now I just need to find ryanofsky's branch, I seem to be clikcing all the wrong "Load more comments..")
19:31:51 <ryanofsky> the second big commit which is the "rename" commit is kind of eyeglazing but still boring and reviewable i think
19:31:52 <MarcoFalke> Can this channel be opened again or at least mention in the title that log in is required?
19:31:59 <MarcoFalke> People keep running into this issue
19:32:39 <meshcollider> achow101: I'm happy to do that for a while because otherwise this is going to take forever to get in
19:33:17 <provoostenator> achow101: https://github.com/ryanofsky/bitcoin/commits/pr/keyman
19:33:46 <instagibbs> if feature freeze is the plan, I think people should be allowed a window to squeeze in near-merged features :)
19:33:49 <instagibbs> with heads up
19:34:10 <instagibbs> unless the freeze is like... a week
19:34:25 <achow101> I think some large-ish changes could also become scripted-diffs, so that will help review
19:34:36 <ryanofsky> i'm fine with a feature freeze, but not sure what a feature freeze would be doing...
19:35:16 <jnewbery> If ryanofsky's branch really is two big commits (that become the first PR) and then a bunch of small commits, I also don't understand the need for a feature freeze
19:35:28 <jnewbery> it sounds like the disruptive rebasey part would be those first two commits
19:35:33 <sipa> i think it's simpler to have reviewers lined up for a "it will be merged on day X", and then the PR author can rebase that day exactly, everyone give a final ack, and it's merged
19:35:34 <jnewbery> and once they're in they're in
19:35:43 <ryanofsky> yeah exactly all the commits are small and normal except 2
19:37:02 <instagibbs> ok provided you think it's only 1 painful break, fine
19:37:11 <ryanofsky> if the concern is difficulty rebasing the 2 big commits, i've done than several times, and it hasn't been a big deal. can continue to do it if desired
19:37:15 <instagibbs> let's just do that
19:37:21 <meshcollider> Alright achow101 happy?
19:37:33 <achow101> ok
19:38:02 <meshcollider> Sweet, any other topics?
19:38:07 <provoostenator> achow101: if you're replacing the branch, please make a new PR...
19:38:18 <achow101> yes, new prs will be opened
19:38:38 <jnewbery> thanks!
19:38:41 <provoostenator> Great, so I don't have to write a Chrome plugin to auto-click "Load More..."
19:38:54 <meshcollider> Please do that anyway :p
19:39:56 <JeremyCrookshank> Hello?
19:40:00 <instagibbs> since #16944 got un-scope-creeped, I think it's close to merge (selfish reminder)
19:40:03 <gribble> https://github.com/bitcoin/bitcoin/issues/16944 | gui: create PSBT with watch-only wallet by Sjors · Pull Request #16944 · bitcoin/bitcoin · GitHub
19:40:33 <instagibbs> JeremyCrookshank, pong
19:40:42 <JeremyCrookshank> Oh thankgod people can see my messages now
19:40:45 <JeremyCrookshank> :)
19:41:05 <meshcollider> Yep I'm going to take a look at that today instagibbs
19:41:15 <meshcollider> #endmeeting