feat(nns): Allow Governance to request rewards from Node Reward canister instead of registry (behind flag) (#4693)

# What

This allows a test version of governance to rely on the new Node Rewards
canister to calculate node rewards. In the normal case, it will continue
to rely on Registry.

We also fix an issue in the Node Reward canister, where a query was used
when an update should have been used. Since this canister is not yet
considered "live" and this API endpoint should not yet be relied upon,
we are overriding the DIDC_CHECK on backwards compatibility.

# Why

This is part of the effort to allow node rewards to be calculated based
on useful work.

Author: max-dfinity

Cargo.lock

@@ -68,6 +68,7 @@ DEPENDENCIES = [
     "//rs/nns/gtc_accounts",
     "//rs/nns/handlers/root/interface",
     "//rs/nns/sns-wasm",
+    "//rs/node_rewards/canister/api",
     "//rs/protobuf",
     "//rs/registry/canister",
     "//rs/rust_canisters/http_types",

rs/nns/governance/BUILD.bazel

@@ -65,6 +65,7 @@ ic-nns-gtc-accounts = { path = "../gtc_accounts" }
 ic-nns-governance-api = { path = "./api" }
 ic-nns-governance-init = { path = "./init" }
 ic-nns-handler-root-interface = { path = "../handlers/root/interface" }
+ic-node-rewards-canister-api = { path = "../../node_rewards/canister/api" }
 ic-protobuf = { path = "../../protobuf" }
 ic-sns-init = { path = "../../sns/init" }                                                         # This is just for a couple of PB definitions.
 ic-sns-root = { path = "../../sns/root" }                                                         # This is just for a couple of PB definitions.

rs/nns/governance/Cargo.toml

@@ -67,6 +67,7 @@ use crate::{
         },
     },
     proposals::{call_canister::CallCanister, sum_weighted_voting_power},
+    use_node_provider_reward_canister,
 };
 use async_trait::async_trait;
 use candid::{Decode, Encode};
