policy: Add refactor carve out #1945
Conversation
I have managed to burn out or bore our reviewers/maintainers. Getting two acks is becoming increasingly difficult. I've pestered everyone to the limit that I feel socially comfortable doing so am requesting a carve out to the 2-ACK before merge rule. The primary justification is that I feel we should have a bit more of BDFL and a bit less total consensus if we are to push forwards.
|
Good luck getting other ACKs on this :). |
|
In my experience with bitcoin core, if you don't get enough ACKs, it just gets closed instead of merged. I don't know why rust-bitcoin should be different? |
|
@yancyribbens the main reason to be different is that Bitcoin Core is much more "feature complete" than this library. As far as its role of being a p2p node enforcing the exact consensus rules, it's more-or-less done. With a few big exceptions, like BIP-324, which are self-contained projects that nonetheless take multiple years to spec and implement. Another reason is that Bitcoin Core, as developer-starved as it is, actually has more active maintainers and contributors than rust-bitcoin. Finally, despite both these reasons, Core does struggle with a lack of momentum on its non-core functionality. And there's a lot of effort being put into separating out the wallet, GUI, RPC, etc., into separate binaries and eventually even separate repos, so that these other things can move more quickly. It's acknowledged that the Core "most things die on the vine" development model is maybe alright for the core consensus stuff but it's not the right model for less-critical user-facing stuff. I'd argue that rust-bitcoin doesn't have anything like "core consensus stuff" (maybe the libsecp bindings come close) and that the whole project probably should shift toward a faster-moving model. |
Some mentions and further arguments to back up this PR and attempt to get it in:
|
There was a problem hiding this comment.
ACK 55be538. Same reasons as apoelstra. And this is only for re-factors that are not adding any new features.
|
I guess this PR needs more than 2 ACKS :) |
|
Yes, I think we should leave this open for 2 weeks to wait for NACKs. |
|
I will email all maintainers to flag this PR, that way any that do not see notifications will not get rugged by this. I sure wish we had a public mailing list so there was public proof that I send the email. Sent Email(Email addresses removed.) Date: Wed, 19 Jul 2023 09:20:27 +1000 Hi lads, I have put up a PR proposing a carve out to the 2-ACK before merge Trying to find a balance between spamming all you guys and making I wanted to email you all so that if it does merge no one feels Thanks for your patience, |
|
For the reasons @apoelstra mentioned, it makes sense for rust-bitcoin to be more permissive than bitcoin. However I don't think two acks is very uncommon in my experience with other projects and I don't understand why it's so difficult to get two acks here. Maybe there needs to be more dedicated maintainers? I'm not sure what the process is to becoming a maintainer. |
|
I agree this makes sense for refactors ACK 55be538 |
You could read over the 600 patches that I did in the last 18 months and see if you are still enthusiastic about reviewing refactor work :) |
I'm fine to help review. I'm not a maintainer so my acks don't matter however :) |
|
Yeah I think I can ACK this. I was indeed saying I think a minimim review period would be good. I actually think 2 weeks + 1 ACK is a way saner merge policy than any 2 ACKs (I've seen merge times of less than a day 😅) |
|
Ack 55be538 |
|
That is an ack from every maintainer except @Kixunil who hasn't been seen around for awhile, I believe he is on holidays. |
|
Can we merge this @apoelstra? I believe that the following three PRs are also good examples of minor fixes that no one cares enough about to review and are harmless to merge.
Do we want to add some way that maintainers can come back at a later date and see which PRs were merged with one ack? Labels are no good because they will likely get forgotten. I can't think of another way ATM. |
|
Yes, but I also can't think of a way. And sure, I'll merge this. We are technically 6 hours short of 2 weeks here :P. But we are only waiting on Kix's approval, and I don't think his absense should override six ACKs. |
|
I have reduced the number of github-required ACKs to 1. However, for nontrivial PRs or PRs that have been ACKed for less than 2 weeks, we still require 2 ACKs. |
|
Awesome, onwards and upwards! |
I have managed to burn out or bore our reviewers/maintainers. Getting two acks is becoming increasingly difficult. I've pestered everyone to the limit that I feel socially comfortable doing so, and as such, am requesting a carve out to the 2-ACK before merge rule.
The primary justification is that I feel we should have a bit more of BDFL and a bit less total consensus if we are to push forwards.
Example PRs where this change would apply
Script::emptytoScript::new#1925networkmodule and move/clean up types #1854