Skip to content

Commit 66ffd52

Browse files
michael-weigeltMichael Weigelt
michael-weigelt
and
Michael Weigelt
authored
feat: [EXC-2018] Charge for snapshot data download (#4787)
This PR introduces charging for instructions executed during canister snapshot binary data download. --------- Co-authored-by: Michael Weigelt <michael.weigelt@dfinity.com>
1 parent c90a650 commit 66ffd52

File tree

9 files changed

+98
-16
lines changed

9 files changed

+98
-16
lines changed

‎rs/config/src/subnet_config.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,11 @@ pub const DEFAULT_UPLOAD_CHUNK_INSTRUCTIONS: NumInstructions = NumInstructions::
165165
pub const DEFAULT_CANISTERS_SNAPSHOT_BASELINE_INSTRUCTIONS: NumInstructions =
166166
NumInstructions::new(2_000_000_000);
167167

168+
/// Baseline cost for up/downloading binary snapshot data (5M instructions).
169+
/// The cost is based on the benchmarks: rs/execution_environment/benches/management_canister/
170+
pub const DEFAULT_CANISTERS_SNAPSHOT_DATA_BASELINE_INSTRUCTIONS: NumInstructions =
171+
NumInstructions::new(5_000_000);
172+
168173
/// The cycle cost overhead of executing canister instructions when running in Wasm64 mode.
169174
/// This overhead is a multiplier over the cost of executing the same instructions
170175
/// in Wasm32 mode. The overhead comes from the bound checks performed in Wasm64 mode
@@ -272,6 +277,9 @@ pub struct SchedulerConfig {
272277

273278
/// Number of instructions to count when creating or loading a canister snapshot.
274279
pub canister_snapshot_baseline_instructions: NumInstructions,
280+
281+
/// Number of instructions to count when uploading or downloading binary snapshot data.
282+
pub canister_snapshot_data_baseline_instructions: NumInstructions,
275283
}
276284

277285
impl SchedulerConfig {
@@ -301,6 +309,8 @@ impl SchedulerConfig {
301309
upload_wasm_chunk_instructions: DEFAULT_UPLOAD_CHUNK_INSTRUCTIONS,
302310
canister_snapshot_baseline_instructions:
303311
DEFAULT_CANISTERS_SNAPSHOT_BASELINE_INSTRUCTIONS,
312+
canister_snapshot_data_baseline_instructions:
313+
DEFAULT_CANISTERS_SNAPSHOT_DATA_BASELINE_INSTRUCTIONS,
304314
}
305315
}
306316

@@ -344,6 +354,7 @@ impl SchedulerConfig {
344354
accumulated_priority_reset_interval: ACCUMULATED_PRIORITY_RESET_INTERVAL,
345355
upload_wasm_chunk_instructions: NumInstructions::from(0),
346356
canister_snapshot_baseline_instructions: NumInstructions::from(0),
357+
canister_snapshot_data_baseline_instructions: NumInstructions::from(0),
347358
}
348359
}
349360

