19:00:07 <wumpus> #startmeeting
19:00:07 <lightningbot> Meeting started Thu Aug 13 19:00:07 2020 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:07 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:11 <jnewbery> hi
19:00:18 <jonasschnelli> Hi
19:00:19 <troygiorshev> hi
19:00:20 <hebasto> hi
19:00:36 <luke-jr> hi
19:00:42 <wumpus> #bitcoin-core-dev 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 kvaciral ariard digi_james
19:00:44 <wumpus> amiti fjahr jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2
19:00:51 <sipsorcery> hi
19:00:55 <achow101> hi
19:00:59 <jamesob> hi
19:01:06 <aj> hi
19:01:09 <fjahr> hi
19:01:11 <wumpus> one proposed topic in http://gnusha.org/bitcoin-core-dev/proposedmeetingtopics.txt: Can we recreate bitcoin-core/gui (Luke-jr)
19:01:19 <wumpus> any last minute topic proposals?
19:01:33 <jeremyrubin> hi
19:01:37 <ajonas> hi
19:01:43 <sipa> hi
19:02:15 <wumpus> #topic High priority for review
19:02:31 <wumpus> https://github.com/bitcoin/bitcoin/projects/8   10 blockers, 1 bugfix, 3 chasing conept
19:02:44 <jonatack> hi
19:02:55 <wumpus> anything to add/remove or that is probably ready for merge?
19:03:03 <luke-jr> …
19:03:27 <jonatack> i'd like to nominate #19628 for blockers
19:03:29 <gribble> https://github.com/bitcoin/bitcoin/issues/19628 | net: change CNetAddr::ip to have flexible size by vasild · Pull Request #19628 · bitcoin/bitcoin · GitHub
19:03:32 <wumpus> luke-jr: ?
19:04:36 <wumpus> jonatack: added
19:05:28 <jnewbery> #11082 is the only one that needs rebase. There was discussion last week about whether it was even needed anymore now that we have the settings file
19:05:30 <gribble> https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub
19:06:05 <wumpus> let's remove it for now
19:07:04 <wumpus> I see the other ones were rebased, great
19:08:09 <wumpus> ok, that concludes the topic if no one else has suggestions
19:08:46 <wumpus> #topic Can we recreate bitcoin-core/gui (Luke-jr)
19:09:11 <luke-jr> sorry, my IRC flaked out (I saw nothing for 5 minutes)
19:09:12 <wumpus> I'd personally rather not, just now that all kinds of issues and PRs were moved there
19:09:49 <jeremyrubin> To be clear
19:09:53 <jeremyrubin> This is for a repo
19:09:56 <michaelfolkson> What was the motivation for wanting this? Sorry missing context
19:10:02 <jeremyrubin> not for the actual QT impl
19:10:24 <luke-jr_> Can we recreate bitcoin-core/gui so GitHub will let us do PRs from the same <user>/bitcoin forks instead of making a new remote for everyone?
19:10:24 <wumpus> he wants to inherit it from bitcoin/bitcoin I think instead of having it as completely disparate repo
19:10:29 <luke-jr_> Or Maybe our contacts at GitHub can just link it?
19:10:30 <wumpus> I'm not convinced
19:10:46 <luke-jr_> As things are, every dev needs to create a new fork to make PRs on it
19:11:08 <achow101> I'm not sure that's possible by ourselves
19:11:12 <wumpus> yes, that is true, though if they drift further apart in the future, that's what you'll need to do anyway
19:11:26 <achow101> we may need to ask github support to do that
19:11:27 <luke-jr_> achow101: we could delete, refork, and rename; but probably better if GitHub just links it
19:11:30 <hebasto> hope, that gui repo would be interested for just created designers community.
19:11:39 <jeremyrubin> michaelfolkson: I think it was to allow more focus on the gui work in the gui subrepo
19:11:48 <jnewbery> wumpus: if you push a branch to <developer>/bitcoin , then you can't open a PR against bitcoin-core/gui because they don't share a common fork in gitub
19:12:01 <wumpus> right now they're basically the same, but it's not clear that will always be the case, e.g. after process separation there might be repo separation as well
19:12:02 <jeremyrubin> oops I hit enter late XD
19:12:13 <jnewbery> if we delete bitcoin-core/gui and then recreate it as a fork of bitcoin/bitcoin, you'd be able to
19:12:27 <luke-jr_> I suppose if the thought is to remove core code from /gui, and remove GUI from the core codebase, then it makes sense to leave it alone..
19:12:31 <wumpus> then again if a lot of people feel like it would be easier if it's a bitcoin/bitcoin fork we could try asking github
19:12:37 <jonatack> I agree that it's annoying friction, and things seems quiet in GUI-land.
19:13:10 <luke-jr_> but I think we've been planning that for years, and progress is slow
19:13:16 <wumpus> deleting the repo right now sounds like a bad plan to me, just now that eerything was set up there
19:13:45 <wumpus> might as well move things back to the main repo and delete it and give up on the idea then
19:13:56 <luke-jr_> also, while it used to be possible to have multiple forks under the same org (so bitcoin-core/gui and bitcoin-core/foobar), GitHub seems to have removed that possibility too
19:14:07 <jnewbery> wumpus: I don't think so
19:14:42 <jnewbery> just because things aren't perfect the first time doesn't mean you should give up on the idea
19:14:44 <wumpus> luke-jr_: that seems another issue, would make it imopssible to move bitcoin itself there
19:14:53 <luke-jr_> wumpus: right, that's my concern with that
19:15:16 <wumpus> jnewbery: it's just that it's not clear that the idea will work at all, causing the people that spent lots of time movign issues around extra work by doing it again seems terrible
19:15:34 <michaelfolkson> It seems to me that the experiment has been very short. I will try to find out if there is genuine interest in contributing, reviewing on that Bitcoin Design Slack
19:15:35 <wumpus> fanquake is not here but I think he'd agree
19:15:44 <jeremyrubin> Maybe a magic friend at github can help us add a feature for this?
19:16:09 <aj> yeah, isn't at least asking someone at github to tweak the database worthwhile?
19:16:10 <hebasto> michaelfolkson: great!
19:16:18 <wumpus> also some GUI people have created issues there, would you have them make them again?
19:16:34 <wumpus> it just seems nasty to me, sorry
19:16:40 <jnewbery> aj: yes. I think moneyball and fanquake are the most likely to know someone at github
19:16:42 <luke-jr_> wumpus: I agree recreating is probably a bad approach, but IF we wanted to do it, better sooner than later
19:17:13 <michaelfolkson> The motivations for it were attracting more contributors/reviewers and reducing noise for those not interested in GUI. I think the rationale made sense and it hasn't been given enough time at this stage
19:17:17 <luke-jr_> probably the best approach would be to allow PRs across code "networks"
19:17:29 <luke-jr_> if GitHub could add that, it'd be nice
19:17:34 <wumpus> michaelfolkson: I agree
19:18:19 <jeremyrubin> Let's ask github if they can tweak it first before doing anything?
19:18:23 <wumpus> I don't actually want to give up the idea, just don't want to take drastic measures like that
19:18:26 <luke-jr> jeremyrubin: +1
19:18:27 <sipa> i certainly like it, it means i can mute the gui PR mails/notofications :)
19:18:36 <luke-jr> sipa: well, procmail can do that much :P
19:18:49 <wumpus> sipa: wait, you can't do that now?
19:19:08 <moneyball> let's see if fanquake will follow-up with github as he's had the most recent contact with them. if not, i am happy to
19:19:13 <wumpus> how does it make a difference for notifications?
19:19:25 <sipa> wumpus: i can do that since the gui repo is split up
19:19:27 <ajonas> michaelfolkson: there are designers interested in the slack group. I put them in touch with hebasto, but they are looking for other devs to connect with.
19:19:31 <wumpus> sipa: ohh right
19:19:32 <luke-jr> wumpus: sipa means not giving up the idea; it's the same whether the repos are linked or not AFAIK
19:19:40 <sipa> wumpus: sorry, i was talking about the split itself; not the recreatio
19:19:51 <wumpus> yes, makes sense then
19:20:05 <michaelfolkson> ajonas: We set up a separate Slack channel and have notifications set up from GitHub
19:20:06 <luke-jr> (still here)
19:20:44 <jeremyrubin> Are there other topics? moneyball's suggestion sounds like a concrete next step and revisit next week if no word back
19:20:54 <wumpus> I don't think there are any other topics
19:21:34 <wumpus> unless anyone has one now
19:21:46 <jnewbery> I have a quick proposed topic
19:22:01 <jnewbery> moving functions in net_processing into PeerLogicValidation
19:22:18 <jeremyrubin> is there a relevant PR?
19:22:31 <wumpus> #topic Moving functions from net_processing to PeerLogicValidation (jnewbery)
19:22:34 * luke-jr wishes C++ allowed for private methods to not be in the header
19:22:36 <jnewbery> currently, most of the logic and much of the state in net_processing is static functions and globals
19:22:44 <jeremyrubin> #19704
19:22:45 <gribble> https://github.com/bitcoin/bitcoin/issues/19704 | Net processing: move ProcessMessage() to PeerLogicValidation by jnewbery · Pull Request #19704 · bitcoin/bitcoin · GitHub
19:23:13 <jnewbery> on a recent PR of mine, several reviewers suggested making new state a member of PLV rather than a global
19:23:28 <jnewbery> doing so would involve moving almost all of net_processing into PLV
19:23:41 <jnewbery> is that a sensible thing to do, or do people have strong objections to that?
19:23:46 <aj> luke-jr: you can get most of the way by moving the private members into a friend class
19:23:52 <jamesob> doesn't sound worth the churn unless there are big benefits to fuzzing/testability
19:24:39 <wumpus> luke-jr: the usual way to do that is a private implementation class (pimpl) pattern
19:24:58 <luke-jr> yes, just feels ugly
19:25:07 <jnewbery> MarcoFalke sdaftuar and theuni were the reviewers who suggested this
19:25:54 <wumpus> luke-jr: maybe, but a lot of software e.g. qt does it everywhere
19:25:58 <luke-jr> to be clear, I don't object to doing this, just find C++ a bit annoying in this regard
19:26:59 <jnewbery> jamesob: I think removing globals and better encapsulation benefits testability, no?
19:27:01 <wumpus> I'm not sure really it's worth changing around just for code organization, there's nothing wrong with functinos
19:27:08 <wumpus> not everythign has to be an object or a method
19:27:52 <jamesob> jnewbery: yeah I agree if what we're talking about is replacing global state with something that's more tightly scoped, but if it's mostly a matter of functions vs. methods I think the difference is pretty negligible. but haven't looked at it in a while...
19:28:08 <wumpus> agree wrt global state
19:28:24 <jamesob> it'd be nice if people could motivate changes like these with large draft branches that demonstrate better testability
19:28:30 <jnewbery> I'm not so interested in opinions like "I'm not sure if it's worth it". More looking for "this is a bad idea and we do it this way because..."
19:28:41 <jnewbery> (if you're not sure it's worth it, just don't review it)
19:29:13 <wumpus> okay sorry...
19:29:35 <jeremyrubin> It sounds like these changes are requested by reviewers already of other work that is made better by it, right jnewbery
19:29:36 <wumpus> will not give my oopinion on this again
19:29:48 <jamesob> wumpus: that was probably mostly directed at me
19:29:52 <jnewbery> jeremyrubin: exactly
19:31:14 <cfields> jnewbery: I didn't look too deeply into it when I +1'd. I certainly didn't realize it'd be a big change.
19:32:09 <cfields> I figured it made sense logically to be there, but yeah, if it means moving everything in, that wouldn't make much sense.
19:32:14 <jeremyrubin> jnewbery: do you feel it's blocking for the other work?
19:32:31 <aj> "not worth it -> don't review" seems like a bad idea?
19:32:55 <jnewbery> it's not blocking. I can abandon it.
19:33:25 <jnewbery> It just seems like it's a better design and allows better testing
19:33:31 <aj> just seems like it'd result in only interested people review, lots of shallow acks, stuff gets merged without deep review
19:33:33 <michaelfolkson> I know video calls (especially when livestreamed, transcribed) aren't for everyone. But if anyone wants to chat Signet this is scheduled for next Wednesday: https://www.meetup.com/BitDevsLDN/events/272121581/
19:33:56 <luke-jr> aj: that already happens I think
19:34:24 <luke-jr> aj: though usually the result is more of "not enough reviews to get merged"
19:34:27 <aj> jnewbery: (agree it's a better design, not sure it's worth it, think doing tx relay overhaul and separating that chunk of code into a separate file might be a start)
19:35:32 <jonatack> jnewbery: i don't know how much work it entails, but sometimes seeing the actual code helps to resolve these questions, as well as looking at if it's a priority worth attempting to do so before writing the code
19:35:32 <jeremyrubin> jnewbery: I'd like to see how it interacts with turning the individual handlers into functions as well. I could see it being good or bad for that.
19:35:48 <jnewbery> aj: that seems tangentially related
19:37:07 <jnewbery> jonatack: 19704 is the first step
19:37:15 <jeremyrubin> jnewbery: it seems this would make it easier to do that right? Less argument passing all over
19:37:27 <jeremyrubin> But then those funcs would also have to methods on the class
19:37:40 <jeremyrubin> So maybe might make sense to encapsulate it in a separate class?
19:37:56 <jnewbery> yes, instead of every function in net_processing having 10 arguments for passing params and connman and banman and ....., they're methods
19:38:00 <cfields> jnewbery: as a concrete answer: it makes sense to me as part of an effort to encapsulate and enhance what an instance of plv can do by itself, primarily for testing.
19:38:26 <jnewbery> cfields: that's how I feel too, which is why I went ahead after you, Marco and Suhas suggested it
19:38:29 <cfields> but absent that greater effort, it doesn't do much good.
19:38:45 <jnewbery> The reason I raised it here was to poll if there was a good reason that it's the way it is now
19:38:54 <dongcarl> cfields: is there a particular test you have in mind that this would enable?
19:38:59 <jeremyrubin> I think it sounds like a general "we're slow to make changes, so this won't have impact because we won't do the follow up for a while"
19:39:11 <jeremyrubin> But at some point we need to make a step
19:39:24 <jeremyrubin> dongcarl: imagine writing individual unit tests for message handlers
19:39:39 <jnewbery> jeremyrubin: +1. We've got to do something eventually instead of always saying that things don't seem worth it
19:39:40 <cfields> dongcarl: I've always, for ex, wanted to be able to run instances of our net/net_processing against themselves.
19:40:06 <jamesob> I think in general for issues like these, someone should come up with a branch that implements (i) the necessary changes and (ii) the new tests that we'd like to be able to do. seeing the tests motivates the changes.
19:40:16 <cfields> (that was the primary goal with CConnman, which never fully worked out :( )
19:40:39 <jeremyrubin> I think there's a balance to that. If the work is huge like this, it sounds like setting jnewbery up for rebase hell
19:40:46 <jnewbery> jamesob: I'm not looking for motivation. I'm looking for anti-motivation, ie is there a strong reason not to do it
19:41:10 <jamesob> the strong motivation not to do it would be a continuation of death-by-a-thousand-small-refactorings going on in the repo
19:41:16 <aj> jnewbery: "we've got to do something" -- work on the things that are clearly worth it?
19:41:19 <jeremyrubin> jnewbery: I think you're inadvdertently asking people what color the shed should be :P
19:41:44 <jnewbery> aj: I think better encapsulated code is worth it
19:41:48 <cfields> jnewbery: I think this was actually the intention for PLV.
19:42:18 <jnewbery> cfields: ah. That makes sense. I haven't looked at the history.
19:42:52 <sipa> is the question if those fields should move into PLV at all, or if they that should happen now?
19:43:07 <jeremyrubin> BTW here's a concrete motivation: Makes it easier to do individual function handlers. Function handlers can be dispatched in O(1), we currently do O(N) string matches to process a message. Concrete motivation for this step in that direction.
19:43:13 <sipa> i think my answers are (1) yes (2) don't care
19:43:35 <jnewbery> The question is "Is there a good reason _not_ to move these functions and data into PLV?"
19:44:08 <jnewbery> If the answer is 'no', then I'll proceed. If the answer is 'yes', then I'll stop.
19:44:13 <jonatack> jnewbery: it doesn't look like a slog to review. more a question of priority and opportunity cost. review pings are piling up atm.
19:44:27 <jeremyrubin> I don't have any.
19:44:36 <jeremyrubin> (objections or reasons not to)
19:44:55 <jeremyrubin> I think jnewbery is only interested in technical objections.
19:45:07 <jeremyrubin> He has 4 strong reviewers who will spend time on it.
19:45:13 <jnewbery> jeremyrubin: I also like individual function handlers. Smaller chunks of code are easier to review/understand and hide less bugs.
19:45:22 <jnewbery> but that wasn't really my main motivation here
19:45:31 <jnewbery> (also easier to test)
19:45:36 <jeremyrubin> But it's a question of if anyone has any technical problems with this move, e.g., conflicting work
19:45:49 <cfields> jnewbery: only one I can think of is: it may reveal a tangled mess that takes more effort than expected to untangle. But you kinda have to work through it to determine that.
19:46:09 <jamesob> moving big swathes of code creates rebase burden for everyone
19:46:19 <jnewbery> cfields: working through a tangled mess is good work. I've done it before :)
19:46:50 <luke-jr> rebase burden will happen no matter what PRs do it
19:47:18 <luke-jr> helpful refactors may reduce it in the long run though
19:47:26 <jnewbery> better encapsulated code is good medicine: more rebase pain now, for much less rebase pain forever after it's done
19:47:33 <luke-jr> ^
19:48:12 <jnewbery> I didn't expect this topic to be so heated :)
19:48:28 <jeremyrubin> jnewbery: new around here are ya?
19:48:29 <jnewbery> It sounds like there aren't any fundamental objections
19:48:45 <sipa> i think it only looks that way because the people here aren't familiar with the concrete implications
19:48:54 <sipa> so we have to talk in generalities
19:49:08 <sipa> it's probably best left to discussion on the PR?
19:49:28 <jnewbery> sipa: yes. For specifics we can use the PR.
19:49:50 <jnewbery> just wanted to make sure there were no high-level reasons why we shouldn't even consider it
19:50:27 <jeremyrubin> Any other 'quick' topics :)
19:51:04 <jamesob> haha
19:51:55 <luke-jr> lol
19:53:03 <wumpus> #endmeeting