From 0ef4700d7640380d97c5fed682f3dc675ccc907b Mon Sep 17 00:00:00 2001 From: Seulgi Kim Date: Mon, 3 Jun 2019 18:37:34 +0900 Subject: [PATCH] Handle nomination expiration properly --- core/src/consensus/solo/mod.rs | 3 +- core/src/consensus/stake/mod.rs | 70 +++++++++++++++++++------ core/src/consensus/tendermint/engine.rs | 3 +- 3 files changed, 57 insertions(+), 19 deletions(-) diff --git a/core/src/consensus/solo/mod.rs b/core/src/consensus/solo/mod.rs index e73fbc18de..921e321384 100644 --- a/core/src/consensus/solo/mod.rs +++ b/core/src/consensus/solo/mod.rs @@ -120,7 +120,8 @@ impl ConsensusEngine for Solo { for (address, reward) in rewards { self.machine.add_balance(block, &address, reward)?; } - self.machine.increase_term_id(block, last_term_finished_block_num)?; + + stake::on_term_close(block.state_mut(), last_term_finished_block_num)?; Ok(()) } diff --git a/core/src/consensus/stake/mod.rs b/core/src/consensus/stake/mod.rs index 25d752f943..e3dd2ad0a4 100644 --- a/core/src/consensus/stake/mod.rs +++ b/core/src/consensus/stake/mod.rs @@ -23,7 +23,7 @@ use std::collections::HashMap; use ccrypto::Blake; use ckey::{public_to_address, recover, Address, Public, Signature}; -use cstate::{ActionHandler, StateResult, TopLevelState, TopState}; +use cstate::{ActionHandler, StateResult, TopLevelState, TopState, TopStateView}; use ctypes::errors::{RuntimeError, SyntaxError}; use ctypes::util::unexpected::Mismatch; use ctypes::{CommonParams, Header}; @@ -96,7 +96,20 @@ impl ActionHandler for Stake { Action::SelfNominate { deposit, metadata, - } => self_nominate(state, sender, sender_public, deposit, 0, 0, metadata), + } => { + let (current_term, nomination_ends_at) = { + let metadata = state.metadata()?.expect("Metadata must exist"); + const DEFAULT_NOMINATION_EXPIRATION: u64 = 24; + let current_term = metadata.current_term_id(); + let expiration = metadata + .params() + .map(CommonParams::nomination_expiration) + .unwrap_or(DEFAULT_NOMINATION_EXPIRATION); + let nomination_ends_at = current_term + expiration; + (current_term, nomination_ends_at) + }; + self_nominate(state, sender, sender_public, deposit, current_term, nomination_ends_at, metadata) + } Action::ChangeParams { metadata_seq, params, @@ -188,8 +201,6 @@ fn self_nominate( nomination_ends_at: u64, metadata: Bytes, ) -> StateResult<()> { - // TODO: proper handling of get_current_term - // TODO: proper handling of NOMINATE_EXPIRATION let blacklist = Banned::load_from_state(state)?; if blacklist.is_banned(&sender) { return Err(RuntimeError::FailedToHandleCustomAction("Account is blacklisted".to_string()).into()) @@ -281,8 +292,8 @@ fn change_params( Ok(()) } -#[allow(dead_code)] -pub fn on_term_close(state: &mut TopLevelState, current_term: u64) -> StateResult<()> { +pub fn on_term_close(state: &mut TopLevelState, last_term_finished_block_num: u64) -> StateResult<()> { + let current_term = state.metadata()?.expect("The metadata must exist").current_term_id(); // TODO: total_slash = slash_unresponsive(headers, pending_rewards) // TODO: pending_rewards.update(signature_reward(blocks, total_slash)) @@ -309,6 +320,8 @@ pub fn on_term_close(state: &mut TopLevelState, current_term: u64) -> StateResul revert_delegations(state, &reverted)?; // TODO: validators, validator_order = elect() + + state.increase_term_id(last_term_finished_block_num)?; Ok(()) } @@ -846,12 +859,23 @@ mod tests { assert!(result.is_err(), "Cannot self-nominate without a sufficient balance"); } + fn increase_term_id_until(state: &mut TopLevelState, term_id: u64) { + let mut block_num = state.metadata().unwrap().unwrap().last_term_finished_block_num() + 1; + while state.metadata().unwrap().unwrap().current_term_id() != term_id { + assert_eq!(Ok(()), state.increase_term_id(block_num)); + block_num += 1; + } + } + #[test] fn self_nominate_returns_deposits_after_expiration() { let address_pubkey = Public::random(); let address = public_to_address(&address_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); + increase_term_id_until(&mut state, 29); + state.add_balance(&address, 1000).unwrap(); let stake = Stake::new(HashMap::new()); @@ -860,7 +884,7 @@ mod tests { // TODO: change with stake.execute() self_nominate(&mut state, &address, &address_pubkey, 200, 0, 30, b"".to_vec()).unwrap(); - let result = on_term_close(&mut state, 29); + let result = on_term_close(&mut state, pseudo_term_to_block_num_calculator(29)); assert_eq!(result, Ok(())); assert_eq!(state.balance(&address).unwrap(), 800, "Should keep nomination before expiration"); @@ -876,7 +900,7 @@ mod tests { "Keep deposit before expiration", ); - let result = on_term_close(&mut state, 30); + let result = on_term_close(&mut state, pseudo_term_to_block_num_calculator(30)); assert_eq!(result, Ok(())); assert_eq!(state.balance(&address).unwrap(), 1000, "Return deposit after expiration"); @@ -892,6 +916,8 @@ mod tests { let delegator = public_to_address(&address_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); + increase_term_id_until(&mut state, 29); state.add_balance(&address, 1000).unwrap(); let stake = { @@ -910,7 +936,7 @@ mod tests { }; stake.execute(&action.rlp_bytes(), &mut state, &delegator, &delegator_pubkey).unwrap(); - let result = on_term_close(&mut state, 29); + let result = on_term_close(&mut state, pseudo_term_to_block_num_calculator(29)); assert_eq!(result, Ok(())); let account = StakeAccount::load_from_state(&state, &delegator).unwrap(); @@ -918,7 +944,7 @@ mod tests { let delegation = Delegation::load_from_state(&state, &delegator).unwrap(); assert_eq!(delegation.get_quantity(&address), 40, "Should keep delegation before expiration"); - let result = on_term_close(&mut state, 30); + let result = on_term_close(&mut state, pseudo_term_to_block_num_calculator(30)); assert_eq!(result, Ok(())); let account = StakeAccount::load_from_state(&state, &delegator).unwrap(); @@ -971,6 +997,7 @@ mod tests { let address = public_to_address(&address_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); state.add_balance(&address, 1000).unwrap(); let stake = Stake::new(HashMap::new()); @@ -1000,7 +1027,7 @@ mod tests { current_term, custody_until ); - on_term_close(&mut state, current_term).unwrap(); + on_term_close(&mut state, pseudo_term_to_block_num_calculator(current_term)).unwrap(); } } @@ -1010,6 +1037,7 @@ mod tests { let address = public_to_address(&address_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); state.add_balance(&address, 1000).unwrap(); let stake = Stake::new(HashMap::new()); @@ -1024,7 +1052,7 @@ mod tests { .unwrap(); jail(&mut state, &address, custody_until, released_at).unwrap(); for current_term in 0..=custody_until { - on_term_close(&mut state, current_term).unwrap(); + on_term_close(&mut state, pseudo_term_to_block_num_calculator(current_term)).unwrap(); } let current_term = custody_until + 1; @@ -1064,6 +1092,7 @@ mod tests { let address = public_to_address(&address_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); state.add_balance(&address, 1000).unwrap(); let stake = Stake::new(HashMap::new()); @@ -1078,7 +1107,7 @@ mod tests { jail(&mut state, &address, custody_until, released_at).unwrap(); for current_term in 0..released_at { - on_term_close(&mut state, current_term).unwrap(); + on_term_close(&mut state, pseudo_term_to_block_num_calculator(current_term)).unwrap(); let candidates = Candidates::load_from_state(&state).unwrap(); assert_eq!(candidates.get_candidate(&address), None); @@ -1087,7 +1116,7 @@ mod tests { assert!(jail.get_prisoner(&address).is_some()); } - on_term_close(&mut state, released_at).unwrap(); + on_term_close(&mut state, pseudo_term_to_block_num_calculator(released_at)).unwrap(); let candidates = Candidates::load_from_state(&state).unwrap(); assert_eq!(candidates.get_candidate(&address), None, "A prisoner should not become a candidate"); @@ -1106,6 +1135,7 @@ mod tests { let delegator = public_to_address(&delegator_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); state.add_balance(&address, 1000).unwrap(); let stake = { @@ -1131,7 +1161,7 @@ mod tests { let result = stake.execute(&action.rlp_bytes(), &mut state, &delegator, &delegator_pubkey); assert!(result.is_ok()); - on_term_close(&mut state, current_term).unwrap(); + on_term_close(&mut state, pseudo_term_to_block_num_calculator(current_term)).unwrap(); } let action = Action::DelegateCCS { @@ -1150,6 +1180,7 @@ mod tests { let delegator = public_to_address(&delegator_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); state.add_balance(&address, 1000).unwrap(); let stake = { @@ -1174,7 +1205,7 @@ mod tests { stake.execute(&action.rlp_bytes(), &mut state, &delegator, &delegator_pubkey).unwrap(); for current_term in 0..=released_at { - on_term_close(&mut state, current_term).unwrap(); + on_term_close(&mut state, pseudo_term_to_block_num_calculator(current_term)).unwrap(); } let delegation = Delegation::load_from_state(&state, &delegator).unwrap(); @@ -1192,6 +1223,7 @@ mod tests { let delegator = public_to_address(&delegator_pubkey); let mut state = helpers::get_temp_state(); + assert_eq!(Ok(()), state.update_params(0, CommonParams::default_for_test())); state.add_balance(&address, 1000).unwrap(); let stake = { @@ -1214,7 +1246,7 @@ mod tests { }; stake.execute(&action.rlp_bytes(), &mut state, &delegator, &delegator_pubkey).unwrap(); for current_term in 0..custody_until { - on_term_close(&mut state, current_term).unwrap(); + on_term_close(&mut state, pseudo_term_to_block_num_calculator(current_term)).unwrap(); } let current_term = custody_until + 1; @@ -1298,4 +1330,8 @@ mod tests { let jail = Jail::load_from_state(&state).unwrap(); assert_eq!(jail.get_prisoner(&criminal), None, "Should be removed from the jail"); } + + fn pseudo_term_to_block_num_calculator(term_id: u64) -> u64 { + term_id * 10 + 1 + } } diff --git a/core/src/consensus/tendermint/engine.rs b/core/src/consensus/tendermint/engine.rs index 23737cc00a..d5c03a40d0 100644 --- a/core/src/consensus/tendermint/engine.rs +++ b/core/src/consensus/tendermint/engine.rs @@ -202,8 +202,9 @@ impl ConsensusEngine for Tendermint { self.machine.add_balance(block, &address, reward)?; } - self.machine.increase_term_id(block, last_term_finished_block_num)?; stake::move_current_to_previous_intermediate_rewards(&mut block.state_mut())?; + stake::on_term_close(block.state_mut(), last_term_finished_block_num)?; + Ok(()) }