Tendermint epoch transitions by feynmanliang · Pull Request #6085 · openethereum/parity-ethereum

feynmanliang · 2017-07-18T21:16:00Z

Resolves #6046

parity-cla-bot · 2017-07-18T21:16:01Z

It looks like @feynmanliang hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

feynmanliang · 2017-07-18T22:19:47Z

[clabot:check]

parity-cla-bot · 2017-07-18T22:19:48Z

It looks like @feynmanliang signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

gavofyork · 2017-07-19T08:16:45Z

many thanks for the contribution!

rphmeier · 2017-07-19T11:16:01Z

@feynmanliang Thanks! It's almost there (and sorry for being a bit ambiguous about this), but there are just a couple things missing:

There should be an EpochVerifier which has a verify_light function that checks a block's seal under the validator set
The EpochVerifier should also have a check_finality_proof which does the same.
The validator set proof should be combined with the block header as a finality proof, and in conjunction can be used to produce an EpochVerifier. Look at how this is done in AuthorityRound for guidance.

How to check the seal:
In tendermint blocks have instant finality, so any block which is correctly signed is finalized. as such, only each block can only finalize itself, unlike authority round where any block can finalize an unbounded amount of others.

For checking the seal of a header, take a look at the verify_block_family function of Tendermint. The basic steps are:

iterate over header.seal.get(2) (fail if None) as UntrustedRlp
extract the signatures from each Precommit
The block is sealed if the number of unique signers is a majority of the epoch set, and unsealed otherwise.

rphmeier · 2017-07-21T10:03:04Z

+	}
+
+	fn check_finality_proof(&self, proof: &[u8]) -> Option<Vec<H256>> {
+		panic!("Tendermint blocks have instant finality, so `check_finality_proof` should never get called.")


tendermint blocks do have instant finality, so the "proof" here would just be an encoded header (which we check for seal validity).

the way warp sync for PoA works is by providing a list of validator set changes along with proofs of finality of the change. A change is signalled, and the old validator set approves the signal by finalizing the block where the signal occurred. From this proved list of validator set handoffs we can get to the most recent finalized block with full security. check_finality_proof is the way we check that signals were finalized, so it can't panic.

👍

To clarify, you're referring to https://github.com/paritytech/parity/blob/master/ethcore/src/snapshot/consensus/authority.rs?utf8=%E2%9C%93#L365?

yes, and https://github.com/paritytech/parity/blob/master/ethcore/src/snapshot/consensus/authority.rs?utf8=%E2%9C%93#L199

rphmeier · 2017-07-21T10:04:44Z

+		let ref header_signatures_field = header.seal().get(2).ok_or(BlockError::InvalidSeal)?;
+		for rlp in UntrustedRlp::new(header_signatures_field).iter() {
+			let signature = rlp.as_val()?; // TODO: why is `rlp` have type ()?
+			signatures.insert(signature);


this should also ensure that
a) each signature is a valid signature of header.bare_hash()
b) the public key recovered from each signature corresponds to an address in self.subchain_validators

I would recommend making the hashset store only those Addresses as opposed to the signatures themselves.

I am a little bit lost here. Looking at verify_block_family, it pattern matches on first converting the Header into either a Proposal or VoteStep. Then for Proposals it checks if the proposer is an authority in the parent block and is the proposer of the current proposal, and for VoteStep it does the counting of signatures.

Are we to assume here that the Header will always correspond to a VoteStep?

rphmeier · 2017-07-21T10:10:56Z

