contrib: add vasild to trusted keys #25871
Conversation
There was a problem hiding this comment.
ACK a16317e
I have verified the fingerprint from https://people.freebsd.org/~vd/vdcv/vdcv.html and consider your experience, pull requests, interactions with other contributors etc. good enough to be a maintainer for p2p/networking module.
It would have been better if the scope was wider and included privacy, however I respect your scoping about this role and confident you would anyways try to improve privacy when required.
|
About "privacy" - I agree with @achow101 that "privacy" is not a submodule, ie we don't have |
There was a problem hiding this comment.
Per my comment at the last weekly IRC meeting https://www.erisian.com.au/bitcoin-core-dev/log-2022-08-18.html#l-331, leaving to others to verify the key.
ACK a16317e
|
To help this along, in last week's IRC Core dev meeting this received effective Concept ACKs from laanwj, jarolrod, michaelfolkson, hebasto, achow101, lightlike, jonatack. Like @1440000bytes I have cross checked the PGP fingerprint on https://people.freebsd.org/~vd/vdcv/vdcv.html and uploaded it from keys.openpgp.org. However, I am perfectly happy with the scope (as others have said, things like privacy and security impact the roles of all maintainers not just a subset) and I also think @vasild is a great match for this scope. ACK a16317e |
|
I would like to see your response to my question from #25870 (comment) before this is merged, regardless of if you wish to engage with the other aspects of process within that issue. |
It is robust. The code would benefit from some improvements internally, but it is of relatively high-quality.
Not leave PRs that are ready for merge unmerged. I am also, always, careful to not break it. As with other modules in this project, the stake is very high for not breaking existent code or functionality. However, that is more for reviewers to achieve, rather than maintainers.
Remain decentralized (increasing the traffic requirements becomes a centralization factor at some point), accessible (one can run full node on a phone nowadays) and cooperative with other implementations. Encrypting P2P traffic over clearnet seems to be important. |
|
Concept ACK - I didn't verify the key though. |
|
Concept ACK. |
|
Concept ACK |
~0. If this was just over the
it doesn't look like you've been majorly involved in the discussion / reviewed any of them. This includes the just-merged #25717, which is probably one of the most involved changes to our p2p logic in some time (I see you left a comment with a coverage report post-merge). It seems a little odd to me to make someone a maintainer over an area of the codebase when they don't have a history of actively writing/reviewing the code or contributing to the higher level discussion. The complexity / nuance (and the stakes) for the net processing code are high enough that I think that having separate maintainers for that the |
Partially agree with the opinion although would be interested to see names for the contributors who would like to be maintainers for p2p as I couldn't find anyone in IRC or #25870 |
|
Oftentimes I would skip reviewing a PR if there is already a crowd of reviewers on it. Why? On one hand, one more pair of eyes cannot hurt, but on the other hand I have to spend my scarce resources wisely. I know the fellows will bring the crowded PR into shape and thus it is better for me to review another one that is under-reviewed. I do not feel like every PR out there needs my fingerprints. Also, reviewers, in addition to finding strict bugs, post suggestions on how to improve the code. This is sometimes subjective. Too many reviewers and inevitably there will be contradicting suggestions which could be counter productive. The BIP155/addrv2, Torv3, I2P and CJDNS PRs are missing from the list above. Sometimes modules are overlapping and boundaries unclear, but those should classify as high level peer interaction? Some quick stats for fun: in the last 2 years 389 P2P PRs/issues were opened. For any given contributor it is true that there are more than 100 PRs/issues in which the contributor neither commented nor authored them (was not involved). For example I only authored 56 and commented on 150, leaving 183 (389-56-150) ones in which I was not involved at all. That is, in addition to the 18 PRs from your list, there are 165 others which I did not touch. These numbers do not take into account the actual value brought into the project - maybe I only posted style nits suggestions? IMO they are mostly for fun and not for drawing conclusions. Thanks! |
There was a problem hiding this comment.
ACK a16317e
Verified key, fingerprint matches both the one I have locally and the one posted up at https://people.freebsd.org/~vd/vdcv/vdcv.html.
|
Concept ACK vasild for Networking scope maintenance I think the low-level networking is a signficant scope in itself, I would say there are many things belonging there: CJDNS, I2P, Tor, the peers monitoring tooling, the resource consumption control settings, the sockets handling, BIP324 support, likely refactoring like #25572. Beyond, it could be fruitful to monitor more actively all the ongoing development on the Tor mailing lists, and bring on the surface if there any interesting issues affecting us. Tor is permanently attacked, and Bitcoin transaction-relay target could be a target so juicy that it would change the incentives to run Tor exit nodes. It could be interesting also to explore the Tor's pluggable transports (though here iirc the dissemination strategy of the entry points really differs from our assumptions underlying the p2p network and the poisson distribution of block issuance is a leak itself). The anonymity network space is like 40 years old and there are many papers to check if we could reuse insights. The P2P stack (block-relay, addr-relay, transaction-relay, peers connection strategy) sounds to me voluminous itself, and I think it would deserve its own maintainer. The discussions there are likely to become more and more abstract and quantitative-based. Interactions with higher layers are likely to become more tight, especially as we weight things like p2p packages or transaction-relay spam mitigations. Of course, it sounds any maintenance could become more general in time, though IMHO at the really time with the complexity of the codebase inceasing, subsystems and their maintenance are more likely to become specialized. I don't know who is handling the labeling today, though it could be constructive to introduce a new "Net" label to dissociate from "P2P" and thus offer better visibility about low-level networking PRs towards reviewers. Thanks to Vasil for offering the maintenance commitment. |
I'd support a maintainer with a primary focus on net_processing, but I don't think rejecting (or accepting) vasild as maintainer is going to help or hinder that in any way. |
|
Concept ACK @vasild. Will verify the keys at some point. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
-1 on maintaining Mostly, I disagree with your replies in this thread. From #25871 (comment):
As pointed out by fanquake, most of the impactful From #25871 (comment):
I disagree, the boundaries are pretty clear here. Enabling support for Torv3, I2P and CJDNS is mostly a IMO, we have achieved a decent module boundary between From #25871 (comment):
By "robust" you mean "it has been working for 10+ years and only with focused review attention are we able to maintain this status"? You also make no distinction here between |
I agree with @dergoegge about the response. It makes me wonder if the nominee has the time to be a maintainer if resources are scarce.
Maybe but maybe not. A maintainer in name who is not involved enough could cause more problems than not. Not to mention having additional access which is a security consideration as well. |
|
Scoping wasn't considered important in an IRC meeting:
As MarcoFalke mentioned in this comment scope of a maintainer may change with time and almost everyone merges PRs from different modules/areas. Everyone currently is a general maintainer including the last 2 maintainers recently added. #25560 is closed but #25839 is still open in which scope of maintainers could be discussed in detail. Call for Maintainer: P2P & Networking #25870 is still open in which Jeremy nominated jonatack and there could be more.
Almost every maintainer is involved in more than just bitcoin core, have a life and there is nothing wrong with time management.
Disagree with this and looking at the pull requests authored or reviewed by vasild, I would consider him a good maintainer for P2P and privacy related pull requests. Scoping is useless when every maintainer eventually will be a general maintainer and pull requests are merged after review or its always possible to comment later if you see anything wrong. |
|
I'm not exactly sure where there is going. Challenge is fine but it becomes a bit of a joke when someone like @vasild who has been very active in this repo for a sustained period, clearly has expertise in the scope described and brings a vast amount of experience as a software engineer prior to this project is asked to jump through these ill defined hoops. It has been brought up previously that new maintainers on this project are increasingly young, inexperienced and arguably lacking the wisdom and versatility that comes through working in industry or external projects for many years prior.
This is the kind of thing that I would like to see from an experienced, versatile reviewer. Rather than getting their name on a PR that is already under review by a crowd of reviewers, seeking out the PRs that aren't getting attention is a positive attribute. There is room for different approaches (ultra focused on a single scope versus moving across different scopes) but highlighting the ability to add value outside of their scope as a negative seems absurd to me. It is a recipe for PRs falling between cracks because maintainers are unwilling or unable to add value outside of what they consider their scope.
A nonsense comment. Resources are scarce for every single maintainer, reviewer and indeed on this project in its entirety. If resources weren't scarce we would have open PRs in single digits. PR review is and always will be a bottleneck on this project.
This is uninformed and frankly insulting to all the work @vasild has been doing. If you have anything to provide as evidence that suggests @vasild might pose a security risk to this project then please provide it. Otherwise he falls into the same bracket as all other current maintainers in that there was no evidence prior to them being added as a maintainer that they posed a security risk. Discussing interlocking scopes and where the gaps are in terms of current maintainers' skillsets is fine. But can we refrain from uninformed criticism of individuals' work and abilities and certainly throwing around "security risk" accusations with zero evidence to back it up. It certainly isn't a recipe for attracting experienced developers to this project with expertise in areas we may be lacking. |
|
@michaelfolkson thanks for your feedback. While my comments where not mean to disparage the contributions of @vasild, it appears that they are interpreted as such. I have met @vasild personally and I didn't mean to offend him nor the contributions he has made. I had meant to voice concern that it seems a maintainer position is a large commitment and could be bad for the project and the individual if they are over stretched or not prepared. That's only my opinion, nothing more.
Again, my reaction to this comment was not to say talk badly about @vasild. I just wanted to point out that choosing any maintainer is better than no maintainer is not necessarily helpful. I disagree with how you interpret my comments, however thank you for challenging my post in case others interrupt it the same way. |
|
Since all concerns were about that, one obvious solution here would be to remove |
|
i think the difficulty here is that without having some sort of process (which I attempted a bit of, but perhaps not with the right approach), there's just not much consensus on the "who/what/where/when/why" of adding a contrib for a scoped purpose, so this seems to be languishing a bit for unstatable reasons. Given that the current process is -- albeit undocumentedly -- that current maintainers just "decide" who also to be a maintainer, I'm curious if @fanquake's ambivalelnce (under a "fuck yes" or "no" consent rule) means that this is dead-in-the-water? Are there particular things that @vasild should be doing or saying to try to move things forward (where forward could be merge, or close pr without merge)? Does he need to go in-person to a CoreDev.tech-type event to make the case (although the stated goals of that initiative say no politicking welcome). Would be great to get this out of limbo. |
Right, the scope could be edited if that is what is currently blocking this. @fanquake suggested "lower level networking, sockets". I do think though that @vasild has demonstrated he can add value outside whatever his eventual scope is.
Without laanwj, sipa as maintainers we don't have too many options with deep experience of this part of the codebase so I hope this isn't stuck for a long period of time. |
For Networking scope maintenance.
|
Renewing my Concept ACK vasild for Networking scope maintenance. While I'm listening to other contributors concerns that maintainers should express some leadership and vision in the handling of their module(s) maintenance, from my opinion this will be a notifiable change in the project culture. As of today, I think the majority of the contributors opinions is to understand maintenance as a janitorial role rather than as a "project manager" one. That the project could benefit from more priorities communication and active coordination by and from the contributors is something for sure we can discuss, however I believe it's beyond the scope of this specific appointment proposal to vasild as a maintainer. Of course, we could still devise for an unbounded period of time what should be the current role description and all the tasks that a maintainer candidate should be evaluated to be competent to perform. However, I think this is coming in a relative blindness of the fast-evolving and highly-dynamic environment that Bitcoin open-source protocol development constitutes. I would say our design, development and release process are continuously changing and adapting to the reality of the ecosystem, e.g as the recent discussions about mempool policy changes shown up, and as such maintenance is hard to be understood as a strictly fixed set of tasks, at least from my perspective. Daily practice and initiative mindset trump over a pinned job description in FOSS. All that in consideration, I don't think we should save us from better defining the scope between "Networking" and "P2P" functional modules and logical boundaries, even if in the light of today's state of the codebase there is a lot of intertwined code and functions. Again, I would like to underscore a new label "net" would be a productivity gain by ensuring better review scope. Lastly, I think we should be fairly liberal in our nomination of maintainers when the volunteering is coming from an experienced and established contributor. Technical knowledge gaps can be improved with dedicated time, and I hold the belief we can be clearly open-minded on someone learning the job on the trade. Somehow, we're all responsible to provide feedback towards maintainers in the performance of their tasks and to improve our process in a collaborative and constructive way. Personally, I'm grateful we still have contributors step in to volunteer for project maintenance. As we all know, a janitorial role which is both a curse and a blessing. |
|
This was discussed on the yesterday's IRC meeting. @dergoegge, @fanquake, has any of the discussion changed your opinion? @dergoegge, you posted "-1 on maintaining @JeremyRubin posted this. |
|
FWIW in very broad terms I am also Concept ACK @vasild. I think he has good experience and temperament, and has shown himself to be a capable reviewer and contributor. |
|
For some reason I feel pressured to comment, even though I would prefer to just ignore. I don't see why this should be blocked on me, if the maintainers want to merge this they can, I won't be mad if they do. It does seem a little like we would mostly be adding a maintainer just for the sake of adding a maintainer, instead of there actually being a need for it. Are there any net PRs that need merging right now? Has merging been the bottle neck for the last couple months? This question also came up during the IRC meeting, without response it seems (irc log). If the current maintainers feel like they can't keep up with merging (or evaluating what needs merging) then it would seem reasonable to add a new maintainer. Mostly up to them in my opinion. |
|
I very much agree with @dergoegge here. As far as I'm concerned, the question about whether a maintainer is needed is one that should be answered by the current maintainers - they're the ones best placed to see whether they need help, and if so, for what focus area(s). I have no problem personally with @vasild being added, but it should be driven by need. |
Who is expected to merge net PRs from this list?
I don't think it was a bottle neck for mempool PRs as well. Maintainers merge pull requests only after being reviewed by enough contributors who are familiar with that part of codebase.
If maintainers have to decide everything what is even the point of this pull request. Maintainers could select new maintainers in some private chatroom. Also these comments from current maintainers: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-06-30#827832 |
ghost
mentioned this pull request
|
ℹ️ 140 days since this pull request was opened 16 ACKs (Including 3 present maintainers) @JeremyRubin has no opinion anymore Why is this pull request not being merged by maintainers? |
|
I was challenged on the mailing list to be more transparent with communications on this and I think it is a fair challenge. As I've already said in previous comments I don't know why this hasn't been merged. I feel like Gloria's trusted keys merge was rushed through and I can only speculate on the reasons why. I feel like this merge has been deliberately stalled for months and I can only speculate on the reasons why. I'm still waiting on @fanquake and/or @glozow to comment despite multiple requests for them to do so. I don't think we want to open the floodgates to everyone being a maintainer and the bar for adding a new maintainer should be very high. If I'm honest I would have liked the bar to have been higher for Gloria before she was made maintainer but I can understand why many people were enthusiastic for her to be made maintainer as soon as possible. (I ACKed her PR also). In my opinion as the remaining maintainers @fanquake and @glozow should comment on this PR. Individually they should ACK this PR or they should clearly state what they expect from @vasild before they feel comfortable ACKing this PR or they should clearly state why they don't think @vasild should be made a maintainer now or potentially ever. Until they do that they are wasting my time and everybody else's who receives notifications from this repository or who follows the mailing list. I don't want to spend 2023 regularly checking back on this PR wondering what is happening. edit: If either @glozow or @fanquake want to chat privately I'm happy to via DM on IRC or via phone/video call. The offer is there if either of them want it. |
unsubscribe button is available (I use it liberally) This PR being open 5 months isn't a reflection on @vasild and should not effect his current work in any way. Making this extremely personal on his behalf is presumptive and unproductive. Move on. |
|
I'd recommend closing this @vasild. I apologise on behalf of the project. I also apologise to all the reviewers who spent time reviewing and commenting on this PR. Your time was not spent wisely. Barring some dark secret about @vasild that I'm not aware of there are two maintainers going forward who can block the merging of whatever they like without giving a rationale. To me that is deeply concerning. For posterity: https://gnusha.org/bitcoin-core-dev/2023-01-12.log |
|
I remember the uproar when @luke-jr didn't merge a PR adding a new BIP maintainer within a month of it being opened. He was hounded on IRC by certain individuals. Here it is fine to have a Bitcoin Core maintainer PR dangling for 5 months and I'm accused of being "aggressive" for having the cheek to ask why. Certain people should be ashamed of themselves. edit: "Rude" and "aggressive". A double whammy, appreciate it. |
No, you are not. My comment about "aggressive" is an observation I have made about your general behavior over the past several years. It is not just about this PR. |
|
Some comments from the IRC meeting that reflects how process of adding maintainers in this project has FAILED: 19:12 Previous maintainers were also all selected because existing maintainers saw a need, and nominated somewhere. It's unclear to me where this is coming from here.
19:13 from what I can tell, I believe the concern is the lack of addressing the specific concerns brought up in #25871 (comment)
19:10 however, this is basically new territory for us, as far as I can remember, all previous maintainers were added without any disagreement at all Previous maintainer was added with some disagreements. Even if this is a new territory it needs to be resolved instead of ignoring and moving on. 19:16 <aj> next steps? stop trying to make vasild a maintainer when there's no particular need, work on making good PRs and reviewing them 19:20 <aj> i've only found it hard to get review of changes in net; not merges once review's passed. 19:20 <aj> this entire topic seems much more like it's following https://www.openculture.com/2022/01/read-the-cias-simple-sabotage-field-manual.html rather than trying to solve actual problems to me
19:23 I could just nack and close the pr if it makes you feel better ... Did not expect this comment from @achow101 19:25 frankly, I think opinions aren't being shared because of potential backlash from aggressive users such as yourself and bytes1440000 This is an excuse. If someone wants to be transparent and share their opinions in public, there is nothing stopping them. If you can share disagreement in a pull request or other pull requests there is nothing special in this case. Not sharing opinion has stalled this PR and affected the process. Not signs of responsible maintainers. If interested to help in improving the process and start with some documentation, please review #26868 |
|
I no longer Concept ACK this. Working on a much longer response. |
|
I want to withdraw my ACK, because now I don't consider my initial motivations valid:
For (1), I'm just not sure it would help. I think there has to be some evidence — maintainers or contributors calling for the lack of maintenance (e.g., a lag in merges). I'm not convinced this is the case. If I see this is the case, I'd probably ACK again. (2) was just a wrong attitude, I think. A maintainer is not an honorable role, it's a function. |
|
I also want to withdraw my ACK..... because why not? This PR is dead anyway. |
|
With five reasonably active maintainers right now, it is pretty clear that there is no urgent rush to assign a new maintainer, especially if there seems to be mild disagreement? Overall I think that there are scopes and room for more maintainers in this project. However, trying to force urgency where there is none, by bringing up speculation, accusations and pressure doesn't seem productive and out of scope for an Open Source software project. |
|
@MarcoFalke: Why can't the two maintainers who appeared to be in "mild disagreement" prior to yesterday's IRC meeting explain what their "mild disagreement" is? I appreciate you trying to cover for them but can they not speak/type? |
|
In case it is unclear, I am not and wasn't speaking on behalf of anyone. I was referring to the previous comments in this thread. |
|
@MarcoFalke: Assuming the two maintainers stay mute what are the next steps in your view? Is there a route to @vasild becoming maintainer? What would you expect him to do over what sort of timeframe? Is there someone else who the two mute maintainers think should be a P2P maintainer as opposed to @vasild? Or do you think there shouldn't be a P2P maintainer added in the next x months or y years? To be respectful to @vasild there has to be some guidance. Currently he is sitting there with no clue what is going on (presumably, I haven't spoken to him). It has been brought up that without sipa, wumpus as maintainers (they leave a massive hole honestly) we are severely lacking in the part of the codebase Vasil has deep experience of. So what's the solution (in your view)? |
No, I literally said: "Overall I think that there are scopes and room for more maintainers in this project." Also, I am pretty sure, re-reading the previous comments that vasild wouldn't be a P2P/net_processing maintainer anyway. The scope was limited to low level net? So regardless of whether this pull is merged or not, I think, that there will be scopes and room for more maintainers. Though, with people retracting their ACK here, it seems questionable that this will be merged? |
|
Here are my thoughts on the purpose and benefits of a maintainer scope, my personal framework for deciding whether trusted-key is appropriate, and why I have neither ACKed nor NACKed this. A key to merge things is a tool, not a badge indicating someone is special. Software maintenance includes much more than merging PRs - making sure security issues are addressed, bugs are fixed, the repo is a productive place for development, etc. - and Bitcoin Core is too broad in scope for one person to do all of it. However, having lots of "general maintainers" can be like having lots of cooks in a kitchen without deciding who does what. Adding more cooks does not help as much as dividing up tasks does. Splitting based on areas of code is a sensible method, hence the idea of a maintainer scope. It means that, apart from helping generally with maintenance, any PR, issue, or security question that falls under a given area is automatically delegated to this maintainer to triage, review, and merge. This makes individuals' roles more manageable, while fewer things fall through the cracks and merge decisions are of higher quality. Bitcoin Core has many stuck PRs due to lack of thorough review, not a lack of ACKs or lack of people to push the merge button. In fact, I would say a more important role of a maintainer is to understand when to not merge a PR that has any number of ACKs. If this is surprising to you because you thought it was 1 github account 1 vote, you are setting the bar for Bitcoin Core too low. Merging based solely on votes is not "decentralized," it's just irresponsible. I think a p2p maintainer would be extremely valuable, as in, one or more persons X such that:
In that case, I think (just me - I am not recommending this as a formal framework) X contributes to the maintenance Bitcoin Core and a key to merge PRs is an appropriate tool to continue doing so, assuming they accept that responsibility. For me, @vasild isn't this person (maybe he is for somebody else, or will be in the future), which is why I have not ACKed this. This has nothing to do with how skilled or deserving I think Vasil is. Thank you very much for your contributions. I had not NACKed this either because my opinion could change over time, NACKs are sometimes needlessly interpreted as personal attacks, and Brink has been antagonized on Twitter each time multiple grantees have similar opinions about this. So I'll add that if you wish to have more decentralization in Bitcoin Core funding, you can start by creating a nonprofit, gathering donations, and funding somebody who works on Bitcoin Core. |
I still think vasild would be the most suitable maintainer choice for net code, and if there were to be a maintainer added he'd be the best choice. That said, personally it doesn't seem to me that maintainers are a bottleneck for the project right now, but reviewers and other people that get directly involved with those PRs. If adding a maintainer is so controversial among contributors it might be better to close this, and revisit it at some later time. |
|
@glozow: I appreciate you finally (after 5 months and many requests) commenting on this. Let me digest what you said and get back to you.
This isn't a funding issue. This is an issue where you and @fanquake have blocked this PR from being merged for 5 months without explaining why. I could set up Brink 2 with $1 billion of donations but if you and @fanquake block anyone from Brink 2 becoming a maintainer it has no impact on what we are discussing here. |
There was a problem hiding this comment.
NACK to making vasild maintainer at this point in time.
I appreciate vasild and all of his efforts, and think he is doing very useful work which I hope he will continue doing.
As has been said by many others, I simply don't get the impression that we have a maintainership bottleneck, especially if we let them do their work instead of forcing them to get involved in needless drama. Let's focus our scarce time on coding and reviewing - it's been clear for a long time that we don't have consensus here, we don't need every single person or maintainer to comment to see that.
Again, thank you for your work vasild, this is not a vote against you.
|
NACK In addition to my own analysis, I've also reached out to several contributors privately to form this opinion. While vasild has done great work and review in networking parts of the codebase, I don't think he is currently a good fit to be a maintainer. I also do not think a maintainer looking in the network or P2P area is needed at this time. My primary concern is on some of his statements regarding review of PRs. While maintainership is largely a janitorial role, that does not mean it is automatable or follows a set of specific rules. There are judgement calls that need to be made, and these involve looking at and scrutinizing a PR that may be ready to be merged. This means potentially providing a detailed review of it that may prevent it from being merged even if there are multiple ACKs. Personally, I try to understand the code and ACK it before I merge it, and I hope that other maintainers do the same. What concerns me are some of the statements that vasild has made in this thread regarding review, for example:
These statements, coupled with
suggest to me that vasild may expect to just look at the ACKers and ACK counts of a PR and then merge it if it meets a particular threshold. If that is what we wanted, we would just use a bot to do that rather than having people who have expertise in an area become a maintainer. One of the jobs a maintainer should be doing is actual review of the PRs to be merged, and based on these statements, I'm not confident that vasild would be doing that. This is not to disparage vasild's work or his reviews. His work and reviews are useful and valuable. But the attitudes towards review need to be different between maintainers and reviewers. Maintainers inherently need to look at the things that everyone else has already looked at, if only to give it a final once over before merging (but hopefully, an actual review, not just looking it over). So an attitude of "it already has lots of eyes so I don't need to" does not sit well with me. I also want to mention that the people who have become maintainers in the past have had this kind of maintainer attitude towards review prior to becoming a maintainer. The maintainers would often look for reviews from these people to determine whether something was ready to be merged. These people would often call out which PRs are ready to be merged so that the maintainers could do that. They were reviewing a wide assortment of PRs. They were, in effect, doing maintainer duties without having the actual permissions to do the merges themselves. A few people may think that these "requirements" are new, but I wouldn't say that they are. Previous maintainer additions were almost always foregone conclusions. They largely were the obvious choise to be a maintainer with a focus in some area because of the way that they were reviewing and calling for things to be merged. I think this is the first time that we have had a maintainer nominee who I did not think was an obvious choice for the position. Although I initially Concept ACK'd the proposal, that was more of a "I guess so" rather than a "duh, that's the obvious choice". Since this is the first time for me that I did not consider a nominee to be the obvious candidate for maintainership, it has taken a while for me to articulate why I feel this way. Lastly, I don't think we need another maintainer, regardless of scope. Having looked (briefly) at every single PR in this repo, there aren't really any PRs that look like they are ready to be merged that haven't been merged. As always, the bottleneck is really in doing review. Adding a maintainer doesn't change that. In terms of the proposed scope of networking, it doesn't seem like there has been any net related PRs that were ready and not merged for a while. Personally I've been branching out and looking at PRs outside of my nominal scope, and I thnk the other maintainers have been doing this as well. It does not feel like there is a lack of merging of PRs in that scope, but I don't work there actively so please correct me if that's wrong. |
|
Closing this as it is not going to be merged at the moment. Doing so feels like failing all the people who ACKed it, sorry. I may reopen it in the future, if dynamics change. @achow101, merging PRs based on ACK count is pretty dump, I think this is so obviously wrong that it has not even been discussed. At the same time reviewing every PR that a maintainer merges is unrealistic. To use the numbers from my comment above:
I work on this full time and have been one of the most active contributors. Even with that, I only touched about half of the net/p2p PRs (~200 from ~400). Most other contributors touched less than 100 PRs (from ~400). I didn't keep the exact stats, but they can be retrieved from github to confirm. So, the point is to be able to judge by just briefly looking. I hope existent maintainers can do that with net/p2p PRs, given that they have not been very active in that area. Unmerged PRs is a sign that a maintainer is needed, but the lack of unmerged PRs does not necessary mean that a maintainer is not needed (you know, from Lastly, I do not take any of this personally and I respect all of your opinions, even if I don't agree with some of them.
@laanwj, that means a lot, thank you! ❤️ |
23 participants
(this is a followup from yesterday's IRC discussion)
For Networking scope maintenance.
I have been signing commits with that key and it is available at
keys.openpgp.org.