Introduce a SignatoryManager service. #509
No reviewers
Labels
No labels
DB & Storage
Deployment
Error Handling & Logging
Maintenance
Payment Backend
backport
backport v0.13.x
backport v0.14.x
backport v0.15.x
bindings
blocked
bug
cdk-sql
ci
cli
deps
documentation
duplicate
enhancement
good first issue
help wanted
invalid
keep-open
ldk-node-ui
migrations
mint
mutation-testing
needs rebase
needs review
new nut
nut change
question
ready
rust-version
rustfmt
stacked hold
stale
testing
wallet
weekly-report
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
cashubtc/cdk!509
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "proto/independent-signatory"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
The SignatoryManager manager provides an API to interact with keysets, private keys, and all key-related operations, offering segregation between the mint and the most sensible part of the mind: the private keys.
Although the default signatory runs in memory, it is completely isolated from the rest of the system and can only be communicated through the interface offered by the signatory manager. Only messages can be sent from the mintd to the Signatory trait through the Signatory Manager.
This pull request sets the foundation for eventually being able to run the Signatory and all the key-related operations in a separate service, possibly in a foreign service, to offload risks, as described in #476.
The Signatory manager is concurrent and deferred any mechanism needed to handle concurrency to the Signatory trait.
TODO
Notes to the reviewers
Maybe a new terminology, other than Signatory and SignatoryManager can be used, but the idea I believe is solid.
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just final-checkbefore committingFunctionality like this should happen in the core mint instead of here. The message should be sent to the blind signer only if all checks are successful, including scripts etc.
Possibly out of scope but it would be nice if this parameter would accept a slice of integers (all possible amounts) instead of a single exponent (max order).
@thesimplekid @callebtc Does it look better?
Basically so far I have:
If this makes sense,
I'll do the preparations (like preparinghttps://github.com/cashubtc/cdk/pull/550, only rebasing is missing if it makes sense.3), fix merging issues, and add some comments.3was only required if the signatory binary is not in the same crate asmintd, but it makes sense to perhaps remove it for now and revisit it laterYou mean crate right?
This is a really good start and a structure that makes sense. I think there are still some open questions around what should be handled by the signatory and what by the mint. For example the proof verification I don't think the checking of p2pk and htlc conditions should be done by the signatory and it should only handle stuff that actually required the mint private keys. However, we do want to provide enough information that the signatory can be more then just a blind signer, doing ie rate limiting and we don't have a strong picture of what that will look like yet.
What is check ln?
@ -0,0 +1,40 @@[package]These break MSRV, I'll figure that out on the management-rpc branch and report back
@ -0,0 +1,21 @@//! In memory signatoryI don't think the full verification of the proof should be in the signatory only the verification of the signature.
@ -0,0 +1,40 @@[package]I'll fix this, making this optional (only to compile the binary)
I'll rename it better; it is checked if the configuration has lightning configuration. I don't need that. I think I'll create another function instead of altering that function.
@ -0,0 +1,40 @@[package]I this fixed now @thesimplekid ?
@ -0,0 +1,6 @@fn main() {println!("cargo:rerun-if-changed=src/proto/signatory.proto");#[cfg(feature = "grpc")]This will make sure
cargoonly recompiles when theprotofile changes.@ -0,0 +1,6 @@fn main() {println!("cargo:rerun-if-changed=src/proto/signatory.proto");#[cfg(feature = "grpc")]Thank you! I'll add it shortly, I'm rebasing first.
can we week serde at 1? I know we changed uuid bc of the wasm build issue
these should all be version
0.7.1@ -0,0 +1,21 @@//! In memory signatorymaybe instead of a
verify_prooffn we have a verify signature fn to make it clear we only verify the signature on the proof.@ -0,0 +1,222 @@use std::net::SocketAddr;What do you think if implementing start and stop on
MwmorySignatoryinstead of this fngithub.com/cashubtc/cdk@e1458b07a8/crates/cdk-mint-rpc/src/proto/server.rs (L50-L120)We still want to commit to MSRV with grpc enabled. The required dep version on used the
cdk-mint-rpc. I think you are using them maybe we just remove the comment.This should be moved up under features?
@ -71,1 +75,4 @@/// Set signatory servicepub fn with_signatory(mut self, signatory: Arc<dyn Signatory + Sync + Send + 'static>) -> Self {self.signatory = Some(signatory);Why do we need a
with_signatoryandwith_remote_signatorycan't we just usewith_signatoryand its something that implements the trait and we don't care what?Do we need a grpc feature? If the signatory is just something that impl the
Signatorytrait does cdk need to know if its rpc or not?@ -71,1 +75,4 @@/// Set signatory servicepub fn with_signatory(mut self, signatory: Arc<dyn Signatory + Sync + Send + 'static>) -> Self {self.signatory = Some(signatory);Good point, I'll refactor this.
It was all due the MSRV, I'll revisit before having this PR ready for review
After addressing the MSRV, (as you described in https://github.com/cashubtc/cdk/pull/509#discussion_r1956554070) I will drop the GRPC feature
@ -0,0 +1,21 @@//! In memory signatoryShould the functions also take a batch of messages/proofs to limit the amount of network calls?
possibly closes https://github.com/cashubtc/cdk/issues/614
Really nice work
@ -0,0 +57,4 @@}#[tracing::instrument(skip_all)]async fn runner(@ -0,0 +60,4 @@async fn runner(mut receiver: mpsc::Receiver<Request>,handler: Arc<dyn Signatory + Send + Sync>,) {What is the advantage to having this runner and then using the channels to pass the request? Wouldn't another option be to having the signatory on the service?
@ -0,0 +99,4 @@}#[tracing::instrument(skip_all)]async fn blind_sign(Can we add tracing::instrument to these?
@ -0,0 +1,158 @@use std::path::Path;@ -0,0 +1,222 @@use std::net::SocketAddr;@ -0,0 +20,4 @@T: Signatory + Send + Sync + 'static,{#[tracing::instrument(skip_all)]async fn blind_sign(Can we add tracing::instrument to these?
@ -0,0 +1,31 @@CREATE TABLE proof_new (y BLOB PRIMARY KEY,amount INTEGER NOT NULL,keyset_id TEXT NOT NULL, -- no FK constraint hereWhy can't we use the FK?
@ -61,3 +67,4 @@cdk-signatory = { workspace = true, default-features = false }getrandom = { version = "0.2", features = ["js"] }[[example]]Should the grpc feature be optional here by adding a feature to cdk?
@ -64,0 +29,4 @@pub fn auth_keysets(&self) -> KeysetResponse {KeysetResponse {keysets: self.keysetsDo we know that the signatory will return only active and only one keyset?
@ -48,6 +51,11 @@ pub use verification::Verification;/// Cashu Mint#[derive(Clone)]pub struct Mint {@ -396,82 +328,79 @@ impl Mint {Ok(fee)}Since we just call to_owned on this later should we just make this a vec?
@ -0,0 +60,4 @@async fn runner(mut receiver: mpsc::Receiver<Request>,handler: Arc<dyn Signatory + Send + Sync>,) {This alternative runs the signatory on a separate thread, passing messages. Otherwise, it would be a direct function call. It is a minor improvement that can be dropped.
Are there any tests you added or think we can added to better cover or do you thing its well covered? Thinking mainly around rotation and generation.
@ -0,0 +1,31 @@CREATE TABLE proof_new (y BLOB PRIMARY KEY,amount INTEGER NOT NULL,keyset_id TEXT NOT NULL, -- no FK constraint hereBecause the database no longer runs in the same file. Remember the signatory runs and owns their own file, potentially on another machine. Even in the same machine, it is sub-efficient to use the same SQLite.
@ -61,3 +67,4 @@cdk-signatory = { workspace = true, default-features = false }getrandom = { version = "0.2", features = ["js"] }[[example]]Potentially, but it should be on by default and only disabled whenever it is not possible (wasm). It would enable the gRPC client in the cdk.
@ -64,0 +29,4 @@pub fn auth_keysets(&self) -> KeysetResponse {KeysetResponse {keysets: self.keysetsNo, it will return all keys. That's why the CDK will cache it. I will optimise the CDK usage by keeping the active keys in a separate struct.
Most CDK functions are now synchronous though, since the keys is already part of memory and replaceable as well.
@ -0,0 +60,4 @@async fn runner(mut receiver: mpsc::Receiver<Request>,handler: Arc<dyn Signatory + Send + Sync>,) {Thats fine keep it as it is.
I think the tests are good enough. The only bit that I removed was knowing the upcoming private key (for instance the test
test_mint_keyset_gen), since that is unknown to the CDK.Did you remove it or add it to the signatory?
@ -0,0 +1,173 @@syntax = "proto3";Should this be changed to match the proposed spec? https://github.com/cashubtc/nuts/pull/250
@ -0,0 +8,4 @@use crate::signatory::{RotateKeyArguments, Signatory, SignatoryKeySet, SignatoryKeysets};/// A client for the Signatory service.pub struct SignatoryRpcClient {Should we allow callers to include a gRPC
Metadatastruct that is included in each request? This can enable a signatory to handle multiple mints.@ -0,0 +1,173 @@syntax = "proto3";It is already up to date
Already fixed
This will be done in #762
@ -0,0 +1,21 @@//! In memory signatoryAlready done
@ -0,0 +1,21 @@//! In memory signatoryI think this conversation should be moved to https://github.com/cashubtc/nuts/pull/250
@ -0,0 +99,4 @@}#[tracing::instrument(skip_all)]async fn blind_sign(Done in bb6a154d
Looks good just noted a few things.
Have you tested a mint upgrade. Starting a mint on the last version 0.9.2. We should expect that once upgraded the keysets are not changed and proofs generated on past versions can still be verified?
On that note on an upgrade there isn't anything that an operator needs to do in order to run the db_signatory correct?
@ -0,0 +1,40 @@[package]Since we import cdk-common we shouldn't need cashu as cdk-common re-exports everything from cashu.
@ -0,0 +34,4 @@[target.'cfg(target_arch = "wasm32")'.dependencies]tokio = { workspace = true, features = ["rt", "macros", "sync", "time"] }getrandom = { version = "0.2", features = ["js"] }newest is 0.3 is there a reason we can update?
@ -0,0 +1,15 @@#[cfg(not(target_arch = "wasm32"))]Can we refactor this to not have the cli mod. https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#sub-modules
@ -0,0 +1,345 @@//! Main Signatory implementationShould do this TODO now?
@ -396,82 +328,79 @@ impl Mint {Ok(fee)Since we call
to_owndedon this later should we just change the fn def@ -0,0 +1,345 @@//! Main Signatory implementationOn it right now
@ -396,82 +328,79 @@ impl Mint {Ok(fee)I'll make it happen
@ -0,0 +34,4 @@[target.'cfg(target_arch = "wasm32")'.dependencies]tokio = { workspace = true, features = ["rt", "macros", "sync", "time"] }getrandom = { version = "0.2", features = ["js"] }I just did it, let's see if the CI likes it
@ -0,0 +32,4 @@// Represents a blinded messagemessage BlindedMessage {uint64 amount = 1;string keyset_id = 2;Should this be bytes?
@ -0,0 +32,4 @@// Represents a blinded messagemessage BlindedMessage {uint64 amount = 1;string keyset_id = 2;Can we fix in the NUT? Perhaps we can merge this as is, and fix it in the NUT then here in a follow-up PR.
@ -0,0 +34,4 @@[target.'cfg(target_arch = "wasm32")'.dependencies]tokio = { workspace = true, features = ["rt", "macros", "sync", "time"] }getrandom = { version = "0.2", features = ["js"] }Did this get reverted in a force push by accident I still see 0.2?
@ -44,1 +44,4 @@# -Z minimal-versionssync_wrapper = "0.1.2"bech32 = "0.9.1"We can move arc-swap to the normal deps not under z minimal
I think this is good to merge if we just add the test. We can resolve the other comments in a follow up.
@ -854,3 +679,4 @@assert_eq!(2, keysets.keysets.len());for keyset in &keysets.keysets {if keyset.id == first_keyset_id {assert!(!keyset.active);Offline we talked about a test to ensure the signatory generates the same keyset as the current logic. This seems to be the test we want but its removed or is moved? I tested it manually with cdk-mintd and it is correct would be good to lock it in with a test.
@ -854,3 +679,4 @@assert_eq!(2, keysets.keysets.len());for keyset in &keysets.keysets {if keyset.id == first_keyset_id {assert!(!keyset.active);I'm going to make sure it is added back.
@ -0,0 +34,4 @@[target.'cfg(target_arch = "wasm32")'.dependencies]tokio = { workspace = true, features = ["rt", "macros", "sync", "time"] }getrandom = { version = "0.2", features = ["js"] }On main ci seems okay with 0.3 is there something specific to the signatory that there is an error with 0.3?
@ -44,1 +44,4 @@# -Z minimal-versionssync_wrapper = "0.1.2"bech32 = "0.9.1"Sure
@ -0,0 +34,4 @@[target.'cfg(target_arch = "wasm32")'.dependencies]tokio = { workspace = true, features = ["rt", "macros", "sync", "time"] }getrandom = { version = "0.2", features = ["js"] }I'll rebase and check
@ -0,0 +32,4 @@// Represents a blinded messagemessage BlindedMessage {uint64 amount = 1;string keyset_id = 2;We can do this in a follow up pr.
@ -854,3 +679,4 @@assert_eq!(2, keysets.keysets.len());for keyset in &keysets.keysets {if keyset.id == first_keyset_id {assert!(!keyset.active);Fixed in af3f5859 /cc @thesimplekid
ACK LGTM af3f585960f7caa42b78ff0d140db521ade9004a