‎rs/execution_environment/benches/lib/src/common.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,9 @@ where
314314
subnet_configs
315315
.scheduler_config
316316
.canister_snapshot_baseline_instructions,
317+
subnet_configs
318+
.scheduler_config
319+
.canister_snapshot_data_baseline_instructions,
317320
);
318321
for Benchmark(id, wat, expected_ops) in benchmarks {
319322
run_benchmark(

‎rs/execution_environment/src/canister_manager.rs

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,9 @@ use ic_management_canister_types_private::{
2727
StoredChunksReply, UploadChunkReply,
2828
};
2929
use ic_registry_provisional_whitelist::ProvisionalWhitelist;
30-
use ic_replicated_state::canister_state::system_state::wasm_chunk_store::WasmChunkHash;
30+
use ic_replicated_state::canister_state::system_state::wasm_chunk_store::{
31+
WasmChunkHash, CHUNK_SIZE,
32+
};
3133
use ic_replicated_state::canister_state::WASM_PAGE_SIZE_IN_BYTES;
3234
use ic_replicated_state::{
3335
canister_snapshots::CanisterSnapshot,
@@ -2044,10 +2046,11 @@ impl CanisterManager {
20442046
pub(crate) fn read_snapshot_data(
20452047
&self,
20462048
sender: PrincipalId,
2047-
canister: &CanisterState,
2049+
canister: &mut CanisterState,
20482050
snapshot_id: SnapshotId,
20492051
kind: CanisterSnapshotDataKind,
20502052
state: &ReplicatedState,
2053+
subnet_size: usize,
20512054
) -> Result<ReadCanisterSnapshotDataResponse, CanisterManagerError> {
20522055
// Check sender is a controller.
20532056
validate_controller(canister, &sender)?;
@@ -2064,6 +2067,29 @@ impl CanisterManager {
20642067
snapshot_id,
20652068
});
20662069
}
2070+
2071+
// Charge upfront for the baseline plus the maximum possible size of the returned slice or fail.
2072+
let num_response_bytes = match &kind {
2073+
CanisterSnapshotDataKind::WasmModule { size, .. } => *size,
2074+
CanisterSnapshotDataKind::MainMemory { size, .. } => *size,
2075+
CanisterSnapshotDataKind::StableMemory { size, .. } => *size,
2076+
// In this case, we might overcharge. But the stored chunks are also charged fully even if they are smaller.
2077+
CanisterSnapshotDataKind::WasmChunk { .. } => CHUNK_SIZE,
2078+
};
2079+
let size = NumInstructions::new(num_response_bytes);
2080+
if let Err(err) = self.cycles_account_manager.consume_cycles_for_instructions(
2081+
&sender,
2082+
canister,
2083+
self.config
2084+
.canister_snapshot_data_baseline_instructions
2085+
.saturating_add(&size),
2086+
subnet_size,
2087+
// For the `read_snapshot_data` operation, it does not matter if this is a Wasm64 or Wasm32 module.
2088+
WasmExecutionMode::Wasm32,
2089+
) {
2090+
return Err(CanisterManagerError::CanisterSnapshotNotEnoughCycles(err));
2091+
};
2092+
20672093
let res = match kind {
20682094
CanisterSnapshotDataKind::StableMemory { offset, size } => {
20692095
if size > MAX_SLICE_SIZE_BYTES {

‎rs/execution_environment/src/canister_manager/tests.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,7 @@ fn canister_manager_config(
312312
SchedulerConfig::application_subnet().upload_wasm_chunk_instructions,
313313
ic_config::embedders::Config::default().wasm_max_size,
314314
SchedulerConfig::application_subnet().canister_snapshot_baseline_instructions,
315+
SchedulerConfig::application_subnet().canister_snapshot_data_baseline_instructions,
315316
DEFAULT_WASM_MEMORY_LIMIT,
316317
MAX_NUMBER_OF_SNAPSHOTS_PER_CANISTER,
317318
)

‎rs/execution_environment/src/canister_manager/types.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ pub(crate) struct CanisterMgrConfig {
9090
pub(crate) upload_wasm_chunk_instructions: NumInstructions,
9191
pub(crate) wasm_chunk_store_max_size: NumBytes,
9292
pub(crate) canister_snapshot_baseline_instructions: NumInstructions,
93+
pub(crate) canister_snapshot_data_baseline_instructions: NumInstructions,
9394
pub(crate) default_wasm_memory_limit: NumBytes,
9495
pub(crate) max_number_of_snapshots_per_canister: usize,
9596
}
@@ -113,6 +114,7 @@ impl CanisterMgrConfig {
113114
upload_wasm_chunk_instructions: NumInstructions,
114115
wasm_chunk_store_max_size: NumBytes,
115116
canister_snapshot_baseline_instructions: NumInstructions,
117+
canister_snapshot_data_baseline_instructions: NumInstructions,
116118
default_wasm_memory_limit: NumBytes,
117119
max_number_of_snapshots_per_canister: usize,
118120
) -> Self {
@@ -133,6 +135,7 @@ impl CanisterMgrConfig {
133135
upload_wasm_chunk_instructions,
134136
wasm_chunk_store_max_size,
135137
canister_snapshot_baseline_instructions,
138+
canister_snapshot_data_baseline_instructions,
136139
default_wasm_memory_limit,
137140
max_number_of_snapshots_per_canister,
138141
}

‎rs/execution_environment/src/execution_environment.rs

Lines changed: 45 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,7 @@ impl ExecutionEnvironment {
380380
heap_delta_rate_limit: NumBytes,
381381
upload_wasm_chunk_instructions: NumInstructions,
382382
canister_snapshot_baseline_instructions: NumInstructions,
383+
canister_snapshot_data_baseline_instructions: NumInstructions,
383384
) -> Self {
384385
// Assert the flag implication: DTS => sandboxing.
385386
assert!(
@@ -404,6 +405,7 @@ impl ExecutionEnvironment {
404405
upload_wasm_chunk_instructions,
405406
config.embedders_config.wasm_max_size,
406407
canister_snapshot_baseline_instructions,
408+
canister_snapshot_data_baseline_instructions,
407409
config.default_wasm_memory_limit,
408410
config.max_number_of_snapshots_per_canister,
409411
);
@@ -1672,15 +1674,20 @@ impl ExecutionEnvironment {
16721674

16731675
Ok(Ic00Method::ReadCanisterSnapshotData) => {
16741676
#[allow(clippy::bind_instead_of_map)]
1675-
let res = ReadCanisterSnapshotDataArgs::decode(payload).and_then(|args| {
1676-
match self.config.canister_snapshot_download {
1677-
FlagStatus::Disabled => Ok((vec![], None)),
1678-
FlagStatus::Enabled => {
1679-
let canister_id = args.get_canister_id();
1680-
// TODO: [EXC-2018] do we need to account for copying the bytes -> costs and round_limits?
1681-
self.read_snapshot_data(*msg.sender(), &state, args)
1682-
.map(|res| (res, Some(canister_id)))
1683-
}
1677+
let res = ReadCanisterSnapshotDataArgs::decode(payload).and_then(|args| match self
1678+
.config
1679+
.canister_snapshot_download
1680+
{
1681+
FlagStatus::Disabled => Ok((vec![], None)),
1682+
FlagStatus::Enabled => {
1683+
let canister_id = args.get_canister_id();
1684+
self.read_snapshot_data(
1685+
*msg.sender(),
1686+
&mut state,
1687+
args,
1688+
registry_settings.subnet_size,
1689+
)
1690+
.map(|res| (res, Some(canister_id)))
16841691
}
16851692
});
16861693
ExecuteSubnetMessageResult::Finished {
@@ -2451,14 +2458,38 @@ impl ExecutionEnvironment {
24512458
fn read_snapshot_data(
24522459
&self,
24532460
sender: PrincipalId,
2454-
state: &ReplicatedState,
2461+
state: &mut ReplicatedState,
24552462
args: ReadCanisterSnapshotDataArgs,
2463+
subnet_size: usize,
24562464
) -> Result<Vec<u8>, UserError> {
2457-
let canister = get_canister(args.get_canister_id(), state)?;
2458-
self.canister_manager
2459-
.read_snapshot_data(sender, canister, args.get_snapshot_id(), args.kind, state)
2465+
let canister_id = args.get_canister_id();
2466+
// Take canister out.
2467+
let mut canister = match state.take_canister_state(&canister_id) {
2468+
None => {
2469+
return Err(UserError::new(
2470+
ErrorCode::CanisterNotFound,
2471+
format!("Canister {} not found.", &canister_id),
2472+
))
2473+
}
2474+
Some(canister) => canister,
2475+
};
2476+
2477+
let result = self
2478+
.canister_manager
2479+
.read_snapshot_data(
2480+
sender,
2481+
&mut canister,
2482+
args.get_snapshot_id(),
2483+
args.kind,
2484+
state,
2485+
subnet_size,
2486+
)
24602487
.map(|res| Encode!(&res).unwrap())
2461-
.map_err(UserError::from)
2488+
.map_err(UserError::from);
2489+
2490+
// Put canister back.
2491+
state.put_canister_state(canister);
2492+
result
24622493
}
24632494

24642495
fn read_canister_snapshot_metadata(

‎rs/execution_environment/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ impl ExecutionServices {
145145
scheduler_config.heap_delta_rate_limit,
146146
scheduler_config.upload_wasm_chunk_instructions,
147147
scheduler_config.canister_snapshot_baseline_instructions,
148+
scheduler_config.canister_snapshot_data_baseline_instructions,
148149
));
149150
let sync_query_handler = Arc::new(InternalHttpQueryHandler::new(
150151
logger.clone(),

‎rs/execution_environment/src/scheduler/test_utilities.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -944,6 +944,8 @@ impl SchedulerTestBuilder {
944944
self.scheduler_config.upload_wasm_chunk_instructions,
945945
self.scheduler_config
946946
.canister_snapshot_baseline_instructions,
947+
self.scheduler_config
948+
.canister_snapshot_data_baseline_instructions,
947949
);
948950
let scheduler = SchedulerImpl::new(
949951
self.scheduler_config,

‎rs/test_utilities/execution_environment/src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1754,6 +1754,7 @@ pub struct ExecutionTestBuilder {
17541754
heap_delta_rate_limit: NumBytes,
17551755
upload_wasm_chunk_instructions: NumInstructions,
17561756
canister_snapshot_baseline_instructions: NumInstructions,
1757+
canister_snapshot_data_baseline_instructions: NumInstructions,
17571758
replica_version: ReplicaVersion,
17581759
precompiled_universal_canister: bool,
17591760
cycles_account_manager_config: Option<CyclesAccountManagerConfig>,
@@ -1799,6 +1800,8 @@ impl Default for ExecutionTestBuilder {
17991800
upload_wasm_chunk_instructions: scheduler_config.upload_wasm_chunk_instructions,
18001801
canister_snapshot_baseline_instructions: scheduler_config
18011802
.canister_snapshot_baseline_instructions,
1803+
canister_snapshot_data_baseline_instructions: scheduler_config
1804+
.canister_snapshot_data_baseline_instructions,
18021805
replica_version: ReplicaVersion::default(),
18031806
precompiled_universal_canister: true,
18041807
cycles_account_manager_config: None,
@@ -2415,6 +2418,7 @@ impl ExecutionTestBuilder {
24152418
self.heap_delta_rate_limit,
24162419
self.upload_wasm_chunk_instructions,
24172420
self.canister_snapshot_baseline_instructions,
2421+
self.canister_snapshot_data_baseline_instructions,
24182422
);
24192423
let (query_stats_collector, _) =
24202424
ic_query_stats::init_query_stats(self.log.clone(), &config, &metrics_registry);

0 commit comments

Comments
 (0)