Signatory: serve keys from memory - #2270
Conversation
|
@cdk-bot review |
ed0e8c7 to
626e49e
Compare
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Consistent lock ordering is needed in rotate_keyset (medium) - A concurrent manual/management key rotation can permanently deadlock the signatory's in-memory keyset locks; while stuck, operations that need
keysets.read()such as signing and proof verification can also stall until restart.
Additional locations included in summary:- crates/cdk-signatory/src/db_signatory.rs:379
626e49e to
9e6a282
Compare
|
@cdk-bot review |
9e6a282 to
b0e1be8
Compare
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Embedded mint resilient boot fails before the retry path can run (high) - The embedded mint's advertised resilient boot path does not work when the initial keyset database load fails:
build_with_seedfails during construction before the background retry can recover and publish keysets.
Unanchored locations included in summary:- crates/cdk-signatory/src/db_signatory.rs:306
- crates/cdk/src/mint/builder.rs:589
|
@cdk-bot review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2270 +/- ##
==========================================
- Coverage 74.20% 74.19% -0.01%
==========================================
Files 363 363
Lines 85179 85241 +62
==========================================
+ Hits 63206 63248 +42
- Misses 21973 21993 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3cbc305 to
c6a7392
Compare
|
@cdk-bot review |
cdk-bot
left a comment
There was a problem hiding this comment.
Verified findings approved for disclosure:
- Comment still describes dropped background keyset loading (low) - Maintainers may be led to debug or code for a transient background-loading state that this PR explicitly removed; it also contradicts the ADR shipped in the same PR.
- Remove leftover unused KeysetsNotLoaded error variant (low) - This adds a public error variant and saga failure classification that are never exercised under the PR's strict-boot model, creating misleading API surface and future maintenance risk.
Additional locations included in summary:- crates/cdk-common/src/error.rs:748
c6a7392 to
214d40b
Compare
|
@cdk-bot review |
| // The signatory may not report an active keyset yet (for example a | ||
| // remote signatory that is still connecting). Start anyway and let the | ||
| // keyset drain task install keysets if they arrive over the | ||
| // subscription; endpoints return keyset errors until then. | ||
| if active_keys == 0 { |
There was a problem hiding this comment.
I still think I prefer a fail fast if the mint does not have keysets?
thesimplekid
left a comment
There was a problem hiding this comment.
Ill merge this now and move a few things to make sure we clean up in the follow up pr.
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.
214d40b to
783efcb
Compare
Description
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 signatorywithout 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 #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.
Notes to the reviewers
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)