(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
.
(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
.
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.
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.
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
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
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.
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.
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
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!
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.
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.
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.
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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
Type | Reviewers |
---|---|
ACK | Zero-1729, aureleoules |
Concept NACK | yancyribbens, stickies-v, achow101 |
Concept ACK | mzumsande, promag, ariard, jamesob, naumenkogs, michaelfolkson, MarcoFalke |
Stale ACK | 1440000bytes, jonatack, jarolrod |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
-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.
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.
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.
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.
@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.
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.
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.
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.
For Networking scope maintenance.
a16317e018...902d8810e5
: omit net_processing
and reduce scope to Networking as suggested.
ACK 902d8810e504b806e02ae890145a90e1a98cddef
Happy to move this along if scope narrowing is the only issue.
@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
ACK
or NACK
and defer to the current maintainers.
@michaelfolkson I don’t feel like I’m the person to
ACK
orNACK
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.
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.
All maintainers have ACKed this apart from fanquake and glozow. @michaelfolkson Maybe you missed @hebasto
@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.
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.
@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.
@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.)
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?
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.
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
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
@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.
@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.
@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.
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.
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.
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.
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:
https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-06-30#827832
ℹ️ 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?
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.
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.
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.
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.
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 https://github.com/bitcoin/bitcoin/pull/26868
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.
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?
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.
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.
@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.
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:
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.
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: