< gmaxwell>
Anyone have AMD Ryzen yet? apparently they implement the intel SHA extensions.
< warren>
Yikes ... rescan is very slow.
< bitcoin-git>
[bitcoin] wtogami opened pull request #10207: Clarify importprivkey help text ... example of blank label without rescan (master...importprivkey) https://github.com/bitcoin/bitcoin/pull/10207
< kallewoof>
I think the PR review request part of the weekly meetings is a bad idea. It risks alienating the contributors who aren't hard core enough to participate and/or who aren't in a compatible time zone. I also don't think I would ever ask for a review for anything as I wouldn't consider my PRs important enough to take anyone's time at a specific time.
< kallewoof>
At the same time it's a bit frustrating watching PRs just gather dust for no (to oneself) apparent reason. Maybe that's just me.
< kallewoof>
Meetings start 4 in the morning where I am, FWIW.
< warren>
kallewoof: you make a good point, although I don't have a good suggestion. Asking for reviews during the meeting is not decision making. It is merely humans asking other humans to take a look at something. Perhaps folks here could volunteer to talk with you and others at other hours to better communication and coordination in other schedules.
< kallewoof>
I am a bit surprised that there's no better method to keep the ball rolling so to say. After the initial comment spree (if any) it sort of just sits there. If you're like me, who hates bugging people for no particular reason, that can be a long time. E.g. #9517 is over 3 months old and I don't even know if people want it merged.
< gribble>
https://github.com/bitcoin/bitcoin/issues/9517 | [refactor] Switched httpserver.cpp to use RAII wrapped libevents. by kallewoof · Pull Request #9517 · bitcoin/bitcoin · GitHub
< kallewoof>
And it's a very simple PR. +8 lines -16 lines.
< gmaxwell>
kallewoof: sometimes they get forgotten and you need to nag. It's suboptimal but there is often more in flight than we can really manage. squeaky wheel and all that.
< gmaxwell>
kallewoof: in any case, sorry about that I will test now.
< gmaxwell>
Recommended procedure is: (1) wait until we're not closing for a major release (e.g. now is fine), (2) nag. (3) apologize for nagging by cross responding to other people's nags: end result everyone happy.
< kallewoof>
gmaxwell: Yeah, I can sort of grasp that this is the approach, but for a newcomer I think it's very intimidating. (Not necessarily speaking about myself there, though i do hate nagging.) It'd be a pity for a bright individual to lose their steam and go somewhere else because they are too scared to bug you guys. That's more of an issue here since everyone here is a damn genius.
< kallewoof>
Friendly geniuses, but that's not necessarily obvious to someone who hasn't spent a great deal of time watching you interact.
< kallewoof>
My approach so far has been to jump to e.g. page 5-6 in the PR list and just go from bottom and upwards. But I am not exactly the most qualified to review a lot of the stuff there. :)
< gmaxwell>
kallewoof: people do go and advocate for other PRs, which does rescue this case in some cases. But it happens at a much greater rate when they've taken some personal interest in the result; and so good cleanup work like the RAII wrapper takes a backseat because as good as it may be, it isn't sexy.
< gmaxwell>
It's certantly something we need to improve-- and you can help improve it by also advocating for other people. I dunno about you but I find it less intimidating to ask for other people than for myself.
< kallewoof>
That makes a lot of sense, yeah.
< kallewoof>
TBH I think part of the problem is that you aren't sure if what you are suggesting is desired or not. Getting a concept ACK from someone goes a long way and makes the wait less of an issue, at least in my book.
< kallewoof>
Or a concept NACK for that matter.
< gmaxwell>
kallewoof: ah, well in this case you got suggestions from wumpus that weren't a "don't do this", which was not a Concept NACK.
< gmaxwell>
Part of it is familarity with the codebase-- I would expect wumpus to be the person most opinionated with this code. (I believe he wrote all of it.)
< gmaxwell>
well almost all of it.
< gmaxwell>
if you don't already know its handy to run a git blame / git log on whatever your change and get an idea of who has been working on that part most recently, since they're most likely to have the most useful review and strongest opinions.
< kallewoof>
*nods* this is true, but even a code suggestion can be less than a concept ACK. "Merge desire opinions aside, you can improve this code by doing XYZ". That's how I interpret it usually. I wouldn't be surprised to get a concept NACK later.
< kallewoof>
Oh... I never thought of that, actually.
< kallewoof>
That might make it easier to nag as it's sort of a suggestion on top of their work..
< gmaxwell>
oh, I would not expect a person to suggest changes and then concept NACK for a simple patch. For a complex patch where the implications are unclear, perhaps.
< kallewoof>
*nods*
< gmaxwell>
Generally if people mildly dislike a change you'll get a thundering silence, not exactly useful feedback to you-- but often people would prefer to bite their tongue in case someone else likes it.
< gmaxwell>
Suggestions are an investment of resources into your change, in some sense they're better than an ACK in that an ACK can be a drive by review, while if someone makes suggesitons they've basically put themselves on the hook to explain the suggestions to you and collaborate. :)
< kallewoof>
Right. I've learned a tremendous amount since becoming a contributor. I wish I'd joined an OSS project much earlier. In that sense even a NACK can be appreciated and feel rewarding.
< gmaxwell>
I think the most important thing to learn is that-- at least once you're beyond a basic level of skill-- your real contribution is the attempt, it doesn't matter if the result ends up in the finished version or stays there, but we're all pushing towards an improved piece of software, and if we keep pushing we'll be successful, even though-- in fact, especially because-- not every change makes it
< gmaxwell>
in.
< gmaxwell>
now, ... trying to test your patch and I'm bombing out with a perplexing multiple definition error that looks like you missed an include guard. But it has an include guard.
< kallewoof>
I agree that that's exactly the right approach to take, and you've given me feedback that I think will help me advocate my own stuff or stuff I feel about by others. Maybe I'll make a PR to the contribution doc summarizing what you said, cause some of it was NOT obvious, at least not to me.
< kallewoof>
Weird. Testing.
< gmaxwell>
test/test_test_bitcoin-raii_event_tests.o:/home/gmaxwell/src/bitcoin/src/./support/events.h:30: first defined here
< gmaxwell>
libbitcoin_server.a(libbitcoin_server_a-httpserver.o): In function `obtain_event(event_base*, int, short, void (*)(int, short, void*), void*)':
< gmaxwell>
the test. missed that it was the test.
< kallewoof>
Weird. I'm not getting that compile error (on lubuntu).
< gmaxwell>
kallewoof: in any case, I went and made all the function definitions in that header static inline, it's fine then. AFAICT the duplicate symbol is real.
< gmaxwell>
May just be that your linker is handling it differently.
< kallewoof>
Yeah they should be static and/or inline for sure.
< gmaxwell>
or moved out of the header.
< gmaxwell>
(I tend to have relatively few optinions about these things, because I am not much of a C++ coder; and don't really have a sense of taste for such things beyond preferring almost everything to look like C, which is usually the wrong answer. :) )
< * kallewoof>
laughs
< gmaxwell>
in any case seems to work for me, I'll ACK when you've updated to do something about the duplicate function definitions.
< kallewoof>
Thanks a lot :) I just pushed a commit that adds inline.
< cfields>
kallewoof: you on osx?
< gmaxwell>
kallewoof: obtain_event_base too.
< kallewoof>
cfields: Yep. I do have a linux box too though.
< kallewoof>
gmaxwell: Yeah, sorry. I force-pushed a squash fix of that one. Was hoping you wouldn't grab before I did.
< cfields>
kallewoof: the osx linker is very forgiving. too much so, imo.
< NicolasDorier>
kallewoof: Yeah, the solution to our time zone problem is called the ACK Begging. It works, just need to find the right balance to not get a kick in the butt while getting things committed.
< kallewoof>
cfields: I compiled fine under lubuntu too, though.
< NicolasDorier>
btcdrak taught me this technique. Now I think I surpassed the master.
< cfields>
ah
< cfields>
kallewoof: fyi, I've written a pretty full-featured c++ wrapper for libevent. Maybe we should collaborate a bit to avoid stepping on eachother.
< kallewoof>
cfields: That would be lovely. :)
< cfields>
kallewoof: i'm getting ready to libevent-ify the net code, and I'd really rather not litter it with c code. I think we're probably in agreement there :)
< kallewoof>
Isn't that what #9517 does already?
< gribble>
https://github.com/bitcoin/bitcoin/issues/9517 | [refactor] Switched httpserver.cpp to use RAII wrapped libevents. by kallewoof · Pull Request #9517 · bitcoin/bitcoin · GitHub
< kallewoof>
Oh wait, that's just the httpserver part.
< cfields>
kallewoof: yea, but it doesn't handle refcounting, std::function, etc. I'd much prefer more native looking callbacks and functors.
< kallewoof>
That sounds nice yeah. I'd love to help if there's anything I can do.
< cfields>
(it's been a while since I've worked with it, I don't recall exactly what state it's in)
< kallewoof>
Oh, wow.
< cfields>
kallewoof: the goal would be to be able to cleanly work with events like: bufferevent.read_cb([](size_t bytes){printf("read %lu bytes\n", bytes);});
< kallewoof>
Looks pretty neat. Would this be a separate library? It looks like it would be.
< cfields>
it's intended to be a standalone wrapper, but included in our repo
< * kallewoof>
nods
< cfields>
kallewoof: anyway, that's the reason I haven't chimed in on your PRs. I'd like something a little different, but I don't like standing in the way of obvious improvements either.
< kallewoof>
cfields: I initially started writing something like that (less elegant) but wumpus suggested he wanted something more light weight. I think a fully featured wrapper might be useful, but it adds some amount of maintainer load in case libevent changes. But then again, I doubt libevent will change a whole lot ever, so I don't think it would be a prob.
< cfields>
kallewoof: yea, understandable. fwiw, I don't intend to wrap everything, just the stuff we use
< kallewoof>
Yeah, uh, s/fully featured//
< cfields>
kallewoof: the reason I ultimately decided on that route is I found my own code to be unreadble without it :(
< cfields>
kallewoof: anyway, I'm open to whatever is cleanest/simplest. Maybe minimal would be better. But we should collaborate either way :)
< kallewoof>
I admittedly haven't done a lot. I just fixed the stuff that had // TODO: RAII comments on them, in my never ending quest to find something that needs doing that I am capable of. :P
< kallewoof>
cfields: I would love to collaborate! :)
< cfields>
kallewoof: great, give me a few days and I'll push up what I have. Don't let me keep you from working on what you already have though, we can converge later
< cfields>
kallewoof: one of the most important things (imo) is tracking ownership by passing objects around. For example, if an event needs a pointer to its base, it should hold a reference and keep the base alive. Otherwise, it gets very difficult to guarantee that everything will tear down safely.
< kallewoof>
cfields: I noticed that too. I tried to do something clever but it kind of backfired and I simplified it. It ended up not being intuitive at all. (I think it was something like, a wrapper holding an object lost the object after a certain operation, but I can't remember.)
< cfields>
heh
< cfields>
in the example above, the CBufferEvent requires a CEventBase in its ctor. CEventBase contains a shared_ptr<event_base>. That way, the base lives at least as long as any bufferevents using it.
< cfields>
so there's no way to create a bufferevent without a valid base, and no way for the base to be deleted out from under a bufferevent.
< kallewoof>
In some cases the underlying libevent struct takes ownership of an argument, e.g. I believe the evhttp takes ownership of passed-in evhttp_requests. That is what got me.
< kallewoof>
Even with shared pointers, the request should sometimes be deleted (if not passed into evhttp) and sometimes not (if passed in).
< cfields>
ah yea, I remember seeing some wonky stuff in evhttp.
< kallewoof>
I think part of the problem with my approach was that it wasn't especially elegant, so there was no reason for its existence other than simply wrapping libevent into C++ish fluff. Having a more solid solution like what you're proposing is probably more meaningful.
< cfields>
oh i see, it's conditional on what happens in different places
< cfields>
sorry, I really shouldn't speak on the evhttp code, I'm not familiar enough with it.
< kallewoof>
To be honest, neither am I. I just learned enough to make it work replacing the stuff that was there already.
< cfields>
kallewoof: well I agree with trying to keep it as minimal as possible. I think there's some middle ground that maintains the api while adding protection and type/memory safety.
< cfields>
kallewoof: sounds like a good approach if it gets the job done :)
< cfields>
kallewoof: I'll ping you in the next few days. keep up the good work :)
< kallewoof>
Looking forward to it! :)
< bitcoin-git>
[bitcoin] kallewoof closed pull request #9872: [qa] Multi-chain support in test framework (master...qa-multi-chain-support) https://github.com/bitcoin/bitcoin/pull/9872
<@wumpus>
meeting yesterday wasn't really conclusive on that, and the rc2 backported changes were deemed too important to leave out, also I forgot to do a translations update for rc1. Hopefully rc2 can be final...
< bitcoin-git>
[bitcoin] kallewoof opened pull request #10211: [doc] Contributor fixes & new "finding reviewers" section (master...contributor-finding-reviewers) https://github.com/bitcoin/bitcoin/pull/10211
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10212: Make sure parameter names in .cpp and .h files are in sync (master...make-doxygen-happy-by-using-consistent-parameter-names) https://github.com/bitcoin/bitcoin/pull/10212
< cfields>
gitian builders: detached sigs for 0.14.1rc2 are up