Skip to content

Commit

Permalink
chore: CON-1434 Increment metric in case of NiDkgTag mismatches (#3329
Browse files Browse the repository at this point in the history
)

With #3039 we will start to serialize
maps of the form `(nidkg_tag, nidkg_transcript)` as a vector of
`nidkg_transcript`s. This is fine since the NiDkg tag is also part of
the NiDkg transcript. More generally, the change makes the assumption
`nidkg_tag == nidkg_transcript.tag` explicit.

As an extra pre-caution, this PR adds a log and metric to increase our
confidence that this assumption already holds today (and thus that the
change in #3039 is safe to roll out).
  • Loading branch information
eichhorl authored Jan 7, 2025
1 parent 0cd1ac6 commit 67cd0a7
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 0 deletions.
1 change: 1 addition & 0 deletions rs/consensus/src/consensus/validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1313,6 +1313,7 @@ impl Validator {
self.state_manager.as_ref(),
&proposal.context,
&self.metrics.dkg_validator,
&self.log,
)
.map_err(|err| {
err.map(
Expand Down
28 changes: 28 additions & 0 deletions rs/consensus/src/dkg/payload_validator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use ic_interfaces::{
};
use ic_interfaces_registry::RegistryClient;
use ic_interfaces_state_manager::StateManager;
use ic_logger::{warn, ReplicaLogger};
use ic_registry_client_helpers::subnet::SubnetRegistry;
use ic_replicated_state::ReplicatedState;
use ic_types::{
Expand Down Expand Up @@ -123,6 +124,7 @@ pub(crate) fn validate_payload(
state_manager: &dyn StateManager<State = ReplicatedState>,
validation_context: &ValidationContext,
metrics: &IntCounterVec,
log: &ReplicaLogger,
) -> ValidationResult<PayloadValidationError> {
let current_height = parent.height.increment();
let registry_version = pool_reader
Expand Down Expand Up @@ -164,6 +166,28 @@ pub(crate) fn validate_payload(
)
.into());
}
for (tag, transcript) in summary_payload.dkg.current_transcripts() {
if *tag != transcript.dkg_id.dkg_tag {
metrics.with_label_values(&["tag_mismatch"]).inc();
warn!(
log,
"Current transcript key {:?} doesn't match transcript tag {:?}!",
tag,
transcript.dkg_id.dkg_tag
);
}
}
for (tag, transcript) in summary_payload.dkg.next_transcripts() {
if *tag != transcript.dkg_id.dkg_tag {
metrics.with_label_values(&["tag_mismatch"]).inc();
warn!(
log,
"Next transcript key {:?} doesn't match transcript tag {:?}!",
tag,
transcript.dkg_id.dkg_tag
);
}
}
Ok(())
}
BlockPayload::Data(data_payload) => {
Expand Down Expand Up @@ -275,6 +299,7 @@ fn validate_dealings_payload(
mod tests {
use super::*;
use ic_consensus_mocks::{dependencies_with_subnet_params, Dependencies};
use ic_logger::no_op_logger;
use ic_metrics::MetricsRegistry;
use ic_test_utilities_consensus::fake::FakeContentSigner;
use ic_test_utilities_registry::SubnetRecordBuilder;
Expand Down Expand Up @@ -343,6 +368,7 @@ mod tests {
state_manager.as_ref(),
&context,
&mock_metrics(),
&no_op_logger(),
)
.is_ok());

Expand All @@ -364,6 +390,7 @@ mod tests {
state_manager.as_ref(),
&context,
&mock_metrics(),
&no_op_logger(),
)
.is_ok());
})
Expand Down Expand Up @@ -559,6 +586,7 @@ mod tests {
state_manager.as_ref(),
&context,
&mock_metrics(),
&no_op_logger(),
);

result
Expand Down

0 comments on commit 67cd0a7

Please sign in to comment.