Skip to content

Remove PoW consensus #1

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 6 commits into from
Dec 24, 2019
Merged

Remove PoW consensus #1

merged 6 commits into from
Dec 24, 2019

Conversation

sgkim126
Copy link
Contributor

@sgkim126 sgkim126 commented Dec 15, 2019

It fixes #4

@sgkim126 sgkim126 requested a review from majecty December 15, 2019 17:01
@sgkim126 sgkim126 force-pushed the pow branch 2 times, most recently from c816527 to 6cd9101 Compare December 15, 2019 17:16
@majecty
Copy link
Contributor

majecty commented Dec 16, 2019

This is so satisfying.

@@ -265,7 +239,6 @@ pub trait ConsensusEngine: Sync + Send {
header.hash()
}

/// In PoW consensus, the higher scored block becomes the best block.
Copy link
Contributor

Choose a reason for hiding this comment

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

We may remove the "score" in the header. When using the Tendermint consensus algorithm, we can't define a valid "score" value. A finalized block must have a higher score than a proposal block. Since the content of a block is not changed whether it is finalized or not. The score always has a weird value.

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 made an issue for it.

let (work, is_new) = {
let mut sealing_work = self.sealing_work.lock();
let last_work_hash = sealing_work.queue.peek_last_ref().map(|pb| pb.block().header().hash());
let mut sealing_work = self.sealing_work.lock();
Copy link
Contributor

@majecty majecty Dec 16, 2019

Choose a reason for hiding this comment

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

@sgkim126 What do you think about removing SealingWork and SealingQueue. IMO, they were used by PoW consensus to try to make multiple blocks' seal at the same time. When we are using the Tendermint Consensus, we only create a seal at a time.
Also in another point of view, the sealing_work.queue was needed because sealing in the PoW consensus is an asynchronous job. Creating a seal in the Tendermint consensus is a blocking job. We do not need to use the queue.

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. I'll try.

@sgkim126 sgkim126 changed the title Remove PoW consensus [WIP] Remove PoW consensus Dec 17, 2019
@sgkim126 sgkim126 mentioned this pull request Dec 22, 2019
@sgkim126 sgkim126 force-pushed the pow branch 2 times, most recently from 9b03e27 to 5e9217a Compare December 22, 2019 08:18
@sgkim126 sgkim126 changed the title [WIP] Remove PoW consensus Remove PoW consensus Dec 22, 2019
@sgkim126
Copy link
Contributor Author

Travis failed because it pulls the pr for 19. I don't know why but I think it's the fault of Travis.

Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

LGTM

@majecty majecty merged commit 1fb82f8 into CodeChain-io:master Dec 24, 2019
@sgkim126 sgkim126 deleted the pow branch December 24, 2019 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove PoW supprot
2 participants