kernel: De-globalize static validation variables - #30425
Merged
Merged
Conversation
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
These were found while looking for static mutable state in the kernel library.
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This was referenced Jul 11, 2024
dergoegge
approved these changes
Jul 15, 2024
maflcko
reviewed
Jul 15, 2024
maflcko
left a comment
maflcko
left a comment
Member
There was a problem hiding this comment.
ACK 51fa262 🍚
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 51fa26239af9bbfd44029aaf595cb4c6a8d4a75d 🍚
aL3wyo63PKckFQ0ghZy3cIXx4jGARGHgX+Z8LGNKlGlYbKt9fqrhMBJdzW63qGSmHLejkehE7rj+yIC2LCi8AA==
| CBlockIndex* m_best_header GUARDED_BY(::cs_main){nullptr}; | ||
|
|
||
| /** The last header for which a headerTip notification was issued. */ | ||
| CBlockIndex* m_last_notified_header GUARDED_BY(::cs_main){nullptr}; |
Member
There was a problem hiding this comment.
39f9b80: Not sure about making this public mutable. Would it not be better to make this a private field (along with making NotifyHeaderTip a private method)?
fanquake
added a commit
that referenced
this pull request
Jul 17, 2024
fa92705 refactor: Make m_last_notified_header private (MarcoFalke) Pull request description: Seems brittle to expose mutable fields public. Fix it by making it private. Fixes #30425 (comment) ACKs for top commit: dergoegge: utACK fa92705 Tree-SHA512: d9841c42571144ced0edeaa4bb1d96a177a011dca37c8342c66513477c37278602a1b88beb93068b94fc4443b1552c8fc9f98bcf0bda7d0fc101e61e90c33944
ryanofsky
added a commit
that referenced
this pull request
Jul 26, 2024
fae0db0 fuzz: Deglobalize signature cache in sigcache test (TheCharlatan) Pull request description: The body of the fuzz test should ideally be a pure function. If data is persisted in the cache over many iterations, and there is a crash, reproducing it from the input might be difficult. Solve this by getting rid of the global state. This is a follow-up from #30425. ACKs for top commit: dergoegge: utACK fae0db0 ryanofsky: Code review ACK fae0db0 Tree-SHA512: 93dcbb9f2497f13856970469042d6870f04de10fe206827a8db1aae7fc8f3ac7fd900bee7945b5fe4c9e33883268dabb15be7e7bc91cf353ffc0d118cd60e97d
Kino1994
pushed a commit
to Kino1994/bitcoin-full-history
that referenced
this pull request
Jun 28, 2026
…ivate 3700dd7 refactor: Make m_last_notified_header private (MarcoFalke) Pull request description: Seems brittle to expose mutable fields public. Fix it by making it private. Fixes bitcoin/bitcoin#30425 (comment) ACKs for top commit: dergoegge: utACK 3700dd7 Tree-SHA512: d9841c42571144ced0edeaa4bb1d96a177a011dca37c8342c66513477c37278602a1b88beb93068b94fc4443b1552c8fc9f98bcf0bda7d0fc101e61e90c33944
BigcoinBGC
pushed a commit
to BigcoinBGC/bigcoin
that referenced
this pull request
Jun 30, 2026
…ivate b367fab refactor: Make m_last_notified_header private (MarcoFalke) Pull request description: Seems brittle to expose mutable fields public. Fix it by making it private. Fixes bitcoin/bitcoin#30425 (comment) ACKs for top commit: dergoegge: utACK b367fab Tree-SHA512: d9841c42571144ced0edeaa4bb1d96a177a011dca37c8342c66513477c37278602a1b88beb93068b94fc4443b1552c8fc9f98bcf0bda7d0fc101e61e90c33944
Kino1994
pushed a commit
to Kino1994/bitcoin-full-history
that referenced
this pull request
Aug 19, 2026
…ivate fa927055dd43dda945396574273a210186beec9f refactor: Make m_last_notified_header private (MarcoFalke) Pull request description: Seems brittle to expose mutable fields public. Fix it by making it private. Fixes bitcoin/bitcoin#30425 (comment) ACKs for top commit: dergoegge: utACK fa927055dd43dda945396574273a210186beec9f Tree-SHA512: d9841c42571144ced0edeaa4bb1d96a177a011dca37c8342c66513477c37278602a1b88beb93068b94fc4443b1552c8fc9f98bcf0bda7d0fc101e61e90c33944
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
This pull request is part of the libbitcoinkernel project.