< bitcoin-git>
bitcoin/master 79d6332 Andrew Chow: moveonly: Fix indentation in bumpfee RPC
< bitcoin-git>
[bitcoin] meshcollider merged pull request #18654: rpc: separate bumpfee's psbt creation function into psbtbumpfee (master...psbtbumpfee) https://github.com/bitcoin/bitcoin/pull/18654
< wumpus>
fanquake: thanks
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19707: net: Remove unused conn_type default arg in OpenNetworkConnection (master...1908-netDefault) https://github.com/bitcoin/bitcoin/pull/19707
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #16548: Make the global flag *fDiscover* an instance variable of CConnman (master...bitcoin_issue_14210) https://github.com/bitcoin/bitcoin/pull/16548
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #13360: [Policy] Reject SIGHASH_SINGLE with output out of bound (master...insecure_single) https://github.com/bitcoin/bitcoin/pull/13360
< bitcoin-git>
bitcoin/master b3fbc94 John Newbery: Apply cfilters review fixups
< bitcoin-git>
bitcoin/master 132b30d Jim Posen: [net] Signal NODE_COMPACT_FILTERS if we're serving compact filters.
< bitcoin-git>
bitcoin/master f5c003d Jim Posen: [test] Add test for NODE_COMPACT_FILTER.
< bitcoin-git>
[bitcoin] laanwj merged pull request #19070: p2p: Signal support for compact block filters with NODE_COMPACT_FILTERS (master...2020-05-node-compact-filters) https://github.com/bitcoin/bitcoin/pull/19070
< bitcoin-git>
[bitcoin] hebasto opened pull request #19710: bench: Prevent thread oversubscription and decreases the variance of result values (master...200813-var) https://github.com/bitcoin/bitcoin/pull/19710
< fjahr>
re loading all GH comments: I remember having that discussion here as well back then (probably when fanquake made the comment). There is a browser extension that offers that functionality and it worked well but seemed stupid to keep it just for that and some of the other functionality is annoying: https://github.com/sindresorhus/refined-github
< michaelfolkson>
tl;dr Rust does not have a stable ABI
< michaelfolkson>
.. but we can override this
< luke-jr>
frankly it'd probably be good enough if they simply limit ABI breakage to minor (major?) version bumps
< luke-jr>
so you could dynamic link stuff so long as you kept the same compiler
< luke-jr>
michaelfolkson: having to annotate every interface in code isn't a solution
< luke-jr>
especially when it's losing features (eg, Rust's decision to sort fields a certain optimal way)
< luke-jr>
the template/generic point does seem fundamentally unsolvable, though
< luke-jr>
a real world point on the topic: it's frustrating when Chromium releases new security fixes, and I have to forego having a browser for 48 hours while the entire thing recompiles.. if it was using proper shared libraries, I could just rebuild the affected one(s) and go.
< luke-jr>
(Chromium isn't Rust, but this is a static compiling/linking issue, not a Rust issue specifically)
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Aug 13 19:00:07 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< 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
< 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
< wumpus>
let's remove it for now
< wumpus>
I see the other ones were rebased, great
< wumpus>
ok, that concludes the topic if no one else has suggestions
< wumpus>
#topic Can we recreate bitcoin-core/gui (Luke-jr)
< luke-jr>
sorry, my IRC flaked out (I saw nothing for 5 minutes)
< wumpus>
I'd personally rather not, just now that all kinds of issues and PRs were moved there
< jeremyrubin>
To be clear
< jeremyrubin>
This is for a repo
< michaelfolkson>
What was the motivation for wanting this? Sorry missing context
< jeremyrubin>
not for the actual QT impl
< 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?
< wumpus>
he wants to inherit it from bitcoin/bitcoin I think instead of having it as completely disparate repo
< luke-jr_>
Or Maybe our contacts at GitHub can just link it?
< wumpus>
I'm not convinced
< luke-jr_>
As things are, every dev needs to create a new fork to make PRs on it
< achow101>
I'm not sure that's possible by ourselves
< wumpus>
yes, that is true, though if they drift further apart in the future, that's what you'll need to do anyway
< achow101>
we may need to ask github support to do that
< luke-jr_>
achow101: we could delete, refork, and rename; but probably better if GitHub just links it
< hebasto>
hope, that gui repo would be interested for just created designers community.
< jeremyrubin>
michaelfolkson: I think it was to allow more focus on the gui work in the gui subrepo
< 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
< 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
< jeremyrubin>
oops I hit enter late XD
< jnewbery>
if we delete bitcoin-core/gui and then recreate it as a fork of bitcoin/bitcoin, you'd be able to
< 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..
< 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
< jonatack>
I agree that it's annoying friction, and things seems quiet in GUI-land.
< luke-jr_>
but I think we've been planning that for years, and progress is slow
< wumpus>
deleting the repo right now sounds like a bad plan to me, just now that eerything was set up there
< wumpus>
might as well move things back to the main repo and delete it and give up on the idea then
< 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
< jnewbery>
wumpus: I don't think so
< jnewbery>
just because things aren't perfect the first time doesn't mean you should give up on the idea
< wumpus>
luke-jr_: that seems another issue, would make it imopssible to move bitcoin itself there
< luke-jr_>
wumpus: right, that's my concern with that
< 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
< 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
< wumpus>
fanquake is not here but I think he'd agree
< jeremyrubin>
Maybe a magic friend at github can help us add a feature for this?
< aj>
yeah, isn't at least asking someone at github to tweak the database worthwhile?
< hebasto>
michaelfolkson: great!
< wumpus>
also some GUI people have created issues there, would you have them make them again?
< wumpus>
it just seems nasty to me, sorry
< jnewbery>
aj: yes. I think moneyball and fanquake are the most likely to know someone at github
< luke-jr_>
wumpus: I agree recreating is probably a bad approach, but IF we wanted to do it, better sooner than later
< 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
< luke-jr_>
probably the best approach would be to allow PRs across code "networks"
< luke-jr_>
if GitHub could add that, it'd be nice
< wumpus>
michaelfolkson: I agree
< jeremyrubin>
Let's ask github if they can tweak it first before doing anything?
< wumpus>
I don't actually want to give up the idea, just don't want to take drastic measures like that
< luke-jr>
jeremyrubin: +1
< sipa>
i certainly like it, it means i can mute the gui PR mails/notofications :)
< luke-jr>
sipa: well, procmail can do that much :P
< wumpus>
sipa: wait, you can't do that now?
< 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
< wumpus>
how does it make a difference for notifications?
< sipa>
wumpus: i can do that since the gui repo is split up
< 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.
< wumpus>
sipa: ohh right
< luke-jr>
wumpus: sipa means not giving up the idea; it's the same whether the repos are linked or not AFAIK
< sipa>
wumpus: sorry, i was talking about the split itself; not the recreatio
< wumpus>
yes, makes sense then
< michaelfolkson>
ajonas: We set up a separate Slack channel and have notifications set up from GitHub
< luke-jr>
(still here)
< jeremyrubin>
Are there other topics? moneyball's suggestion sounds like a concrete next step and revisit next week if no word back
< wumpus>
I don't think there are any other topics
< wumpus>
unless anyone has one now
< jnewbery>
I have a quick proposed topic
< jnewbery>
moving functions in net_processing into PeerLogicValidation
< jeremyrubin>
is there a relevant PR?
< wumpus>
#topic Moving functions from net_processing to PeerLogicValidation (jnewbery)
< * luke-jr>
wishes C++ allowed for private methods to not be in the header
< jnewbery>
currently, most of the logic and much of the state in net_processing is static functions and globals
< jnewbery>
on a recent PR of mine, several reviewers suggested making new state a member of PLV rather than a global
< jnewbery>
doing so would involve moving almost all of net_processing into PLV
< jnewbery>
is that a sensible thing to do, or do people have strong objections to that?
< aj>
luke-jr: you can get most of the way by moving the private members into a friend class
< jamesob>
doesn't sound worth the churn unless there are big benefits to fuzzing/testability
< wumpus>
luke-jr: the usual way to do that is a private implementation class (pimpl) pattern
< luke-jr>
yes, just feels ugly
< jnewbery>
MarcoFalke sdaftuar and theuni were the reviewers who suggested this
< wumpus>
luke-jr: maybe, but a lot of software e.g. qt does it everywhere
< luke-jr>
to be clear, I don't object to doing this, just find C++ a bit annoying in this regard
< jnewbery>
jamesob: I think removing globals and better encapsulation benefits testability, no?
< wumpus>
I'm not sure really it's worth changing around just for code organization, there's nothing wrong with functinos
< wumpus>
not everythign has to be an object or a method
< 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...
< wumpus>
agree wrt global state
< jamesob>
it'd be nice if people could motivate changes like these with large draft branches that demonstrate better testability
< 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..."
< jnewbery>
(if you're not sure it's worth it, just don't review it)
< wumpus>
okay sorry...
< jeremyrubin>
It sounds like these changes are requested by reviewers already of other work that is made better by it, right jnewbery
< wumpus>
will not give my oopinion on this again
< jamesob>
wumpus: that was probably mostly directed at me
< jnewbery>
jeremyrubin: exactly
< 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.
< cfields>
I figured it made sense logically to be there, but yeah, if it means moving everything in, that wouldn't make much sense.
< jeremyrubin>
jnewbery: do you feel it's blocking for the other work?
< aj>
"not worth it -> don't review" seems like a bad idea?
< jnewbery>
it's not blocking. I can abandon it.
< jnewbery>
It just seems like it's a better design and allows better testing
< aj>
just seems like it'd result in only interested people review, lots of shallow acks, stuff gets merged without deep review
< 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/
< luke-jr>
aj: that already happens I think
< luke-jr>
aj: though usually the result is more of "not enough reviews to get merged"
< 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)
< 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
< 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.
< jnewbery>
aj: that seems tangentially related
< jnewbery>
jonatack: 19704 is the first step
< jeremyrubin>
jnewbery: it seems this would make it easier to do that right? Less argument passing all over
< jeremyrubin>
But then those funcs would also have to methods on the class
< jeremyrubin>
So maybe might make sense to encapsulate it in a separate class?
< jnewbery>
yes, instead of every function in net_processing having 10 arguments for passing params and connman and banman and ....., they're methods
< 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.
< jnewbery>
cfields: that's how I feel too, which is why I went ahead after you, Marco and Suhas suggested it
< cfields>
but absent that greater effort, it doesn't do much good.
< jnewbery>
The reason I raised it here was to poll if there was a good reason that it's the way it is now
< dongcarl>
cfields: is there a particular test you have in mind that this would enable?
< 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"
< jeremyrubin>
But at some point we need to make a step
< jeremyrubin>
dongcarl: imagine writing individual unit tests for message handlers
< jnewbery>
jeremyrubin: +1. We've got to do something eventually instead of always saying that things don't seem worth it
< cfields>
dongcarl: I've always, for ex, wanted to be able to run instances of our net/net_processing against themselves.
< 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.
< cfields>
(that was the primary goal with CConnman, which never fully worked out :( )
< 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
< jnewbery>
jamesob: I'm not looking for motivation. I'm looking for anti-motivation, ie is there a strong reason not to do it
< jamesob>
the strong motivation not to do it would be a continuation of death-by-a-thousand-small-refactorings going on in the repo
< aj>
jnewbery: "we've got to do something" -- work on the things that are clearly worth it?
< jeremyrubin>
jnewbery: I think you're inadvdertently asking people what color the shed should be :P
< jnewbery>
aj: I think better encapsulated code is worth it
< cfields>
jnewbery: I think this was actually the intention for PLV.
< jnewbery>
cfields: ah. That makes sense. I haven't looked at the history.
< sipa>
is the question if those fields should move into PLV at all, or if they that should happen now?
< 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.
< sipa>
i think my answers are (1) yes (2) don't care
< jnewbery>
The question is "Is there a good reason _not_ to move these functions and data into PLV?"
< jnewbery>
If the answer is 'no', then I'll proceed. If the answer is 'yes', then I'll stop.
< 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.
< jeremyrubin>
I don't have any.
< jeremyrubin>
(objections or reasons not to)
< jeremyrubin>
I think jnewbery is only interested in technical objections.
< jeremyrubin>
He has 4 strong reviewers who will spend time on it.
< jnewbery>
jeremyrubin: I also like individual function handlers. Smaller chunks of code are easier to review/understand and hide less bugs.
< jnewbery>
but that wasn't really my main motivation here
< jnewbery>
(also easier to test)
< jeremyrubin>
But it's a question of if anyone has any technical problems with this move, e.g., conflicting work
< 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.
< jamesob>
moving big swathes of code creates rebase burden for everyone
< jnewbery>
cfields: working through a tangled mess is good work. I've done it before :)
< luke-jr>
rebase burden will happen no matter what PRs do it
< luke-jr>
helpful refactors may reduce it in the long run though
< jnewbery>
better encapsulated code is good medicine: more rebase pain now, for much less rebase pain forever after it's done
< luke-jr>
^
< jnewbery>
I didn't expect this topic to be so heated :)
< jeremyrubin>
jnewbery: new around here are ya?
< jnewbery>
It sounds like there aren't any fundamental objections
< sipa>
i think it only looks that way because the people here aren't familiar with the concrete implications
< sipa>
so we have to talk in generalities
< sipa>
it's probably best left to discussion on the PR?
< jnewbery>
sipa: yes. For specifics we can use the PR.
< jnewbery>
just wanted to make sure there were no high-level reasons why we shouldn't even consider it
< jeremyrubin>
Any other 'quick' topics :)
< jamesob>
haha
< luke-jr>
lol
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Aug 13 19:53:03 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)