@@ -91,8 +92,8 @@ use ic_nns_common::{
 };
 use ic_nns_constants::{
     CYCLES_MINTING_CANISTER_ID, GENESIS_TOKEN_CANISTER_ID, GOVERNANCE_CANISTER_ID,
-    LIFELINE_CANISTER_ID, REGISTRY_CANISTER_ID, ROOT_CANISTER_ID, SNS_WASM_CANISTER_ID,
-    SUBNET_RENTAL_CANISTER_ID,
+    LIFELINE_CANISTER_ID, NODE_REWARDS_CANISTER_ID, REGISTRY_CANISTER_ID, ROOT_CANISTER_ID,
+    SNS_WASM_CANISTER_ID, SUBNET_RENTAL_CANISTER_ID,
 };
 use ic_nns_governance_api::{
     pb::v1::{
@@ -105,6 +106,9 @@ use ic_nns_governance_api::{
     proposal_validation,
     subnet_rental::SubnetRentalRequest,
 };
+use ic_node_rewards_canister_api::monthly_rewards::{
+    GetNodeProvidersMonthlyXdrRewardsRequest, GetNodeProvidersMonthlyXdrRewardsResponse,
+};
 use ic_protobuf::registry::dc::v1::AddOrRemoveDataCentersProposalPayload;
 use ic_sns_init::pb::v1::SnsInitPayload;
 use ic_sns_swap::pb::v1::{self as sns_swap_pb, Lifecycle, NeuronsFundParticipationConstraints};
@@ -120,6 +124,7 @@ use registry_canister::{
 };
 use rust_decimal::Decimal;
 use rust_decimal_macros::dec;
+use std::str::FromStr;
 use std::sync::Arc;
 use std::{
     borrow::Cow,
@@ -7721,8 +7726,15 @@ impl Governance {
         let mut rewards = vec![];
 
         // Maps node providers to their rewards in XDR
-        let xdr_permyriad_rewards: NodeProvidersMonthlyXdrRewards =
-            self.get_node_providers_monthly_xdr_rewards().await?;
+        let (reg_rewards, maybe_version) = self.get_node_providers_monthly_xdr_rewards().await?;
+        let registry_version = maybe_version.ok_or_else(|| {
+            GovernanceError::new_with_message(
+                ErrorType::External,
+                "Registry version was not available in the response to \
+                    get_node_providers_monthly_xdr_rewards, which indicates a problem."
+                    .to_string(),
+            )
+        })?;
 
         // The average (last 30 days) conversion rate from 10,000ths of an XDR to 1 ICP
         let icp_xdr_conversion_rate = self.get_average_icp_xdr_conversion_rate().await?.data;
@@ -7742,9 +7754,7 @@ impl Governance {
         // `rewards`
         for np in &self.heap_data.node_providers {
             if let Some(np_id) = &np.id {
-                let np_id_str = np_id.to_string();
-                let xdr_permyriad_reward =
-                    *xdr_permyriad_rewards.rewards.get(&np_id_str).unwrap_or(&0);
+                let xdr_permyriad_reward = *reg_rewards.get(np_id).unwrap_or(&0);
 
                 if let Some(reward_node_provider) =
                     get_node_provider_reward(np, xdr_permyriad_reward, xdr_permyriad_per_icp)
@@ -7759,8 +7769,6 @@ impl Governance {
             xdr_permyriad_per_icp: icp_xdr_conversion_rate.xdr_permyriad_per_icp,
         };
 
-        let registry_version = xdr_permyriad_rewards.registry_version.unwrap();
-
         Ok(MonthlyNodeProviderRewards {
             timestamp: self.env.now(),
             rewards,
@@ -7774,8 +7782,83 @@ impl Governance {
 
     /// A helper for the Registry's get_node_providers_monthly_xdr_rewards method
     async fn get_node_providers_monthly_xdr_rewards(
-        &mut self,
-    ) -> Result<NodeProvidersMonthlyXdrRewards, GovernanceError> {
+        &self,
+    ) -> Result<(BTreeMap<PrincipalId, u64>, Option<u64>), GovernanceError> {
+        if use_node_provider_reward_canister() {
+            self.get_node_providers_monthly_xdr_rewards_from_node_provider_reward_canister()
+                .await
+        } else {
+            self.get_node_providers_monthly_xdr_rewards_from_registry()
+                .await
+        }
+    }
+
+    async fn get_node_providers_monthly_xdr_rewards_from_node_provider_reward_canister(
+        &self,
+    ) -> Result<(BTreeMap<PrincipalId, u64>, Option<u64>), GovernanceError> {
+        let response: Vec<u8> = self.env.call_canister_method(
+            NODE_REWARDS_CANISTER_ID,
+            "get_node_providers_monthly_xdr_rewards",
+            Encode!(&GetNodeProvidersMonthlyXdrRewardsRequest {registry_version: None }).unwrap(),
+        ).await
+            .map_err(|(code, msg)| {
+                GovernanceError::new_with_message(
+                    ErrorType::External,
+                    format!(
+                        "Error calling 'get_node_providers_monthly_xdr_rewards': code: {:?}, message: {}",
+                        code, msg
+                    ),
+                )
+            })?;
+
+        let response =
+            Decode!(&response, GetNodeProvidersMonthlyXdrRewardsResponse).map_err(|err| {
+                GovernanceError::new_with_message(
+                    ErrorType::External,
+                    format!(
+                        "Cannot decode return type from get_node_providers_monthly_xdr_rewards \
+                        as GetNodeProvidersMonthlyXdrRewardsResponse'. Error: {}",
+                        err,
+                    ),
+                )
+            })?;
+
+        let GetNodeProvidersMonthlyXdrRewardsResponse { rewards, error } = response;
+
+        if let Some(err_msg) = error {
+            return Err(GovernanceError::new_with_message(
+                ErrorType::External,
+                format!(
+                    "Error calling 'get_node_providers_monthly_xdr_rewards': {}",
+                    err_msg
+                ),
+            ));
+        }
+
+        if let Some(rewards) = rewards {
+            let ic_node_rewards_canister_api::monthly_rewards::NodeProvidersMonthlyXdrRewards {
+                rewards,
+                registry_version,
+            } = rewards;
+            let rewards = rewards
+                .into_iter()
+                .map(|(principal, amount)| (PrincipalId::from(principal), amount))
+                .collect();
+            return Ok((rewards, registry_version));
+        }
+
+        Err(GovernanceError::new_with_message(
+            ErrorType::Unspecified,
+            "get_node_providers_monthly_xdr_rewards returned empty response, \
+                which should be impossible.",
+        ))
+    }
+
+    /// A helper to get the node provider rewards from registry (instead of Node Provider Reward Canister)
+    /// This will be removed once the Node Provider Reward Canister is in use.
+    async fn get_node_providers_monthly_xdr_rewards_from_registry(
+        &self,
+    ) -> Result<(BTreeMap<PrincipalId, u64>, Option<u64>), GovernanceError> {
         let registry_response:
             Vec<u8> = self
             .env
@@ -7796,14 +7879,35 @@ impl Governance {
             })?;
 
         Decode!(&registry_response, Result<NodeProvidersMonthlyXdrRewards, String>)
-            .map_err(|err| GovernanceError::new_with_message(
-                ErrorType::External,
-                format!(
-                    "Cannot decode return type from get_node_providers_monthly_xdr_rewards'. Error: {}",
-                    err,
-                ),
-            ))?
+            .map_err(|err| {
+                GovernanceError::new_with_message(
+                    ErrorType::External,
+                    format!(
+                        "Cannot decode return type from get_node_providers_monthly_xdr_rewards'. Error: {}",
+                        err,
+                    ),
+                )
+            })?
             .map_err(|msg| GovernanceError::new_with_message(ErrorType::External, msg))
+            .map(|response| {
+                let NodeProvidersMonthlyXdrRewards {
+                    rewards,
+                    registry_version,
+                } = response;
+
+                let rewards = rewards
+                    .into_iter()
+                    .map(|(principal_str, amount)| {
+                        (
+                            PrincipalId::from_str(&principal_str)
+                                .expect("Could not get principal from string"),
+                            amount,
+                        )
+                    })
+                    .collect();
+
+                (rewards, registry_version)
+            })
     }
 
     /// A helper for the CMC's get_average_icp_xdr_conversion_rate method

rs/nns/governance/src/governance.rs

@@ -212,6 +212,9 @@ thread_local! {
 
     static IS_DISBURSE_MATURITY_ENABLED: Cell<bool>
         = const { Cell::new(cfg!(not(any(feature = "canbench-rs", feature = "test")))) };
+
+    static USE_NODE_PROVIDER_REWARD_CANISTER: Cell<bool>
+        = const { Cell::new(cfg!(feature = "test")) };
 }
 
 thread_local! {
@@ -317,6 +320,20 @@ pub fn temporarily_disable_disburse_maturity() -> Temporary {
     Temporary::new(&IS_DISBURSE_MATURITY_ENABLED, false)
 }
 
+pub fn use_node_provider_reward_canister() -> bool {
+    USE_NODE_PROVIDER_REWARD_CANISTER.get()
+}
+
+#[cfg(any(test, feature = "canbench-rs", feature = "test"))]
+pub fn temporarily_enable_node_provider_reward_canister() -> Temporary {
+    Temporary::new(&USE_NODE_PROVIDER_REWARD_CANISTER, true)
+}
+
+#[cfg(any(test, feature = "canbench-rs", feature = "test"))]
+pub fn temporarily_disable_node_provider_reward_canister() -> Temporary {
+    Temporary::new(&USE_NODE_PROVIDER_REWARD_CANISTER, false)
+}
+
 pub fn decoder_config() -> DecoderConfig {
     let mut config = DecoderConfig::new();
     config.set_skipping_quota(DEFAULT_SKIPPING_QUOTA);

rs/nns/governance/src/lib.rs

@@ -13,8 +13,8 @@ use ic_nns_common::{
     types::UpdateIcpXdrConversionRatePayload,
 };
 use ic_nns_constants::{
-    CYCLES_MINTING_CANISTER_ID, GOVERNANCE_CANISTER_ID, LEDGER_CANISTER_ID, REGISTRY_CANISTER_ID,
-    SNS_WASM_CANISTER_ID,
+    CYCLES_MINTING_CANISTER_ID, GOVERNANCE_CANISTER_ID, LEDGER_CANISTER_ID,
+    NODE_REWARDS_CANISTER_ID, REGISTRY_CANISTER_ID, SNS_WASM_CANISTER_ID,
 };
 use ic_nns_governance::{
     governance::{Environment, Governance, HeapGrowthPotential, RngError},
@@ -23,14 +23,15 @@ use ic_nns_governance::{
         GovernanceError, ManageNeuron, Motion, NetworkEconomics, Neuron, NnsFunction, Proposal,
         Vote,
     },
+    use_node_provider_reward_canister,
 };
 use ic_nns_governance_api::pb::v1::{manage_neuron_response, ManageNeuronResponse};
 use ic_sns_root::{GetSnsCanistersSummaryRequest, GetSnsCanistersSummaryResponse};
 use ic_sns_swap::pb::v1 as sns_swap_pb;
 use ic_sns_wasm::pb::v1::{DeployedSns, ListDeployedSnsesRequest, ListDeployedSnsesResponse};
 use icp_ledger::{AccountIdentifier, Subaccount, Tokens};
 use lazy_static::lazy_static;
-use maplit::hashmap;
+use maplit::{btreemap, hashmap};
 use rand::{RngCore, SeedableRng};
 use rand_chacha::ChaCha20Rng;
 use registry_canister::pb::v1::NodeProvidersMonthlyXdrRewards;
@@ -50,6 +51,7 @@ use ic_nns_governance::governance::tla::{
 };
 use ic_nns_governance::governance::RandomnessGenerator;
 use ic_nns_governance::{tla_log_request, tla_log_response};
+use ic_node_rewards_canister_api::monthly_rewards::GetNodeProvidersMonthlyXdrRewardsResponse;
 
 lazy_static! {
     pub(crate) static ref SNS_ROOT_CANISTER_ID: PrincipalId = PrincipalId::new_user_test_id(213599);
@@ -566,17 +568,32 @@ impl Environment for FakeDriver {
         }
 
         if method_name == "get_node_providers_monthly_xdr_rewards" {
-            assert_eq!(PrincipalId::from(target), REGISTRY_CANISTER_ID.get());
-
-            return Ok(Encode!(&Ok::<NodeProvidersMonthlyXdrRewards, String>(
-                NodeProvidersMonthlyXdrRewards {
-                    rewards: hashmap! {
-                        PrincipalId::new_user_test_id(1).to_string() => NODE_PROVIDER_REWARD,
-                    },
-                    registry_version: Some(5)
-                }
-            ))
-            .unwrap());
+            if use_node_provider_reward_canister() {
+                assert_eq!(PrincipalId::from(target), NODE_REWARDS_CANISTER_ID.get());
+
+                return Ok(Encode!(&GetNodeProvidersMonthlyXdrRewardsResponse {
+                    rewards: Some(ic_node_rewards_canister_api::monthly_rewards::NodeProvidersMonthlyXdrRewards {
+                        rewards: btreemap! {
+                            PrincipalId::new_user_test_id(1).0 => NODE_PROVIDER_REWARD,
+                        },
+                        registry_version: Some(5)
+                    }),
+                    error: None
+                })
+                .unwrap());
+            } else {
+                assert_eq!(PrincipalId::from(target), REGISTRY_CANISTER_ID.get());
+
+                return Ok(Encode!(&Ok::<NodeProvidersMonthlyXdrRewards, String>(
+                    NodeProvidersMonthlyXdrRewards {
+                        rewards: hashmap! {
+                            PrincipalId::new_user_test_id(1).to_string() => NODE_PROVIDER_REWARD,
+                        },
+                        registry_version: Some(5)
+                    }
+                ))
+                .unwrap());
+            }
         }
 
         if method_name == "get_average_icp_xdr_conversion_rate" {

rs/nns/governance/tests/fake.rs

@@ -4,11 +4,12 @@ In general, upcoming/unreleased behavior changes are described here. For details
 on the process that this file is part of, see
 `rs/nervous_system/changelog_process.md`.
 
-
 # Next Upgrade Proposal
 
 ## Added
 
+* Governance now gets node provider rewards from the Node Reward Canister in test builds.
+
 ## Changed
 
 * The `_pb` methods now always panic.

rs/nns/governance/unreleased_changelog.md

@@ -198,6 +198,7 @@ rust_ic_test_suite(
     ),
     aliases = ALIASES,
     data = [
+        # Keep sorted.
         ":root-canister",
         ":upgrade-test-canister",
         "//rs/ledger_suite/icp/ledger:ledger-canister-wasm-notify-method",
@@ -206,6 +207,7 @@ rust_ic_test_suite(
         "//rs/nns/gtc:genesis-token-canister",
         "//rs/nns/handlers/lifeline/impl:lifeline_canister",
         "//rs/nns/sns-wasm:sns-wasm-canister",
+        "//rs/node_rewards/canister:node-rewards-canister",
         "//rs/registry/canister:registry-canister",
         "//rs/universal_canister/impl:universal_canister.wasm.gz",
     ],
@@ -216,6 +218,7 @@ rust_ic_test_suite(
         "GOVERNANCE_CANISTER_TEST_WASM_PATH": "$(rootpath //rs/nns/governance:governance-canister-test)",
         "LEDGER_CANISTER_NOTIFY_METHOD_WASM_PATH": "$(rootpath //rs/ledger_suite/icp/ledger:ledger-canister-wasm-notify-method)",
         "LIFELINE_WASM_PATH": "$(rootpath //rs/nns/handlers/lifeline/impl:lifeline_canister)",
+        "NODE_REWARDS_CANISTER_WASM_PATH": "$(rootpath //rs/node_rewards/canister:node-rewards-canister)",
         "REGISTRY_CANISTER_WASM_PATH": "$(rootpath //rs/registry/canister:registry-canister)",
         "ROOT_CANISTER_WASM_PATH": "$(rootpath //rs/nns/handlers/root/impl:root-canister)",
         "SNS_WASM_CANISTER_WASM_PATH": "$(rootpath //rs/nns/sns-wasm:sns-wasm-canister)",

rs/nns/handlers/root/impl/BUILD.bazel

@@ -802,7 +802,7 @@ fn test_cmc_automatically_refunds_when_memo_is_garbage() {
     let assert_canister_statuses_fixed = |test_phase| {
         assert_eq!(
             btreemap! {
-                btreemap! { "status".to_string() => "running".to_string() } => 11,
+                btreemap! { "status".to_string() => "running".to_string() } => 17,
                 btreemap! { "status".to_string() => "stopped".to_string() } => 0,
                 btreemap! { "status".to_string() => "stopping".to_string() } => 0,
             },