Auto-rotate keysets on an age interval - #2253
Conversation
c065043 to
6880927
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2253 +/- ##
==========================================
+ Coverage 74.05% 74.24% +0.18%
==========================================
Files 362 362
Lines 84799 85520 +721
==========================================
+ Hits 62797 63493 +696
- Misses 22002 22027 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Auto-rotation waits a full interval before checking overdue keysets after restart (low) - Already-overdue active keysets are not rotated until one full configured interval after every auto-rotation task start/restart, undermining the configured maximum keyset age.
6880927 to
103a4e6
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Mint::stop() can abort auto-rotation while keysets are cleared (low) - A stop/restart during the narrow reload window can leave the embedded signatory's in-memory keyset state empty, causing all subsequent signing requests on that mint instance to fail with UnknownKeySet until the signatory is rebuilt.
Unanchored locations included in summary:- crates/cdk/src/mint/mod.rs:476
- crates/cdk-signatory/src/db_signatory.rs:92
|
@cdk-bot review |
1 similar comment
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- reload_keys_from_db can publish a stale keyset snapshot when concurrent rotations race (medium) - A concurrent auto-rotation and manual rotation can leave the signatory's in-memory active keyset map and keyset update channel on an older snapshot than the database. The newly committed keyset remains in the DB but is not served/signed with until a later reload or restart.
Additional locations included in summary:- crates/cdk-signatory/src/db_signatory.rs:222
Unanchored locations included in summary: - crates/cdk-signatory/src/db_signatory.rs:92
- crates/cdk-signatory/src/db_signatory.rs:329
- crates/cdk-signatory/src/db_signatory.rs:222
|
@cdk-bot review |
| if let Err(err) = signatory.rotate_aged_keysets(interval).await { | ||
| tracing::error!("Automatic keyset rotation failed: {}", err); |
There was a problem hiding this comment.
I think we probably want to retry before the next tick? For example I think here if it fails and its a long tick like a monthly rotation it will not retry until the next tick so another month?
There was a problem hiding this comment.
I have a local version where the ticker is fixed, like every 30s. So it would not be deadly to wait for the next one.
There was a problem hiding this comment.
But I don't think 30s is likely in practice is more likely to be a month or something so waiting a full extra month is a big difference
There was a problem hiding this comment.
I think that's fine; rotate_aged_keysets should be cheap enough to run and exit.
| let max_age = max_age.as_secs(); | ||
|
|
There was a problem hiding this comment.
We should check this is not zero? The api accepts Duration that can be sub second and we would later panic if this is Zero. I see no reason to support sub second roations so just returning an error on zero to be safe.
There was a problem hiding this comment.
We need to check that the as_secs() is zero not that the Duration is non zero to address the sub second issue.
There was a problem hiding this comment.
d7fc216 to
d5edeb0
Compare
|
@cdk-bot review |
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Reload keysets after partial auto-rotation failures (low) - A transient or persistent DB error in one due unit can make other successfully rotated units remain stale in memory and be repeatedly re-rotated, causing incorrect keyset publication lag and unnecessary keyset proliferation.
Additional locations included in summary:- crates/cdk-signatory/src/db_signatory.rs:417
e7842cd to
d8d355d
Compare
|
@cdk-bot review |
|
On the commit description for 6ff93ab I think we should default to rotating keysets, the linked nutshell pr rotates every 90 days for example. |
The signatory round-tripped the database on the hot path: rotate_keyset re-read the active keyset to compute the next derivation index and then reloaded the entire keyset set to refresh memory. It also loaded keysets synchronously in new and propagated any database error, so a database that was briefly unavailable at startup took the whole process down. Treat the database as a persistence layer only. It is read once on boot and written only inside the rotate_keyset transaction; every other operation is served from and mutates in-memory state. Rotation now reads the next index from memory and updates the in-memory maps directly after committing, mirroring what a fresh boot would compute. Make boot resilient. new attempts the load once (a healthy database still comes up ready), and on failure it returns anyway and keeps retrying in the background with exponential backoff. Until the first load succeeds, key-using operations return the new transient KeysetsNotLoaded error; subscribe_keysets stays open so consumers receive the keysets once they load. The embedded mint no longer refuses to start without an active keyset: it boots and installs keysets through the existing drain once the background load publishes them. This is groundwork to simplify cashubtc#2253 (auto-rotate keysets on an age interval): with memory as the source of truth and rotation updating it in place, a periodic rotator only has to call rotate_keyset, and the resilient boot keeps it running through transient DB outages. Document the persistence model and the boot behavior in ADR-0003.
d8d355d to
041033e
Compare
cdk-bot
left a comment
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Stale comment in
build_with_seedmisdescribesDbSignatory::try_newboot semantics (low) - see inline comment
| let signatory = Arc::new(cdk_signatory::embedded::Service::new(Arc::new( | ||
| in_memory_signatory, | ||
| ))); | ||
| // Resilient boot for the embedded mint: `try_new` returns even if the |
There was a problem hiding this comment.
Incorrect comment about embedded-mint boot resilience
The new comment in MintBuilder::build_with_seed states:
// Resilient boot for the embedded mint: `try_new` returns even if the
// database is briefly unavailable and loads keysets in the background.This describes the contract of DbSignatory::new (try-once, then background retry with backoff, serving Error::KeysetsNotLoaded until loaded), not DbSignatory::try_new, which is what is actually called here. try_new attempts the boot load once and bubbles up any error — the very next lines show DbSignatory::try_new(...).await?, so a database that is briefly unavailable fails the mint boot; nothing loads in the background.
This is confirmed by try_new's own doc comment added in this PR ("There is no background retry: an embedded signatory is part of the mint process, so a failed load should fail the mint boot rather than leave it serving Error::KeysetsNotLoaded"), the module-level docs in db_signatory.rs, and ADR-0003 ("The embedded mint uses DbSignatory::try_new, which attempts the load once and bubbles up any error so the mint fails to boot instead of serving without keys").
The comment misstates failure semantics on a boot-critical path: a maintainer or operator reading it would conclude the embedded mint survives transient database outages at startup when it does not. Suggested fix: rewrite the comment to describe the strict boot, e.g. "Strict boot for the embedded mint: try_new bubbles up any database error so a failed keyset load fails the mint boot instead of serving without keys."
The signatory round-tripped the database on the hot path: rotate_keyset re-read the active keyset to compute the next derivation index and then reloaded the whole keyset set to refresh memory. The database's role in steady-state operation was never pinned down. Treat the database as a persistence layer only. It is read once on boot and written only inside the rotate_keyset transaction; every other operation is served from and mutates in-memory state. Rotation reads the next index from memory and updates the in-memory maps directly after committing, mirroring what a fresh boot would compute, so it never reads the database back. Load strictly at boot. new runs the load once and propagates any error, so a failed load fails construction rather than leaving a signatory without keys; both the embedded mint and the standalone signatory server want a bad database to surface at startup. The in-memory state lives in DbSignatory's own fields, with no wrapper indirection. Serialize rotations. rotate_keyset holds a mutex across the whole operation, so two concurrent rotations of the same unit cannot read the same derivation index and derive duplicate keysets. This guards the standalone gRPC server, which calls rotate_keyset directly rather than through the embedded single-runner service. Relax the embedded mint boot: a signatory reporting no active keyset is a warning rather than a hard error, so an unconfigured mint still starts and its endpoints return keyset errors until it is configured. This is groundwork to simplify cashubtc#2253 (auto-rotate keysets on an age interval): with memory as the source of truth and rotation updating it in place, a periodic rotator only has to call rotate_keyset. Document the persistence model, the strict boot, and rotation serialization in ADR-0003.
The signatory round-tripped the database on the hot path: rotate_keyset re-read the active keyset to compute the next derivation index and then reloaded the whole keyset set to refresh memory. The database's role in steady-state operation was never pinned down. Treat the database as a persistence layer only. It is read once on boot and written only inside the rotate_keyset transaction; every other operation is served from and mutates in-memory state. Rotation reads the next index from memory and updates the in-memory maps directly after committing, mirroring what a fresh boot would compute, so it never reads the database back. Load strictly at boot. new runs the load once and propagates any error, so a failed load fails construction rather than leaving a signatory without keys; both the embedded mint and the standalone signatory server want a bad database to surface at startup. The in-memory state lives in DbSignatory's own fields, with no wrapper indirection. Serialize rotations. rotate_keyset holds a mutex across the whole operation, so two concurrent rotations of the same unit cannot read the same derivation index and derive duplicate keysets. This guards the standalone gRPC server, which calls rotate_keyset directly rather than through the embedded single-runner service. Relax the embedded mint boot: a signatory reporting no active keyset is a warning rather than a hard error, so an unconfigured mint still starts and its endpoints return keyset errors until it is configured. This is groundwork to simplify cashubtc#2253 (auto-rotate keysets on an age interval): with memory as the source of truth and rotation updating it in place, a periodic rotator only has to call rotate_keyset. Document the persistence model, the strict boot, and rotation serialization in ADR-0003.
The sweep read its due set from a process-local snapshot, so it could not tell an aged keyset from one a peer had already replaced. Every rotation had to carry the keyset it meant to replace and re-check it under the global lock, returning ConcurrentUpdate when a peer won, which the sweep then had to tell apart from a real failure. Reading the due set inside the rotation transaction removes the question. The reload runs under the global keyset lock, held to commit, so a unit a peer just rotated carries a fresh valid_from and is not due. Age decides, and rotate splits into begin/stage/finish so the sweep and a mint-initiated rotation share the loading and the commit. The sweep is now all-or-nothing, which Postgres forces once the units share a transaction.
The auto-rotation loop polls faster than its interval so that age, not process uptime, drives rotation. Every tick opened a transaction and took the global keyset lock before checking whether anything was due, taxing every instance in the fleet for nothing. Check the in-memory snapshot first: it only ever lags the database, so it over-reports due-ness and can never miss a keyset that is actually due. Mint::stop no longer waits without bound for an in-flight sweep, which can block on that same lock. After a timeout it leaves the rotation to finish detached rather than holding up shutdown. An unparsable rotation interval environment variable now fails startup instead of falling back to the 90-day default, which would rotate keys for an operator who was trying to disable rotation.
041033e to
5b062db
Compare
3 participants
Description
This PR is heavily inspired by cashubtc/nutshell#1058 and depends #2270
Mints need to move to fresh signing keys periodically without an operator rotating them by hand. The signatory owns the private keys and each keyset's valid_from timestamp, and that timestamp does not cross the Signatory trait, so the rotation decision belongs in the signatory rather than the mint. Rotated keysets already reach mints through the keyset subscription, so no restart or mint-initiated rotate is needed.
Add a background task to the embedded signatory that rotates every active keyset once it has been valid for at least a configured interval. The replacement keeps the previous amounts, input fee and id version, and carries final_expiry forward by the keyset's active age (saturating) so it stays valid at least as long as the keyset it replaces. The task holds a weak reference to the signatory and skips the immediate first tick, so it stops on its own once the signatory is dropped and never rotates a freshly created keyset before it has aged.
Auto-rotation runs directly against the signatory's lock-guarded state rather than through the embedded actor, because the age check needs valid_from. A per-unit recheck skips a keyset whose active id changed since it was snapshotted, so a mint-initiated rotation racing the task does not issue a redundant keyset.
Bind the task to shutdown. The embedded service owns the task handle and aborts it on drop, and the mint stores an abort handle and aborts it in stop(), before the early return so rotation halts even when no other services were started. The task is spawned once at build time because it needs the concrete embedded signatory, so a later start() does not bring it back; this is documented on the builder setter and on stop().
Expose the interval where each signatory is configured: a flag on the standalone signatory binary, a builder method for the embedded signatory, and a mintd config key with a matching environment variable. Auto-rotation is disabled unless an interval is set, and a remote signatory manages its own schedule.
Cover the behavior with tests for the age threshold, metadata carry-over across both keyset versions, the final_expiry push and its absence, multi-unit rotation, the no-op and skip-already-rotated cases, task teardown when the signatory is dropped, and that stop() halts rotation while a later start() does not resume it. Document the interval in the mintd README and example config.
Notes to the reviewers
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)