contrib: add vasild to trusted keys #25871

pull vasild wants to merge 1 commits into bitcoin:master from vasild:vasild_key changing 1 files +1 −0
  1. vasild commented at 8:02 am on August 19, 2022: contributor

    (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.

  2. DrahtBot added the label Scripts and tools on Aug 19, 2022
  3. unknown approved
  4. unknown commented at 4:05 pm on August 20, 2022: none

    ACK https://github.com/bitcoin/bitcoin/pull/25871/commits/a16317e0186ef6735ece7228d1f91615e2b19e94

    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.

  5. vasild commented at 9:08 am on August 22, 2022: contributor
    About “privacy” - I agree with @achow101 that “privacy” is not a submodule, ie we don’t have src/privacy.cpp. Same applies for “security”. Privacy and security are embedded in the fabric of this project and my impression is that people always have them as top priority in their heads. No need to mention them as something separate.
  6. jonatack approved
  7. jonatack commented at 9:27 am on August 22, 2022: contributor

    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 a16317e0186ef6735ece7228d1f91615e2b19e94

  8. michaelfolkson commented at 2:50 pm on August 22, 2022: contributor

    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 a16317e0186ef6735ece7228d1f91615e2b19e94

  9. JeremyRubin commented at 8:27 pm on August 23, 2022: contributor
    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.
  10. vasild commented at 12:49 pm on August 24, 2022: contributor

    @JeremyRubin,

    What do you see as the current state of the P2P & Networking system

    It is robust. The code would benefit from some improvements internally, but it is of relatively high-quality.

    and what goals would you be hoping to accomplish in particular in your tenure as a P2P & Networking focused maintainer?

    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.

    What should the general goals of Bitcoin Core (not Bitcoin) with respect to P2P & Networking?

    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.

  11. mzumsande commented at 3:48 pm on August 24, 2022: contributor
    Concept ACK - I didn’t verify the key though.
  12. promag commented at 10:17 pm on August 24, 2022: member
    Concept ACK
  13. naumenkogs commented at 7:38 am on September 1, 2022: member
    Concept ACK. I think vasild is in a good position for this role: he seems to understand most of the p2p-related PRs. This is indeed an improvement over the no-p2p-maintainer status quo
  14. maflcko commented at 7:43 am on September 1, 2022: member
    Concept ACK
  15. fanquake commented at 10:36 am on September 1, 2022: member

    For maintaining the … peer interaction, net processing,

    ~0. If this was just over the lower level networking, sockets, it’s pretty clear you’re one of the most active contributors there. However for net processing and peer interaction code, I’m not sure that’s the case. Looking at some of the most significant net processing changes over the past 12 - 24 months (apologies if I’ve missed some):

    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 lower level networking, sockets is warranted. When you look over that same group of net processing / peer interaction PRs, there is a cohort of regular contributors that I’d think would probably be better positioned to be a maintainer for it.

  16. ghost commented at 2:48 pm on September 1, 2022: none

    there is a cohort of regular contributors that I’d think would probably be better positioned to be a maintainer for it

    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

  17. vasild commented at 3:55 pm on September 2, 2022: contributor

    @fanquake,

    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!

  18. Zero-1729 approved
  19. Zero-1729 commented at 0:08 am on September 3, 2022: contributor

    ACK a16317e0186ef6735ece7228d1f91615e2b19e94

    Verified key, fingerprint matches both the one I have locally and the one posted up at https://people.freebsd.org/~vd/vdcv/vdcv.html.

  20. ariard commented at 8:57 pm on September 5, 2022: member

    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.

  21. maflcko commented at 9:30 am on September 12, 2022: member

    When you look over that same group of net processing / peer interaction PRs, there is a cohort of regular contributors that I’d think would probably be better positioned to be a maintainer for it.

    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.

  22. jarolrod approved
  23. jarolrod commented at 3:05 am on September 13, 2022: member

    ACK a16317e0186ef6735ece7228d1f91615e2b19e94

    ack as P2P/networking code, can’t verify keys, but vasild is more than qualified and is a key reviewer in this area of the code. Thanks for volunteering to be maintainer

  24. jamesob commented at 3:16 am on September 13, 2022: member
    Concept ACK @vasild. Will verify the keys at some point.
  25. DrahtBot commented at 9:38 am on September 23, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See 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.

    Conflicts

    No conflicts as of last run.

  26. dergoegge commented at 6:03 pm on September 24, 2022: member

    -1 on maintaining net processing

    Mostly, I disagree with your replies in this thread.

    From #25871 (comment):

    Oftentimes I would skip reviewing a PR if there is already a crowd of reviewers on it.

    As pointed out by fanquake, most of the impactful net processing PRs appear to fall in this category for you. Personally, I would expect a history of reviewing these types of PRs from a proposed maintainer. I can’t comfortably be on board with someone as maintainer who has not shown an interest in significant changes to the proposed module (by review or authoring).

    From #25871 (comment):

    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?

    I disagree, the boundaries are pretty clear here. Enabling support for Torv3, I2P and CJDNS is mostly a net (lower level networking, connection management, etc.) change that aligns well with a net processing change to enable the relay of longer address formats (i.e. BIP155/addrv2). Connecting to a i2p/Tor/cjdns address obviously works even if it is not rumored on the network, meaning the net changes can clearly be separated from the net processing ones.

    IMO, we have achieved a decent module boundary between net and net processing. Maintaining that boundary as well as strengthening it, is something I would like to at least see mentioned here. Any maintainer of net or net processing who does not understand this relationship would risk entangling these layers once again.

    From #25871 (comment):

    It is robust. The code would benefit from some improvements internally, but it is of relatively high-quality.

    By “robust” you mean “it has been working for 10+ years and only with focused review attention are we able to maintain this status”? net processing has close to no unit test coverage and afaict the fuzz coverage is mostly limited to a high level fuzzing target of ProcessMessage(s) (Aside from a couple components like TxRequestTracker or TxOrphanage which are also fuzzed. A trend that I would like to see continued!). In my view, this currently makes net processing quite brittle to subtle changes that humans likely won’t catch, especially with long time contributors leaving and new ones joining.

    You also make no distinction here between net and net processing but (imo) the state of these two modules is not quite the same.

  27. yancyribbens commented at 11:36 pm on September 28, 2022: contributor

    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 agree with @dergoegge about the response. It makes me wonder if the nominee has the time to be a maintainer if resources are scarce.

    This is indeed an improvement over the no-p2p-maintainer status quo

    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.

  28. ghost commented at 0:55 am on September 29, 2022: none

    Scoping wasn’t considered important in an IRC meeting:

    19:31 achow101: tbh maintainers have merged things outside of their explicit scopes, and I don’t think that’s necessarily a bad thing

    19:30 laanwj: we don’t have that kind of precise scoping for maintainers, i don’t think that’s necessary

    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.


    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.

    Almost every maintainer is involved in more than just bitcoin core, have a life and there is nothing wrong with time management.

    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.

    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.

  29. ghost commented at 9:50 pm on September 29, 2022: none
    Adding more links because some reviewers are confused by OP: #26065 #25355 #24991 #23542 #23077 #22834 #22112 #20685 #20234 #18722
  30. michaelfolkson commented at 11:15 am on September 30, 2022: contributor

    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.

    Oftentimes I would skip reviewing a PR if there is already a crowd of reviewers on it

    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.

    It makes me wonder if the nominee has the time to be a maintainer if resources are scarce.

    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.

    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.

    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.

  31. yancyribbens commented at 11:41 am on September 30, 2022: contributor

    @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.

    This is indeed an improvement over the no-p2p-maintainer status quo

    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.

  32. mzumsande commented at 2:59 am on October 2, 2022: contributor
    Since all concerns were about that, one obvious solution here would be to remove net_processing from the scope for now, and contribute to it as an author/reviewer for some more time before maintaining there. net, privacy networks etc. is a field large enough to warrant its own maintainer.
  33. JeremyRubin commented at 6:02 pm on October 2, 2022: contributor

    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.

  34. michaelfolkson commented at 9:55 am on October 3, 2022: contributor

    Since all concerns were about that, one obvious solution here would be to remove net_processing from the scope for now, and contribute to it as an author/reviewer for some more time before maintaining there. net, privacy networks etc. is a field large enough to warrant its own maintainer.

    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.

    a thought: the past few years vasild, laanwj, and myself have been working on or reviewing a large-ish chunk of net code that few others have been. i believe sipa knows the code well, too, along with (maybe) a couple others. laanwj has been merging these changes. so it makes sense to me that one of these would maintain (jonatack)

    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.

  35. contrib: add vasild to trusted keys
    For Networking scope maintenance.
    902d8810e5
  36. vasild force-pushed on Oct 5, 2022
  37. vasild commented at 8:00 am on October 5, 2022: contributor
    a16317e018...902d8810e5: omit net_processing and reduce scope to Networking as suggested.
  38. Zero-1729 approved
  39. Zero-1729 commented at 1:05 am on October 10, 2022: contributor

    ACK 902d8810e504b806e02ae890145a90e1a98cddef

    Happy to move this along if scope narrowing is the only issue.

  40. michaelfolkson commented at 8:20 pm on October 14, 2022: contributor

    @fanquake: You happy to ACK this now the scope has been changed to Networking? @dergoegge @yancyribbens: Same question. @JeremyRubin: You happy to ACK this now @vasild has responded to your question? Obviously you can only speak for yourself and not for others. @glozow: As a maintainer who hasn’t commented yet are you happy to ACK this?

    All maintainers have ACKed this apart from fanquake and glozow.

    This has received ACKs from laanwj, jarolrod, michaelfolkson, hebasto, achow101, mzumsande, jonatack, MarcoFalke, promag, naumenkogs, 1440000bytes, Zero-1729, ariard, jamesob

  41. yancyribbens commented at 6:08 am on October 15, 2022: contributor
    @michaelfolkson I don’t feel like I’m the person to ACKor NACK and defer to the current maintainers.
  42. ghost commented at 5:24 pm on October 15, 2022: none

    @michaelfolkson I don’t feel like I’m the person to ACKor NACK and defer to the current maintainers.

    If current maintainers would decide another maintainer there will be a bias. Bitcoin Core maintainers have more respect, privileges, responsibilities etc. even if we consider it ‘janitorial’. Sometimes even in soft forks indirectly. Its mainly because Bitcoin has only one implementation used by majority of nodes. I am not sure about other countries but in my country, some elections are open for everyone and others are decided by people who are elected and closed door meetings between them.

    I don’t think we are trying to create a similar system and most of the people are interested in coding rather than politics. However, it has become a part of Bitcoin and Bitcoin Core for a few reasons that are irrelevant to discuss here. I was even asked to not comment in a PR by @michaelfolkson however, I still agree with his comments in this PR.

    1. Scope is edited if it really matters: #25871 (comment)
    2. Please don’t insult the only 5-10 developers that focus on privacy in bitcoin as @michaelfolkson said based on contributions:

      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.

  43. ghost commented at 5:50 pm on October 15, 2022: none

    All maintainers have ACKed this apart from fanquake and glozow. @michaelfolkson Maybe you missed @hebasto

  44. yancyribbens commented at 9:00 am on October 16, 2022: contributor
    @1440000bytes I’m not dictating what others do and I think it goes without saying that people can respond as they wish. Your remark about government feels a bit off topic but I share your dislike for government corruption. I’ve traveled extensively through a lot of countries, many of which have a corrupt political system.
  45. michaelfolkson commented at 11:53 am on October 16, 2022: contributor

    @michaelfolkson Maybe you missed @hebasto

    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.

  46. aureleoules commented at 9:48 am on October 17, 2022: member

    ACK 902d8810e504b806e02ae890145a90e1a98cddef - for networking scope maintenance As it can be verified, vasild has been actively working on networking code for many years and has contributed several improvements. I believe he would be a great maintainer for this area.

    I verified that commits of vasild, including 902d8810e504b806e02ae890145a90e1a98cddef, are signed with the added key. Also checked that the key matches the one shown on vasild’s GitHub profile description.

  47. JeremyRubin commented at 10:13 pm on October 17, 2022: contributor

    @michaelfolkson You happy to ACK this now @vasild has responded to your question? Obviously you can only speak for yourself and not for others.

    personally, no I don’t think so; also I mostly agree with dergoegge.

    There are a myriad of issues in the networking stack that I think should be addressed, and so I’d like to see someone with more dissatisfaction ==> ambition to fix those issues, or at least a set of issues they think are compelling and have a plan for. To the extent we’re adding a maintainer for networking, I would expect them to express more vision and leadership around how the module should be improved and how the goals (including stated ones around encryption) will be able to be moved forward meaningfully.

    I also strongly disagree with @vasild’s statement:

    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.

    From bitcoincore.org

    Project maintainers have commit access and are responsible for merging patches from contributors. They perform a janitorial role merging patches that the team agrees should be merged. They also act as a final check to ensure that patches are safe and in line with the project goals. The maintainers’ role is by agreement of project contributors.

    If a maintainer doesn’t believe that maintainers are doing a final check of safety, then that’s misunderstanding what contributors currrently expect a maintainer should do before merging.

  48. maflcko approved
  49. maflcko commented at 8:39 am on October 19, 2022: member
    Concept ACK
  50. michaelfolkson commented at 8:55 am on November 7, 2022: contributor

    @fanquake: You happy to ACK this now the scope has been changed to Networking?

    @glozow: As a maintainer who hasn’t commented yet are you happy to ACK this?

    By all means wait until after the release has been fully completed (it has already been 24 days since I asked at this point and almost 3 months since this pull request was opened so what’s another few weeks) but I’d appreciate a response to this and I’m sure @vasild would too. This isn’t a complex PR to review, you either think @vasild has proved himself capable enough to be a maintainer over years of sustained contributions or he hasn’t. You are free to suggest a tweak to the scope if that is what is delaying your response.

    (edit: I looked up to see how long glozow’s trusted keys PR took to get merged. It was opened on July 1st and was merged on July 7th after discussion of some people’s reservations, a couple of initial NACKs and some unmet requests for phone calls etc. So 6 days from opening to merging. 3 months seems like an excessively long time for such a PR to be open to me.)

  51. ghost commented at 11:12 pm on November 8, 2022: none

    personally, no I don’t think so; also I mostly agree with dergoegge. @JeremyRubin who do you think should be maintainers of p2p/net and know tor and i2p works?

  52. JeremyRubin commented at 11:51 pm on November 8, 2022: contributor

    i think that the need for such a role should be more clearly specified, and then volunteers can step forward against that role description and their work history and views can be evaluated. but there isn’t agreement on evaluating maintainers like that, so it’s not clear it matters.

    my take on this current situation is that if vasild wants to be a maintainer, he should aim to talk with anyone who expressed ambivalence about his proposal to do the role and take notes on what they would be expecting of him, and then try again 1 release from now if he wants to meet the expectations of other contributors. from what i can tell, that would minimally involve taking a more active interest in reviewing PRs in the covered areas (maybe current maintainers can tag ones they think should be reviewed by maintainers in this scope to aid that process?) and thinking through a bit more about how the scoped parts should evolve longer term and coming up with some more detailed communications.

    but i am not a maintainer so i can’t tell if that’s what the other maintainers would expect for this to move forward, or if there are other interpersonal issues or something holding this up. your guess as good as mine.

  53. ghost commented at 0:14 am on November 9, 2022: none

    my take on this current situation is that if vasild wants to be a maintainer, he should aim to talk with anyone who expressed ambivalence about his proposal to do the role and take notes on what they would be expecting of him, and then try again 1 release from now if he wants to meet the expectations of other contributors. from what i can tell, that would minimally involve taking a more active interest in reviewing PRs in the covered areas (maybe current maintainers can tag ones they think should be reviewed by maintainers in this scope to aid that process?) and thinking through a bit more about how the scoped parts should evolve longer term and coming up with some more detailed communications.

    I cannot comment on these expectations although @vasild should

    • We all love Vasil for his contributions particularity i2p
    • Scope if it really matters has been changed
    • Marco agrees
    • I am even okay with having 2 new maintainers: @vasild and @jonatack

    However, we cannot force anyone. Why not move this PR forward and merge it. If there were will be any issues as some reviewers said, this can always be revoked.

    I respect @vasild for some of the pull requests that matter: censorship resistance in p2p and privacy

  54. michaelfolkson commented at 9:14 am on November 9, 2022: contributor

    @1440000bytes: For what seems like the thousandth time please try to stay on topic in comments on pull requests in this repo. This isn’t a PR to add multiple maintainers. You are already ACKing @vasild and suggestions for additional maintainers isn’t helping.

    As was discussed at the initial IRC meeting:

    015:29 < achow101> I do not think we should discuss any further on whether a particular person is a good fit for maintainer before they have stated that they are willing to be a maintainer
    115:29 < achow101> as otherwise we may put undue pressure (in either direction) on that decision
    215:30 < achow101> remember that maintainership is a position people volunteer for, not be appointed to
    

    Given this one has been like pulling teeth (I’m not sure why, no fault of @vasild) it doesn’t seem there will be much of an appetite to follow this up with more. But that is not a subject for the review comments of this PR.

  55. ghost commented at 9:37 am on November 9, 2022: none

    @1440000bytes: For what seems like the thousandth time please try to stay on topic in comments on pull requests in this repo. This isn’t a PR to add multiple maintainers. You are already ACKing @vasild and suggestions for additional maintainers isn’t helping.

    I don’t think this PR is also about forcing people to comment, give deadlines and mention how other maintainers were added in different pull requests.

    You seem to do these kind of moderations in this repo regularly although, not everything you write is correct. My comment is on topic and I am okay with multiple maintainers for networking module. I mentioned one name as it was suggested in an issue by Jeremy whom I was responding to.

  56. michaelfolkson commented at 9:59 am on November 9, 2022: contributor

    @1440000bytes: I won’t be responding to you again on this PR as I think this is very noisy and I don’t think you will take suggestions or follow requests from anybody least of all me.

    I don’t think this PR is also about forcing people to comment, give deadlines and mention how other maintainers were added in different pull requests.

    You have ACKed this PR and seem to want this PR to be merged. If that is indeed the case (rather than just seeking attention on pull requests and adding noise for all the contributors to this repo) I would expect you to be interested in why it hasn’t been merged after approximately 3 months and what the remaining maintainers are thinking with regards to this pull request. That is what I have enquired about. Changing the subject or increasing the scope of the pull request discussion, whether you know it or not, is harmful to your supposed aim of getting this PR merged.

    I am not forcing anyone to do anything. But by taking on the responsibility of maintainership releases are within the maintainers’ remit and I don’t think it is unreasonable to request comment on a PR for adding a new maintainer that has a large number of ACKs (including from a majority of the maintainers) and has been open for almost 3 months. Obviously the release takes priority currently but once that is completed I hope (and presumably you hope too) to hear from them.

  57. ariard commented at 1:04 am on November 10, 2022: member

    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.

  58. vasild commented at 1:37 pm on December 16, 2022: contributor
    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 net processing”, and afterwards the scope has been reduced to exclude net processing, so it is somewhat uncertain how to interpret your opinion now. @fanquake’s concerns, if I read it correctly, were similar around the scope and net processing. @JeremyRubin posted this.
  59. jamesob commented at 1:45 pm on December 16, 2022: member
    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.
  60. dergoegge commented at 4:23 pm on December 19, 2022: member

    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.

  61. sipa commented at 4:42 pm on December 19, 2022: member
    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.
  62. ghost commented at 6:56 pm on December 19, 2022: none

    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.

    Username Focus Area
    @MarcoFalke General, QA
    @fanquake General, Build
    @hebasto General, UI/UX
    @achow101 General, Wallet
    @glozow General, Mempool

    Who is expected to merge net PRs from this list?

    Has merging been the bottle neck for the last couple months?

    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.

    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).

    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:

    #25871 (comment)

    #25839 (review)

    https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-06-30#827832

  63. ghost commented at 2:36 pm on January 6, 2023: none

    ℹ️ 140 days since this pull request was opened

    16 ACKs (Including 3 present maintainers) @JeremyRubin has no opinion anymore @dergoegge wont mind if pull request is merged @fanquake did not respond after scope has been changed


    Why is this pull request not being merged by maintainers?

  64. michaelfolkson commented at 12:22 pm on January 8, 2023: contributor

    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.

  65. instagibbs commented at 7:59 pm on January 12, 2023: member

    I don’t want to spend 2023 regularly checking back on this PR wondering what is happening.

    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.

  66. michaelfolkson commented at 8:03 pm on January 12, 2023: contributor

    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

  67. michaelfolkson commented at 10:02 pm on January 12, 2023: contributor

    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.

  68. achow101 commented at 10:21 pm on January 12, 2023: member

    I’m accused of being “aggressive” for having the cheek to ask why.

    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.

  69. ghost commented at 0:29 am on January 13, 2023: none

    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.

    1. Everyone is aware p2p and net modules have no maintainers since @sipa and @laanwj stepped down as maintainers
    2. Adding @vasild as new maintainer was ACKed by 3 present maintainers. Why would they do it if there isn’t any need? Even @fanquake who had some disagreement never mentioned there isn’t any need.

    19:13 from what I can tell, I believe the concern is the lack of addressing the specific concerns brought up in #25871 (comment)

    1. Scope was changed after this comment in #25871 (comment)
    2. Commenter had no issues if this pull request is merged later #25871 (comment) (Similar to some disagreements in PR that added last maintainer’s keys)

    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

    • Bitcoin Core needs a maintainer for p2p/net module. Some maintainers agree with the ’need’ part and this has nothing to do with making good PRs or reviewing which is true for all modules including mempool
    • Bitcoin Core lacks reviewers overall and this has nothing to do with adding of new maintainer as it was done for other modules
    • Accusing others of sabotaging when they ask real questions about the politics and issues with adding new maintainer wont solve any problems

    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 https://github.com/bitcoin/bitcoin/pull/26868

  70. achow101 commented at 1:00 am on January 13, 2023: member
    I no longer Concept ACK this. Working on a much longer response.
  71. naumenkogs commented at 7:25 am on January 13, 2023: member

    I want to withdraw my ACK, because now I don’t consider my initial motivations valid:

    1. I thought this would improve the review process of p2p PRs somehow.
    2. It also felt like “deserved” for vasild to get this role.

    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.

  72. michaelfolkson commented at 7:29 am on January 13, 2023: contributor
    I also want to withdraw my ACK….. because why not? This PR is dead anyway.
  73. maflcko commented at 10:04 am on January 13, 2023: member
    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.
  74. michaelfolkson commented at 10:34 am on January 13, 2023: contributor
    @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?
  75. maflcko commented at 10:39 am on January 13, 2023: member
    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.
  76. michaelfolkson commented at 10:46 am on January 13, 2023: contributor
    @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)?
  77. maflcko commented at 10:53 am on January 13, 2023: member

    Or do you think there shouldn’t be a P2P maintainer added in the next x months or y years?

    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?

  78. glozow commented at 10:54 am on January 13, 2023: member

    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:

    • X has a good understanding of p2p - they have a mental model of what its design goals are, what known issues exist, what problems should be prioritized, etc.
    • Given a p2p-related security disclosure, X would be able to assess its severity, know how to fix it, and who else to consult.
    • X reviews most things in p2p, both identifying bugs and helping projects make progress. If something had 5 ACKs and X said it was conceptually flawed, I wouldn’t merge it.

    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.

  79. laanwj commented at 11:00 am on January 13, 2023: member

    With five reasonably active maintainers right now, it is pretty clear that there is no urgent rush to assign a new maintainer

    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.

  80. michaelfolkson commented at 11:01 am on January 13, 2023: contributor

    @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.

    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.

    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.

  81. stickies-v commented at 11:45 am on January 13, 2023: contributor

    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.

  82. achow101 commented at 8:06 pm on January 13, 2023: member

    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:

    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.

    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.

    These statements, coupled with

    Not leave PRs that are ready for merge unmerged.

    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.

  83. vasild commented at 8:29 am on January 14, 2023: contributor

    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:

    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.

    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 A=>B does not follow that !A=>!B). Prematurely merged PRs are a bigger danger, more subtle and difficult to spot. I second @glozow’s comment above “… important role of a maintainer is to understand when to not merge a PR …”

    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.

    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. @laanwj, that means a lot, thank you! :heart:

  84. vasild closed this on Jan 14, 2023

  85. bitcoin locked this on Jan 14, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me