Skip to content

Add method to check the end of a term #1544

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

Closed
foriequal0 wants to merge 1 commit into from
Closed

Add method to check the end of a term #1544

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
21 changes: 21 additions & 0 deletions core/src/client/importer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ use crate::types::BlockId;
use crate::verification::queue::{BlockQueue, HeaderQueue};
use crate::verification::{self, PreverifiedBlock, Verifier};
use crate::views::{BlockView, HeaderView};
use blockchain::BlockChain;

pub struct Importer {
/// Lock used during block import
Expand Down Expand Up @@ -206,6 +207,8 @@ impl Importer {
client.db().write_buffered(batch);
chain.commit();

self.check_term_end(block.header(), &chain, client);

@majecty majecty May 17, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This could make a problem.
In Tendermint Consensus, there will be proposal blocks which are executed but not finalized. Since the proposal blocks' timestamp will be different, some of it cause term end, and others do not cause term end. So the code, saving the "last term" to DB should not be called here. The code should be called when a block is finalized. And the finalization time would be "chain_notify".

@sgkim126 is handling a similar problem while implementing changing common params. Please check his work.

@sgkim126 sgkim126 May 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, it can be a problem when the block is a proposal block or there is a reorganization.
https://github.com/CodeChain-io/codechain/blob/master/core/src/client/client.rs#L150
To handle it, I extracted new_blocks method on the client.
I'm considering this method to check the params.

@sgkim126 sgkim126 May 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

However, you need to change Importer::import_verified_blocks to stop when the current block is the end of a term.

@foriequal0 foriequal0 May 17, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I should change how the solo closes term. I think the solo should closes term still controllable manner (not non-deterministic time-based manner) but it is easily determined by just looking the header. Solo should closes term by block number base. The current manual trigger causes some problems. When the solo is configured with multiple nodes, they cannot agree when the term ends. and the differences between solo and the corgi make the problem harder such as how we deal with reorganization, when do we stop at on import_verified_blocks.


if hash == chain.best_block_hash() {
let mut state_db = client.state_db().write();
let state = block.state();
Expand All @@ -215,6 +218,21 @@ impl Importer {
route
}

fn check_term_end(&self, header: &Header, chain: &BlockChain, client: &Client) {
let last_term_end = chain.last_term_end();
if let Some(term_id) = self.engine.is_term_end(header, last_term_end) {
cdebug!(CLIENT, "Term end at block {}", header.hash());

let mut batch = DBTransaction::new();
chain.update_last_term_end(&mut batch, term_id, header.number());

// always write the batch directly since term end are
// fetched from a DB iterator and DB iterators are only available on
// flushed data.
client.db().write(batch).expect("DB flush failed");
}
}

fn check_and_close_block(&self, block: &PreverifiedBlock, client: &Client) -> Result<LockedBlock, ()> {
let engine = &*self.engine;
let header = &block.header;
Expand Down Expand Up @@ -404,6 +422,9 @@ impl Importer {
client.db().write_buffered(batch);
chain.commit();

// FIXME: Check if this line is still necessary.
// self.check_term_end(&header, &chain, client);

route
}
}
5 changes: 5 additions & 0 deletions core/src/consensus/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ use crate::error::Error;
use crate::header::Header;
use crate::transaction::{SignedTransaction, UnverifiedTransaction};
use crate::views::HeaderView;
use blockchain::TermEnd;
use Client;

pub enum Seal {
Expand Down Expand Up @@ -199,6 +200,10 @@ pub trait ConsensusEngine: Sync + Send {
Ok(())
}

fn is_term_end(&self, _chain_head: &M::Header, _last_term_end: Option<TermEnd>) -> Option<u64> {
None
}

/// Populate a header's fields based on its parent's header.
/// Usually implements the chain scoring rule based on weight.
fn populate_from_parent(&self, _header: &mut Header, _parent: &Header) {}
Expand Down