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

feat: [EXC-1768] Add system API to get costs of management canister calls. #3584

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

michael-weigelt
Copy link
Contributor

@michael-weigelt michael-weigelt commented Jan 23, 2025

Design Doc

Spec PR

This is the base of a stack of PRs. Before merging this one, I'll merge the stack into this after each part has been reviewed.

@github-actions github-actions bot added the feat label Jan 23, 2025
.get_subnet_size(&subnet_id.into())
.map(|x| x as u32)
.ok_or(HypervisorError::SubnetNotFound)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to trace these new syscalls

Comment on lines +1240 to +1248
/// This system call indicates the cycle cost of signing with schnorr,
/// i.e., the management canister's `sign_with_schnorr` for the key
/// with name given by `src` + `size`.
///
/// Traps if `src`/`size` cannot be decoded to a valid key name.
///
/// The amount of cycles is represented by a 128-bit value and is copied
/// to the canister memory starting at the location `dst`.
fn ic0_cost_schnorr(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of the key name, should the argument be the key ID (as in sign_with_schnorr and schnorr_public_key)? Since we currently support two different schnorr algorithms, there are also two "Schnorr" keys with the same name. You could imagine that they may be deployed to two different subnets with different replication factors, or perhaps that the Ed25519 variant could be more expensive/cheaper than Bip340. IIUC the current implementation would return the same cost for both keys.

To be future proof, I would say the same argument applies for ECDSA and VetKD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I checked for Ecdsa, but did not take another closer look at Schnorr.
Yes, in this case, we need to disambiguate keys of the same name even within a given signing scheme.

I tried to avoid encoding KeyId, because initially most of the information therein seemed redundant for this endpoint. But it looks like I have to use it in some form. Would the (scheme, key_name, curve)-tuple be unique, conceptually? Or are you implying that 'algorithm' and 'curve' are also distinct choices?

Copy link
Contributor

Choose a reason for hiding this comment

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

Schnorr key IDs have an "algorithm" variant, whereas ECDSA and VetKD use "curve". So assuming the scheme is derived from the function name I think the tuples (key_name, curve) for VetKD and ECDSA, and (key_name, algorithm) for Schnorr should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll ping you when I have the next iteration ready.

heap: &mut [u8],
) -> HypervisorResult<()>;

/// This system call indicates the cycle cost of signing with ecdsa,
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
/// This system call indicates the cycle cost of signing with ecdsa,
/// This system call indicates the cycle cost of threshold key derivation,

) -> HypervisorResult<()>;

/// This system call indicates the cycle cost of signing with ecdsa,
/// i.e., the management canister's `vetkd_encrypted_key` for the key
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
/// i.e., the management canister's `vetkd_encrypted_key` for the key
/// i.e., the management canister's `vetkd_derive_encrypted_key` for the key

///
/// The amount of cycles is represented by a 128-bit value and is copied
/// to the canister memory starting at the location `dst`.
fn ic0_cost_vetkey(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be cleaner if the name was ic0_cost_vetkd_derive_encrypted_key?

Also ic0_cost_sign_with_ecdsa and ic0_cost_sign_with_schnorr above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants