-
Notifications
You must be signed in to change notification settings - Fork 55
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
base: main
Are you sure you want to change the base?
Conversation
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()?; | ||
} | ||
} |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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).
45d8352
to
a11988b
Compare
a11988b
to
a8c289e
Compare
@thesimplekid @callebtc Does it look better? Basically so far I have:
If this makes sense, |
There was a problem hiding this 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> { |
There was a problem hiding this comment.
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.
crates/cdk-signatory/Cargo.toml
Outdated
tonic = "0.12.3" | ||
prost = "0.13.4" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 ?
b8e13e4
to
77ead1b
Compare
6178232
to
5c71573
Compare
c9197b3
to
7fe0c8e
Compare
55713c6
to
e06ec34
Compare
06d0573
to
1fe6b06
Compare
@@ -0,0 +1,4 @@ | |||
fn main() { | |||
#[cfg(feature = "grpc")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[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.
There was a problem hiding this comment.
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.
0b1263f
to
9a5878b
Compare
There was a problem hiding this comment.
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?
serde = { version = "1.0.210", features = ["derive"] } | ||
uuid = { version = "=1.12.1", features = ["v4", "serde"] } |
There was a problem hiding this comment.
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
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", | ||
] } |
There was a problem hiding this comment.
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())?; |
There was a problem hiding this comment.
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.
/// 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(()) | ||
} |
There was a problem hiding this comment.
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
cdk/crates/cdk-mint-rpc/src/proto/server.rs
Lines 50 to 120 in e1458b0
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(); | |
} | |
} | |
# 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 |
There was a problem hiding this comment.
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"] |
There was a problem hiding this comment.
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?
/// 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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"))] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
crates/cdk/src/mint/config.rs
Outdated
There was a problem hiding this comment.
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.
9a5878b
to
18f7297
Compare
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-check
before committing