Skip to content

Commit 270e63b

Browse files
authored
fix(PocketIC): consensus registry version of instance resumed from existing state (#7502)
This PR fixes the consensus registry version of a PocketIC instance resumed from an existing state: for canister http outcalls to work that registry version must be equal to the latest registry version which was not the case for a PocketIC instance resumed from an existing state (in that case the consensus registry version was equal to the registry version at which the corresponding subnet was created which is not always equal to the latest registry version if there are multiple subnets). More details: - the function `check_share_against_context` [requires](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/https_outcalls/consensus/src/payload_builder/utils.rs#L85) the registry version of the canister http outcall share (set to the [latest registry version](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/state_machine_tests/src/lib.rs#L2347) by PocketIC) to match the [consensus registry version](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/https_outcalls/consensus/src/payload_builder.rs#L181-L194) retrieved from the consensus pool cache set by PocketIC [here](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/state_machine_tests/src/lib.rs#L1773) when loading an existing subnet and [here](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/state_machine_tests/src/lib.rs#L1644) when creating a new subnet; - the function `make_registry_cup` used so far creates a CUP with the [registry version](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/consensus/cup_utils/src/lib.rs#L187) at which the corresponding registry key `catch_up_package_contents_{subnet_id}` was [inserted](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/state_machine_tests/src/lib.rs#L497-L503) into the registry (the registry version at which the corresponding subnet was created by PocketIC); - the function `make_registry_cup_from_cup_contents` used in this PR creates a CUP with a [registry version](https://github.com/dfinity/ic/blob/7a8c16764cdcfb901759ff62e64ac983633fcc17/rs/consensus/cup_utils/src/lib.rs#L33) specified in the function's input parameters which allows PocketIC to use the latest registry version in the CUP.
1 parent 54879a4 commit 270e63b

File tree

2 files changed

+71
-34
lines changed

2 files changed

+71
-34
lines changed

packages/pocket-ic/tests/tests.rs

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1314,18 +1314,7 @@ fn test_vetkd() {
13141314
}
13151315
}
13161316

1317-
#[test]
1318-
fn test_canister_http() {
1319-
let pic = PocketIc::new();
1320-
1321-
// Create a canister and charge it with initial cycles.
1322-
let canister_id = pic.create_canister();
1323-
pic.add_cycles(canister_id, INIT_CYCLES);
1324-
1325-
// Install the test canister wasm file on the canister.
1326-
let test_wasm = test_canister_wasm();
1327-
pic.install_canister(canister_id, test_wasm, vec![], None);
1328-
1317+
fn test_canister_http(pic: &PocketIc, canister_id: Principal) {
13291318
// Submit an update call to the test canister making a canister http outcall
13301319
// and mock a canister http outcall response.
13311320
let call_id = pic
@@ -1370,6 +1359,48 @@ fn test_canister_http() {
13701359
assert_eq!(http_response.unwrap().body, body);
13711360
}
13721361

1362+
#[test]
1363+
fn test_canister_http_on_fresh_and_resumed_instance() {
1364+
// create an empty PocketIC state to be used:
1365+
// - initially by a fresh PocketIC instance;
1366+
// - later by a PocketIC instance resumed from that state.
1367+
let state = PocketIcState::new();
1368+
1369+
// create a fresh PocketIC instance with two application subnets
1370+
// so that the latest registry version is different
1371+
// from the registry version at which one of the subnets was created
1372+
// (this scenario led to a bug in PocketIC canister http outcalls)
1373+
let pic = PocketIcBuilder::new()
1374+
.with_application_subnet()
1375+
.with_application_subnet()
1376+
.with_state(state)
1377+
.build();
1378+
1379+
// create a test canister on every subnet
1380+
let topology = pic.topology();
1381+
let mut canisters = vec![];
1382+
for app_subnet in topology.get_app_subnets() {
1383+
let canister_id = pic.create_canister_on_subnet(None, None, app_subnet);
1384+
pic.add_cycles(canister_id, INIT_CYCLES);
1385+
pic.install_canister(canister_id, test_canister_wasm(), vec![], None);
1386+
canisters.push(canister_id);
1387+
}
1388+
// ensure that canister http outcalls work on every subnet
1389+
for canister_id in &canisters {
1390+
test_canister_http(&pic, *canister_id);
1391+
}
1392+
1393+
// drop the first PocketIC instance and serialize its state
1394+
let state = pic.drop_and_take_state().unwrap();
1395+
1396+
// create the second PocketIC instance resuming from the existing state
1397+
let pic = PocketIcBuilder::new().with_state(state).build();
1398+
// ensure that canister http outcalls still work on every subnet
1399+
for canister_id in &canisters {
1400+
test_canister_http(&pic, *canister_id);
1401+
}
1402+
}
1403+
13731404
#[test]
13741405
fn test_canister_http_with_transform() {
13751406
let pic = PocketIc::new();

rs/state_machine_tests/src/lib.rs

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use ic_config::{
1414
subnet_config::SubnetConfig,
1515
};
1616
use ic_consensus::consensus::payload_builder::PayloadBuilderImpl;
17-
use ic_consensus_cup_utils::{make_registry_cup, make_registry_cup_from_cup_contents};
17+
use ic_consensus_cup_utils::make_registry_cup_from_cup_contents;
1818
use ic_consensus_utils::crypto::SignVerify;
1919
use ic_crypto_test_utils_crypto_returning_ok::CryptoReturningOk;
2020
use ic_crypto_test_utils_ni_dkg::{
@@ -146,7 +146,6 @@ use ic_types::{
146146
},
147147
canister_http::{CanisterHttpResponse, CanisterHttpResponseContent},
148148
consensus::{
149-
CatchUpPackage,
150149
block_maker::SubnetRecords,
151150
certification::{Certification, CertificationContent},
152151
},
@@ -516,6 +515,28 @@ fn add_subnet_local_registry_records(
516515
);
517516
}
518517

518+
fn make_fresh_registry_cup(
519+
registry_client: Arc<FakeRegistryClient>,
520+
subnet_id: SubnetId,
521+
replica_logger: &ReplicaLogger,
522+
) -> pb::CatchUpPackage {
523+
let registry_version = registry_client.get_latest_version();
524+
let cup_contents = registry_client
525+
.get_cup_contents(subnet_id, registry_version)
526+
.unwrap()
527+
.value
528+
.unwrap();
529+
let cup = make_registry_cup_from_cup_contents(
530+
registry_client.as_ref(),
531+
subnet_id,
532+
cup_contents,
533+
registry_version,
534+
replica_logger,
535+
)
536+
.unwrap();
537+
cup.into()
538+
}
539+
519540
/// Convert an object into CBOR binary.
520541
fn into_cbor<R: Serialize>(r: &R) -> Vec<u8> {
521542
let mut ser = serde_cbor::Serializer::new(Vec::new());
@@ -1625,22 +1646,11 @@ impl StateMachine {
16251646
// Since the latest registry version could have changed,
16261647
// we update the CUP in `FakeConsensusPoolCache` to refer
16271648
// to the latest registry version.
1628-
let registry_version = self.registry_client.get_latest_version();
1629-
let cup_contents = self
1630-
.registry_client
1631-
.get_cup_contents(self.subnet_id, registry_version)
1632-
.unwrap()
1633-
.value
1634-
.unwrap();
1635-
let cup = make_registry_cup_from_cup_contents(
1636-
self.registry_client.as_ref(),
1649+
let cup_proto = make_fresh_registry_cup(
1650+
self.registry_client.clone(),
16371651
self.subnet_id,
1638-
cup_contents,
1639-
registry_version,
16401652
&self.replica_logger,
1641-
)
1642-
.unwrap();
1643-
let cup_proto: pb::CatchUpPackage = cup.into();
1653+
);
16441654
self.consensus_pool_cache.update_cup(cup_proto);
16451655
}
16461656

@@ -1758,18 +1768,14 @@ impl StateMachine {
17581768

17591769
let registry_client = FakeRegistryClient::new(Arc::clone(&registry_data_provider) as _);
17601770
registry_client.update_to_latest_version();
1761-
1762-
// get the CUP from the registry
1763-
let cup: CatchUpPackage =
1764-
make_registry_cup(&registry_client, subnet_id, &replica_logger).unwrap();
1765-
let cup_proto: pb::CatchUpPackage = cup.into();
1766-
// now we can wrap the registry client into an Arc
17671771
let registry_client = Arc::new(registry_client);
17681772

17691773
let canister_http_pool = Arc::new(RwLock::new(CanisterHttpPoolImpl::new(
17701774
metrics_registry.clone(),
17711775
replica_logger.clone(),
17721776
)));
1777+
let cup_proto =
1778+
make_fresh_registry_cup(registry_client.clone(), subnet_id, &replica_logger);
17731779
let consensus_pool_cache = Arc::new(FakeConsensusPoolCache::new(cup_proto));
17741780
let crypto = CryptoReturningOk::default();
17751781
let canister_http_payload_builder = Arc::new(CanisterHttpPayloadBuilderImpl::new(

0 commit comments

Comments
 (0)