Conversation
Remove the hard NoActiveKeyset check in Mint::new_internal() that prevented a mint from starting without active non-Auth keysets. All downstream code handles empty active keysets gracefully.
gudnuf
force-pushed
the
feat/relax-active-keyset-requirement
branch
from
a0492e0 to
4c8b6ea
Compare
Only auto-create keysets for units with no keyset history (first-time bootstrap). If inactive keysets exist for a unit, the operator deliberately deactivated them and both the builder and init_keysets respect that.
gudnuf
force-pushed
the
feat/relax-active-keyset-requirement
branch
from
4c8b6ea to
d597650
Compare
| if let Some((input_fee_ppk, amounts)) = supported_units.get(&unit) { | ||
| // If no keyset is currently active for this unit, the operator | ||
| // deliberately deactivated it. Respect that and skip reactivation. | ||
| if !keysets.iter().any(|k| k.active) { |
There was a problem hiding this comment.
this I'm not sure about. Claude claims that init_keysets could be removed because it is now redundant with the mint builder.
There was a problem hiding this comment.
I think we have some preexisting split logic that should be cleaned up but for this I think as it is, is okay.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1899 +/- ##
==========================================
- Coverage 63.70% 63.64% -0.07%
==========================================
Files 329 330 +1
Lines 55042 55104 +62
==========================================
+ Hits 35067 35073 +6
- Misses 19975 20031 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Check if version matches explicit preference | ||
| if let Some(want_v2) = self.use_keyset_v2 { | ||
| let is_v2 = | ||
| keyset.id.get_version() == cdk_common::nut02::KeySetVersion::Version01; | ||
| if want_v2 && !is_v2 { | ||
| tracing::info!("Rotating keyset for unit {} due to explicit V2 preference (current is V1)", unit); | ||
| rotate = true; | ||
| } else if !want_v2 && is_v2 { | ||
| tracing::info!("Rotating keyset for unit {} due to explicit V1 preference (current is V2)", unit); | ||
| rotate = true; | ||
| } | ||
| } | ||
| } else { | ||
| // No active keyset for this unit | ||
| tracing::info!("Rotating keyset for unit {} (no active keyset found)", unit); | ||
| rotate = true; | ||
| // No active keyset for this unit. Check if any keyset (active or not) has | ||
| // ever existed. If inactive keysets exist, this is a deliberate deactivation | ||
| // and we should respect it. Only auto-create on first-time bootstrap. | ||
| let unit_has_history = active_keysets.keysets.iter().any(|k| k.unit == *unit); | ||
|
|
||
| if unit_has_history { | ||
| tracing::info!( | ||
| "No active keyset for unit {}, inactive keysets exist (respecting deliberate deactivation)", | ||
| unit | ||
| ); | ||
| } else { | ||
| tracing::info!( | ||
| "No keyset history for unit {}, bootstrapping initial keyset", | ||
| unit | ||
| ); | ||
| rotate = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this does not account for keysetv2 and those will reactivate?
There was a problem hiding this comment.
why do you say that? This looks correct to me:
- load all keysets
- for each supported unit: if there are inactive keysets then don't reactivate them
One confusing thing I noticed about this function is that that let active_keysets = signatory.keysets().await?; is all keysets not just active
There was a problem hiding this comment.
also btw, I think v2 is covered by the integration tests already if its true the builder uses v2 keysets by default
| if let Some((input_fee_ppk, amounts)) = supported_units.get(&unit) { | ||
| // If no keyset is currently active for this unit, the operator | ||
| // deliberately deactivated it. Respect that and skip reactivation. | ||
| if !keysets.iter().any(|k| k.active) { |
There was a problem hiding this comment.
I think we have some preexisting split logic that should be cleaned up but for this I think as it is, is okay.
| /// Deactivate a specific keyset by ID without activating a replacement | ||
| async fn deactivate_keyset(&mut self, id: Id) -> Result<(), Error>; | ||
|
|
There was a problem hiding this comment.
This spec will need to be updated to support this. But its not merged yet so thing its okay CDK goes first.
There was a problem hiding this comment.
Ahh I missed that
NUT-02 says "Mints can have multiple keysets at the same time but MUST have at least one active keyset".
So this would be a breaking change
There was a problem hiding this comment.
Sorry looks like I forgot to link the Mint remote signer spec that I meant cashubtc/nuts#250.
But yes thats a good catch on NUT-02 as well. I guess the current way to spin down a mint is to disable minting but keep the keyset active.
2 participants
Description
This is the first phase of #1895. In this PR I make it so that a mint operator can deactivate a keyset by ID and allow the mint to start with no active keysets. The mint will still auto-rotate when no keysets for a unit exist or when the config changes compared to an active keyset. Rotation will not occur only if the keyset was explicitly deactivated.
Notes to the reviewers
I did this separate from enforcing expiry so that a mint operator can simply deactivate a keyset without an expiry and then leave the mint up until they feel like rugging. I could see an argument for only allowing no active keysets if they were deactivated due to expiry, but imo a mint should be able to run with no active keysets regardless of expiry.
If this is approved, then I will open a followup that adds expiry enforcement by disabling expired keysets and then extending input/output validation to disallow any operations for expired keysets
Suggested CHANGELOG Updates
CHANGED
Keyset rotation no longer happens automatically if keysets are deliberately deactivated
ADDED
Ability to deactivate keysets
REMOVED
Startup validation that checks for active keysets
FIXED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)