+		match self.validators.epoch_set(first, self, signal_number, set_proof) {
+			Ok((list, _)) => {
+				// Tendermint blocks have instant finality, so no need to check finality proof
+				ConstructedVerifier::Trusted(Box::new(EpochVerifier {


should still return Unconfirmed if some block hash is returned by epoch_set, and finality proofs may be checked (but not here). The "proof" passed here may be invalid, so this is also something of a verification function under the consensus logic.

👍 will add the logic for checking finality of the block hash returned by epoch_set.

Does something need to be done wrt checking validity of the "proof" passed here, or is it sufficient to have that happen at https://github.com/paritytech/parity/blob/master/ethcore/src/snapshot/consensus/authority.rs?utf8=%E2%9C%93#L199?

it should decode correctly into the two portions (signal proof and finality proof), and each portion should be checked. signal proof is checked if epoch_set returns Ok, and finality_proof will be checked at that point long as we return ConstructedVerifier::Unconfirmed from epoch_verifier

Is the finality proof for tendermint just ::rlp::encode(header)?

rphmeier · 2017-07-21T10:11:38Z

+		let mut signatures = HashSet::new();
+		let ref header_signatures_field = header.seal().get(2).ok_or(BlockError::InvalidSeal)?;
+		for rlp in UntrustedRlp::new(header_signatures_field).iter() {
+			let signature = rlp.as_val()?; // TODO: why is `rlp` have type ()?


i don't understand this comment. rlp has type UntrustedRlp

My bad, this was left over from my debugging >.<

rphmeier · 2017-07-21T13:16:08Z

Some comments I just made are shown as outdated, but they are still relevant

feynmanliang · 2017-07-21T20:34:48Z

@rphmeier thank you for your comments and helping me through this!

rphmeier · 2017-07-22T12:44:31Z

 use spec::CommonParams;
-use engines::{Engine, Seal, EngineError};
+use engines::{Engine, Seal, EngineError, ConstructedVerifier};
+use engines::authority_round::{combine_proofs, destructure_proofs};


could the tendermint module get its own versions of these functions (as the engines will be split into seperate crates in the future)?

rphmeier · 2017-07-22T12:45:08Z

 }

-fn destructure_proofs(combined: &[u8]) -> Result<(BlockNumber, &[u8], &[u8]), Error> {
+pub fn destructure_proofs(combined: &[u8]) -> Result<(BlockNumber, &[u8], &[u8]), Error> {


if these are kept private it gives us more freedom to upgrade their internal behavior

rphmeier · 2017-07-22T12:50:00Z

LGTM except for combine_proofs/destructure_proofs grumble. I'll also check out the branch and make sure everything's working before merging. Adding tests for the tendermint::EpochVerifier behavior would be good too.

feynmanliang · 2017-07-22T23:12:24Z

Copied {combine,destructure}_proofs over; I'm happy to do the tests (can you point me to a reference of what these might look like?).

feynmanliang · 2017-07-22T23:14:14Z

+		}
+
+		let n = addresses.len();
+		let threshold = self.subchain_validators.len() * 2/3;


Should this be a majority (i.e. 1/2) or 2/3 (for Byzantine fault tolerance)

Tendermint requires 2/3

feynmanliang · 2017-07-22T23:14:56Z

+
+		let n = addresses.len();
+		let threshold = self.subchain_validators.len() * 2/3;
+		if n > threshold {


Does it matter if it's ">" or ">="? I'd imagine in the case of three subchain_validators we'd want verification to pass when we get 2 of them

should always be >. In the case of 3 subchain validators you would actually need all 3 to be honest. With an amount of byzantine players f, we have to have at least 3f + 1 players total for safety.

rphmeier · 2017-07-23T14:35:02Z

One way you could test is by making EpochVerifier generic over a Recover trait (with fn recover(&self, H520) -> Result<Address, Error> -- in the actual engine this would be instantiated with an implementation that actually does the recovery, but in a test you can mock it with some custom mapping. Then you can test behavior of EpochVerifier<MockRecover> to ensure that check_finality_proof and verify_light work correctly.

feynmanliang · 2017-07-24T01:22:21Z

@rphmeier I added a recover closure parameter to EpochVerifier's constructor so I could stub it out in the tests.

rphmeier · 2017-07-24T11:11:45Z


 struct EpochVerifier {
 	subchain_validators: SimpleList,
+	recover: Box<Fn(&Signature, &Message) -> Result<Address, Error> + Send + Sync>,


Can we make this a generic parameter instead or even one thing in cfg(test) and another in cfg(not(test))? Doing several virtual calls per header verification will probably be a performance hit we don't have to take during ancient block download after warp sync.

I think we might need a virtual here if we want to have the signatures used in the mock recover to be grabbed from the environment. The alternative would be to hard code those addresses (which should remain fairly stable as long as res/tendermint.json isn't modified) and signatures (which will change whenever vote_info.sha3() or tap.sign change). I'll push a commit for this and see what you think

rphmeier · 2017-07-24T17:26:39Z


+struct EpochVerifier {
+	subchain_validators: SimpleList,
+	recover: Box<Fn(&Signature, &Message) -> Result<Address, Error> + Send + Sync>,


@feynmanliang

Try

struct EpochVerifier<F> where F: Fn(&Signature, &Message) -> Result<Address, Error> + Send + Sync { subchain_validators: SimpleList, recover: F, }

and just don't box the closures when you instantiate EpochVerifier.

rphmeier · 2017-07-24T20:20:06Z


-struct EpochVerifier {
+struct EpochVerifier<F>
+		where F: Fn(&Signature, &Message) -> Result<Address, Error> + Send + Sync {


style: brace on next line, unindented

also the where clause should be indented one level, not two

rphmeier · 2017-07-24T20:20:14Z


-impl super::EpochVerifier for EpochVerifier {
+impl <F> super::EpochVerifier for EpochVerifier<F>
+		where F: Fn(&Signature, &Message) -> Result<Address, Error> + Send + Sync {


style: brace on next line, unindented

rphmeier · 2017-07-25T20:42:54Z

@feynmanliang

LGTM, just will ask for another quick look-over by @keorn

gavofyork · 2017-07-26T15:19:09Z

@keorn ?

feynmanliang added 3 commits July 18, 2017 13:57

Adds signals_epoch_end to tendermint

799fab5

Adds is_epoch_end

8132529

Adds snapshot_components

d977921

gavofyork added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 19, 2017

gavofyork requested review from keorn and rphmeier July 19, 2017 08:16

rphmeier added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 19, 2017

feynmanliang added 4 commits July 20, 2017 17:23

Adds tendermint Epoch Verifier

f398fdf

Fix documentation typos

813260a

Change check_finality_proof to panic

f40b781

Fix compilation

c81e640

rphmeier reviewed Jul 21, 2017

View reviewed changes

rphmeier mentioned this pull request Jul 21, 2017

Aura test update (master) #6115

Closed

Adds Unconfirmed path to epoch_verifier

009da22

feynmanliang added 2 commits July 21, 2017 14:45

Verify if address is validator in EpochVerifier

f9ef415

check_finality_proof errors on failure

e5b9e24

rphmeier reviewed Jul 22, 2017

View reviewed changes

rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 22, 2017

rphmeier mentioned this pull request Jul 22, 2017

Contract-based Tendermint test paritytech/parity-import-tests#1

Open

Don't share combine/destructure_proofs

e03f9fb

feynmanliang commented Jul 22, 2017

View reviewed changes

Remove invalid import

24f7bb4

feynmanliang added 4 commits July 23, 2017 16:30

Remove duplicate epoch verifier trait

09a74ba

Fix docs

74668cc

Adds recover stub to tendermint EpochVerifier

4b752f1

Adds verify_light test

fe3ee5e

rphmeier reviewed Jul 24, 2017

View reviewed changes

Avoid boxed closure

df332f0

rphmeier reviewed Jul 24, 2017

View reviewed changes

Style fixes

d13050d

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 25, 2017

keorn merged commit 5eb8cea into openethereum:master Jul 26, 2017

feynmanliang deleted the tendermint-epoch-transitions branch July 26, 2017 16:12

Conversation

feynmanliang commented Jul 18, 2017

Uh oh!

parity-cla-bot commented Jul 18, 2017

Uh oh!

feynmanliang commented Jul 18, 2017

Uh oh!

parity-cla-bot commented Jul 18, 2017

Uh oh!

gavofyork commented Jul 19, 2017

Uh oh!

rphmeier commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier commented Jul 21, 2017

Uh oh!

feynmanliang commented Jul 21, 2017

Uh oh!

rphmeier Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier commented Jul 22, 2017

Uh oh!

feynmanliang commented Jul 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier commented Jul 23, 2017

Uh oh!

feynmanliang commented Jul 24, 2017

Uh oh!

rphmeier Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feynmanliang Jul 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

rphmeier Jul 21, 2017 •

edited

Loading

rphmeier Jul 21, 2017 •

edited

Loading

rphmeier Jul 22, 2017 •

edited

Loading

rphmeier Jul 24, 2017 •

edited

Loading

feynmanliang Jul 24, 2017 •

edited

Loading