Proposal 133900
Vote: Adopt
Reason: Build is successful, hashes match, commits match descriptions and the reasoning behind the changes is sound.
As this is my first review in this topic, some of the allocated time was spent setting up a server structure for verification (which took longer than expected due to some glitches) and bringing myself up to speed on the current state of the protocol. For this review I’ve focused selectively on consensus-related commits with the intention of broadening the focus for future reviews.
Review
Features:
[e7b16eab4]
Increases DYNAMIC_DELAY_MAX_NON_RANK_0_BLOCKS from 5 to 10, meaning that once 10 (up from 5) non-rank-0 blocks have been notarised within the previous 30 rounds, a dynamic delay of up to 10 seconds is triggered in which blocks from rank-0 block makers have priority, in order to combat a scenario in which malicious nodes are able to force their own blocks to be notarised. The threshold is being increased because the delay was being triggered a bit too often, leading to potential network slowing.
[e3c408cd0]
Changes the is_latency_sensitive flag from false to true, with the effect that all ingress messages are pushed to nodes in full, instead of only those messages smaller than 1024 bytes with adverts being used for larger messages, in order to reduce latency.
[c5e9e2af1]
Reduces the consensus bouncer function refresh period in order to allow faster catch-up for lagging nodes, which is necessary now that all subnets bar one have had their notarisation delay reduced to 0.3 seconds and can therefore can produce a significantly faster rate of blocks.
[39f67a628]
Adds VetKd variant to the MasterPublicKeyId enum as the first step in the implementation of the VetKeys encryption feature.
Bugfixes:
[f306fcb8c]
Adds field certification_share_height to NodeMetrics. Within the recovery get_node_metrics function, which fetches a response from nodes, the certification_height metric is now taken from the validated artifact pool (as is the new metric) instead of using the last certified height from the state manager.
[3d590ec5a]
Removes async / await from several lines in the orchestrator code + related minor changes.
[e70f04d9f]
Adds format and example to printed instructions to enter an SSH key.
[55297ccfd]
Updates 2 references from nns_url to nns_urls in keeping with an earlier update.
Chores:
[80dc25b52]
Moves get_adjusted_notary_delay and get_adjusted_notary_delay_from_settings functions from rs/consensus/utils/src/lib.rs to rs/consensus/src/consensus/notary.rs + related code changes.
[d3cfffac1]
Changes some of the metrics for the certification pool. In particular, adds “per type” metrics for the height of objects and number of artifacts in a certification pool.
[4f7cebee1]
Replaces current_replica_version with block.version() in deliver_batches function for the purpose of logging a replica version during batch delivery, and removes a redundant second log.
[10150fb34]
Renames certification_pool.persistent_pool to certification_pool.validated.
Proposal 133901
Vote: Adopt
Reason: Build is successful. Hashes match. Only one commit, matching description.
[cc1319059]
Switches the HASHES_IN_BLOCKS_FEATURE_ENABLED flag to true in order to implement the hashes-in-block feature. This feature adds only the hashes of ingress messages to blocks instead of the messages in full, with the aim of increasing throughput, and is explained in detail here.
Proposal 133902
Vote: Adopt
Reason: Build is successful. Hashes match. Only one commit, matching description.
[f910b32ef]
Upgrades Linux kernel version to 6.11 and installs the kernel. (The commit notes should specify “Linux kernel” but do not.)