Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update bech32 dependency #2117

Merged
merged 1 commit into from Oct 9, 2023

Conversation

tcharding
Copy link
Member

Update the bech32 dependency to use the newly release beta version.

The main fix here is silent, a bug fix in bech32 that was being hit by our fuzzing suite.

Update the `bech32` dependency to use the newly release beta version.

The main fix here is silent, a bug fix in `bech32` that was being hit by
our fuzzing suite.
@tcharding
Copy link
Member Author

tcharding commented Oct 9, 2023

Our two ack policy sucks, this is a trivial change and is needed to unblock CI for other PRs but does not qualify for either of the two carve-outs we have. Does the two ack policy really improve the quality of code merged when it pretty much just pressures devs to review - that does not seem to me to be in the spirit of the whole thing. I rekon we should have more benevolent dictator for life and less force-devs-to-ack-to-make-it-seem-like-we-did-deep-review.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 33ee49f

@vincenzopalazzo
Copy link
Contributor

Our two ack policy sucks, this is a trivial change and is needed to unblock CI for other PRs but does not qualify for either of the two carve-outs we have. Does the two ack policy really improve the quality of code merged when it pretty much just pressures devs to review - that does not seem to me to be in the spirit of the whole thing. I rekon we should have more benevolent dictator for life and less force-devs-to-ack-to-make-it-seem-like-we-did-deep-review.

I agree, I have some experience with GNU projects where there is a very nice policy of trivial commits. This basically allows you to open a patch and merge it when there is a simple change. It is also safe to do because whoever can merge a patch is also a trusted person (has right to commit), so it should be fine

Open a PR will notify all the listeners to changes about the change, and for this reason is better than push on master directly

Happy to chat in another place (maybe an issue?) about this

@apoelstra
Copy link
Member

@tcharding I think "changing from one prerelease to another" qualifies for the "minor CI fixes" carveout.

But sure, if you think in principle it doesn't, then I'll just pull rank as BDFL and merge this.

But having said this, you need to update the Cargo-*.lock files before I can pass this :).

Copy link
Member

@clarkmoody clarkmoody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 33ee49f

Copy link
Contributor

@realeinherjar realeinherjar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 33ee49f

@tcharding
Copy link
Member Author

But sure, if you think in principle it doesn't, then I'll just pull rank as BDFL and merge this.

I don't want to be seen to be pushing my own changes through so I'm being really careful to only call carve-out if it is 100% covered. I think you should be able to BDFL things in much more fuzzily.

@tcharding
Copy link
Member Author

But having said this, you need to update the Cargo-*.lock files before I can pass this :).

Yep I forgot to do it yesterday, saw your message on another PR and did it this morning, must have been in flight when you commented.

@apoelstra
Copy link
Member

I don't want to be seen to be pushing my own changes through so I'm being really careful to only call carve-out if it is 100% covered. I think you should be able to BDFL things in much more fuzzily.

Ok, that's fair, good for you to be fairly conservative when claiming "carve-out". But I think this one qualifies as a CI fix :).

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 33ee49f

@apoelstra apoelstra merged commit 2095fae into rust-bitcoin:master Oct 9, 2023
29 checks passed
@tcharding tcharding deleted the 10-10-use-bech32-beta branch October 9, 2023 23:04
@yancyribbens
Copy link
Contributor

Our two ack policy sucks..

I think the two-ack policy would be fine if it's easy to get people who are maintainers to review and ack. The problem as I see it is there isn't a lot of people who are maintainers that are also active reviewers. Personally, I feel more safe when more people review my changes, but for CI changes I get that it maybe doesn't need as much scrutiny.

. I rekon we should have more benevolent dictator for life

Well if we had more benevolent dictators for life, would we need things like bitcoin? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants