go/common/sgx/pcs/policy: Add FMSP whitelist to quote policy #6331
Conversation
✅ Deploy Preview for oasisprotocol-oasis-core ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
peternose
force-pushed
the
peternose/feature/whitelist-fmspcs
branch
from
cce6686 to
abe74f5
Compare
peternose
force-pushed
the
peternose/feature/whitelist-fmspcs
branch
4 times, most recently
from
dfa53c6 to
1b9286a
Compare
peternose
requested review from
kostko,
peterjgilbert,
pro-wh and
ptrus
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6331 +/- ##
==========================================
- Coverage 64.31% 64.27% -0.05%
==========================================
Files 698 698
Lines 67826 67892 +66
==========================================
+ Hits 43623 43638 +15
- Misses 19177 19245 +68
+ Partials 5026 5009 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
peternose
force-pushed
the
peternose/feature/whitelist-fmspcs
branch
from
1b9286a to
c20cbdd
Compare
| return fmt.Errorf("node: malformed SGX constraints: %w", err) | ||
| } | ||
| if err := sc.ValidateBasic(teeCfg); err != nil { | ||
| if err := sc.ValidateBasic(teeCfg, isFeatureVersion242); err != nil { |
There was a problem hiding this comment.
How will this look when other features are added? Will there be additional version flags like these? Should we instead pass context or somehow make this part of teeCfg?
There was a problem hiding this comment.
How will this look when other features are added? Will there be additional version flags like these? Should we instead pass context or somehow make this part of
teeCfg?
- If other features are added in the same version, than this remains the same.
- If another feature is added in e.g. 24.3, than we would need to add a new flag
isFeatureVersion243, pass context or something else. But the trick is that we don't need to do this as this code should be removed in version 24.3, after we do the upgrade, because state sync will work without these changes, as they are here just to prevent filling the whitelist.
There was a problem hiding this comment.
But I guess this can only be removed if there are no transactions posted that would violate this constraint? As otherwise iterative block sync would fail.
There was a problem hiding this comment.
But I guess this can only be removed if there are no transactions posted that would violate this constraint? As otherwise iterative block sync would fail.
No, wrong thinking. This is here to forbid posting transactions that would violate this constraint (non-empty whitelist) until we do the feature version bump.
This validation ensures we don't have consensus problems, as once we merge this PR in oasis core version 25.6, a new node running version 25.6 could post a policy with non-empty whitelist while an old node with version 25.5 would reject it. Having this check ensures that new nodes must submit empty whitelists until we do the feature version bump. And once we do the bump, this validation can be removed as it is not needed anymore as all nodes should be running at least 25.6. version.
There was a problem hiding this comment.
But what happens if someone posted a transaction with a non-empty whitelist right now and the transaction is included in a block, but fails (because currently it is an unknown field)? Then once this check is removed in a future version, syncing from genesis will cause divergence when processing this transaction (which will now succeed during iterative block sync)? So this works as long as no such transaction is included in a block before the governance vote.
There was a problem hiding this comment.
Oh, now I see. Yes, that would be a problem.
I suggest we merge these changes, and after we upgrade, we do state sync to check that there are no such transactions (this means that there could still be a tx with a whitelist but also with another unknown field, and that field would still fail the tx). If there are, we prettify the code, if not, we can remove these changes.
There was a problem hiding this comment.
I suggest we merge these changes, and after we upgrade, we do state sync to check that there are no such transactions.
Looks safe to me and indeed simpler.
If we find such transactions then probably SGXConstraintsV2 behind feature flag might be cleaner on the long run (unlikely to happen so lets ignore this).
martintomazic
left a comment
martintomazic
left a comment
There was a problem hiding this comment.
Is the plan to release this as part of the 25.9 and bind all 4 breaking changes under feature flag (assuming #6410 will be ready in time as well)?
Yes, 25.9 or latter. There are still some breaking changes we would like to merge, before we enable all of them. |
Ensure the FMSPC whitelist in the quote policy remains empty until the next feature version is enabled. Once that feature is enabled, these changes can be removed.
peternose
force-pushed
the
peternose/feature/whitelist-fmspcs
branch
from
c20cbdd to
8ca2ad2
Compare
|
Mainnet genesis synced successfully, so merging. Will test latter also testnet. |
Testnet also synced successfully. |
No description provided.