wormhole-attester: port the v1.3.0 healthcheck hotfix #454
Conversation
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
This PR fixes a missing assignment to HealthCheckState's enable flag. This would cause the attester to ignore all settings (including defaults) and keep the enable flag set to false because we would never bother setting it from attestation_cfg that came at runtime. The bug is fixed and the healthcheck behavior is hardened. Currently, the lazy_static block will explicitly initialize the global with default values from attestation_cfg (enable is true, window size is 100). This prevents a similar human error from disabling the healthcheck. Secondly, main.rs overwrites the default values using the HealthCheckState::new() constructor, instead of individual struct member assignments. This ensures that all necessary values or their defaults are passed as the desired healthcheck state.
This change fixes a bug that would cause a gigantic allocation to happen, after the attestation_cfg::default_healthcheck_window_size function is taken without being called and converted to a very large usize value. This would result in a big allocation which would crash the attester. At the core it's a misunderstanding about "Fn() as usize" conversion which unfortunately is safe Rust.
This commit changes the `let hc = <lock the mutex>` into a direct assignment to the locked mutex guard. Rust mutices are typically scope-based which makes using them in variables more error prone.
This speeds up the build of the container, and prevents useless triggers from the attester codebase
ali-behjati
approved these changes
Jan 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes a missing assignment to HealthCheckState's enable
flag. This would cause the attester to ignore all settings (including
defaults) and keep the enable flag set to false because we would never
bother setting it from attestation_cfg that came at runtime.
The bug is fixed and the healthcheck behavior is hardened. Currently,
the lazy_static block will explicitly initialize the global with
default values from attestation_cfg (enable is true, window size is
100). This prevents a similar human error from disabling the
healthcheck. Secondly, main.rs overwrites the default values using the
HealthCheckState::new() constructor, instead of individual struct
member assignments. This ensures that all necessary values or their
defaults are passed as the desired healthcheck state.