Skip to content

Commit

Permalink
chore: memoize address_bytes of verification key (#1444)
Browse files Browse the repository at this point in the history
## Summary
Added a lazily-initialized field `address_bytes` to `VerificationKey`.

## Background
Testing showed that `address_bytes` was called multiple times on a given
`VerificationKey` (up to 11 times in some cases). Each time the key's
bytes were being hashed, so this change ensures that hashing only
happens once for a given verification key instance.

Note that this was implemented previously in #1111 and was then reverted
in #1124. However, when reverted, the manual impls of `PartialEq`, `Eq`,
`PartialOrd`, `Ord` and `Hash` were left as-is, as were the unit tests
for these. Hence this PR doesn't need to make any changes to these trait
impls or tests.

## Changes
- Added `address_bytes: OnceLock<[u8; ADDRESS_LEN]>` to
`VerificiationKey`.

## Testing
No new tests required.  

## Related Issues
Closes #1351.
  • Loading branch information
Fraser999 committed Sep 4, 2024
1 parent 2be574b commit e9d5f0f
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 22 deletions.
53 changes: 36 additions & 17 deletions crates/astria-core/src/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use std::{
Hash,
Hasher,
},
sync::OnceLock,
};

use base64::{
Expand Down Expand Up @@ -78,6 +79,7 @@ impl SigningKey {
pub fn verification_key(&self) -> VerificationKey {
VerificationKey {
key: self.0.verification_key(),
address_bytes: OnceLock::new(),
}
}

Expand Down Expand Up @@ -131,9 +133,13 @@ impl From<[u8; 32]> for SigningKey {
}

/// An Ed25519 verification key.
#[derive(Clone, Copy)]
#[derive(Clone)]
pub struct VerificationKey {
key: Ed25519VerificationKey,
// The address-bytes are lazily-initialized. Since it may or may not be initialized for any
// given instance of a verification key, it is excluded from `PartialEq`, `Eq`, `PartialOrd`,
// `Ord` and `Hash` impls.
address_bytes: OnceLock<[u8; ADDRESS_LEN]>,
}

impl VerificationKey {
Expand All @@ -160,31 +166,35 @@ impl VerificationKey {
/// Returns the sequencer address of this verification key.
///
/// The address is the first 20 bytes of the sha256 hash of the verification key.
// Silence the clippy lint because the function body asserts that the panic
// cannot happen.
#[allow(clippy::missing_panics_doc)]
#[must_use]
pub fn address_bytes(&self) -> [u8; ADDRESS_LEN] {
fn first_20(array: [u8; 32]) -> [u8; ADDRESS_LEN] {
[
array[0], array[1], array[2], array[3], array[4], array[5], array[6], array[7],
array[8], array[9], array[10], array[11], array[12], array[13], array[14],
array[15], array[16], array[17], array[18], array[19],
]
}
/// this ensures that `ADDRESS_LEN` is never accidentally changed to a value
/// that would violate this assumption.
#[allow(clippy::assertions_on_constants)]
const _: () = assert!(ADDRESS_LEN <= 32);
let bytes: [u8; 32] = Sha256::digest(self).into();
first_20(bytes)
*self.address_bytes.get_or_init(|| {
fn first_20(array: [u8; 32]) -> [u8; ADDRESS_LEN] {
[
array[0], array[1], array[2], array[3], array[4], array[5], array[6], array[7],
array[8], array[9], array[10], array[11], array[12], array[13], array[14],
array[15], array[16], array[17], array[18], array[19],
]
}
/// this ensures that `ADDRESS_LEN` is never accidentally changed to a value
/// that would violate this assumption.
#[allow(clippy::assertions_on_constants)]
const _: () = assert!(ADDRESS_LEN <= 32);
let bytes: [u8; 32] = Sha256::digest(self).into();
first_20(bytes)
})
}
}

impl Debug for VerificationKey {
fn fmt(&self, formatter: &mut Formatter<'_>) -> fmt::Result {
let mut debug_struct = formatter.debug_struct("VerificationKey");
debug_struct.field("key", &BASE64_STANDARD.encode(self.key.as_ref()));
if let Some(address_bytes) = self.address_bytes.get() {
debug_struct.field("address_bytes", &BASE64_STANDARD.encode(address_bytes));
} else {
debug_struct.field("address_bytes", &"unset");
}
debug_struct.finish()
}
}
Expand Down Expand Up @@ -234,6 +244,7 @@ impl TryFrom<&[u8]> for VerificationKey {
let key = Ed25519VerificationKey::try_from(slice).map_err(Error)?;
Ok(Self {
key,
address_bytes: OnceLock::new(),
})
}
}
Expand All @@ -245,6 +256,7 @@ impl TryFrom<[u8; 32]> for VerificationKey {
let key = Ed25519VerificationKey::try_from(bytes).map_err(Error)?;
Ok(Self {
key,
address_bytes: OnceLock::new(),
})
}
}
Expand Down Expand Up @@ -305,18 +317,22 @@ mod tests {
// A key which compares greater than "low" ones below, and with its address uninitialized.
let high_uninit = VerificationKey {
key: SigningKey::from([255; 32]).0.verification_key(),
address_bytes: OnceLock::new(),
};
// A key equal to `high_uninit`, but with its address initialized.
let high_init = VerificationKey {
key: high_uninit.key,
address_bytes: OnceLock::new(),
};
// A key which compares less than "high" ones above, and with its address uninitialized.
let low_uninit = VerificationKey {
key: SigningKey::from([0; 32]).0.verification_key(),
address_bytes: OnceLock::new(),
};
// A key equal to `low_uninit`, but with its address initialized.
let low_init = VerificationKey {
key: low_uninit.key,
address_bytes: OnceLock::new(),
};

assert!(high_uninit.cmp(&high_uninit) == Ordering::Equal);
Expand Down Expand Up @@ -422,12 +438,15 @@ mod tests {
// Check verification keys compare equal if and only if their keys are equal.
let key0 = VerificationKey {
key: SigningKey::from([0; 32]).0.verification_key(),
address_bytes: OnceLock::new(),
};
let other_key0 = VerificationKey {
key: SigningKey::from([0; 32]).0.verification_key(),
address_bytes: OnceLock::new(),
};
let key1 = VerificationKey {
key: SigningKey::from([1; 32]).0.verification_key(),
address_bytes: OnceLock::new(),
};

assert!(key0 == other_key0);
Expand Down
2 changes: 1 addition & 1 deletion crates/astria-sequencer/src/app/tests_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ async fn app_begin_block_remove_byzantine_validators() {
// assert that validator with pubkey_a is removed
let validator_set = app.state.get_validator_set().await.unwrap();
assert_eq!(validator_set.len(), 1);
assert_eq!(validator_set.get(verification_key(2)).unwrap().power, 1,);
assert_eq!(validator_set.get(&verification_key(2)).unwrap().power, 1,);
}

#[tokio::test]
Expand Down
8 changes: 4 additions & 4 deletions crates/astria-sequencer/src/authority/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ impl From<[u8; ADDRESS_LEN]> for ValidatorSetKey {
}
}

impl From<VerificationKey> for ValidatorSetKey {
fn from(value: VerificationKey) -> Self {
impl From<&VerificationKey> for ValidatorSetKey {
fn from(value: &VerificationKey) -> Self {
Self(value.address_bytes())
}
}
Expand All @@ -45,7 +45,7 @@ impl ValidatorSet {
Self(
updates
.into_iter()
.map(|update| (update.verification_key.into(), update))
.map(|update| ((&update.verification_key).into(), update))
.collect::<BTreeMap<_, _>>(),
)
}
Expand All @@ -59,7 +59,7 @@ impl ValidatorSet {
}

pub(super) fn push_update(&mut self, update: ValidatorUpdate) {
self.0.insert(update.verification_key.into(), update);
self.0.insert((&update.verification_key).into(), update);
}

pub(super) fn remove<T: Into<ValidatorSetKey>>(&mut self, address: T) {
Expand Down
2 changes: 2 additions & 0 deletions crates/astria-sequencer/src/benchmark_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ pub(crate) fn transactions(tx_types: TxTypes) -> &'static Vec<Arc<SignedTransact
.unwrap()
}

// allow: false-positive as described in "Known problems" of lint.
#[allow(clippy::mutable_key_type)]
fn sequence_actions() -> Vec<Arc<SignedTransaction>> {
let mut nonces_and_chain_ids = HashMap::new();
signing_keys()
Expand Down

0 comments on commit e9d5f0f

Please sign in to comment.