Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a SignatoryManager service. #509

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

crodas
Copy link
Contributor

@crodas crodas commented Dec 20, 2024

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

  • Add standalone binary to spawn a GRPC server inside cdk-signatory
  • Add GRPC client on cdk
  • Add tests
  • Add cdk-signatory to the pipeline

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

Comment on lines 242 to 258
if let Ok(secret) =
<&crate::secret::Secret as TryInto<crate::nuts::nut10::Secret>>::try_into(&proof.secret)
{
// Checks and verifes known secret kinds.
// If it is an unknown secret kind it will be treated as a normal secret.
// Spending conditions will **not** be check. It is up to the wallet to ensure
// only supported secret kinds are used as there is no way for the mint to
// enforce only signing supported secrets as they are blinded at
// that point.
match secret.kind {
Kind::P2PK => {
proof.verify_p2pk()?;
}
Kind::HTLC => {
proof.verify_htlc()?;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality 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.

derivation_path,
Some(0),
unit.clone(),
max_order,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@crodas
Copy link
Contributor Author

crodas commented Jan 21, 2025

@thesimplekid @callebtc Does it look better?

Basically so far I have:

  1. Move the signatory to their own repo. The same code can be embedded or run in a its own binary and connect through an URL
  2. Regardless of embedded or remote, the signatory works in their own tokio thread, and they communicate with the mintd through messages
  3. A lot of code that can be merged before in its own PR, which aims to have fewer dependency and reduce the duplicate code, such as the Database instantiation

If this makes sense, I'll do the preparations (like preparing 3), fix merging issues, and add some comments. 3 was only required if the signatory binary is not in the same crate as mintd, but it makes sense to perhaps remove it for now and revisit it later #550, only rebasing is missing if it makes sense.

@crodas crodas mentioned this pull request Jan 21, 2025
2 tasks
Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the signatory to their own repo.

You 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.

Ok(blinded_signature)
}

async fn verify_proof(&self, proof: Proof) -> Result<(), Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the full verification of the proof should be in the signatory only the verification of the signature.

Comment on lines 24 to 25
tonic = "0.12.3"
prost = "0.13.4"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These break MSRV, I'll figure that out on the management-rpc branch and report back

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix this, making this optional (only to compile the binary)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I this fixed now @thesimplekid ?

@crodas crodas force-pushed the proto/independent-signatory branch from b8e13e4 to 77ead1b Compare January 21, 2025 16:23
@thesimplekid thesimplekid added enhancement New feature or request mint labels Jan 22, 2025
@crodas crodas force-pushed the proto/independent-signatory branch 2 times, most recently from 6178232 to 5c71573 Compare January 23, 2025 16:13
@crodas crodas force-pushed the proto/independent-signatory branch 4 times, most recently from c9197b3 to 7fe0c8e Compare February 2, 2025 23:40
@crodas crodas force-pushed the proto/independent-signatory branch 10 times, most recently from 55713c6 to e06ec34 Compare February 5, 2025 02:04
@crodas crodas force-pushed the proto/independent-signatory branch 7 times, most recently from 06d0573 to 1fe6b06 Compare February 5, 2025 23:55
@@ -0,0 +1,4 @@
fn main() {
#[cfg(feature = "grpc")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[cfg(feature = "grpc")]
println!("cargo:rerun-if-changed=src/proto/signatory.proto");
#[cfg(feature = "grpc")]

This will make sure cargo only recompiles when the proto file changes.

Copy link
Contributor Author

@crodas crodas Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll add it shortly, I'm rebasing first.

@crodas crodas force-pushed the proto/independent-signatory branch 5 times, most recently from 0b1263f to 9a5878b Compare February 13, 2025 19:44
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about this being a binary within the cdk-signatory crate?

Comment on lines +31 to +32
serde = { version = "1.0.210", features = ["derive"] }
uuid = { version = "=1.12.1", features = ["v4", "serde"] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we week serde at 1? I know we changed uuid bc of the wasm build issue

Comment on lines +29 to +39
cdk-cln = { path = "../cdk-cln", version = "0.7.0", default-features = false }
cdk-lnbits = { path = "../cdk-lnbits", version = "0.7.0", default-features = false }
cdk-phoenixd = { path = "../cdk-phoenixd", version = "0.7.0", default-features = false }
cdk-lnd = { path = "../cdk-lnd", version = "0.7.0", default-features = false }
cdk-fake-wallet = { path = "../cdk-fake-wallet", version = "0.7.0", default-features = false }
cdk-strike = { path = "../cdk-strike", version = "0.7.0" }
cdk-axum = { path = "../cdk-axum", version = "0.7.0", default-features = false }
cdk-mint-rpc = { path = "../cdk-mint-rpc", version = "0.7.0", default-features = false, optional = true }
cdk-signatory = { path = "../cdk-signatory", default-features = false, features = [
"grpc",
] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should all be version 0.7.1

.get_keypair_for_amount(&proof.keyset_id, &proof.amount)
.await?;

verify_message(&key_pair.secret_key, proof.c, proof.secret.as_bytes())?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe instead of a verify_proof fn we have a verify signature fn to make it clear we only verify the signature on the proof.

Comment on lines +45 to +55
/// Runs the signatory server
pub async fn grpc_server(signatory: MemorySignatory, addr: SocketAddr) -> Result<(), Error> {
tracing::info!("grpc_server listening on {}", addr);
Server::builder()
.add_service(signatory_server::SignatoryServer::new(CdkSignatory(
signatory,
)))
.serve(addr)
.await?;
Ok(())
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think if implementing start and stop on MwmorySignatory instead of this fn

impl MintRPCServer {
pub fn new(addr: &str, port: u16, mint: Arc<Mint>) -> Result<Self, Error> {
Ok(Self {
socket_addr: format!("{addr}:{port}").parse()?,
mint,
shutdown: Arc::new(Notify::new()),
handle: None,
})
}
pub async fn start(&mut self, tls_dir: Option<PathBuf>) -> Result<(), Error> {
tracing::info!("Starting RPC server {}", self.socket_addr);
let server = match tls_dir {
Some(tls_dir) => {
tracing::info!("TLS configuration found, starting secure server");
let cert = std::fs::read_to_string(tls_dir.join("server.pem"))?;
let key = std::fs::read_to_string(tls_dir.join("server.key"))?;
let client_ca_cert = std::fs::read_to_string(tls_dir.join("ca.pem"))?;
let client_ca_cert = Certificate::from_pem(client_ca_cert);
let server_identity = Identity::from_pem(cert, key);
let tls_config = ServerTlsConfig::new()
.identity(server_identity)
.client_ca_root(client_ca_cert);
Server::builder()
.tls_config(tls_config)?
.add_service(CdkMintServer::new(self.clone()))
}
None => {
tracing::warn!("No valid TLS configuration found, starting insecure server");
Server::builder().add_service(CdkMintServer::new(self.clone()))
}
};
let shutdown = self.shutdown.clone();
let addr = self.socket_addr;
self.handle = Some(Arc::new(tokio::spawn(async move {
let server = server.serve_with_shutdown(addr, async {
shutdown.notified().await;
});
server.await?;
Ok(())
})));
Ok(())
}
pub async fn stop(&self) -> Result<(), Error> {
self.shutdown.notify_one();
if let Some(handle) = &self.handle {
while !handle.is_finished() {
tracing::info!("Waitning for mint rpc server to stop");
tokio::time::sleep(Duration::from_millis(100)).await;
}
}
tracing::info!("Mint rpc server stopped");
Ok(())
}
}
impl Drop for MintRPCServer {
fn drop(&mut self) {
tracing::debug!("Dropping mint rpc server");
self.shutdown.notify_one();
}
}

Comment on lines +17 to +19
# We do not commit to a MSRV with grpc enabled
grpc = ["mint", "cdk-signatory/grpc"]
# We do not commit to a MSRV with grpc enabled
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -105,3 +109,4 @@ criterion = "0.5.1"
[[bench]]
name = "dhke_benchmarks"
harness = false
default = ["mint", "wallet"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved up under features?

Comment on lines +87 to +89
/// connect to a remote signatary instead of a creating a local one
pub fn with_remote_signatory(mut self, url: String) -> Self {
self.signatory_info = Some(SignatoryInfo::Remote(url));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a with_signatory and with_remote_signatory can't we just use with_signatory and its something that implements the trait and we don't care what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll refactor this.

.map_err(|e| anyhow!("Remote signatory error: {}", e.to_string()))?,
)
as Arc<dyn Signatory + Sync + Send + 'static>,
#[cfg(not(feature = "grpc"))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a grpc feature? If the signatory is just something that impl the Signatory trait does cdk need to know if its rpc or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was all due the MSRV, I'll revisit before having this PR ready for review

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After addressing the MSRV, (as you described in #509 (comment)) I will drop the GRPC feature

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this just got put back in on a rebase we shouldn't need it now that this info is stored in db.

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 cashubtc#476.

The Signatory manager is concurrent and deferred any mechanism needed to handle
concurrency to the Signatory trait.
Since the SignatoryManager spawns a worker (with `tokio::spawn`) to execute the
Signatory (which implements the `Signatory` trait), a mpsc channel is used to
exchange messages.

In side Tokio tests, each test has its own Tokio runtime. When the first test
is done, since `instantiate` shares all the Mint instances, their Tokio runtime
is dropped, and the tokio::spawn's task gets killed, breaking the channel.

Since the SignatoryManager has no way to respawn, understandably so, since the
task runs in an infinite loop, receiving messages and spawning new tasks to
non-blocking. There are two choices: either create one Mint per test (which is
what this PR does) or compile a modified version of the SignatoryManager for
tests, which performs the calls to the Signatory trait in place instead of
converting it to a message and passing to the signatory Tokio task.

The first option is more straightforward and fixes other instances of the code
where `tokio::spawn` is used, such as subscriptions.

A third option could be explored, which would imply spawning a Tokio runtime in
another OS-level thread, creating the Mint in that context, and sharing it with
everything else; perhaps this should explored further if the idea is to test
Mint this way.
@crodas crodas force-pushed the proto/independent-signatory branch from 9a5878b to 18f7297 Compare February 21, 2025 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants