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

fix incorrect FFI binding for pubkey_combine #670

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

apoelstra
Copy link
Member

Fixes #669.

Needs backport.

@apoelstra apoelstra force-pushed the 2023-12--ffi-fix branch 2 times, most recently from aaa9c16 to 68d021b Compare December 18, 2023 21:20
@tcharding
Copy link
Member

tcharding commented Dec 19, 2023

I rekon if we are going to backport this would be nicer have a bit more description in the commit log.

ACK 484e5d8

@apoelstra
Copy link
Member Author

The one line commit log, combined with the one line diff, seem pretty self explanatory to me.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 484e5d8

@apoelstra
Copy link
Member Author

Would one or both of you @tcharding @Kixunil like to be a maintainer on this repo?

I think our informal processes are fine to make sure that e.g. crypto stuff doesn't get merged without a crypto person reviewing. There's no reason to use Github to only allow a narrower set of reviewers.

I'll go ahead and merge this with just your ACKs since we're not actually even enforcing the "maintainer must ACK" rule here..

@apoelstra
Copy link
Member Author

Lol, oops, I actually screwed up this PR. I updated the secp-sys version but did not re-run the vendoring script to rename all the 0_9_1 functions to 0_9_2.

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 3, 2024

Yeah, I'd like to help maintain this. It's a key piece of the rust-bitcoin infrastructure so being more involved with it should help.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 3373cc9

@apoelstra
Copy link
Member Author

Thanks! Added you as a maintainer.

@apoelstra apoelstra merged commit 60a5e36 into rust-bitcoin:master Jan 3, 2024
20 checks passed
@apoelstra apoelstra deleted the 2023-12--ffi-fix branch January 3, 2024 19:13
@apoelstra
Copy link
Member Author

Tagged, published, and yanked previous 0.9.x versions of secp-sys.

@apoelstra
Copy link
Member Author

Oops, crap, I need to update rust-secp along with rust-secp-sys, since this was actually a breaking change to rust-secp-sys.

@tcharding
Copy link
Member

I'm willing to be a maintainer also. I'm not super confident with unsafe but willing to invest the time and take the responsibility.

@apoelstra
Copy link
Member Author

Bumped you from "Triage" to "Maintain".

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.

Incorrect FFI binding for secp256k1_ec_pubkey_combine
3 participants