feat: add upper bound for max epoch in zklogin sig #16233
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
joyqvq
force-pushed
the
migrate-tests-zk-multisig
branch
4 times, most recently
from
945844b
to
1e91dd8
Compare
joyqvq
force-pushed
the
max-epoch-check
branch
from
121c3be
to
e923f03
Compare
joyqvq
force-pushed
the
max-epoch-check
branch
from
e923f03
to
b32ec21
Compare
| @@ -106,7 +106,8 @@ pub trait Message { | |||
|
|
|||
| /// Verify that the message is from the correct epoch (e.g. for CertifiedCheckpointSummary | |||
| /// we verify that the checkpoint is from the same epoch as the committee signatures). | |||
| fn verify_epoch(&self, epoch: EpochId) -> SuiResult; | |||
| fn verify_epoch(&self, epoch: EpochId, upper_bound_for_max_epoch: Option<EpochId>) | |||
There was a problem hiding this comment.
Is upper_bound_for_max_epoch only used for Transaction? If so I am not convinced it belongs to the general message trait.
There was a problem hiding this comment.
theres actually no concrete type at a few places where the verify_epoch is invoked, i can only access it via the Message trait:
impl<T> Envelope<T, AuthoritySignInfo>
where
T: Message + AuthenticatedMessage + Serialize,
{
pub fn verify_signatures_authenticated(
&self,
committee: &Committee,
verify_params: &VerifyParams,
) -> SuiResult {
self.data.verify_epoch(self.auth_sig().epoch, None)?;
and
impl<T> Envelope<T, AuthoritySignInfo>
where
T: Message + UnauthenticatedMessage + Serialize,
{
pub fn verify_authority_signatures(&self, committee: &Committee) -> SuiResult {
self.data.verify_epoch(self.auth_sig().epoch, None)?;
There was a problem hiding this comment.
there are 4 concrete types for the message trait, 2 out of them actually just return Ok(()) noop, im guessing its a bit messy to remove the trait?
There was a problem hiding this comment.
I think that just means verify_epoch shouldn't be a trait API for Message. SenderSignedData and SignedCheckpointSummary should have its own function to check this.
There was a problem hiding this comment.
verify_max_epoch_for_all_sigs i added this call for SenderSignedData and this is only triggered inside verify_tx. the other other implementations like Checkpoint, TransactionEffects etc should not change its interface
signed_tx.verify_epoch(self.committee.epoch())?;
signed_tx.verify_max_epoch_for_all_sigs(
self.committee.epoch(),
self.zk_login_params.zklogin_max_epoch_upper_bound,
)?;
however it 2 iterations of the for loop instead of 1. lmk if there is better idea how to refactor this @lxfind @mystenmark
| test_cluster.wait_for_authenticator_state_update().await; | ||
| test_cluster | ||
| .wait_for_epoch_with_timeout(Some(9), Duration::from_secs(190)) |
There was a problem hiding this comment.
why do we need to wait 9 epochs in this test?
There was a problem hiding this comment.
because the zk proof is generated for max epoch = 10.
There was a problem hiding this comment.
i probably can regenerate all zkproofs against a smaller epoch to avoid timeouts esp in simtest, i will follow up a PR for that
| @@ -12,7 +12,7 @@ use tracing::{info, warn}; | |||
|
|
|||
| /// The minimum and maximum protocol versions supported by this build. | |||
| const MIN_PROTOCOL_VERSION: u64 = 1; | |||
| const MAX_PROTOCOL_VERSION: u64 = 36; | |||
| const MAX_PROTOCOL_VERSION: u64 = 37; | |||
There was a problem hiding this comment.
fyi @benr-ml is adding 37 in another PR
crates/sui-types/src/signature.rs
Outdated
| @@ -32,6 +32,7 @@ pub struct VerifyParams { | |||
| pub zk_login_env: ZkLoginEnv, | |||
| pub verify_legacy_zklogin_address: bool, | |||
| pub accept_zklogin_in_multisig: bool, | |||
| pub zklogin_max_epoch_upper_bound: Option<EpochId>, | |||
There was a problem hiding this comment.
Should this be Option<u64>? This is not the actual epoch, but just number of epochs of validity? May also want to consider renaming some of these because it's hard to tell which one is actual epoch ID and which one represents a delta (i.e. max number of valid epochs)
crates/sui-types/src/signature.rs
Outdated
| @@ -41,21 +42,27 @@ impl VerifyParams { | |||
| zk_login_env: ZkLoginEnv, | |||
| verify_legacy_zklogin_address: bool, | |||
| accept_zklogin_in_multisig: bool, | |||
| zklogin_max_epoch_upper_bound: Option<EpochId>, | |||
crates/sui-types/src/signature.rs
Outdated
| fn verify_user_authenticator_epoch( | ||
| &self, | ||
| epoch: EpochId, | ||
| upper_bound_max_epoch: Option<EpochId>, |
Description
currently a zklogin sig is valid as long as max_epoch is larger than current epoch. this adds a protocol config that the max_epoch cannot be too large, the config sets the upper bound to 30 + current epoch.
based on new testing framework in simtest only.
Test Plan
simtest
If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.
Type of Change (Check all that apply)
Release notes
Protocol version upgrades to 42. This adds a feature flag to set the upper bound of the max epoch for a zklogin signature.