Skip to content

Handle nomination expiration properly #1598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 11, 2019
Merged

Handle nomination expiration properly #1598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/consensus/solo/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ impl ConsensusEngine for Solo<CodeChainMachine> {
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(())
}

Expand Down
70 changes: 53 additions & 17 deletions core/src/consensus/stake/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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))

Expand All @@ -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(())
}

Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If on_term_close handles increasing term id, this will be term_close_until(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It doesn't close terms. It just increases term id on the metadata

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a meaning to differentiate closing the term from increasing the term id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It is hard to create test cases if we close terms to set the states.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

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());
Expand All @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pseudo_term_to_block_num_calculator is somewhat misleading. Actual bulk term increasing thing is done with increase_term_id_until, on_term_close is increasing it just one step. I think turn increase_term_id_until into term_close_until or something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't increase the term bulky.
The second parameter of on_term_close is the block number of closing block.
And you cannot rename increase_term_id_until to term_close_until, because it doesn't close term, it merely increases term id.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand it now that pseudo_term_to_block_num_calculator is not actually related term id. It was confusing for a while.
I think increasing term id by closing the term is more realistic than calling increase_term_id directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the purpose of this function to set the states for testing not calling on_term_close.

assert_eq!(result, Ok(()));

assert_eq!(state.balance(&address).unwrap(), 800, "Should keep nomination before expiration");
Expand All @@ -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");
Expand All @@ -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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we add this state.update_params(0, CommonParams::default_for_test()); to helpers::get_temp_state();? or let calling update_params() create metadata automatically?
And state.update_params() is not a subject for this tests. I think it can just .unwrap() rather than assert_eq!()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a test whether the initial state is a root of the empty trie.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making a local test helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't have worth to make a helper function.
And please prefer to use assert_eq if the argument derives Debug and PartialEq.
You should read the backtrace if you use unwrap, because the test fails in Option.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay then. I'll comment like // Arrange, // Action, // Assert to separate sections to convey the intention of a test in that situation.

increase_term_id_until(&mut state, 29);
state.add_balance(&address, 1000).unwrap();

let stake = {
Expand All @@ -910,15 +936,15 @@ 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();
assert_eq!(account.balance, 100 - 40);
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();
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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();
}
}

Expand All @@ -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());
Expand All @@ -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;
Expand Down Expand Up @@ -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());
Expand All @@ -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);
Expand All @@ -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");
Expand All @@ -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 = {
Expand All @@ -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 {
Expand All @@ -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 = {
Expand All @@ -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();
Expand All @@ -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 = {
Expand All @@ -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;
Expand Down Expand Up @@ -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
}
}
3 changes: 2 additions & 1 deletion core/src/consensus/tendermint/engine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}

Expand Down