Skip to content

Commit

Permalink
feat: [IC-272] add log_visibility to canister_settings
Browse files Browse the repository at this point in the history
  • Loading branch information
maksymar committed Jan 23, 2024
1 parent 9a1d428 commit 4bb93cb
Show file tree
Hide file tree
Showing 18 changed files with 239 additions and 12 deletions.
2 changes: 1 addition & 1 deletion rs/cycles_account_manager/src/lib.rs
Expand Up @@ -44,7 +44,7 @@ const SECONDS_PER_DAY: u128 = 24 * 60 * 60;

/// Maximum payload size of a management call to update_settings
/// overriding the canister's freezing threshold.
const MAX_DELAYED_INGRESS_COST_PAYLOAD_SIZE: usize = 200;
const MAX_DELAYED_INGRESS_COST_PAYLOAD_SIZE: usize = 238;

This comment has been minimized.

Copy link
@ilbertt

ilbertt Jan 27, 2024

@maksymar what's the reason of this increase?

This comment has been minimized.

Copy link
@maksymar

maksymar Jan 29, 2024

Author Contributor

The reason for this increase is that an extra field log_visibility was added to CanisterSettingsArgs here.

  • CanisterSettingsArgs is a field of UpdateSettingsArgs
  • the total size of an UpdateSettings payload is checked in is_delayed_ingress_induction_cost() against MAX_DELAYED_INGRESS_COST_PAYLOAD_SIZE here.

This comment has been minimized.

Copy link
@ilbertt

ilbertt Jan 29, 2024

Thanks for the clarification.
My question was also related to what is the calculation that gives you a +38 increase. Could you please clarify also this point?

This comment has been minimized.

Copy link
@maksymar

maksymar Jan 29, 2024

Author Contributor

log_visibility field is an optional field of LogVisibility type, which is an enum that uses CandidType derive. in total it adds an extra 19 bytes, which might seem excessive for a simple enum with only two variants, but that's because of candid serialization (includes extra info for type identification and other protocol-related details).

additionally there's a max_delayed_ingress_cost_payload_size_test test here that checks the size of UpdateSettingsArgs with a safety margin factor of x2, which in total adds +38 to initial 200 bytes. hope that makes sense.

This comment has been minimized.

Copy link
@ilbertt

ilbertt Jan 29, 2024

Yes that makes sense! Thanks a lot for the clarification!


/// Errors returned by the [`CyclesAccountManager`].
#[derive(Clone, Debug, PartialEq, Eq)]
Expand Down
5 changes: 5 additions & 0 deletions rs/execution_environment/src/canister_manager.rs
Expand Up @@ -628,6 +628,9 @@ impl CanisterManager {
if let Some(freezing_threshold) = settings.freezing_threshold() {
canister.system_state.freeze_threshold = freezing_threshold;
}
if let Some(log_visibility) = settings.log_visibility() {
canister.system_state.log_visibility = log_visibility;
}
}

