Authority round consensus engine #3426
Conversation
keorn
added
A3-inprogress ⏳
| if let Some(engine) = self.engine.upgrade() { | ||
| engine.step.fetch_add(1, AtomicOrdering::SeqCst); | ||
| engine.proposed.store(false, AtomicOrdering::SeqCst); | ||
| if let Some(ref channel) = *engine.message_channel.try_lock().expect("Could not acquire message channel work.") { |
There was a problem hiding this comment.
what are the possible scenarios where this could occur? what would happen if you used a blocking lock call?
| @@ -167,6 +167,8 @@ pub enum BlockError { | |||
| UnknownParent(H256), | |||
| /// Uncle parent given is unknown. | |||
| UnknownUncleParent(H256), | |||
| /// The same author issued different votes at the same step. | |||
| DoubleVote(H160) | |||
There was a problem hiding this comment.
trailing comma missing.
one nice thing in a future redesign of the engine trait might involve an associated Error type to convey potential consensus errors (which are unique to each engine) as opposed to bundling all possible consensus errors in with block errors.
@gavofyork this is one of the use-cases of the diverging ! being promoted to a full type -- you might have something like an InstantSeal engine where consensus errors are not possible, and this makes it possible to specify it in the type-system.
| @@ -200,6 +202,7 @@ impl fmt::Display for BlockError { | |||
| RidiculousNumber(ref oob) => format!("Implausible block number. {}", oob), | |||
| UnknownParent(ref hash) => format!("Unknown parent: {}", hash), | |||
| UnknownUncleParent(ref hash) => format!("Unknown uncle parent: {}", hash), | |||
| DoubleVote(ref address) => format!("Author {} issued to many blocks.", address), | |||
keorn
added
A0-pleasereview 🤓
| proposed: AtomicBool::new(false) | ||
| }); | ||
| let handler = TransitionHandler { engine: Arc::downgrade(&engine) }; | ||
| engine.transistion_service.register_handler(Arc::new(handler))?; |
There was a problem hiding this comment.
spoke to @arkpar about this and we decided to hold off until the next release to start using ? to maintain a bit of backwards compatibility.
There was a problem hiding this comment.
we should just port everything over at once using untry.
|
Changes Unknown when pulling 9d46401 on auth-round into * on master*. |
|
Changes Unknown when pulling 9d46401 on auth-round into * on master*. |
|
Changes Unknown when pulling 071e66c on auth-round into * on master*. |
|
Changes Unknown when pulling fde6ff2 on auth-round into * on master*. |
| proposed: AtomicBool::new(false) | ||
| }); | ||
| let handler = TransitionHandler { engine: Arc::downgrade(&engine) }; | ||
| try!(engine.transistion_service.register_handler(Arc::new(handler))); |
There was a problem hiding this comment.
typo with this field name: transistion
| } | ||
|
|
||
| fn header_signature(header: &Header) -> Result<Signature, ::rlp::DecoderError> { | ||
| UntrustedRlp::new(&header.seal()[1]).as_val::<H520>().map(Into::into) |
| impl AuthorityRound { | ||
| /// Create a new instance of AuthorityRound engine. | ||
| pub fn new(params: CommonParams, our_params: AuthorityRoundParams, builtins: BTreeMap<Address, Builtin>) -> Result<Arc<Self>, Error> { | ||
| let initial_step = (unix_now().as_secs() / our_params.step_duration.as_secs()) as usize; |
There was a problem hiding this comment.
won't this be different every time you instantiate the engine? what would happen if a few rounds are gone through and the one authority has to restart?
There was a problem hiding this comment.
Yes, it will be different, all nodes rely on absolute unix time (transition according to it). This means they will always have the same step.
| self.proposed.store(true, AtomicOrdering::SeqCst); | ||
| return Some(vec![encode(&step).to_vec(), encode(&(&*signature as &[u8])).to_vec()]); | ||
| } else { | ||
| trace!(target: "poa", "generate_seal: FAIL: Accounts secret key unavailable."); |
There was a problem hiding this comment.
Here and below: If this is a misconfiguration error that user can make, it should be visible with a warn level
| /// This assumes that all uncles are valid uncles (i.e. of at least one generation before the current). | ||
| fn on_close_block(&self, _block: &mut ExecutedBlock) {} | ||
|
|
||
| fn is_sealer(&self, author: &Address) -> Option<bool> { |
There was a problem hiding this comment.
why have this return Option if it's unconditionally Some?
There was a problem hiding this comment.
Its part of the trait. This engine always has an answer if the address is a sealer, Ethash never has an answer.
There was a problem hiding this comment.
couldn't you express it as Ethash always returning true, in that any address may seal a block for the purposes of consensus?
There was a problem hiding this comment.
but with Ethash the generate_seal is called when external work is submitted. is_sealer really asks "I have a block here authored by this address, should I expect the Engine to be able to seal it for me now?", maybe should be is_sealer_now.
Ethash should return always false if there is no Option, but what it really should say is "idk", which I think is represented by None.
There was a problem hiding this comment.
ok, so this is only really valid for engines where seals_internally is true?
just curious :) I think this falls beyond the scope of this PR
There was a problem hiding this comment.
yep, It is actually used to determine if miner seals_internally.
|
Changes Unknown when pulling 8efaf08 on auth-round into * on master*. |
rphmeier
added
A8-looksgood 🦄
Authorities release blocks in a round based on unix epoch time. It remains lively under benign faults.
When ran with
--force-sealingit will release blocks even when no transactions are present making it BFT up to 50% with finality ofauthority_count * block_time.Initial docs