Conversation
|
Please, expand SEV acronym in description. |
Done |
daniel-wong-dfinity-org
left a comment
daniel-wong-dfinity-org
left a comment
There was a problem hiding this comment.
My main concern (non-blocking) is that it seems you made a small change to the intended behavior. Before, I think you were allowed to set sev_enabled to its current value. Now, you are not allowed to do this. This new approach is valid and safer (and therefore, arguably superior). OTOH, when fixing bugs, I think we normally just get rid of bad thing, not try to make a more general improvement. But anyway, I do not seriously object, and this is maybe what should have been done in the first place -> I preemptively approve, but I just bring this up to make sure we are changing intentionally.
Yes, this is intentional. I don't see a reason why someone needs to propose to set the SEV feature flag to the same value as it's been set form the moment the subnet was created. This seems to indicate fundamental misunderstanding of how this feature flag works, and thus I think it should be rejected. Note that there were no unit tests to specify this behavior, thus one could see this as a (less significant) bug, not a novel behavioral change (which would justify a |
Per @jasonz-dfinity's review on #9956: because `merge_subnet_record` wholesale-replaces `subnet_record.features` via `maybe_set_option!`, a proposal could disable SEV on an SEV-enabled subnet by setting `features = Some(SubnetFeatures { ..., sev_enabled: None })`. The restored strict check (introduced in #3044) only panicked when `payload.features.sev_enabled` was explicitly `Some(_)`, so this implicit transition slipped through. Compare the effective `sev_enabled` (post-replacement) against the existing record via the rust-level `SubnetFeatures`, which collapses `None` and `Some(false)` to `false`. This catches both explicit and implicit changes while still permitting no-op updates (the case #3044 originally fixed). Adds two regression tests (explicit and implicit disable) plus a no-op test, against an SEV-enabled fixture seeded with chip-ID'd nodes so the SEV invariant stays satisfied.
2 participants
This PR fixes the validation of the SEV (AMD Secure Encrypted Virtualization) feature that prevented change-subnet proposals that attempted to set a (non-SEV-related) feature for the first time (i.e., when the subnet doesn't yet have any features).