Skip to content

Add a new field 'next validator set hash' in the header #153

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

Jump to bottom
Merged
merged 1 commit into from
Feb 13, 2020
Merged

Add a new field 'next validator set hash' in the header #153

merged 1 commit into from
Feb 13, 2020

Conversation

junha1
Copy link
Contributor

@junha1 junha1 commented Feb 10, 2020 •
edited
Loading

I will add unit/e2e tests after #130 and reviews

@junha1 junha1 force-pushed the master branch 3 times, most recently from 373487a to bd50f18 Compare February 11, 2020 01:40
@junha1 junha1 requested review from sgkim126 and majecty February 11, 2020 01:48
majecty
majecty reviewed Feb 11, 2020
View reviewed changes
types/src/header.rs Outdated
@@ -50,6 +51,8 @@ pub struct Header {
transactions_root: H256,
/// State root.
state_root: H256,
/// Validator set root. Note that this is for the next block.
validator_set_root: H256,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you choose the term "root"?
Is the validator set is stored in a tree?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding the "next_" prefix to the field name?
When I first read the "validator_set_root", it seems that it is the current block's validator set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then I'll rename it as next_validator_set_hash after your overall review

@junha1 junha1 requested a review from majecty February 11, 2020 05:31
majecty
majecty requested changes Feb 11, 2020
View reviewed changes
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.

Let's squash the copyright commit to the parent commit.
It is hard to know the commit actually update the needed files.

core/src/verification/canon_verifier.rs Outdated
@@ -21,7 +21,7 @@ use crate::consensus::CodeChainEngine;
use crate::error::Error;
use ctypes::{CommonParams, Header};

/// A canonial verifier -- this does full verification.
/// A canonical verifier -- this does full verification.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a new commit for the typo.
A commit should change code only for one purpose.

sgkim126
sgkim126 requested changes Feb 11, 2020
View reviewed changes
types/src/validator_set.rs

// It will be hashed in the header.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct CompactValidatorSet(pub Vec<(Public, u64)>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this structure?
Why now implementing hash function instead of create_compact_validator_set for NextValidator?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggested to Junha to add this struct in the "types/src" directory.
If someone wants to know what is the "validator_set_root" in the header, reading this struct will help them.

Copy link
Contributor Author

@junha1 junha1 Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Because it will be also passed to the light client for verification. Light client only needs that compact data of the validator set. @sgkim126

@majecty majecty mentioned this pull request Feb 11, 2020
Closed
@junha1 junha1 requested review from majecty and sgkim126 February 11, 2020 09:35
@junha1 junha1 changed the title [WIP] Add a new field 'validator set root' in the header Add a new field 'validator set root' in the header Feb 12, 2020
@junha1 junha1 changed the title Add a new field 'validator set root' in the header Add a new field 'next validator set hash' in the header Feb 12, 2020
majecty
majecty requested changes Feb 12, 2020
View reviewed changes
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.

Could you check how tests code in Foundry are using random?

majecty
majecty requested changes Feb 12, 2020
View reviewed changes
core/src/block.rs Outdated
@@ -232,6 +233,12 @@ impl<'x> OpenBlock<'x> {
e
})?;
self.block.header.set_state_root(state_root);

// Note : Does state always hold validator set?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change this to "FIXME" like the below description?

NextValidators do not exist in the static validator set.
This code does not work expected when using the static validator set.
Since we will change Foundry to use only the dynamic validator set, we don't add code that handles the static validator set.
After changing Foundry to use only the dynamic validator set, we should remove this comment.

Copy link
Contributor Author

@junha1 junha1 Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Technically it doesn't cause an error even for now, since we use unwrap_or_default() in load_from_state().
So just simple comment will be ok.

NextValidators entry doesn't exit in the state during static-validator-set-mode.
It doesn't cause a direct error since we use unwrap_or_default() here, but should be aware of such problem
Remove this comment after we completely omit static-validator-set-mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@majecty updated.

@junha1 junha1 force-pushed the master branch 2 times, most recently from 8d04728 to cabe823 Compare February 12, 2020 06:04
majecty
majecty reviewed Feb 12, 2020
View reviewed changes
core/src/block.rs Outdated
@@ -232,6 +233,16 @@ impl<'x> OpenBlock<'x> {
e
})?;
self.block.header.set_state_root(state_root);

/*
FIXME: NextValidators entry doesn't exit in the state during static-validator-set-mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

exit- > exist?
Please review English grammar before updating PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@majecty typo fixed

@majecty
Copy link
Contributor

majecty commented Feb 12, 2020

@sgkim126 I want to merge this PR.

@somniumism somniumism mentioned this pull request Feb 12, 2020
Merged
6 tasks
@majecty majecty mentioned this pull request Feb 13, 2020
Closed
11 tasks
@sgkim126 sgkim126 merged commit aba1ae9 into CodeChain-io:master Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majecty majecty majecty approved these changes

@sgkim126 sgkim126 sgkim126 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@junha1 @majecty @sgkim126