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

aura: don't report skipped primaries when empty steps are enabled #9435

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Aug 30, 2018

Fix #9325.

When empty steps are enabled we cannot report skipped primaries since we have no meaningful way to prove that the authority didn't actually broadcast an empty step message. This is already the existing behavior when verifying blocks (https://github.com/paritytech/parity-ethereum/blob/master/ethcore/src/engines/authority_round/mod.rs#L1146-L1180).

@andresilva andresilva added A0-pleasereview 🤓 Pull request needs code review. B0-patchthis M4-core ⛓ Core client code / Rust. labels Aug 30, 2018
Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

I think this is okay, as it keeps the behavior aligned with verify_block_family. However, there may still be a way to report skipped when generating seal:

  • Gather all current empty_steps proof via self.empty_steps(..).
  • Check which block is not in the range, get the primary for that block, and call report_skipped for all of them.

That said, I think the root issue is that we didn't include empty steps proof in seal. Our docs for EmptyStep struct still says "Other authorities accumulate these messages and later include them in the seal as proof", so this probably was an intended but not implemented feature.

@andresilva
Copy link
Contributor Author

andresilva commented Aug 30, 2018

@sorpaas The reason we don't report on skipped empty steps is because these messages are broadcast with no reliable delivery semantics, if you don't receive an empty step message it may be because the authority is offline, or it may also be due to some network constraint (or some rogue agent is blocking the message from reaching you). The empty step messages that an authority accumulates are included in the seal (https://github.com/paritytech/parity-ethereum/blob/master/ethcore/src/engines/authority_round/mod.rs#L1000), this is actually required to verify the block difficulty.

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 6, 2018
@5chdn 5chdn merged commit 4e8e5bb into master Sep 6, 2018
@5chdn 5chdn deleted the andre/aura-fix-report-skipped branch September 6, 2018 11:33
5chdn added a commit that referenced this pull request Sep 10, 2018
* parity-version: bump beta to 2.0.4

* [light/jsonrpc] Provide the actual account for `eth_coinbase` RPC and unify error handeling for light and full client (#9383)

* Provide the actual `account` for eth_coinbase

The previous implementation always provided the `zero address` on
`eth_coinbase` RPC. Now, instead the actual address is returned on
success or an error when no account(s) is found!

* full client `eth_coinbase` return err

In the full-client return an error when no account is found instead of
returning the `zero address`

* Remove needless blocks on single import

* Remove needless `static` lifetime on const

* Fix `rpc_eth_author` test

* parity: print correct keys path on startup (#9501)

* aura: don't report skipped primaries when empty steps are enabled (#9435)

* Only check warp syncing for eth_getWorks (#9484)

* Only check warp syncing for eth_getWorks

* Use SyncStatus::is_snapshot_syncing

* Fix Snapshot restoration failure on Windows (#9491)

* Close Blooms DB files before DB restoration

* PR Grumbles I

* PR Grumble

* Grumble
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