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

Refactor Clique stepping #10691

Merged
merged 7 commits into from
Jun 10, 2019
Merged

Refactor Clique stepping #10691

merged 7 commits into from
Jun 10, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented May 23, 2019 •
edited
Loading

As a continuation of #10683, this PR tries to simplify the stepping. The three commits present three stages of simplification.

  1. Tidy things up and use Drop instead of homegrown stop() to shutdown the StepService: fe55764
  2. Move the StepService functionality inside Clique::new; shut it down using impl Drop for Clique: 709e8fe
  3. Don't try to shut down the thread at all, just let it run until the process exits: 56eb2ac

@dvdplm
Use Drop to shutdown stepper thread
fe55764 
Make period == 0 an error and remove the Option from step_service
@dvdplm
Remove StepService
709e8fe 
Remove StepService and spawn the stepping thread in `Clique::new()`. Don't store the thread handle and instead trust the `AtomicBool` to signal shutdown time.
Don't check for `period > 0`: we assume a valid chainspec file.
@dvdplm
Don't shutdown the stepper thread at all, just let it run until exit
56eb2ac 
Also: fix a few warnings and tests
@dvdplm
Put kvdb_memorydb back
f12d348
@dvdplm dvdplm requested review from tomusdrw and niklasad1 May 24, 2019 07:35
@dvdplm dvdplm self-assigned this May 24, 2019
@dvdplm dvdplm added the A0-pleasereview 🤓 Pull request needs code review. label May 24, 2019
tomusdrw
tomusdrw reviewed May 24, 2019
View reviewed changes
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks sane, few grumbles.

BTW why is timely step() necessary for Clique but not for authority round? Isn't it enough if some actions are performed during generate_seal() call of the engine?

ethcore/src/engines/clique/mod.rs Outdated Show resolved Hide resolved
ethcore/src/engines/clique/mod.rs Outdated Show resolved Hide resolved
ethcore/src/engines/clique/mod.rs Show resolved Hide resolved
ethcore/src/engines/clique/mod.rs Show resolved Hide resolved
@dvdplm
Copy link
Collaborator Author

dvdplm commented Jun 4, 2019

@niklasad1 ping

niklasad1
niklasad1 approved these changes Jun 6, 2019
View reviewed changes
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

LGTM,

Regarding @tomusdrw grumbles let's create an issue and investigate it further?! (I don't remember why)

@dvdplm dvdplm requested a review from tomusdrw June 10, 2019 09:30
tomusdrw
tomusdrw approved these changes Jun 10, 2019
View reviewed changes
Copy link
Collaborator

@tomusdrw tomusdrw 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!

ethcore/src/engines/clique/mod.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 10, 2019
@dvdplm dvdplm merged commit 083dcc3 into master Jun 10, 2019
@niklasad1 niklasad1 deleted the dp/refactor/clique-stepping branch June 10, 2019 10:24
dvdplm added a commit that referenced this pull request Jun 10, 2019
@dvdplm
Merge branch 'master' into dp/chore/aura-warn-when-validators-is-1-or…
070e585 
…-even