/// Tries to apply the requested settings on the canister identified by
Expand Down Expand Up @@ -1164,6 +1167,7 @@ impl CanisterManager {
let memory_allocation = canister.memory_allocation();
let freeze_threshold = canister.system_state.freeze_threshold;
let reserved_cycles_limit = canister.system_state.reserved_balance_limit();
let log_visibility = canister.system_state.log_visibility;

Ok(CanisterStatusResultV2::new(
canister.status(),
Expand All @@ -1179,6 +1183,7 @@ impl CanisterManager {
Some(memory_allocation.bytes().get()),
freeze_threshold.get(),
reserved_cycles_limit.map(|x| x.get()),
log_visibility,
self.cycles_account_manager
.idle_cycles_burned_rate(
memory_allocation,
Expand Down
26 changes: 25 additions & 1 deletion rs/execution_environment/src/canister_settings.rs
@@ -1,7 +1,7 @@
use ic_base_types::{NumBytes, NumSeconds};
use ic_cycles_account_manager::{CyclesAccountManager, ResourceSaturation};
use ic_error_types::{ErrorCode, UserError};
use ic_ic00_types::CanisterSettingsArgs;
use ic_ic00_types::{CanisterSettingsArgs, LogVisibility};
use ic_interfaces::execution_environment::SubnetAvailableMemory;
use ic_types::{
ComputeAllocation, Cycles, InvalidComputeAllocationError, InvalidMemoryAllocationError,
Expand All @@ -21,6 +21,7 @@ pub(crate) struct CanisterSettings {
pub(crate) memory_allocation: Option<MemoryAllocation>,
pub(crate) freezing_threshold: Option<NumSeconds>,
pub(crate) reserved_cycles_limit: Option<Cycles>,
pub(crate) log_visibility: Option<LogVisibility>,
}

impl CanisterSettings {
Expand All @@ -31,6 +32,7 @@ impl CanisterSettings {
memory_allocation: Option<MemoryAllocation>,
freezing_threshold: Option<NumSeconds>,
reserved_cycles_limit: Option<Cycles>,
log_visibility: Option<LogVisibility>,
) -> Self {
Self {
controller,
Expand All @@ -39,6 +41,7 @@ impl CanisterSettings {
memory_allocation,
freezing_threshold,
reserved_cycles_limit,
log_visibility,
}
}

Expand All @@ -65,6 +68,10 @@ impl CanisterSettings {
pub fn reserved_cycles_limit(&self) -> Option<Cycles> {
self.reserved_cycles_limit
}

pub fn log_visibility(&self) -> Option<LogVisibility> {
self.log_visibility
}
}

impl TryFrom<CanisterSettingsArgs> for CanisterSettings {
Expand Down Expand Up @@ -111,6 +118,7 @@ impl TryFrom<CanisterSettingsArgs> for CanisterSettings {
memory_allocation,
freezing_threshold,
reserved_cycles_limit,
input.log_visibility,
))
}
}
Expand All @@ -133,6 +141,7 @@ pub(crate) struct CanisterSettingsBuilder {
memory_allocation: Option<MemoryAllocation>,
freezing_threshold: Option<NumSeconds>,
reserved_cycles_limit: Option<Cycles>,
log_visibility: Option<LogVisibility>,
}

#[allow(dead_code)]
Expand All @@ -145,6 +154,7 @@ impl CanisterSettingsBuilder {
memory_allocation: None,
freezing_threshold: None,
reserved_cycles_limit: None,
log_visibility: None,
}
}

Expand All @@ -156,6 +166,7 @@ impl CanisterSettingsBuilder {
memory_allocation: self.memory_allocation,
freezing_threshold: self.freezing_threshold,
reserved_cycles_limit: self.reserved_cycles_limit,
log_visibility: self.log_visibility,
}
}

Expand Down Expand Up @@ -200,6 +211,13 @@ impl CanisterSettingsBuilder {
..self
}
}

pub fn with_log_visibility(self, log_visibility: LogVisibility) -> Self {
Self {
log_visibility: Some(log_visibility),
..self
}
}
}

pub enum UpdateSettingsError {
Expand Down Expand Up @@ -266,6 +284,7 @@ pub(crate) struct ValidatedCanisterSettings {
freezing_threshold: Option<NumSeconds>,
reserved_cycles_limit: Option<Cycles>,
reservation_cycles: Cycles,
log_visibility: Option<LogVisibility>,
}

impl ValidatedCanisterSettings {
Expand Down Expand Up @@ -296,6 +315,10 @@ impl ValidatedCanisterSettings {
pub fn reservation_cycles(&self) -> Cycles {
self.reservation_cycles
}

pub fn log_visibility(&self) -> Option<LogVisibility> {
self.log_visibility
}
}

/// Validates the new canisters settings:
Expand Down Expand Up @@ -492,5 +515,6 @@ pub(crate) fn validate_canister_settings(
freezing_threshold: settings.freezing_threshold(),
reserved_cycles_limit: settings.reserved_cycles_limit(),
reservation_cycles,
log_visibility: settings.log_visibility(),
})
}
1 change: 1 addition & 0 deletions rs/execution_environment/src/execution/install_code.rs
Expand Up @@ -439,6 +439,7 @@ impl InstallCodeHelper {
memory_allocation: original.requested_memory_allocation,
freezing_threshold: None,
reserved_cycles_limit: None,
log_visibility: None,
},
self.canister.memory_usage(),
self.canister.message_memory_usage(),
Expand Down
60 changes: 57 additions & 3 deletions rs/execution_environment/src/execution_environment/tests.rs
Expand Up @@ -11,9 +11,9 @@ use ic_error_types::{ErrorCode, RejectCode, UserError};
use ic_ic00_types::{
self as ic00, BitcoinGetUtxosArgs, BitcoinNetwork, BoundedHttpHeaders, CanisterChange,
CanisterHttpRequestArgs, CanisterIdRecord, CanisterStatusResultV2, CanisterStatusType,
DerivationPath, EcdsaCurve, EcdsaKeyId, EmptyBlob, HttpMethod, Method, Payload as Ic00Payload,
ProvisionalCreateCanisterWithCyclesArgs, ProvisionalTopUpCanisterArgs, TransformContext,
TransformFunc, IC_00,
DerivationPath, EcdsaCurve, EcdsaKeyId, EmptyBlob, HttpMethod, LogVisibility, Method,
Payload as Ic00Payload, ProvisionalCreateCanisterWithCyclesArgs, ProvisionalTopUpCanisterArgs,
TransformContext, TransformFunc, IC_00,
};
use ic_registry_routing_table::canister_id_into_u64;
use ic_registry_routing_table::CanisterIdRange;
Expand Down Expand Up @@ -3215,3 +3215,57 @@ fn test_ingress_snapshot_rejected_because_feature_is_disabled() {
assert_eq!(result, expected_result);
}
}

#[test]
fn test_canister_settings_log_visibility_default_controllers() {
// Arrange.
let mut test = ExecutionTestBuilder::new().build();
let canister_id = test.create_canister(Cycles::new(1_000_000_000));
// Act.
let result = test.canister_status(canister_id);
let canister_status = CanisterStatusResultV2::decode(&get_reply(result)).unwrap();
// Assert.
assert_eq!(
canister_status.settings().log_visibility(),
LogVisibility::Controllers
);
}

#[test]
fn test_canister_settings_log_visibility_create_with_settings() {
// Arrange.
let mut test = ExecutionTestBuilder::new().build();
// Act.
let canister_id = test
.create_canister_with_settings(
Cycles::new(1_000_000_000),
ic00::CanisterSettingsArgsBuilder::new()
.with_log_visibility(LogVisibility::Public)
.build(),
)
.unwrap();
let result = test.canister_status(canister_id);
let canister_status = CanisterStatusResultV2::decode(&get_reply(result)).unwrap();
// Assert.
assert_eq!(
canister_status.settings().log_visibility(),
LogVisibility::Public
);
}

#[test]
fn test_canister_settings_log_visibility_set_to_public() {
// Arrange.
let mut test = ExecutionTestBuilder::new().build();
let canister_id = test.create_canister(Cycles::new(1_000_000_000));
// Act.
test.set_log_visibility(canister_id, LogVisibility::Public)
.unwrap();
let result = test.canister_status(canister_id);
let canister_status = CanisterStatusResultV2::decode(&get_reply(result)).unwrap();
// Assert.
assert_eq!(
canister_status.settings().log_visibility(),
LogVisibility::Public
);
}
5 changes: 5 additions & 0 deletions rs/nns/cmc/cmc.did
@@ -1,12 +1,17 @@
type Cycles = nat;
type BlockIndex = nat64;
type log_visibility = variant {
controllers;
public;
};
type CanisterSettings = record {
controller : opt principal;
controllers : opt vec principal;
compute_allocation : opt nat;
memory_allocation : opt nat;
freezing_threshold : opt nat;
reserved_cycles_limit: opt nat;
log_visibility : opt log_visibility;
};
type Subaccount = opt blob;
type Memo = opt blob;
Expand Down
Expand Up @@ -294,6 +294,12 @@ message WasmChunkStoreMetadata {
uint64 size = 2;
}

enum LogVisibility {
LOG_VISIBILITY_UNSPECIFIED = 0;
LOG_VISIBILITY_CONTROLLERS = 1;
LOG_VISIBILITY_PUBLIC = 2;
}

message CanisterStateBits {
reserved 1;
reserved "controller";
Expand Down Expand Up @@ -362,4 +368,6 @@ message CanisterStateBits {
WasmChunkStoreMetadata wasm_chunk_store_metadata = 40;
// Statistics on query execution for entire lifetime of canister.
TotalQueryStats total_query_stats = 41;
// Log visibility for the canister.
LogVisibility log_visibility = 42;
}
32 changes: 32 additions & 0 deletions rs/protobuf/src/gen/state/state.canister_state_bits.v1.rs
Expand Up @@ -603,6 +603,9 @@ pub struct CanisterStateBits {
/// Statistics on query execution for entire lifetime of canister.
#[prost(message, optional, tag = "41")]
pub total_query_stats: ::core::option::Option<TotalQueryStats>,
/// Log visibility for the canister.
#[prost(enumeration = "LogVisibility", tag = "42")]
pub log_visibility: i32,
#[prost(oneof = "canister_state_bits::CanisterStatus", tags = "11, 12, 13")]
pub canister_status: ::core::option::Option<canister_state_bits::CanisterStatus>,
}
Expand Down Expand Up @@ -743,3 +746,32 @@ impl CyclesUseCase {
}
}
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, ::prost::Enumeration)]
#[repr(i32)]
pub enum LogVisibility {
Unspecified = 0,
Controllers = 1,
Public = 2,
}
impl LogVisibility {
/// String value of the enum field names used in the ProtoBuf definition.
///
/// The values are not transformed in any way and thus are considered stable
/// (if the ProtoBuf definition does not change) and safe for programmatic use.
pub fn as_str_name(&self) -> &'static str {
match self {
LogVisibility::Unspecified => "LOG_VISIBILITY_UNSPECIFIED",
LogVisibility::Controllers => "LOG_VISIBILITY_CONTROLLERS",
LogVisibility::Public => "LOG_VISIBILITY_PUBLIC",
}
}
/// Creates an enum from field names used in the ProtoBuf definition.
pub fn from_str_name(value: &str) -> ::core::option::Option<Self> {
match value {
"LOG_VISIBILITY_UNSPECIFIED" => Some(Self::Unspecified),
"LOG_VISIBILITY_CONTROLLERS" => Some(Self::Controllers),
"LOG_VISIBILITY_PUBLIC" => Some(Self::Public),
_ => None,
}
}
}
4 changes: 3 additions & 1 deletion rs/replica_tests/tests/canister_lifecycle.rs
Expand Up @@ -6,7 +6,7 @@ use ic_error_types::{ErrorCode, RejectCode};
use ic_ic00_types::{
self as ic00, CanisterChange, CanisterIdRecord, CanisterInstallMode,
CanisterSettingsArgsBuilder, CanisterStatusResultV2, CanisterStatusType, EmptyBlob,
InstallCodeArgs, Method, Payload, UpdateSettingsArgs, IC_00,
InstallCodeArgs, LogVisibility, Method, Payload, UpdateSettingsArgs, IC_00,
};
use ic_registry_provisional_whitelist::ProvisionalWhitelist;
use ic_replica_tests as utils;
Expand Down Expand Up @@ -712,6 +712,7 @@ fn can_get_canister_information() {
None,
2592000,
Some(5_000_000_000_000u128),
LogVisibility::default(),
0u128,
0u128,
0u128,
Expand Down Expand Up @@ -769,6 +770,7 @@ fn can_get_canister_information() {
None,
259200,
None,
LogVisibility::default(),
0u128,
0u128,
0u128,
Expand Down
8 changes: 7 additions & 1 deletion rs/replicated_state/src/canister_state/system_state.rs
Expand Up @@ -10,7 +10,7 @@ use crate::page_map::PageAllocatorFileDescriptor;
use crate::{CanisterQueues, CanisterState, InputQueueType, PageMap, StateError};
pub use call_context_manager::{CallContext, CallContextAction, CallContextManager, CallOrigin};
use ic_base_types::NumSeconds;
use ic_ic00_types::{CanisterChange, CanisterChangeDetails, CanisterChangeOrigin};
use ic_ic00_types::{CanisterChange, CanisterChangeDetails, CanisterChangeOrigin, LogVisibility};
use ic_logger::{error, ReplicaLogger};
use ic_protobuf::{
proxy::{try_from_option_field, ProxyDecodeError},
Expand Down Expand Up @@ -331,6 +331,9 @@ pub struct SystemState {

/// Store of Wasm chunks to support installation of large Wasm modules.
pub wasm_chunk_store: WasmChunkStore,

/// Log visibility of the canister.
pub log_visibility: LogVisibility,
}

/// A wrapper around the different canister statuses.
Expand Down Expand Up @@ -700,6 +703,7 @@ impl SystemState {
canister_version: 0,
canister_history: CanisterHistory::default(),
wasm_chunk_store,
log_visibility: LogVisibility::default(),
}
}

Expand Down Expand Up @@ -743,6 +747,7 @@ impl SystemState {
canister_history: CanisterHistory,
wasm_chunk_store_data: PageMap,
wasm_chunk_store_metadata: WasmChunkStoreMetadata,
log_visibility: LogVisibility,
) -> Self {
Self {
controllers,
Expand All @@ -765,6 +770,7 @@ impl SystemState {
wasm_chunk_store_data,
wasm_chunk_store_metadata,
),
log_visibility,
}
}

Expand Down

0 comments on commit 4bb93cb

Please sign in to comment.