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

Make static context const #1639

Merged

Conversation

purpleKarrot
Copy link
Contributor

Fixes #1637

@real-or-random
Copy link
Contributor

Looks good to me.

The only caveat is that this is, strictly speaking, a breaking change. If we want to be strict, we'd need a 0.7.0 instead of a 0.6.1. But I don't think we care?! I mean, any user code writing to the variable is most likely buggy... Hm.

@sipa
Copy link
Contributor

sipa commented Nov 24, 2024

Technically speaking, this is certainly an API change. A constant and non-constant variable are not compatible as far as the C language is concerned. A (buggy) program could fail to compile with this change, while it was fine before.

But is it an ABI break? The symbol still exists, and has the same size and meaning. Linking will not fail for any reason, it is only at runtime that an attempt to modify the variable may (or may not) trigger a memory violation.

@real-or-random
Copy link
Contributor

it is only at runtime that an attempt to modify the variable may (or may not) trigger a memory violation.

The good thing is that a clean crash is probably the worst that can happen in practice...

I think I'm okay with anything here. If you ask me, we can treat this as a breaking or a non-breaking chance, when it comes to both API and ABI.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

In any case, I prefer this proper fix over introducing a second symbol just to keep backwards compatibility.

utACK bfc3ae0

We can debate at release time whether this is breaking or not.

@real-or-random
Copy link
Contributor

cc @jonasnick @sipa

Can you take a look? I think this will be good to have merged because it's addressing a real user issue.

@sipa
Copy link
Contributor

sipa commented Feb 24, 2025

ACK

@purpleKarrot purpleKarrot force-pushed the static-context-constness branch from bfc3ae0 to 432ac57 Compare February 24, 2025 16:25
@purpleKarrot
Copy link
Contributor Author

I just rebased and signed the commit.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK 432ac57

@real-or-random real-or-random merged commit 6c2a39d into bitcoin-core:master Feb 25, 2025
116 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

secp256k1_context_static should be a const variable [edited]
3 participants