* master:
  Stop breaking out of loop if a non-canonical hash is found (#10729)
  Refactor Clique stepping (#10691)
dvdplm added a commit that referenced this pull request Jun 10, 2019
@dvdplm
Merge branch 'master' into dp/chore/aura-log-validator-set-in-epoch-m…
4f00ac7 
…anager

* master:
  Stop breaking out of loop if a non-canonical hash is found (#10729)
  Refactor Clique stepping (#10691)
dvdplm added a commit that referenced this pull request Jun 17, 2019
@dvdplm
Merge branch 'master' into dp/chore/remove-rust-crypto-from-devp2p
17c3a3b 
* master:
  updater: fix static id hashes initialization (#10755)
  Use fewer threads for snapshotting (#10752)
  Die error_chain, die (#10747)
  Fix deprectation warnings on nightly (#10746)
  fix docker tags for publishing (#10741)
  DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705)
  ethcore: enable ECIP-1054 for classic (#10731)
  Stop breaking out of loop if a non-canonical hash is found (#10729)
  Refactor Clique stepping (#10691)
  Use RUSTFLAGS to set the optimization level (#10719)
  SecretStore: non-blocking wait of session completion (#10303)
  removed secret_store folder (#10722)
  SecretStore: expose restore_key_public in HTTP API (#10241)
  Revert "enable lto for release builds (#10717)" (#10721)
  enable lto for release builds (#10717)
  Merge `Notifier` and `TransactionsPoolNotifier` (#10591)
dvdplm added a commit that referenced this pull request Jun 18, 2019
@dvdplm
Merge branch 'master' into dp/chore/update-devp2p-to-edition-2018
d2a8df0 
* master:
  Enable aesni (#10756)
  remove support of old SS db formats (#10757)
  [devp2p] Don't use `rust-crypto` (#10714)
  updater: fix static id hashes initialization (#10755)
  Use fewer threads for snapshotting (#10752)
  Die error_chain, die (#10747)
  Fix deprectation warnings on nightly (#10746)
  fix docker tags for publishing (#10741)
  DevP2p: Get node IP address and udp port from Socket, if not included in PING packet (#10705)
  ethcore: enable ECIP-1054 for classic (#10731)
  Stop breaking out of loop if a non-canonical hash is found (#10729)
  Refactor Clique stepping (#10691)
  Use RUSTFLAGS to set the optimization level (#10719)
  SecretStore: non-blocking wait of session completion (#10303)
  removed secret_store folder (#10722)
  SecretStore: expose restore_key_public in HTTP API (#10241)
  Revert "enable lto for release builds (#10717)" (#10721)
  enable lto for release builds (#10717)
  Merge `Notifier` and `TransactionsPoolNotifier` (#10591)
niklasad1 pushed a commit that referenced this pull request Oct 8, 2019
@dvdplm @niklasad1
Refactor Clique stepping (#10691)
6576bac 
* Use Drop to shutdown stepper thread
Make period == 0 an error and remove the Option from step_service

* Remove StepService

Remove StepService and spawn the stepping thread in `Clique::new()`. Don't store the thread handle and instead trust the `AtomicBool` to signal shutdown time.
Don't check for `period > 0`: we assume a valid chainspec file.

* Don't shutdown the stepper thread at all, just let it run until exit
Also: fix a few warnings and tests

* Put kvdb_memorydb back

* Warn&exit when engine is dropped
Don't sleep too long!

* Don't delay stepping thread

* Better formatting
niklasad1 added a commit that referenced this pull request Oct 23, 2019
@niklasad1
[stable]: backport #10691 and #10683 (#11143)
ebe4890 
* Fix compiler warning (that will become an error) (#10683)

* Remove annoying compiler warnings

* Fix compiler warning (that will become an error)

Fixes #10648

I'm not sure this fix is Good™ but rustc seems happy enough.

There's a deeper issue which may or may not be related to this: the Engine is not shutdown properly and the `StepService` thread keeps running indefinitely after Ctrl-C (so `update_sealing()` is called repeatedly for 300sec). I don't think this is related to Clique as I've seen this happen on mainnet as well, but I wonder if the effects of it are worse for a PoA network where the node can create new blocks all on its own?

* Use saturating_sub

* WIP

* Fix warning, second attempt

The idea here is to avoid using `Arc::get_mut()` (which does not work: fails every time here) and instead trust `drop()` to do the right thing.

This is a conservative change. I think this can be reformed further, e.g. by `impl Drop for StepService` and halt the thread there, or even skip `join()`ing the thread entirely and trust the `AtomicBool` to signal shutdown. I also have doubts abut the `Option<StepService>`: seems a bit much to have an `Option` there and it makes things cumbersome.

* Refactor Clique stepping (#10691)

* Use Drop to shutdown stepper thread
Make period == 0 an error and remove the Option from step_service

* Remove StepService

Remove StepService and spawn the stepping thread in `Clique::new()`. Don't store the thread handle and instead trust the `AtomicBool` to signal shutdown time.
Don't check for `period > 0`: we assume a valid chainspec file.

* Don't shutdown the stepper thread at all, just let it run until exit
Also: fix a few warnings and tests

* Put kvdb_memorydb back

* Warn&exit when engine is dropped
Don't sleep too long!

* Don't delay stepping thread

* Better formatting
@niklasad1 niklasad1 mentioned this pull request Oct 28, 2019
Closed
@soc1c soc1c mentioned this pull request Nov 5, 2019
Merged
36 tasks
s3krit pushed a commit that referenced this pull request Nov 11, 2019
@soc1c @s3krit
v2.5.10 stable (#11239)
f3cdd7b 
* ropsten #6631425 foundation #8798209 (#11201)
* [stable] builtin, istanbul and mordor testnet backports (#11234)
  * ethcore-builtin (#10850)
  * [builtin]: support `multiple prices and activations` in chain spec (#11039)
  * [chain specs]: activate `Istanbul` on mainnet (#11228)
  * ethcore/res: add mordor testnet configuration (#11200)
* Update list of bootnodes for xDai chain (#11236)
* ethcore: remove `test-helper feat` from build (#11047)
* Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159)
* [stable]: backport #10691 and #10683 (#11143)
  * Fix compiler warning (that will become an error) (#10683)
  * Refactor Clique stepping (#10691)
* Add Constantinople eips to the dev (instant_seal) config (#10809)
* Add cargo-remote dir to .gitignore (?)
* Insert explicit warning into the panic hook (#11225)
* Fix docker centos build (#11226)
* Update MIX bootnodes. (#11203)
* Use provided usd-per-eth value if an endpoint is specified (#11209)
* Add new line after writing block to hex file. (#10984)
* Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…)
* Upgrade jsonrpc to latest (#11206)
* [CI] check evmbin build (#11096)
* Correct EIP-712 encoding (#11092)
* [client]: Fix for incorrectly dropped consensus messages (#11086)
* Fix block detail updating (#11015)
* Switching sccache from local to Redis (#10971)
* Made ecrecover implementation trait public (#11188)
* [dependencies]: jsonrpc `14.0.1` (#11183)
* [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
* [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
* util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
* ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
* Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127)
* Cleanup stratum a bit (#11161)
* Upgrade to jsonrpc v14 (#11151)
* SecretStore: expose restore_key_public in HTTP API (#10241)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@tomusdrw tomusdrw tomusdrw approved these changes

Assignees

@dvdplm dvdplm

Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dvdplm @tomusdrw @niklasad1