Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

AuthorityEngine: Minor cleanups. #11408

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

afck
Copy link
Contributor

@afck afck commented Jan 26, 2020

This removes some repetitions, reformats very long lines and adds a few comments.

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

looks good; have a few questions.

step: u64,
/// The hash of the most recent block.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the hash of the parent to the block where the authority emits the EmptyStep message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, an empty step message is created instead of a block, if a validator has no pending transactions. It cannot itself be a parent, and this hash always points to the most recent block.
E.g.:

  • Validator A creates block 1.
  • Validator B has no pending transaction, so it signs an empty step message mB instead whose hash points to block 1.
  • Validator C also has no pending transactions, so it also signs an empty step message mC instead whose hash points to block 1.
  • Validator D creates block 2. The parent is block 1, and the header includes mB and mC.

(Would be good if someone could confirm my understanding!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right, makes sense now. Thank you for explaining.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe your excellent comment here could go into the module level docs or the docs for EmptyStep for the benefit of the next clueless code reader? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

ethcore/engines/authority-round/src/lib.rs Outdated Show resolved Hide resolved
ethcore/engines/authority-round/src/lib.rs Show resolved Hide resolved
ethcore/engines/authority-round/src/lib.rs Show resolved Hide resolved
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. labels Jan 27, 2020
@ordian ordian merged commit e695393 into openethereum:master Jan 27, 2020
@afck afck deleted the afck-aura-clean branch January 27, 2020 13:45
dvdplm added a commit that referenced this pull request Jan 29, 2020
…pstream

* master:
  Add POSDAO transition and malice report queue. (#11245)
  update master/nightly version: v2.8.0 (#11419)
  ethcore/res: remove morden testnet (#11392)
  fix: export hardcoded sync format (#11416)
  update hardcoded headers: mainnet and ropsten (#11414)
  AuthorityEngine: Minor cleanups. (#11408)
  Update POA bootnodes (#11411)
  Add EtherCore support (#11402)
  verification: fix race same block + misc (#11400)
  Update ProgPoW to 0.9.3 (#11407)
  update classic testnet bootnodes (#11398)
  dependencies: bump `derive_more v0.99` (#11405)
  engine error: remove faulty/unused `From` (#11404)
  Switching to stable-track (#11377)
  ethcore/res: fix ethereum classic chainspec blake2_f activation block num (#11391)
  Update copyright notice 2020 (#11386)
afck added a commit to poanetwork/parity-ethereum that referenced this pull request Feb 25, 2020
afck added a commit to poanetwork/parity-ethereum that referenced this pull request Feb 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants