Merge branch 'NNS1-1214' into 'master'

NNS1-1214: Continue to latest_tally after a proposal is decided early until deadline

Previously, the ProposalData::latest_tally is recomputed within process_propossal() when the proposal is open, in 3 cases: (1) when proposal is created (2) for each register_vote (3) process_proposal() triggered by heartbeat (although only the timestamp would be changed).

This change tries to move recompute_tally() out of process_proposal() and only call it for a new vote (i.e. ballots are changed). Therefore every time a new vote is cast, the tally is recomputed regardless of the proposal's status.

Assumptions:
1. It's OK to update latest_tally less often, as long as the tally reflects the ballots correctly (no need for Tally::timestamp_seconds to be updated if yes/no is unchanged)
2. ProposalData::ballots can only be changed by register_vote after proposal initialization
3. The process_proposal() call within the distritributed_rewards() should not cause tally change 

See merge request dfinity-lab/public/ic!12551

Author: jasonz-dfinity

rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto

@@ -1214,10 +1214,9 @@ message ProposalData {
   // respect to rewards.
   map<fixed64, Ballot> ballots = 6;
 
-  // Latest tally. Recomputed for open proposals, when proposals are
-  // processed. If the proposal is decided (not open), then the tally
-  // will never change again. (But the ballots may still change as
-  // neurons may vote after the proposal has been decided.)
+  // Latest tally. Recomputed for every vote. Even after the proposal has been
+  // decided, the latest_tally will still be updated based on the recent vote,
+  // until the voting deadline.
   Tally latest_tally = 7;
 
   // If specified: the timestamp when this proposal was adopted or

rs/nns/governance/src/gen/ic_nns_governance.pb.v1.rs

@@ -1617,10 +1617,9 @@ pub struct ProposalData {
     /// respect to rewards.
     #[prost(map = "fixed64, message", tag = "6")]
     pub ballots: ::std::collections::HashMap<u64, Ballot>,
-    /// Latest tally. Recomputed for open proposals, when proposals are
-    /// processed. If the proposal is decided (not open), then the tally
-    /// will never change again. (But the ballots may still change as
-    /// neurons may vote after the proposal has been decided.)
+    /// Latest tally. Recomputed for every vote. Even after the proposal has been
+    /// decided, the latest_tally will still be updated based on the recent vote,
+    /// until the voting deadline.
     #[prost(message, optional, tag = "7")]
     pub latest_tally: ::core::option::Option<Tally>,
     /// If specified: the timestamp when this proposal was adopted or

rs/nns/governance/src/governance.rs

@@ -4167,68 +4167,82 @@ impl Governance {
     ///
     /// If a proposal is adopted but not executed, this method
     /// attempts to execute it.
-    pub fn process_proposal(&mut self, pid: u64) {
+    pub fn process_proposal(&mut self, proposal_id: u64) {
         let now_seconds = self.env.now();
         // Due to Rust lifetime issues, we must extract a closure that
         // computes the voting period from a topic before we borrow
         // `self.proto` mutably.
         let voting_period_seconds_fn = self.voting_period_seconds();
-        if let Some(p) = self.proto.proposals.get_mut(&pid) {
-            if p.status() != ProposalStatus::Open {
-                return;
-            }
-            let topic = p.topic();
-            let voting_period_seconds = voting_period_seconds_fn(topic);
-            // Recompute the tally here. It is imperative that only
-            // 'open' proposals have their tally recomputed. Votes may
-            // arrive after a decision has been made: such votes count
-            // for voting rewards, but shall not make it into the
-            // tally.
-            p.recompute_tally(now_seconds, voting_period_seconds);
 
-            if !p.can_make_decision(now_seconds, voting_period_seconds) {
-                return;
-            }
-            // This marks the proposal as no longer open.
-            p.decided_timestamp_seconds = now_seconds;
-            if !p.is_accepted() {
-                self.start_process_rejected_proposal(pid);
+        let proposal = match self.proto.proposals.get_mut(&proposal_id) {
+            Some(p) => p,
+            None => {
+                println!(
+                    "{}Cannot find proposal {} when trying to process it.",
+                    LOG_PREFIX, proposal_id
+                );
                 return;
             }
-            // The proposal was adopted, return the rejection fee for non-ManageNeuron
-            // proposals.
-            if !p
-                .proposal
-                .as_ref()
-                .map(|x| x.is_manage_neuron())
-                .unwrap_or(false)
-            {
-                if let Some(nid) = &p.proposer {
-                    if let Some(neuron) = self.proto.neurons.get_mut(&nid.id) {
-                        if neuron.neuron_fees_e8s >= p.reject_cost_e8s {
-                            neuron.neuron_fees_e8s -= p.reject_cost_e8s;
-                        }
+        };
+        let topic = proposal.topic();
+        let voting_period_seconds = voting_period_seconds_fn(topic);
+
+        // Recompute the tally here. It should correctly reflect all votes,
+        // even the ones after the proposal has been decided. It's possible
+        // to have Open status while it does not accept votes anymore, since
+        // the status change happens below this point.
+        if proposal.status() == ProposalStatus::Open
+            || proposal.accepts_vote(now_seconds, voting_period_seconds)
+        {
+            proposal.recompute_tally(now_seconds, voting_period_seconds);
+        }
+
+        if proposal.status() != ProposalStatus::Open {
+            return;
+        }
+
+        if !proposal.can_make_decision(now_seconds, voting_period_seconds) {
+            return;
+        }
+        // This marks the proposal as no longer open.
+        proposal.decided_timestamp_seconds = now_seconds;
+        if !proposal.is_accepted() {
+            self.start_process_rejected_proposal(proposal_id);
+            return;
+        }
+        // The proposal was adopted, return the rejection fee for non-ManageNeuron
+        // proposals.
+        if !proposal
+            .proposal
+            .as_ref()
+            .map(|x| x.is_manage_neuron())
+            .unwrap_or(false)
+        {
+            if let Some(nid) = &proposal.proposer {
+                if let Some(neuron) = self.proto.neurons.get_mut(&nid.id) {
+                    if neuron.neuron_fees_e8s >= proposal.reject_cost_e8s {
+                        neuron.neuron_fees_e8s -= proposal.reject_cost_e8s;
                     }
                 }
             }
-            let original_total_community_fund_maturity_e8s_equivalent =
-                p.original_total_community_fund_maturity_e8s_equivalent;
-            if let Some(action) = p.proposal.as_ref().and_then(|x| x.action.clone()) {
-                // A yes decision as been made, execute the proposal!
-                self.start_proposal_execution(
-                    pid,
-                    &action,
-                    original_total_community_fund_maturity_e8s_equivalent,
-                );
-            } else {
-                self.set_proposal_execution_status(
-                    pid,
-                    Err(GovernanceError::new_with_message(
-                        ErrorType::PreconditionFailed,
-                        "Proposal is missing.",
-                    )),
-                );
-            }
+        }
+        let original_total_community_fund_maturity_e8s_equivalent =
+            proposal.original_total_community_fund_maturity_e8s_equivalent;
+        if let Some(action) = proposal.proposal.as_ref().and_then(|x| x.action.clone()) {
+            // A yes decision as been made, execute the proposal!
+            self.start_proposal_execution(
+                proposal_id,
+                &action,
+                original_total_community_fund_maturity_e8s_equivalent,
+            );
+        } else {
+            self.set_proposal_execution_status(
+                proposal_id,
+                Err(GovernanceError::new_with_message(
+                    ErrorType::PreconditionFailed,
+                    "Proposal is missing.",
+                )),
+            );
         }
     }
 

rs/nns/governance/tests/governance.rs

@@ -2295,6 +2295,184 @@ async fn test_invalid_proposals_fail() {
     .unwrap();
 }
 
+fn get_current_voting_power(gov: &Governance, neuron_id: u64, now: u64) -> u64 {
+    gov.get_neuron(&NeuronId { id: neuron_id })
+        .unwrap()
+        .voting_power(now)
+}
+
+#[tokio::test]
+async fn test_compute_tally_while_open() {
+    // Prepare the test with 2 neurons
+    let fake_driver = fake::FakeDriver::default();
+    let mut gov = Governance::new(
+        GovernanceProto {
+            wait_for_quiet_threshold_seconds: 5,
+            ..fixture_two_neurons_second_is_bigger()
+        },
+        fake_driver.get_fake_env(),
+        fake_driver.get_fake_ledger(),
+        fake_driver.get_fake_cmc(),
+    );
+
+    // Make the proposal from the smaller neuron.
+    let pid = gov
+        .make_proposal(
+            &NeuronId { id: 1 },
+            &principal(1),
+            &Proposal {
+                title: Some("A Reasonable Title".to_string()),
+                summary: "proposal 1".to_string(),
+                action: Some(proposal::Action::Motion(Motion {
+                    motion_text: "".to_string(),
+                })),
+                ..Default::default()
+            },
+        )
+        .await
+        .unwrap();
+
+    // Tally should have the smaller neuron voting yes.
+    assert_eq!(
+        gov.get_proposal_data(pid).unwrap().latest_tally,
+        Some(Tally {
+            timestamp_seconds: fake_driver.now(),
+            no: 0,
+            yes: get_current_voting_power(&gov, 1, fake_driver.now()),
+            total: get_current_voting_power(&gov, 1, fake_driver.now())
+                + get_current_voting_power(&gov, 2, fake_driver.now())
+        })
+    );
+}
+
+#[tokio::test]
+async fn test_compute_tally_after_decided() {
+    // Prepare the test with 2 neurons
+    let fake_driver = fake::FakeDriver::default();
+    let mut gov = Governance::new(
+        GovernanceProto {
+            wait_for_quiet_threshold_seconds: 5,
+            ..fixture_two_neurons_second_is_bigger()
+        },
+        fake_driver.get_fake_env(),
+        fake_driver.get_fake_ledger(),
+        fake_driver.get_fake_cmc(),
+    );
+
+    // Make the proposal from the larger neuron.
+    let pid = gov
+        .make_proposal(
+            &NeuronId { id: 2 },
+            &principal(2),
+            &Proposal {
+                title: Some("A Reasonable Title".to_string()),
+                summary: "proposal 1".to_string(),
+                action: Some(proposal::Action::Motion(Motion {
+                    motion_text: "".to_string(),
+                })),
+                ..Default::default()
+            },
+        )
+        .await
+        .unwrap();
+
+    // Tally should have the larger neuron voting yes, and the proposal is decided.
+    assert_eq!(
+        gov.get_proposal_data(pid).unwrap().latest_tally,
+        Some(Tally {
+            timestamp_seconds: fake_driver.now(),
+            no: 0,
+            yes: get_current_voting_power(&gov, 2, fake_driver.now()),
+            total: get_current_voting_power(&gov, 1, fake_driver.now())
+                + get_current_voting_power(&gov, 2, fake_driver.now())
+        })
+    );
+
+    // Let the smaller neuron vote no.
+    fake::register_vote_assert_success(
+        &mut gov,
+        principal(1),
+        NeuronId { id: 1 },
+        ProposalId { id: 1 },
+        Vote::No,
+    );
+
+    // The tally should still be recomputed.
+    assert_eq!(
+        gov.get_proposal_data(pid).unwrap().latest_tally,
+        Some(Tally {
+            timestamp_seconds: fake_driver.now(),
+            no: get_current_voting_power(&gov, 1, fake_driver.now()),
+            yes: get_current_voting_power(&gov, 2, fake_driver.now()),
+            total: get_current_voting_power(&gov, 1, fake_driver.now())
+                + get_current_voting_power(&gov, 2, fake_driver.now())
+        })
+    );
+}
+
+#[tokio::test]
+async fn test_no_compute_tally_after_deadline() {
+    // Prepare the test with 2 neurons
+    let mut fake_driver = fake::FakeDriver::default();
+    let mut gov = Governance::new(
+        GovernanceProto {
+            wait_for_quiet_threshold_seconds: 5,
+            ..fixture_two_neurons_second_is_bigger()
+        },
+        fake_driver.get_fake_env(),
+        fake_driver.get_fake_ledger(),
+        fake_driver.get_fake_cmc(),
+    );
+
+    // Make the proposal from the larger neuron and let the smaller neuron vote no.
+    let pid = gov
+        .make_proposal(
+            &NeuronId { id: 2 },
+            &principal(2),
+            &Proposal {
+                title: Some("A Reasonable Title".to_string()),
+                summary: "proposal 1".to_string(),
+                action: Some(proposal::Action::Motion(Motion {
+                    motion_text: "".to_string(),
+                })),
+                ..Default::default()
+            },
+        )
+        .await
+        .unwrap();
+
+    // Advance time past the deadline.
+    let previous_time = fake_driver.now();
+    fake_driver.advance_time_by(6);
+
+    // Attempt to cast another vote after deadline which should fail.
+    assert_matches!(
+        fake::register_vote(
+            &mut gov,
+            principal(1),
+            NeuronId { id: 1 },
+            ProposalId { id: 1 },
+            Vote::No,
+        ).command,
+        Some(manage_neuron_response::Command::Error(err))
+            if err.error_type == ErrorType::PreconditionFailed as i32
+    );
+
+    // Simulate a heartbeat.
+    gov.run_periodic_tasks().now_or_never();
+
+    // The tally should not be recomputed after deadline because of heartbeat.
+    // This is important since computing the tally is expensive.
+    assert_eq!(
+        gov.get_proposal_data(pid)
+            .unwrap()
+            .latest_tally
+            .as_ref()
+            .unwrap()
+            .timestamp_seconds,
+        previous_time
+    );
+}
 /// In this scenario, the wait-for-quiet policy make that proposals last though
 /// several reward periods.
 ///