Skip to content

feat(subnet-spiltting): deliver Splitting batch to DSM #11213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
pierugo-dfinity wants to merge 1 commit into from
+171 −20
Draft

feat(subnet-spiltting): deliver Splitting batch to DSM #11213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 170 additions & 20 deletions rs/consensus/src/consensus/batch_delivery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::consensus::{
use ic_consensus_chain_key::ChainKeyPayloadBuilderImpl;
use ic_consensus_dkg::get_vetkey_public_keys;
use ic_consensus_idkg::utils::get_idkg_subnet_public_keys_and_pre_signatures;
use ic_consensus_utils::{membership::Membership, pool_reader::PoolReader};
use ic_consensus_utils::{membership::Membership, pool_reader::PoolReader, subnet_splitting};
use ic_error_types::RejectCode;
use ic_https_outcalls_consensus::payload_builder::CanisterHttpPayloadBuilderImpl;
use ic_interfaces::{
Expand All @@ -24,20 +24,18 @@ use ic_protobuf::{
registry::{crypto::v1::PublicKey as PublicKeyProto, subnet::v1::InitialNiDkgTranscriptRecord},
};
use ic_types::{
Height, PrincipalId, SubnetId,
Height, NodeId, PrincipalId, SubnetId,
batch::{
Batch, BatchContent, BatchMessages, BatchSummary, BlockmakerMetrics, CanisterHttpSpent,
ChainKeyData, ConsensusResponse,
},
consensus::{
Block, BlockPayload, HasVersion,
dkg::RemoteTranscriptResult,
idkg::{self},
},
crypto::randomness_from_crypto_hashable,
crypto::threshold_sig::{
ThresholdSigPublicKey,
ni_dkg::{NiDkgId, NiDkgTag, NiDkgTranscript},
consensus::{Block, BlockPayload, HasVersion, dkg::RemoteTranscriptResult, idkg},
crypto::{
randomness_from_crypto_hashable,
threshold_sig::{
ThresholdSigPublicKey,
ni_dkg::{NiDkgId, NiDkgTag, NiDkgTranscript},
},
},
messages::{CallbackId, Payload, RejectContext},
};
Expand All @@ -64,6 +62,7 @@ pub fn deliver_batches(
pool,
registry_client,
subnet_id,
/*maybe_node_id=*/ None,
log,
max_batch_height_to_deliver,
/*result_processor=*/ None,
Expand All @@ -80,6 +79,7 @@ pub(crate) fn deliver_batches_with_result_processor(
pool: &PoolReader<'_>,
registry_client: &dyn RegistryClient,
subnet_id: SubnetId,
maybe_node_id: Option<NodeId>,
log: &ReplicaLogger,
// This argument should only be used by the ic-replay tool. If it is set to `None`, we will
// deliver all batches until the finalized height. If it is set to `Some(h)`, we will
Expand Down Expand Up @@ -224,13 +224,50 @@ pub(crate) fn deliver_batches_with_result_processor(
let persist_batch = Some(height) == max_batch_height_to_deliver;
let requires_full_state_hash = block.payload.is_summary() || persist_batch;
let batch_content = match block.payload.as_ref() {
BlockPayload::Summary(_summary_payload) => BatchContent::Data {
batch_messages: BatchMessages::default(),
chain_key_data,
consensus_responses,
canister_http_spent,
requires_full_state_hash,
},
BlockPayload::Summary(_summary_payload) => {
if let Some(scheduled) = subnet_splitting::is_split_scheduled(&block) {
let node_id =
maybe_node_id.expect("Subnet splitting not yet supported in ic-replay");
Comment thread
pierugo-dfinity marked this conversation as resolved.
let subnet_splitting::PostSplitAssignment {
new_subnet_id,
other_subnet_id,
} = match subnet_splitting::get_post_split_subnet_assignment(
node_id,
&block,
registry_client,
scheduled,
) {
Ok(assignment) => assignment,
Err(err) => {
warn!(
every_n_seconds => 30,
log,
"Error getting new subnet assignment: {}",
err
);
break;
}
};

info!(
log,
"Delivering splitting block. New subnet assignment: {}", new_subnet_id
);

BatchContent::Splitting {
new_subnet_id,
other_subnet_id,
}
Comment on lines +257 to +260

@pierugo-dfinity pierugo-dfinity Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but according to this comment, this will be overwritten as soon as the destination subnet starts a new round after the split.
Maybe something worth looking into: both the Scheduled summary and the PostSplit summary have the same registry version, so DSM might use the cached OwnSubnetInfo by mistake. But because destination replicas are restarted before executing anything and I assume this cache is stored only in memory, this shouldn't be a problem.

cc @alin-at-dfinity for confirmation

@alin-at-dfinity alin-at-dfinity Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work as is, as per your analysis.

We can (and probably should) make it clearer by explicitly resetting it to default for subnet B in online_split() instead of carrying it over. Not perfect, as it will result in (apparently valid) disabled features and default limits, but it will work better as documentation (we really expect this to be populated, or else).

@pierugo-dfinity pierugo-dfinity Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative is to actually fill it with the proper values by reading the registry at the batch's registry version (which is precisely the version at which the split happened). But we'd then have two individual locations where the registry is read, not perfect either.

} else {
BatchContent::Data {
batch_messages: BatchMessages::default(),
chain_key_data,
consensus_responses,
canister_http_spent,
requires_full_state_hash,
}
}
}
BlockPayload::Data(data_payload) => {
batch_stats.add_from_payload(&data_payload.batch);
BatchContent::Data {
Expand Down Expand Up @@ -577,16 +614,20 @@ mod tests {
//! Finalizer unit tests
use super::*;
use crate::consensus::batch_delivery::generate_responses_to_remote_dkgs;
use ic_consensus_mocks::{Dependencies, DependenciesBuilder};
use ic_crypto_test_utils_ni_dkg::dummy_transcript_for_tests;
use ic_logger::replica_logger::no_op_logger;
use ic_management_canister_types_private::{SetupInitialDKGResponse, VetKdCurve, VetKdKeyId};
use ic_test_utilities::message_routing::FakeMessageRouting;
use ic_test_utilities_registry::SubnetRecordBuilder;
use ic_test_utilities_types::ids::subnet_test_id;
use ic_types::{
PrincipalId, RegistryVersion, SubnetId,
backwards_compatibility::BackwardsCompatible,
batch::{BatchPayload, ValidationContext},
consensus::{
DataPayload, Payload as ConsensusPayload, Rank,
dkg::{DkgDataPayload, RemoteTranscriptResult},
DataPayload, HashedBlock, Payload as ConsensusPayload, Rank,
dkg::{DkgDataPayload, RemoteTranscriptResult, SplittingArgs, SubnetSplittingStatus},
},
crypto::{
CryptoHash, CryptoHashOf,
Expand All @@ -595,10 +636,16 @@ mod tests {
},
},
messages::{CallbackId, Payload},
replica_config::ReplicaConfig,
time::UNIX_EPOCH,
};
use ic_types_test_utils::ids::{NODE_1, NODE_2, NODE_3, NODE_4, SUBNET_1, SUBNET_2};
use rstest::rstest;
use std::str::FromStr;

const SOURCE_SUBNET_ID: SubnetId = SUBNET_1;
const DESTINATION_SUBNET_ID: SubnetId = SUBNET_2;

const TARGET_ID: NiDkgTargetId = NiDkgTargetId::new([8; 32]);

const EXPECTED_FRESH_SUBNET_ID_STR: &str =
Expand Down Expand Up @@ -781,4 +828,107 @@ mod tests {
SubnetId::from(PrincipalId::from_str(EXPECTED_FRESH_SUBNET_ID_STR).unwrap())
);
}

#[rstest]
#[case::node_on_source_subnet(NODE_1, SOURCE_SUBNET_ID, DESTINATION_SUBNET_ID)]
#[case::node_on_destination_subnet(NODE_4, DESTINATION_SUBNET_ID, SOURCE_SUBNET_ID)]
fn test_deliver_splitting_batch(
#[case] node_id: NodeId,
#[case] expected_new_subnet_id: SubnetId,
#[case] expected_other_subnet_id: SubnetId,
) {
ic_test_utilities::artifact_pool_config::with_test_pool_config(|pool_config| {
const SPLITTING_REGISTRY_VERSION: RegistryVersion = RegistryVersion::new(2);
const INTERVAL_LENGTH: u64 = 9;
let summary_height = Height::from(INTERVAL_LENGTH + 1);

let Dependencies {
mut pool,
membership,
registry,
..
} = DependenciesBuilder::multiple_subnets(
pool_config,
vec![
(
1,
SOURCE_SUBNET_ID,
SubnetRecordBuilder::from(&[NODE_1, NODE_2, NODE_3, NODE_4])
.with_dkg_interval_length(INTERVAL_LENGTH)
.build(),
),
(
SPLITTING_REGISTRY_VERSION.get(),
SOURCE_SUBNET_ID,
SubnetRecordBuilder::from(&[NODE_1, NODE_3])
.with_dkg_interval_length(INTERVAL_LENGTH)
.build(),
),
(
SPLITTING_REGISTRY_VERSION.get(),
DESTINATION_SUBNET_ID,
SubnetRecordBuilder::from(&[NODE_2, NODE_4])
.with_dkg_interval_length(INTERVAL_LENGTH)
.build(),
),
],
)
.with_replica_config(ReplicaConfig {
node_id: NODE_1,
subnet_id: SOURCE_SUBNET_ID,
})
.build();

pool.advance_round_normal_operation_n(INTERVAL_LENGTH);

let mut proposal = pool.make_next_block();
let block = proposal.content.as_mut();
block.context.registry_version = SPLITTING_REGISTRY_VERSION;
let mut payload = block.payload.as_ref().as_summary().clone();
payload.dkg.subnet_splitting_status = BackwardsCompatible::new_for_test_only(Some(
SubnetSplittingStatus::Scheduled(SplittingArgs {
source_subnet_id: SOURCE_SUBNET_ID,
destination_subnet_id: DESTINATION_SUBNET_ID,
}),
));
block.payload = ConsensusPayload::new(
ic_types::crypto::crypto_hash,
BlockPayload::Summary(payload),
);
proposal.content = HashedBlock::new(ic_types::crypto::crypto_hash, block.clone());
pool.insert_validated(proposal.clone());
pool.notarize(&proposal);
pool.finalize(&proposal);
pool.insert_random_tape(summary_height);

let message_routing = FakeMessageRouting::new();
*message_routing.next_batch_height.write().unwrap() = summary_height;

let result = deliver_batches_with_result_processor(
&message_routing,
&membership,
&PoolReader::new(&pool),
registry.as_ref(),
SOURCE_SUBNET_ID,
Some(node_id),
&no_op_logger(),
None,
None,
);

assert_eq!(result, Ok(summary_height));
let batches = message_routing.batches.read().unwrap();
assert_eq!(batches.len(), 1);
match &batches[0].content {
BatchContent::Splitting {
new_subnet_id,
other_subnet_id,
} => {
assert_eq!(*new_subnet_id, expected_new_subnet_id);
assert_eq!(*other_subnet_id, expected_other_subnet_id);
}
other => panic!("Expected BatchContent::Splitting, got: {other:?}"),
}
})
}
}
1 change: 1 addition & 0 deletions rs/consensus/src/consensus/finalizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ impl Finalizer {
pool,
&*self.registry_client,
self.replica_config.subnet_id,
Some(self.replica_config.node_id),
&self.log,
None,
Some(&|result, block_stats, batch_stats| {
Expand Down
Loading