Proposals 137921 & 137922 | Tim - CodeGov
Vote: Adopt
Reason: Build is successful, hashes match, commits match descriptions and the reasoning behind the changes is sound. HostOS commits (proposal 137922) mostly overlap with GuestOS commits (proposal 137921) I’ve reviewed commits for Consensus, Crypto, Interface and Node as detailed below.
Review
Features:
[da40cb4ea]
Adds enum MasterPublicKeyId with key identifiers, and methods PublicKey::mainnet_key to return the public production master keys and PublicKey::derive_mainnet_key to derive a public key from the mainnet parameters (key ID, canister ID and derivation path) for both for Ed25519 signatures and for ECDSA and Schnorr keys over the secp256k1 curve.
[388980813]
Replaces type LineDisplayPage with FieldsDisplay, containing a set of standardised fields (TokenAmount etc), and expands and adapts the associated code so that messages returned by the ledgers can conform to ICRC-21, which implements human-readable consent messages for canister calls.
[84538856c]
Changes field guest_launch_measurement_sha256_hex to guest_launch_measurement within type ReviseElectedGuestosVersionsPayload. The changed field can be seen in the current GuestOS proposal (against the previous one for comparison). At its lowest level, the new field contains two subfields - measurement, representing the SEV-SNP measurement in a vector of 48 bytes, and metadata, optionally containing the command line string used to launch the guest. The value of this “measurement” is the hash of the replica image running on the virtual machine. Also related code and test changes as outlined in the commit description.
[70310a5fb]
Adds an update method remove_approval to the ICP ledger canister, which enables a user to revoke approval for another user (or other principal) to transfer tokens from their specified subaccount, as outlined further in the commit notes.
[6290490f9]
Applies parallel mapping to the code that is used for determining which files to set as read-only in CheckpointLayout::mark_files_readonly_and_sync.
[1905e1dd8]
Adds new directory os_tools/guest_disk/, comprising a tool which handles the formatting and activation of encrypted disk partitions in the GuestOS, using either SEV-based encryption or generated key-based encryption depending on whether the trusted execution environment is enabled, along with related changes as outlined in the commit description.
Bugfixes:
[705ab6ab7]
Adapts Orchestrator::start_tasks, ImageUpgrader::execute_upgrade and related code to ensure that the upgrade loop is stopped in conjunction with other orchestrator tasks after the orchestrator detects an upgrade.
[358c24213]
Changes the read state request format as explained in the commit notes and linked documentation.
Performance improvements:
[8a9e16366]
Removes anyhow as a dependency from 6 internal crates. Adds a custom error type P2PError, which is used to replace the anyhow::Error type in these crates for the sake of efficiency.
[552295bb4]
Re-combines message ingress channels for peers (other nodes on the same subnet) and users (accessing the subnet via boundary nodes), effectively reverting commit 5cce4f5cb. Amongst other changes, the old version of the create_ingress_handlers function takes channel and user_ingress_rx as two of its inputs, then merges channel.inbound_rx (containing peer messages) and user_ingress_rx (wrapped in a tokio stream) to pass to run_artifact_processor, whereas the new version just uses channel.inbound_rx for the same purpose. The channel parameter is of type AbortableBroadcastChannel, which in the new version has an added field inbound_tx to represent the user ingress channel. The reasoning for the change is explained at length in the commit description.
Chores:
[09571b845]
Renames type TestSigInputs to TestPreSigRef. Similarly renames various test functions - create_sig_inputs_with_height to create_pre_sig_ref_with_height and so on.
[34f9ec20d]
Modifies logging of response decoding in try_fetch_delegation_from_nns so that in the event of an error the error message also contains the raw response.
[da30c0d38]
Removes 2 unused logging messages relating to canister transforms.
[39c358e8a]
Adapts get_oldest_idkg_state_registry_version so that it considers the full ReplicatedState rather than being limited to the provided IDkgPayload (which is dropped as a parameter in the new version) when determining the oldest registry version of transcripts. This is then utilised in CatchUpPackageMaker::consider_block for determing which blocks should be included in catch-up package. This in turn affects when a node is allowed to leave the subnet as explained in the commit notes.
[65bb95e42]
Adds field transcript_resolution_errors to type IDkgStats, which is renamed to IDkgPayloadStats and moved from consensus/metrics.rs to idkg/src/metrics.rs. This field is then used as a counter for criticial key generation transcript errors during batch delivery.
[09a91114e]
Adds two sets of functions for dealing with hash trees. lookup_lower_bound takes as inputs t, to denote a hash tree, prefix, to denote a subtree within it, and label, expressed as a vector of bytes, then finds the largest label less than or equal to label within the subtree and returns (if successful) this label and its sub-subtree. MixedHashTree::filter_builder allows the construction of a set of filters which can then be used in subsequent methods to return a hash tree with leaves and branches pruned (or not) as determined by the set of filters. Also adds related tests and bench.
[cddf2f8a9]
Removes migration code that is no longer needed following implementation of the ICRC-106 standard, which allows for the discovery of the index canister from the corresponding ledger canister.
[2ee6ac954]
Format changes to .did files (mostly just indentations), no expected impact on code behaviour.
[853d5f2b6]
Various type changes in backup and recovery code and tests.
[fde21389b]
Removes code related to and all mentions of Filebeat, a tool formerly used to fetch and push testnet logs, now that it has been disabled.
[f718b7dbf]
Updates GuestOS, HostOS and SetupOS base image container references.
Refactoring:
[3fc9c04dd]
Removes types ThresholdEcdsaSigInputsRef and ThresholdSchnorrSigInputsRef (which hold transcript references instead of the actual transcripts) along with the corresponding error types, as these are part of a signature creation step that is no longer needed. Adapts build_signature_inputs and ThresholdSignatureBuilder::get_completed_signature to account for the eliminated step, along with related code and test changes as outlined in @eichhorl 's excellent commit notes.
Tests:
[1d53767ab]
Adds nns_recovery_test.rs, containing the initial structure for this test, and related code changes elsewhere.
HostOS-only commits:
[7f8ccf7b3]
Opens outgoing SOCKS proxy connections and removes memory locking as per description.
About CodeGov 
CodeGov has a team of developers who review and vote independently on the following proposal topics: IC-OS Version Election, Protocol Canister Management, Subnet Management, API Boundary API Boundary Node Management, Node Admin and Participant Management. The CodeGov NNS known neuron is configured to follow our reviewers on these technical topics. We also have a group of Followees who vote independently on the Governance and the SNS & Neurons' Fund topics. We strive to be a credible and reliable Followee option that votes on every proposal and every proposal topic in the NNS. We also support decentralisation of SNS projects such as WaterNeuron, KongSwap, and Alice with a known neuron and credible Followees.
Learn more about CodeGov and its mission at codegov.org.