Conversation
| UnvalidatedTransactions(Vec<TransactionId>), | ||
| #[error("failed to build block")] | ||
| BlockBuildingFailed(#[source] ProposedBlockError), | ||
| BlockBuildingFailed(#[from] ProposedBlockError), |
There was a problem hiding this comment.
| BlockBuildingFailed(#[from] ProposedBlockError), | |
| BlockBuildingFailed(#[source] ProposedBlockError), |
There was a problem hiding this comment.
We really should not use #[from]. It makes it too easy to cast things incorrectly.
There was a problem hiding this comment.
Yea sorry I have been stubborn about this. Any references you have for me to look at on that topic? Like linters/style guides. If we have consensus about not using #[from] at all I'll make sure not to in the future.
There was a problem hiding this comment.
I don't think we have anything explicitly about this; we had this general error discussion 0xMiden/protocol#966, maybe @PhilippGackstatter has one for #[from] vs `#[source]?
There was a problem hiding this comment.
I think this blog post motivates adding context well: https://fast.github.io/blog/stop-forwarding-errors-start-designing-them/. In particular the "The Problem: Error Handling Without Purpose" section:
When debugging, does “serialization error: expected , or }” tell you which request, which field, which code path led here?
#[source] is better than #[from] because it forces you to give some thought on how to best add context.
There was a problem hiding this comment.
Thanks yea the article is insightful.
#[source] is better than #[from] because it forces you to give some thought on how to best add context.
I agree with the notion that it could prompt you to add more context. But I don't think it ensures that. And I don't think it is necessary to achieve the goal of ensuring context being added when it is needed or formatting errors in such a way that they dont just forward/bubble up info.
Have updated the PR to use #[source] in any case!
| async fn sign(&self, header: &BlockHeader) -> Result<Signature, Self::Error> { | ||
| Ok(self.sign(header.commitment())) | ||
| } |
There was a problem hiding this comment.
Is this not a blocking operation? How long does signing take?
There was a problem hiding this comment.
Its async only because of the trait definition
There was a problem hiding this comment.
Yes but we shouldn't make blocking calls in an async function. Apparently we can expect this to take a few ms to maybe 100ms which is high but probably acceptable for us.
Its the downside of having this trait -- it doesn't really match reality; one call is async IO, the other is local blocking sync function.
So really this should be doing something like
tokio::task::block_in_place(self.sign(header.commitment())).awaitbut that isn't great either.
crates/validator/src/signers/kms.rs
Outdated
| type Error = KmsSignerError; | ||
|
|
||
| async fn sign(&self, header: &BlockHeader) -> Result<Signature, Self::Error> { | ||
| // KMS backend doesn't support Keccak-256 so we do it ourselves. |
There was a problem hiding this comment.
Could you expand on what this means?
There was a problem hiding this comment.
Updated - anything still unclear?
// AWS KMS does not support SHA3 (Keccak-256), so we need to produce the digest ourselves.There was a problem hiding this comment.
Yes - I don't know how to produce such a digest. Is there an algorithm you followed, or how did you know what to do?
Currently I'm just taking it as being okay; but you could really have written any algorithm here and I wouldn't know any different.
There was a problem hiding this comment.
aka how can I audit this code to ensure its correct.
There was a problem hiding this comment.
How about now?
// The Validator produces Ethereum-style ECDSA (secp256k1) signatures over Keccak-256
// digests. AWS KMS does not support SHA-3 hashing for ECDSA keys
// (ECC_SECG_P256K1 being the corresponding AWS key-spec), so we pre-hash the
// message and pass MessageType::Digest. KMS signs the provided 32-byte digest
// verbatim.| #[group(required = true, multiple = false)] | ||
| pub struct ValidatorKey { |
There was a problem hiding this comment.
Have you tested that this works as expected? The default with an optional and group required sounds like it might be wonky. I think its correctly defined, I'm just curious whether clap handles this properly.
There was a problem hiding this comment.
❮ ./target/debug/miden-node validator start --data-directory /tmp/1 http://0.0.0.0:9191 --key.kms-id bded4b64-636a-4d27-8475-c2b8f8e76e75 --key.hex asdf
error: the argument '--key.kms-id <VALIDATOR_KMS_KEY_ID>' cannot be used with '--key.hex <VALIDATOR_KEY>'
❯ ./target/debug/miden-node bundled bootstrap --validator.key.hex asdb --validator.key.kms-id asdf
error: the argument '--validator.key.hex <VALIDATOR_KEY>' cannot be used with '--validator.key.kms-id <VALIDATOR_KMS_KEY_ID>'
| /// Cannot be used with `key.kms-id`. | ||
| #[arg( | ||
| long = "key.hex", | ||
| env = ENV_VALIDATOR_KEY, | ||
| value_name = "VALIDATOR_KEY", | ||
| default_value = INSECURE_VALIDATOR_KEY_HEX, | ||
| group = "key" | ||
| )] |
There was a problem hiding this comment.
Ah. And you have to repeat because its a different arg prefix?
We should take a look at conf which builds on top of clap-derive but seems less weird around use cases like this.
But for now this is fine 👍
| UnvalidatedTransactions(Vec<TransactionId>), | ||
| #[error("failed to build block")] | ||
| BlockBuildingFailed(#[source] ProposedBlockError), | ||
| BlockBuildingFailed(#[from] ProposedBlockError), |
There was a problem hiding this comment.
I don't think we have anything explicitly about this; we had this general error discussion 0xMiden/protocol#966, maybe @PhilippGackstatter has one for #[from] vs `#[source]?
crates/validator/src/signers/kms.rs
Outdated
| type Error = KmsSignerError; | ||
|
|
||
| async fn sign(&self, header: &BlockHeader) -> Result<Signature, Self::Error> { | ||
| // KMS backend doesn't support Keccak-256 so we do it ourselves. |
There was a problem hiding this comment.
Yes - I don't know how to produce such a digest. Is there an algorithm you followed, or how did you know what to do?
Currently I'm just taking it as being okay; but you could really have written any algorithm here and I wouldn't know any different.
crates/validator/src/signers/kms.rs
Outdated
| type Error = KmsSignerError; | ||
|
|
||
| async fn sign(&self, header: &BlockHeader) -> Result<Signature, Self::Error> { | ||
| // KMS backend doesn't support Keccak-256 so we do it ourselves. |
There was a problem hiding this comment.
aka how can I audit this code to ensure its correct.
| async fn sign(&self, header: &BlockHeader) -> Result<Signature, Self::Error> { | ||
| Ok(self.sign(header.commitment())) | ||
| } |
There was a problem hiding this comment.
Yes but we shouldn't make blocking calls in an async function. Apparently we can expect this to take a few ms to maybe 100ms which is high but probably acceptable for us.
Its the downside of having this trait -- it doesn't really match reality; one call is async IO, the other is local blocking sync function.
So really this should be doing something like
tokio::task::block_in_place(self.sign(header.commitment())).awaitbut that isn't great either.
| let signer = validator_key.into_signer().await?; | ||
| match signer { | ||
| ValidatorSigner::Kms(signer) => { | ||
| Self::bootstrap_with_signer(config, signer, accounts_directory, data_directory) | ||
| .await | ||
| }, | ||
| ValidatorSigner::Local(signer) => { | ||
| Self::bootstrap_with_signer(config, signer, accounts_directory, data_directory) | ||
| .await | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| async fn bootstrap_with_signer( | ||
| config: GenesisConfig, | ||
| signer: impl BlockSigner, |
There was a problem hiding this comment.
Maybe &dyn would make it easier?
| let signer = validator_key.into_signer().await?; | |
| match signer { | |
| ValidatorSigner::Kms(signer) => { | |
| Self::bootstrap_with_signer(config, signer, accounts_directory, data_directory) | |
| .await | |
| }, | |
| ValidatorSigner::Local(signer) => { | |
| Self::bootstrap_with_signer(config, signer, accounts_directory, data_directory) | |
| .await | |
| }, | |
| } | |
| } | |
| async fn bootstrap_with_signer( | |
| config: GenesisConfig, | |
| signer: impl BlockSigner, | |
| let signer = validator_key.into_signer().await?; | |
| Self::bootstrap_with_signer(config, &signer, accounts_directory, data_directory).await | |
| } | |
| async fn bootstrap_with_signer( | |
| config: GenesisConfig, | |
| signer: &dyn BlockSigner, |
There was a problem hiding this comment.
Unfortunately the associated (Error) type is problematic for trait object
the value of the associated type `Error` in `miden_node_utils::signer::BlockSigner` must be specified
We can look at removing the trait instead after this PR maybe.
34a4837 to
d35c8e4
Compare
Context
The genesis bootstrap and validator processes currently only support ECDSA private keys via command line. We want to be able to perform genesis and validator block signing through more secure, remote backends such as AWS KMS.
Relates to #1316.
Tested locally against AWS KMS (both bootstrap and chain running with separate validator).
Changes
BlockSignertrait (removed from miden-base).KmsSignerstruct which implementsBlockSigner.