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.

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

merged 1 commit into from
Jun 11, 2019

Conversation

sgkim126
Copy link
Contributor

@sgkim126 sgkim126 commented Jun 3, 2019

Please see the last commit only.
It depends on #1597, #1594 and #1595

@sgkim126 sgkim126 requested a review from foriequal0 June 3, 2019 10:44
@@ -121,6 +121,9 @@ impl ConsensusEngine for Solo<CodeChainMachine> {
self.machine.add_balance(block, &address, reward)?;
}
self.machine.increase_term_id(block, last_term_finished_block_num)?;

let current_term = block.state().metadata()?.unwrap().current_term_id();
stake::on_term_close(block.state_mut(), current_term)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I assumed that on_term_close should be called after transactions being applied with the same current_term number. I think you can find its evidences in the tests pointing the off-by-one error. But I'm not sure this would be okay too.

Copy link
Contributor Author

@sgkim126 sgkim126 Jun 4, 2019

Choose a reason for hiding this comment

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

The last block changes the term. It means that the term has been changed after executing the last block of the term, not before the first block of the next term. So you should call on_term_close with the changed term id.
Please fix the test cases.

Copy link
Contributor

@foriequal0 foriequal0 Jun 5, 2019

Choose a reason for hiding this comment

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

What about

        let current_term = block.state().metadata()?.unwrap().current_term_id();
        self.machine.increase_term_id(block, last_term_finished_block_num)?;
        stake::on_term_close(block.state_mut(), current_term)?;

?
I think this wouldn't have an issue if it doesn't introduce problems

	let current_term = block.state().metadata()?.unwrap().current_term_id();
        stake::on_term_close(block.state_mut(), current_term)?;
        self.machine.increase_term_id(block, last_term_finished_block_num)?;

because it looks like

   let mut current_term = 0;
   while(true) {
      for tx in transactions { tx.apply(); }
      stake::on_term_close(current_term);

      current_term += 1;
   }

which is a effectively for current_term in 0.. { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good, but it seems that on_term_close doesn't use the last_term_finished_block_num, so the order is not important.
I made it increase the term id after on_term_close and there is no need to pass the term id because on_term_close takes the state.

@@ -204,6 +204,9 @@ impl ConsensusEngine for Tendermint {

self.machine.increase_term_id(block, last_term_finished_block_num)?;
stake::move_current_to_previous_intermediate_rewards(&mut block.state_mut())?;

let current_term = block.state().metadata()?.unwrap().current_term_id();
stake::on_term_close(block.state_mut(), current_term)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@sgkim126 sgkim126 added the do-not-merge Do not merge (for mergify.io) label Jun 4, 2019
@sgkim126
Copy link
Contributor Author

sgkim126 commented Jun 4, 2019

I stopped Travis. I'll run it after merging #1594 and #1595

@sgkim126 sgkim126 removed the do-not-merge Do not merge (for mergify.io) label Jun 4, 2019
@@ -121,6 +121,9 @@ impl ConsensusEngine for Solo<CodeChainMachine> {
self.machine.add_balance(block, &address, reward)?;
}
self.machine.increase_term_id(block, last_term_finished_block_num)?;

let current_term = block.state().metadata()?.unwrap().current_term_id();
stake::on_term_close(block.state_mut(), current_term)?;
Copy link
Contributor

@foriequal0 foriequal0 Jun 5, 2019

Choose a reason for hiding this comment

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

What about

        let current_term = block.state().metadata()?.unwrap().current_term_id();
        self.machine.increase_term_id(block, last_term_finished_block_num)?;
        stake::on_term_close(block.state_mut(), current_term)?;

?
I think this wouldn't have an issue if it doesn't introduce problems

	let current_term = block.state().metadata()?.unwrap().current_term_id();
        stake::on_term_close(block.state_mut(), current_term)?;
        self.machine.increase_term_id(block, last_term_finished_block_num)?;

because it looks like

   let mut current_term = 0;
   while(true) {
      for tx in transactions { tx.apply(); }
      stake::on_term_close(current_term);

      current_term += 1;
   }

which is a effectively for current_term in 0.. { ... }

@sgkim126
Copy link
Contributor Author

sgkim126 commented Jun 7, 2019

I resolved the conflicts.

@@ -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.

@@ -860,7 +882,8 @@ 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);
increase_term_id_until(&mut state, 30);

This comment was marked as outdated.

@@ -846,12 +857,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.

@@ -1174,7 +1212,8 @@ 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).unwrap();

This comment was marked as outdated.

@sgkim126 sgkim126 requested a review from foriequal0 June 10, 2019 10:43
@@ -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.

@@ -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.

How about making a local test helper?

@mergify mergify bot merged commit dc9a0cf into CodeChain-io:master Jun 11, 2019
@sgkim126 sgkim126 deleted the nominate branch June 12, 2019 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants