Skip to content

Commit 065e281

Browse files
eichhorlIDX GitHub Automationpierugo-dfinity
authored
perf: CON-1657 Validate HTTPS outcall signatures in parallel (#11450)
The main bottleneck of validating blocks with many HTTPS outcalls responses is signature verification. Therefore, in #10345 we started using batch verification which has better performance than validating signatures one-by-one. With this PR we additionally pass a rayon thread pool to the HTTPS outcalls payload builder. This thread pool has 16 threads, and is already used by other payload builders to reduce validation times (the `ChainKeyPayloadBuilder` for instance). Similar to that, we use the thread pool to split all signatures of the current block in to at most 16 batches (one for each thread), and then validate each batch in parallel, with a single batch verification call. This reduces the validation time of full HTTPS outcalls payloads by up to ~80%. ### Before ```sql canister_http_payload_verification/mixed_subnet34 time: [107.00 ms 107.66 ms 108.38 ms] canister_http_payload_verification/many_replicated_responses_subnet34 time: [149.01 ms 149.87 ms 150.85 ms] canister_http_payload_verification/many_non_replicated_responses_subnet34 time: [8.2811 ms 8.3068 ms 8.3353 ms] canister_http_payload_verification/many_divergence_responses_subnet34 time: [79.840 ms 80.548 ms 81.381 ms] canister_http_payload_verification/many_flexible_responses_subnet34 time: [179.01 ms 180.38 ms 181.82 ms] ``` ### After ```sql canister_http_payload_verification/mixed_subnet34 time: [21.519 ms 21.788 ms 22.066 ms] change: [-80.032% -79.762% -79.482%] (p = 0.00 < 0.05) Performance has improved. canister_http_payload_verification/many_replicated_responses_subnet34 time: [22.973 ms 23.205 ms 23.457 ms] change: [-84.695% -84.517% -84.330%] (p = 0.00 < 0.05) Performance has improved. canister_http_payload_verification/many_non_replicated_responses_subnet34 time: [3.0804 ms 3.1072 ms 3.1346 ms] change: [-62.932% -62.595% -62.245%] (p = 0.00 < 0.05) Performance has improved. canister_http_payload_verification/many_divergence_responses_subnet34 time: [14.198 ms 14.318 ms 14.452 ms] change: [-82.456% -82.225% -81.996%] (p = 0.00 < 0.05) Performance has improved. canister_http_payload_verification/many_flexible_responses_subnet34 time: [43.591 ms 43.947 ms 44.336 ms] change: [-75.914% -75.637% -75.337%] (p = 0.00 < 0.05) Performance has improved. ``` --------- Co-authored-by: IDX GitHub Automation <infra+github-automation@dfinity.org> Co-authored-by: Pierugo Pace <pierugo.pace@dfinity.org>
1 parent 5b49fd2 commit 065e281

20 files changed

Lines changed: 73 additions & 61 deletions

File tree

Cargo.lock

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

rs/consensus/chain_key/src/lib.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,7 @@ mod tests {
658658
use assert_matches::assert_matches;
659659
use core::{convert::From, iter::Iterator, time::Duration};
660660
use ic_consensus_mocks::{Dependencies, DependenciesBuilder};
661+
use ic_consensus_utils::build_thread_pool;
661662
use ic_crypto_temp_crypto::TempCryptoComponent;
662663
use ic_interfaces::consensus::{InvalidPayloadReason, PayloadValidationFailure};
663664
use ic_interfaces::idkg::IDkgChangeAction;
@@ -676,7 +677,6 @@ mod tests {
676677
use ic_types::time::UNIX_EPOCH;
677678
use ic_types::time::current_time;
678679
use ic_types_test_utils::ids::{node_test_id, subnet_test_id};
679-
use rayon::ThreadPoolBuilder;
680680
use std::str::FromStr;
681681

682682
use super::*;
@@ -873,12 +873,7 @@ mod tests {
873873
pool.get_cache(),
874874
crypto,
875875
state_manager,
876-
Arc::new(
877-
ThreadPoolBuilder::new()
878-
.num_threads(num_threads)
879-
.build()
880-
.unwrap(),
881-
),
876+
build_thread_pool(num_threads),
882877
subnet_id,
883878
registry,
884879
&MetricsRegistry::new(),

rs/consensus/idkg/src/lib.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,11 @@ use crate::{
185185
metrics::{CRITICAL_ERROR_IDKG_RETAIN_ACTIVE_TRANSCRIPTS, IDkgClientMetrics, timed_call},
186186
pre_signer::{IDkgPreSigner, IDkgPreSignerImpl},
187187
signer::{ThresholdSigner, ThresholdSignerImpl},
188-
utils::{IDkgBlockReaderImpl, build_thread_pool},
188+
utils::IDkgBlockReaderImpl,
189+
};
190+
use ic_consensus_utils::{
191+
bouncer_metrics::BouncerMetrics, build_thread_pool, crypto::ConsensusCrypto,
189192
};
190-
use ic_consensus_utils::{bouncer_metrics::BouncerMetrics, crypto::ConsensusCrypto};
191193
use ic_interfaces::{
192194
consensus_pool::{ConsensusBlockCache, ConsensusPoolCache},
193195
crypto::IDkgProtocol,

rs/consensus/idkg/src/payload_verifier.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -548,9 +548,9 @@ mod test {
548548
resharing::{initiate_reshare_requests, update_completed_reshare_requests},
549549
},
550550
test_utils::*,
551-
utils::build_thread_pool,
552551
};
553552
use assert_matches::assert_matches;
553+
use ic_consensus_utils::build_thread_pool;
554554
use ic_crypto_temp_crypto::TempCryptoComponent;
555555
use ic_crypto_test_utils_canister_threshold_sigs::{
556556
CanisterThresholdSigTestEnvironment, dummy_values::dummy_dealings,

rs/consensus/idkg/src/test_utils.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,11 @@ use crate::{
33
complaints::{IDkgComplaintHandlerImpl, IDkgTranscriptLoader, TranscriptLoadStatus},
44
pre_signer::{IDkgPreSignerImpl, IDkgTranscriptBuilder},
55
signer::ThresholdSignerImpl,
6-
utils::build_thread_pool,
76
};
87
use ic_artifact_pool::idkg_pool::IDkgPoolImpl;
98
use ic_config::artifact_pool::ArtifactPoolConfig;
109
use ic_consensus_mocks::{Dependencies, DependenciesBuilder};
11-
use ic_consensus_utils::crypto::ConsensusCrypto;
10+
use ic_consensus_utils::{build_thread_pool, crypto::ConsensusCrypto};
1211
use ic_crypto_temp_crypto::TempCryptoComponent;
1312
use ic_crypto_test_utils_canister_threshold_sigs::{
1413
CanisterThresholdSigTestEnvironment, IDkgParticipants, IntoBuilder,

rs/consensus/idkg/src/utils.rs

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ use ic_types::{
3232
},
3333
registry::RegistryClientError,
3434
};
35-
use rayon::{ThreadPool, ThreadPoolBuilder};
3635
use std::{
3736
cell::RefCell,
3837
collections::{BTreeMap, BTreeSet},
@@ -500,16 +499,6 @@ impl<T: Ord + Copy> IDkgSchedule<T> {
500499
}
501500
}
502501

503-
/// Builds a rayon thread pool with the given number of threads.
504-
pub(crate) fn build_thread_pool(num_threads: usize) -> Arc<ThreadPool> {
505-
Arc::new(
506-
ThreadPoolBuilder::new()
507-
.num_threads(num_threads)
508-
.build()
509-
.expect("Failed to create thread pool"),
510-
)
511-
}
512-
513502
#[cfg(test)]
514503
mod tests {
515504
use super::*;

rs/consensus/src/consensus.rs

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ use ic_types::{
5959
Time, artifact::ConsensusMessageId, consensus::ConsensusMessageHashable,
6060
malicious_flags::MaliciousFlags, replica_config::ReplicaConfig,
6161
};
62-
use rayon::{ThreadPool, ThreadPoolBuilder};
62+
use rayon::ThreadPool;
6363
use std::{
6464
cell::RefCell,
6565
collections::BTreeMap,
@@ -82,9 +82,6 @@ pub const ACCEPTABLE_NOTARIZATION_CERTIFICATION_GAP: u64 = 70;
8282
/// CUPs, which have no upper bound on the height to be validated.
8383
pub(crate) const ACCEPTABLE_NOTARIZATION_CUP_GAP: u64 = 130;
8484

85-
/// The maximum number of threads used to create & validate block payloads in parallel.
86-
pub const MAX_CONSENSUS_THREADS: usize = 16;
87-
8885
#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug, AsRefStr)]
8986
#[strum(serialize_all = "snake_case")]
9087
enum ConsensusSubcomponent {
@@ -99,16 +96,6 @@ enum ConsensusSubcomponent {
9996
Purger,
10097
}
10198

102-
/// Builds a rayon thread pool with the given number of threads.
103-
pub fn build_thread_pool(num_threads: usize) -> Arc<ThreadPool> {
104-
Arc::new(
105-
ThreadPoolBuilder::new()
106-
.num_threads(num_threads)
107-
.build()
108-
.expect("Failed to create thread pool"),
109-
)
110-
}
111-
11299
/// [ConsensusImpl] holds all consensus subcomponents, and implements the
113100
/// Consensus trait by calling each subcomponent in round-robin manner.
114101
pub struct ConsensusImpl {
@@ -622,6 +609,7 @@ mod tests {
622609
use super::*;
623610
use ic_config::artifact_pool::ArtifactPoolConfig;
624611
use ic_consensus_mocks::{Dependencies, DependenciesBuilder};
612+
use ic_consensus_utils::{MAX_CONSENSUS_THREADS, build_thread_pool};
625613
use ic_https_outcalls_consensus::test_utils::FakeCanisterHttpPayloadBuilder;
626614
use ic_logger::replica_logger::no_op_logger;
627615
use ic_metrics::MetricsRegistry;

rs/consensus/src/consensus/validator.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2160,12 +2160,12 @@ impl Validator {
21602160
pub mod test {
21612161
use super::*;
21622162
use crate::consensus::{
2163-
MAX_CONSENSUS_THREADS, block_maker::get_block_maker_delay, build_thread_pool,
2164-
catchup_package_maker::CatchUpPackageMaker,
2163+
block_maker::get_block_maker_delay, catchup_package_maker::CatchUpPackageMaker,
21652164
};
21662165
use assert_matches::assert_matches;
21672166
use ic_config::artifact_pool::ArtifactPoolConfig;
21682167
use ic_consensus_mocks::{Dependencies, DependenciesBuilder, RefMockPayloadBuilder};
2168+
use ic_consensus_utils::{MAX_CONSENSUS_THREADS, build_thread_pool};
21692169
use ic_crypto_test_utils_crypto_returning_ok::CryptoReturningOk;
21702170
use ic_interfaces::{
21712171
messaging::XNetPayloadValidationFailure, p2p::consensus::MutablePool,

rs/consensus/tests/framework/types.rs

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,9 @@ use ic_artifact_pool::{
44
consensus_pool::ConsensusPoolImpl, dkg_pool, idkg_pool,
55
};
66
use ic_config::artifact_pool::ArtifactPoolConfig;
7-
use ic_consensus::consensus::{
8-
ConsensusBouncer, ConsensusImpl, MAX_CONSENSUS_THREADS, build_thread_pool,
9-
};
7+
use ic_consensus::consensus::{ConsensusBouncer, ConsensusImpl};
108
use ic_consensus_idkg::IDkgImpl;
9+
use ic_consensus_utils::{MAX_CONSENSUS_THREADS, build_thread_pool};
1110
use ic_https_outcalls_consensus::test_utils::FakeCanisterHttpPayloadBuilder;
1211
use ic_interfaces::{
1312
batch_payload::BatchPayloadBuilder,

rs/consensus/tests/payload.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,9 @@ mod framework;
44
use crate::framework::ConsensusDriver;
55
use assert_matches::assert_matches;
66
use ic_artifact_pool::{consensus_pool, dkg_pool, idkg_pool};
7-
use ic_consensus::consensus::{MAX_CONSENSUS_THREADS, build_thread_pool};
87
use ic_consensus_certification::CertifierImpl;
98
use ic_consensus_dkg::{DkgKeyManager, get_dkg_summary_from_cup_contents};
10-
use ic_consensus_utils::pool_reader::PoolReader;
9+
use ic_consensus_utils::{MAX_CONSENSUS_THREADS, build_thread_pool, pool_reader::PoolReader};
1110
use ic_crypto_test_utils_crypto_returning_ok::CryptoReturningOk;
1211
use ic_https_outcalls_consensus::test_utils::FakeCanisterHttpPayloadBuilder;
1312
use ic_interfaces_registry::RegistryClient;

0 commit comments

Comments
 (0)