Skip to content

Commit

Permalink
NNS1-829: Appropriately lock neuron on configure_neuron
Browse files Browse the repository at this point in the history
  • Loading branch information
oggy-dfin committed Dec 8, 2021
1 parent d424c76 commit 2c7b141
Show file tree
Hide file tree
Showing 10 changed files with 360 additions and 27 deletions.
1 change: 1 addition & 0 deletions rs/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions rs/nns/governance/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ name = "governance-test"
path = "tests/governance.rs"
required-features = ["test"]

[[test]]
name = "governance-interleaving-test"
path = "tests/interleaving_tests.rs"

[[bench]]
name = "scale"
harness = false
Expand Down Expand Up @@ -66,6 +70,7 @@ ic-base-types = { path = "../../types/base_types", features = ["test"] }
ic-nns-common = { path = "../common", features = ["test"] }
ledger-canister = { path = "../../rosetta-api/ledger_canister", features = ["test"] }
ic-nns-governance = { path = ".", features = ["test"] }
tokio-test = "0.4.2"

[features]
test = ["ic-base-types/test", "ic-nns-common/test", "ledger-canister/test"]
1 change: 1 addition & 0 deletions rs/nns/governance/canister/governance.did
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type Command_1 = variant {
type Command_2 = variant {
Spawn : Spawn;
Split : Split;
Configure : Configure;
DisburseToNeuron : DisburseToNeuron;
ClaimOrRefreshNeuron : ClaimOrRefresh;
MergeMaturity : MergeMaturity;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,7 @@ message Governance {
ManageNeuron.DisburseToNeuron disburse_to_neuron = 5;
ManageNeuron.MergeMaturity merge_maturity = 7;
ManageNeuron.ClaimOrRefresh claim_or_refresh_neuron = 8;
ManageNeuron.Configure configure = 9;
}
}

Expand Down
17 changes: 13 additions & 4 deletions rs/nns/governance/src/governance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4958,14 +4958,23 @@ impl Governance {
caller: &PrincipalId,
c: &manage_neuron::Configure,
) -> Result<(), GovernanceError> {
let now_seconds = self.env.now();

let lock_command = NeuronInFlightCommand {
timestamp: now_seconds,
command: Some(InFlightCommand::Configure(c.clone())),
};
let _lock = self.lock_neuron_for_command(id.id, lock_command)?;

if let Some(neuron) = self.proto.neurons.get_mut(&id.id) {
let now_seconds = self.env.now();
neuron.configure(caller, now_seconds, c)?;
match c

let op = c
.operation
.as_ref()
.expect("Configure must have an operation")
{
.expect("Configure must have an operation");

match op {
manage_neuron::configure::Operation::AddHotKey(k) => {
let hot_key = k.new_hot_key.as_ref().expect("Must have a hot key");
GovernanceProto::add_neuron_to_principal_in_principal_to_neuron_ids_index(
Expand Down
56 changes: 56 additions & 0 deletions rs/nns/governance/tests/common/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
use ic_base_types::PrincipalId;
use ic_nns_common::pb::v1::NeuronId;
use ic_nns_governance::governance::Governance;
use ic_nns_governance::pb::v1::{
manage_neuron, manage_neuron::NeuronIdOrSubaccount, ManageNeuron, ManageNeuronResponse,
};

pub async fn increase_dissolve_delay_raw(
gov: &mut Governance,
principal_id: &PrincipalId,
neuron_id: NeuronId,
delay_increase: u32,
) -> ManageNeuronResponse {
gov.manage_neuron(
principal_id,
&ManageNeuron {
id: None,
neuron_id_or_subaccount: Some(NeuronIdOrSubaccount::NeuronId(neuron_id)),
command: Some(manage_neuron::Command::Configure(
manage_neuron::Configure {
operation: Some(manage_neuron::configure::Operation::IncreaseDissolveDelay(
manage_neuron::IncreaseDissolveDelay {
additional_dissolve_delay_seconds: delay_increase,
},
)),
},
)),
},
)
.await
}

pub async fn set_dissolve_delay_raw(
gov: &mut Governance,
principal_id: &PrincipalId,
neuron_id: NeuronId,
timestamp_seconds: u64,
) -> ManageNeuronResponse {
gov.manage_neuron(
principal_id,
&ManageNeuron {
id: None,
neuron_id_or_subaccount: Some(NeuronIdOrSubaccount::NeuronId(neuron_id)),
command: Some(manage_neuron::Command::Configure(
manage_neuron::Configure {
operation: Some(manage_neuron::configure::Operation::SetDissolveTimestamp(
manage_neuron::SetDissolveTimestamp {
dissolve_timestamp_seconds: timestamp_seconds,
},
)),
},
)),
},
)
.await
}
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ pub struct NeuronBuilder {
neuron_fees: u64,
dissolve_state: Option<neuron::DissolveState>,
followees: HashMap<i32, neuron::Followees>,
kyc_verified: bool,
}

impl From<Neuron> for NeuronBuilder {
Expand All @@ -208,6 +209,7 @@ impl From<Neuron> for NeuronBuilder {
neuron_fees: neuron.neuron_fees_e8s,
dissolve_state: neuron.dissolve_state,
followees: neuron.followees,
kyc_verified: neuron.kyc_verified,
}
}
}
Expand All @@ -224,6 +226,7 @@ impl NeuronBuilder {
neuron_fees: 0,
dissolve_state: None,
followees: HashMap::new(),
kyc_verified: true,
}
}

Expand All @@ -238,6 +241,7 @@ impl NeuronBuilder {
neuron_fees: 0,
dissolve_state: None,
followees: HashMap::new(),
kyc_verified: true,
}
}

Expand Down Expand Up @@ -274,6 +278,16 @@ impl NeuronBuilder {
self
}

pub fn set_dissolve_state(mut self, state: Option<DissolveState>) -> Self {
self.dissolve_state = state;
self
}

pub fn set_kyc_verified(mut self, kyc: bool) -> Self {
self.kyc_verified = kyc;
self
}

pub fn create(self, now: u64, ledger: &mut LedgerBuilder) -> Neuron {
let subaccount = Self::subaccount(self.owner, self.ident);
ledger.add_account(neuron_subaccount(subaccount), self.stake);
Expand All @@ -294,7 +308,7 @@ impl NeuronBuilder {
},
maturity_e8s_equivalent: self.maturity,
dissolve_state: self.dissolve_state,
kyc_verified: true,
kyc_verified: self.kyc_verified,
followees: self.followees,
..Neuron::default()
}
Expand Down Expand Up @@ -707,6 +721,7 @@ impl Environment for NNS {
}
}

pub type LedgerTransform = Box<dyn FnOnce(Box<dyn Ledger>) -> Box<dyn Ledger>>;
/// The NNSBuilder permits the declarative construction of an NNS fixture. All
/// of the methods concern setting or querying what this initial state will
/// be. Therefore, `get_account_balance` on a builder object will only tell
Expand All @@ -717,6 +732,7 @@ pub struct NNSBuilder {
start_time: u64,
ledger_builder: LedgerBuilder,
governance: GovernanceProto,
ledger_transforms: Vec<LedgerTransform>,
}

impl Default for NNSBuilder {
Expand All @@ -725,6 +741,7 @@ impl Default for NNSBuilder {
start_time: DEFAULT_TEST_START_TIMESTAMP_SECONDS,
ledger_builder: LedgerBuilder::default(),
governance: GovernanceProto::default(),
ledger_transforms: Vec::default(),
}
}
}
Expand All @@ -740,13 +757,13 @@ impl NNSBuilder {
rng: StdRng::seed_from_u64(9539),
ledger: self.ledger_builder.create(),
});
let mut ledger: Box<dyn Ledger> = Box::new(fixture.clone());
for t in self.ledger_transforms {
ledger = t(ledger);
}
let mut nns = NNS {
fixture: fixture.clone(),
governance: Governance::new(
self.governance,
Box::new(fixture.clone()),
Box::new(fixture),
),
governance: Governance::new(self.governance, Box::new(fixture), ledger),
initial_state: None,
};
nns.capture_state();
Expand Down Expand Up @@ -817,6 +834,14 @@ impl NNSBuilder {
pub fn get_neuron(&self, ident: u64) -> Option<&Neuron> {
self.governance.neurons.get(&ident)
}

/// Transform the ledger just before it's built, e.g., to allow blocking on
/// its calls for interleaving tests. Multiple transformations can be
/// chained; they are applied in the order in which they were added.
pub fn add_ledger_transform(mut self, transform: LedgerTransform) -> Self {
self.ledger_transforms.push(transform);
self
}
}

#[macro_export]
Expand Down
30 changes: 13 additions & 17 deletions rs/nns/governance/tests/governance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,21 @@ use ledger_canister::Subaccount;
/// ported to the new 'fixtures' module.
mod fake;

mod fixtures;
// Using a `pub mod` works around spurious dead code warnings; see
// https://github.com/rust-lang/rust/issues/46379
pub mod fixtures;

use fixtures::{
principal, prorated_neuron_age, LedgerBuilder, NNSBuilder, NNSStateChange, NeuronBuilder,
ProposalNeuronBehavior, NNS,
};

// Using a `pub mod` works around spurious dead code warnings; see
// https://github.com/rust-lang/rust/issues/46379
pub mod common;

use common::increase_dissolve_delay_raw;

const DEFAULT_TEST_START_TIMESTAMP_SECONDS: u64 = 999_111_000_u64;

fn check_proposal_status_after_voting_and_after_expiration_new(
Expand Down Expand Up @@ -7529,23 +7537,11 @@ fn increase_dissolve_delay(
neuron_id: u64,
delay_increase: u32,
) {
gov.manage_neuron(
increase_dissolve_delay_raw(
gov,
&principal(principal_id),
&ManageNeuron {
id: None,
neuron_id_or_subaccount: Some(NeuronIdOrSubaccount::NeuronId(NeuronId {
id: neuron_id,
})),
command: Some(manage_neuron::Command::Configure(
manage_neuron::Configure {
operation: Some(manage_neuron::configure::Operation::IncreaseDissolveDelay(
manage_neuron::IncreaseDissolveDelay {
additional_dissolve_delay_seconds: delay_increase,
},
)),
},
)),
},
NeuronId { id: neuron_id },
delay_increase,
)
.now_or_never()
.unwrap()
Expand Down
101 changes: 101 additions & 0 deletions rs/nns/governance/tests/interleaving/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
//! Utilities to help with testing interleavings of calls to the governance
//! canister
use async_trait::async_trait;
use futures::channel::mpsc::UnboundedSender as USender;
use futures::channel::oneshot::{self, Sender as OSender};
use std::sync::atomic;
use std::sync::atomic::Ordering as AOrdering;

use ic_nns_governance::{
governance::Ledger,
pb::v1::{governance_error::ErrorType, GovernanceError},
};
use ledger_canister::Subaccount;
use ledger_canister::{AccountIdentifier, Tokens};

/// Reifies the methods of the Ledger trait, such that they can be sent over a
/// channel
#[derive(Debug)]
pub enum LedgerMessage {
Transfer {
amount_e8s: u64,
fee_e8s: u64,
from_subaccount: Option<Subaccount>,
to: AccountIdentifier,
memo: u64,
},
TotalSupply,
BalanceQuery(AccountIdentifier),
}

pub type LedgerControlMessage = (LedgerMessage, OSender<Result<(), GovernanceError>>);

pub type LedgerObserver = USender<LedgerControlMessage>;

/// A mock ledger to test interleavings of governance method calls.
pub struct InterleavingTestLedger {
underlying: Box<dyn Ledger>,
observer: LedgerObserver,
}

impl InterleavingTestLedger {
/// The ledger intercepts calls to an underlying ledger implementation,
/// sends the reified calls over the provided observer channel, and
/// blocks. The receiver side of the channel can then inspect the
/// results, and decide at what point to go ahead with the call to the
/// underlying ledger, or, alternatively, return an error. This is done
/// through a one-shot channel, the sender side of which is sent to the
/// observer.
pub fn new(underlying: Box<dyn Ledger>, observer: LedgerObserver) -> Self {
InterleavingTestLedger {
underlying,
observer,
}
}

// Notifies the observer that a ledger method has been called, and blocks until
// it receives a message to continue.
async fn notify(&self, msg: LedgerMessage) -> Result<(), GovernanceError> {
let (tx, rx) = oneshot::channel::<Result<(), GovernanceError>>();
self.observer.unbounded_send((msg, tx)).unwrap();
rx.await
.map_err(|_e| GovernanceError::new(ErrorType::Unavailable))?
}
}

#[async_trait]
impl Ledger for InterleavingTestLedger {
async fn transfer_funds(
&self,
amount_e8s: u64,
fee_e8s: u64,
from_subaccount: Option<Subaccount>,
to: AccountIdentifier,
memo: u64,
) -> Result<u64, GovernanceError> {
let msg = LedgerMessage::Transfer {
amount_e8s,
fee_e8s,
from_subaccount,
to,
memo,
};
atomic::fence(AOrdering::SeqCst);
self.notify(msg).await?;
self.underlying
.transfer_funds(amount_e8s, fee_e8s, from_subaccount, to, memo)
.await
}

async fn total_supply(&self) -> Result<Tokens, GovernanceError> {
atomic::fence(AOrdering::SeqCst);
self.notify(LedgerMessage::TotalSupply).await?;
self.underlying.total_supply().await
}

async fn account_balance(&self, account: AccountIdentifier) -> Result<Tokens, GovernanceError> {
atomic::fence(AOrdering::SeqCst);
self.notify(LedgerMessage::BalanceQuery(account)).await?;
self.underlying.account_balance(account).await
}
}
Loading

0 comments on commit 2c7b141

Please sign in to comment.