Skip to content

Commit

Permalink
chore(CON-1053): Adapt the signatures of some of functions to support…
Browse files Browse the repository at this point in the history 
… multiple keys
  • Loading branch information
@kpop-dfinity
kpop-dfinity committed Feb 21, 2024
1 parent 6111d5a commit 63384bd
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 81 deletions.
7 changes: 4 additions & 3 deletions rs/consensus/src/consensus/batch_delivery.rs
View file
Expand Up @@ -109,8 +109,9 @@ pub fn deliver_batches(

let randomness = Randomness::from(crypto_hashable_to_seed(&tape));

let ecdsa_subnet_public_key = match get_ecdsa_subnet_public_key(&block, pool, log) {
Ok(maybe_key) => maybe_key,
let ecdsa_subnet_public_keys = match get_ecdsa_subnet_public_key(&block, pool, log)
{
Ok(keys) => keys,
Err(e) => {
// Do not deliver batch if we can't find a previous summary block,
// this means we should continue with the latest CUP.
Expand Down Expand Up @@ -186,7 +187,7 @@ pub fn deliver_batches(
requires_full_state_hash,
messages: batch_messages,
randomness,
ecdsa_subnet_public_keys: ecdsa_subnet_public_key.into_iter().collect(),
ecdsa_subnet_public_keys,
ecdsa_quadruple_ids: get_quadruple_ids_to_deliver(&block),
registry_version: block.context.registry_version,
time: block.context.time,
Expand Down
14 changes: 9 additions & 5 deletions rs/consensus/src/dkg.rs
View file
Expand Up @@ -1560,17 +1560,21 @@ fn bootstrap_ecdsa_summary_from_cup_contents(
subnet_id: SubnetId,
logger: &ReplicaLogger,
) -> Result<ecdsa::Summary, String> {
let Some((key_id, dealings)) =
inspect_ecdsa_initializations(&cup_contents.ecdsa_initializations)?
else {
let initial_dealings = inspect_ecdsa_initializations(&cup_contents.ecdsa_initializations)?;
if initial_dealings.is_empty() {
return Ok(None);
};

// TODO(kpop): use all dealings
let (key_id, dealings) = initial_dealings
.first_key_value()
.expect("Should not be empty");

make_bootstrap_summary_with_initial_dealings(
subnet_id,
key_id,
key_id.clone(),
Height::new(cup_contents.height),
dealings,
dealings.clone(),
logger,
)
.map_err(|err| format!("Failed to create ECDSA summary block: {:?}", err))
Expand Down
100 changes: 53 additions & 47 deletions rs/consensus/src/ecdsa/utils.rs
View file
Expand Up @@ -308,47 +308,46 @@ pub(super) fn transcript_op_summary(op: &IDkgTranscriptOperation) -> String {
/// Return key_id and dealings.
pub(crate) fn inspect_ecdsa_initializations(
ecdsa_initializations: &[pb::EcdsaInitialization],
) -> Result<Option<(EcdsaKeyId, InitialIDkgDealings)>, String> {
if ecdsa_initializations.is_empty() {
return Ok(None);
}
) -> Result<BTreeMap<EcdsaKeyId, InitialIDkgDealings>, String> {
let mut initial_dealings_per_key_id = BTreeMap::new();

// TODO(CON-1053): remove this check
if ecdsa_initializations.len() > 1 {
return Err(
"More than one ecdsa_initialization is not supported. Choose the first one."
.to_string(),
);
}

let ecdsa_init = ecdsa_initializations
.first()
.expect("Error: Ecdsa Initialization is None");
for ecdsa_init in ecdsa_initializations {
let ecdsa_key_id = ecdsa_init
.key_id
.clone()
.expect("Error: Failed to find key_id in ecdsa_initializations")
.try_into()
.map_err(|err| {
format!(
"Error reading ECDSA key_id: {:?}. Setting ecdsa_summary to None.",
err
)
})?;

let ecdsa_key_id = ecdsa_init
.key_id
.clone()
.expect("Error: Failed to find key_id in ecdsa_initializations")
.try_into()
.map_err(|err| {
format!(
"Error reading ECDSA key_id: {:?}. Setting ecdsa_summary to None.",
err
)
})?;

let dealings = ecdsa_init
.dealings
.as_ref()
.expect("Error: Failed to find dealings in ecdsa_initializations")
.try_into()
.map_err(|err| {
format!(
"Error reading ECDSA dealings: {:?}. Setting ecdsa_summary to None.",
err
)
})?;
let dealings = ecdsa_init
.dealings
.as_ref()
.expect("Error: Failed to find dealings in ecdsa_initializations")
.try_into()
.map_err(|err| {
format!(
"Error reading ECDSA dealings: {:?}. Setting ecdsa_summary to None.",
err
)
})?;

Ok(Some((ecdsa_key_id, dealings)))
initial_dealings_per_key_id.insert(ecdsa_key_id, dealings);
}

Ok(initial_dealings_per_key_id)
}

/// Return [`EcdsaConfig`] if it is enabled for the given subnet.
Expand Down Expand Up @@ -439,7 +438,7 @@ pub(crate) fn get_quadruple_ids_to_deliver(
quadruple_ids
}

/// This function returns the ECDSA subnet public key to be added to the batch, if required.
/// This function returns the ECDSA subnet public keys to be added to the batch, if required.
/// We return `Ok(Some(key))`, if
/// - The block contains an ECDSA payload with current key transcript ref, and
/// - the corresponding transcript exists in past blocks, and
Expand All @@ -452,18 +451,9 @@ pub(crate) fn get_ecdsa_subnet_public_key(
block: &Block,
pool: &PoolReader<'_>,
log: &ReplicaLogger,
) -> Result<Option<(EcdsaKeyId, MasterEcdsaPublicKey)>, String> {
) -> Result<BTreeMap<EcdsaKeyId, MasterEcdsaPublicKey>, String> {
let Some(ecdsa_payload) = block.payload.as_ref().as_ecdsa() else {
return Ok(None);
};

let Some(transcript_ref) = ecdsa_payload
.key_transcript
.current
.as_ref()
.map(|unmasked| *unmasked.as_ref())
else {
return Ok(None);
return Ok(BTreeMap::new());
};

let Some(summary) = pool.dkg_summary_block_for_finalized_height(block.height) else {
Expand All @@ -475,6 +465,19 @@ pub(crate) fn get_ecdsa_subnet_public_key(
let chain = build_consensus_block_chain(pool.pool(), &summary, block);
let block_reader = EcdsaBlockReaderImpl::new(chain);

let mut public_keys = BTreeMap::new();

// TODO(CON-1053): add a support for multiple keys
let key_id = ecdsa_payload.key_transcript.key_id.clone();
let Some(transcript_ref) = ecdsa_payload
.key_transcript
.current
.as_ref()
.map(|unmasked| *unmasked.as_ref())
else {
return Ok(BTreeMap::new());
};

let ecdsa_subnet_public_key = match block_reader.transcript(&transcript_ref) {
Ok(transcript) => get_ecdsa_subnet_public_key_(&transcript, log),
Err(err) => {
Expand All @@ -487,8 +490,11 @@ pub(crate) fn get_ecdsa_subnet_public_key(
}
};

Ok(ecdsa_subnet_public_key
.map(|public_key| (ecdsa_payload.key_transcript.key_id.clone(), public_key)))
if let Some(public_key) = ecdsa_subnet_public_key {
public_keys.insert(key_id, public_key);
}

Ok(public_keys)
}

fn get_ecdsa_subnet_public_key_(
Expand Down Expand Up @@ -548,7 +554,7 @@ mod tests {
let init =
inspect_ecdsa_initializations(&[]).expect("Should successfully get initializations");

assert!(init.is_none());
assert!(init.is_empty());
}

#[test]
Expand All @@ -567,7 +573,7 @@ mod tests {
let init = inspect_ecdsa_initializations(&[ecdsa_init])
.expect("Should successfully get initializations");

assert_eq!(init, Some((key_id, initial_dealings)));
assert_eq!(init, BTreeMap::from([(key_id, initial_dealings)]));
}

#[test]
Expand Down
95 changes: 69 additions & 26 deletions rs/orchestrator/src/upgrade.rs
View file
Expand Up @@ -681,16 +681,40 @@ fn reexec_current_process(logger: &ReplicaLogger) -> OrchestratorError {
}

/// Return the threshold ECDSA master public key of the given CUP, if it exists.
fn get_tecdsa_key(cup: &CatchUpPackage) -> Option<(EcdsaKeyId, MasterEcdsaPublicKey)> {
let ecdsa = cup.content.block.get_value().payload.as_ref().as_ecdsa()?;
let transcript_ref = ecdsa.key_transcript.current.as_ref()?;
let transcript = ecdsa
.idkg_transcripts
.get(&transcript_ref.transcript_id())?;

get_tecdsa_master_public_key(transcript)
.ok()
.map(|key| (ecdsa.key_transcript.key_id.clone(), key))
fn get_tecdsa_keys(
cup: &CatchUpPackage,
log: &ReplicaLogger,
) -> BTreeMap<EcdsaKeyId, MasterEcdsaPublicKey> {
let mut public_keys = BTreeMap::new();

let Some(ecdsa) = cup.content.block.get_value().payload.as_ref().as_ecdsa() else {
return public_keys;
};

// TODO(CON-1053): add support for multiple keys
let key_id = ecdsa.key_transcript.key_id.clone();
let Some(transcript) = ecdsa
.key_transcript
.current
.as_ref()
.and_then(|transcript_ref| ecdsa.idkg_transcripts.get(&transcript_ref.transcript_id()))
else {
return public_keys;
};

match get_tecdsa_master_public_key(transcript) {
Ok(public_key) => {
public_keys.insert(key_id, public_key);
}
Err(err) => {
warn!(
log,
"Failed to get the tecdsa public key for key id {}: {:?}", key_id, err,
);
}
};

public_keys
}

/// Get tECDSA public keys of both CUPs and make sure previous keys weren't changed
Expand All @@ -702,25 +726,44 @@ fn compare_tecdsa_public_keys(
path: PathBuf,
log: &ReplicaLogger,
) {
let Some(old_key) = get_tecdsa_key(old_cup) else {
let old_public_keys = get_tecdsa_keys(old_cup, log);
if old_public_keys.is_empty() {
return;
};
let new_key = get_tecdsa_key(new_cup);
}

let new_public_keys = get_tecdsa_keys(new_cup, log);
let mut changes = BTreeMap::new();
// Get the metric here already, which will initialize it with zero
// even if keys haven't changed.
let metric = metrics
.ecdsa_key_changed_errors
.get_metric_with_label_values(&[&old_key.0.name])
.expect("Failed to get ECDSA key changed metric");
if Some(&old_key) != new_key.as_ref() {
error!(
log,
"Threshold ECDSA public key has changed! Old: {:?}, New: {:?}", old_key, new_key,
);
metric.inc();
changes.insert(old_key.0.name, metric.get());

for (key_id, old_public_key) in old_public_keys {
// Get the metric here already, which will initialize it with zero
// even if keys haven't changed.
let metric = metrics
.ecdsa_key_changed_errors
.get_metric_with_label_values(&[&key_id.name])
.expect("Failed to get ECDSA key changed metric");

if let Some(new_public_key) = new_public_keys.get(&key_id) {
if old_public_key != *new_public_key {
error!(
log,
"Threshold ECDSA public key for {} has changed! Old: {:?}, New: {:?}",
key_id,
old_public_key,
new_public_key,
);
metric.inc();
changes.insert(key_id.name.clone(), metric.get());
}
} else {
error!(
log,
"Threshold ECDSA public key for {} has been deleted!", key_id,
);
metric.inc();
changes.insert(key_id.name.clone(), metric.get());
}
}

// We persist the latest value of the changed metrics, such that we can re-apply them
// after the restart. As any increase in the value is enough to trigger the alert, it
// is fine to reset the metric of keys that haven't changed.
Expand Down

0 comments on commit 63384bd

Please sign in to comment.