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

Add a 2/3 quorum option to Authority Round. #10909

Merged
merged 3 commits into from
Aug 21, 2019
Merged

Add a 2/3 quorum option to Authority Round. #10909

merged 3 commits into from
Aug 21, 2019

Conversation

afck
Copy link
Contributor

@afck afck commented Jul 22, 2019

This prevents the "Attack of the Clones": https://arxiv.org/pdf/1902.10244.pdf

PR for POA's aura-pos branch: poanetwork#132
Original issue: poanetwork#108

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@afck
Copy link
Contributor Author

afck commented Jul 22, 2019

cargo audit failed because of memoffset, which is an indirect dependency via crossbeam-epoch: crossbeam-rs/crossbeam#402

@jam10o-new jam10o-new added the A0-pleasereview 🤓 Pull request needs code review. label Jul 22, 2019
@afck
Copy link
Contributor Author

afck commented Jul 25, 2019

I can't reproduce that test failure locally: cargo test in the rpc directory passes for me.

@ordian
Copy link
Collaborator

ordian commented Jul 25, 2019

I can't reproduce that test failure locally: cargo test in the rpc directory passes for me.

That's a known issue: #9636, it fails spuriously. The job is restarted.

dvdplm
dvdplm reviewed Aug 7, 2019
View reviewed changes
ethcore/src/engines/authority_round/finality.rs Outdated
match self.sign_count.entry(*signer) {
Entry::Occupied(mut entry) => {
// decrement count for this signer and purge on zero.
*entry.get_mut() -= 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I'm paranoid but if we ever call remove_signers() with an Occupied entry that is 0, then this will underflow and the signer will not be removed. Maybe you can do if *entry.get() == 1 before this and remove it if it is (and decrement if it isn't)?

ethcore/src/engines/authority_round/finality.rs Show resolved Hide resolved
@dvdplm dvdplm requested a review from andresilva August 7, 2019 15:51
@afck
Copy link
Contributor Author

afck commented Aug 8, 2019

I addressed both comments, but I'm not sure about them:
Both instances represent serious bugs within this module (engines::authority_round::finality): Whenever remove_signers is called, those signers are expected to be present in the map, with an entry ≥ 1.

@dvdplm
Copy link
Collaborator

dvdplm commented Aug 13, 2019

@afck the diff is looking a bit odd here, not sure what might have happened but it seems like a bunch of unrelated commits got added. Makes it a bit hard to review atm, can you take a look at that please?

@afck
Copy link
Contributor Author

afck commented Aug 13, 2019

Sorry! Fixed now.

ordian
ordian approved these changes Aug 14, 2019
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

LGTM, just minor grumbles.

json/src/spec/authority_round.rs Outdated Show resolved Hide resolved
ethcore/src/engines/authority_round/finality.rs Outdated Show resolved Hide resolved
@ordian ordian added this to the 2.7 milestone Aug 14, 2019
@ordian ordian added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 14, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 15, 2019
dvdplm
dvdplm reviewed Aug 15, 2019
View reviewed changes
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.

The module docs for Aura are a bit… scarce! :)
I think that we should document our preference for running with 2/3 quorum somewhere. Maybe this PR is a good opportunity for that?

@dvdplm
Copy link
Collaborator

dvdplm commented Aug 15, 2019

This lgtm; let's wait for @andresilva to give it a once-over and then merge it.

@afck
Copy link
Contributor Author

afck commented Aug 20, 2019

I rebased, and added a few lines to the module documentation.

andresilva
andresilva approved these changes Aug 21, 2019
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

lgtm

@andresilva
Copy link
Contributor

Thanks for the PR and sorry for taking so long to review. It was an easy review at this point though. :)

@dvdplm dvdplm merged commit 175051b into openethereum:master Aug 21, 2019
@afck afck deleted the afck-upstream-23quorum branch August 21, 2019 14:03
dvdplm added a commit that referenced this pull request Aug 21, 2019
@dvdplm
Merge branch 'master' into dp/chore/extract-clique
081e40e 
* master:
  Add a 2/3 quorum option to Authority Round. (#10909)
  Fix rlp decode for inline trie nodes. (#10980)
  Private contract migration and offchain state sync (#10748)
  manual publish jobs for releases, no changes for nightlies (#10977)
dvdplm added a commit that referenced this pull request Aug 22, 2019
@dvdplm
Merge branch 'dp/chore/extract-clique' into dp/chore/extract-spec-fro…
70a5f87 
…m-ethcore

* dp/chore/extract-clique:
  Configuration map of block reward contract addresses (#10875)
  Update ethcore/src/snapshot/consensus/mod.rs
  Add a 2/3 quorum option to Authority Round. (#10909)
  Missing import
  Rename supports_warp to snapshot_mode
  Introduce Snapshotting enum to distinguish the type of snapshots a chain uses
  Add an EngineType enum to tighten up Engine.name()
  signers is already a ref
  Update ethcore/engines/clique/src/lib.rs
  Update ethcore/engines/ethash/Cargo.toml
  Update ethcore/engines/basic-authority/Cargo.toml
  Update ethcore/block-reward/Cargo.toml
dvdplm added a commit that referenced this pull request Aug 23, 2019
@dvdplm
Merge branch 'dp/chore/extract-spec-from-ethcore' into dp/chore/sort-…
8a8e90a 
…out-ClientIoMessage

* dp/chore/extract-spec-from-ethcore:
  double semi
  Extract engines to own crates (#10966)
  Fix import
  missing import
  Configuration map of block reward contract addresses (#10875)
  Update ethcore/src/snapshot/consensus/mod.rs
  Add a 2/3 quorum option to Authority Round. (#10909)
  Missing import
  Rename supports_warp to snapshot_mode
  Introduce Snapshotting enum to distinguish the type of snapshots a chain uses
  Add an EngineType enum to tighten up Engine.name()
  signers is already a ref
  Update ethcore/engines/clique/src/lib.rs
  Update ethcore/engines/ethash/Cargo.toml
  Update ethcore/engines/basic-authority/Cargo.toml
  Update ethcore/block-reward/Cargo.toml
dvdplm added a commit that referenced this pull request Aug 25, 2019
@dvdplm
Merge branch 'dp/chore/sort-out-ClientIoMessage' into dp/feature/eip-…
55e74fe 
…1344-add-ChainID-opcode

* dp/chore/sort-out-ClientIoMessage:
  Extract spec to own crate (#10978)
  EIP 2028: transaction gas lowered from 68 to 16 (#10987)
  Fix merge problem
  double semi
  Extract engines to own crates (#10966)
  Fix import
  missing import
  Configuration map of block reward contract addresses (#10875)
  Update ethcore/src/snapshot/consensus/mod.rs
  Add a 2/3 quorum option to Authority Round. (#10909)
  Missing import
  Rename supports_warp to snapshot_mode
  Introduce Snapshotting enum to distinguish the type of snapshots a chain uses
  Add an EngineType enum to tighten up Engine.name()
  signers is already a ref
  Update ethcore/engines/clique/src/lib.rs
  Update ethcore/engines/ethash/Cargo.toml
  Update ethcore/engines/basic-authority/Cargo.toml
  Update ethcore/block-reward/Cargo.toml
@afck afck mentioned this pull request Oct 1, 2019
Closed
@LukaszRozmej LukaszRozmej mentioned this pull request Feb 7, 2020
Closed
@adria0 adria0 mentioned this pull request Nov 23, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dvdplm dvdplm dvdplm left review comments

@andresilva andresilva andresilva approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@afck @parity-cla-bot @ordian @dvdplm @andresilva @jam10o-new