Fixes for misbehavior reporting in AuthorityRound by andresilva · Pull Request #8998 · openethereum/parity-ethereum

andresilva · 2018-06-27T18:08:44Z

Use the epoch set when validating
Always report on the contract (self.validators)
Use correct epoch number when reporting
Report skipped primaries when generating seal (otherwise the current sealer won't report misbehavior since it won't verify his own block)

andresilva · 2018-06-27T20:59:58Z

Maybe fixes #8994 and #8065. I haven't been able to replicate so far.

tomusdrw · 2018-06-29T07:34:37Z

+				let set_number = if self.immediate_transitions {
+					header.number()
+				} else {
+					self.epoch_set(header)?.1


Is it guaranteed that parent block is already imported when we are doing block_basic verification? If not then zooming to the epoch set will most likely fail. Would be good to have a test case for this one.

Didn't think of this, so it's possible that verify_block_basic is done in parallel and therefore the parent might not be imported yet? This verification must be done in phase 1 since we want TemporarilyInvalid errors to be emitted at this phase (otherwise it won't recover the sync process).

Yes, I believe we do that when the block is imported to the queue:
https://github.com/paritytech/parity/blob/7d7d4822a5087b24b49f2a1ddb111f2daa51a017/ethcore/src/verification/verification.rs#L62

And it seems that both Phase 1 and Phase 2 operate on individual blocks, where there is no guarantee that parent block is already imported. We can only check that in Phase 3 (hence original todo comment was saying that bening reporting should be moved further in the verification.

tomusdrw · 2018-06-29T07:36:22Z

 		let active_set;
-		let validators = if self.immediate_transitions {
-			&*self.validators
+		let (validators, set_number) = if self.immediate_transitions {


This pattern seems to repeat couple of times, might be worth to extract it to a function?

I've refactored part of it into the epoch_set function. Unfortunately it's not easy to get the whole pattern into a function since both branches are returning different types (&ValidatorSet vs SimpleList). It could work by returning Box<ValidatorSet> from the function but I can't clone self.validators.

5chdn · 2018-06-29T08:24:49Z

Test engines::authority_round::tests::reports_skipped failed.

5chdn · 2018-07-05T06:44:14Z

Could you give me an ETA?

andresilva · 2018-07-05T13:38:49Z

I think this is good to review:

Created a CowLike type to return borrowed or owned data and refactor the epoch_set duplication
Let reporting fail on verify_block_basic:
- Only authorities can report and it's expected that they'll be up-to-date and importing
- Even if you are an authority that is syncing the chain the contract will ignore old reports
- This specific check is only relevant if you're importing (since it checks against wall clock)

tomusdrw

LGTM, a comment on why a failure of reporting is allowed would be good for the future.

tomusdrw · 2018-07-05T14:00:01Z

 			Err(BlockError::InvalidSeal) => {
-				let set_number = self.epoch_set(header)?.1;
-				self.validators.report_benign(header.author(), set_number, header.number());
+				if let Ok((_, set_number)) = self.epoch_set(header) {


Maybe add a comment why it may fail and why it's ok?

5chdn · 2018-07-05T17:26:53Z

Needs a second review, anyone?

* aura: only report after checking for repeated skipped primaries * aura: refactor duplicate code for getting epoch validator set * aura: verify_external: report on validator set contract instance * aura: use correct validator set epoch number when reporting * aura: use epoch set when verifying blocks * aura: report skipped primaries when generating seal * aura: handle immediate transitions * aura: don't report skipped steps from genesis to first block * aura: fix reporting test * aura: refactor duplicate code to handle immediate_transitions * aura: let reporting fail on verify_block_basic * aura: add comment about possible failure of reporting

* parity-version: bump stable to 1.10.9 * scripts: remove md5 checksums (#8884) * Add support for --chain tobalaba (#8870) * Add support for --chain tobalaba * Only return error log for rustls (#9025) * Fixes for misbehavior reporting in AuthorityRound (#8998) * aura: only report after checking for repeated skipped primaries * aura: refactor duplicate code for getting epoch validator set * aura: verify_external: report on validator set contract instance * aura: use correct validator set epoch number when reporting * aura: use epoch set when verifying blocks * aura: report skipped primaries when generating seal * aura: handle immediate transitions * aura: don't report skipped steps from genesis to first block * aura: fix reporting test * aura: refactor duplicate code to handle immediate_transitions * aura: let reporting fail on verify_block_basic * aura: add comment about possible failure of reporting

* parity-version: bump beta to 1.11.6 * scripts: remove md5 checksums (#8884) * Add support for --chain tobalaba * Convert indents to tabs :) * Fixes for misbehavior reporting in AuthorityRound (#8998) * aura: only report after checking for repeated skipped primaries * aura: refactor duplicate code for getting epoch validator set * aura: verify_external: report on validator set contract instance * aura: use correct validator set epoch number when reporting * aura: use epoch set when verifying blocks * aura: report skipped primaries when generating seal * aura: handle immediate transitions * aura: don't report skipped steps from genesis to first block * aura: fix reporting test * aura: refactor duplicate code to handle immediate_transitions * aura: let reporting fail on verify_block_basic * aura: add comment about possible failure of reporting * Only return error log for rustls (#9025) * Transaction Pool improvements (#8470) * Don't use ethereum_types in transaction pool. * Hide internal insertion_id. * Fix tests. * Review grumbles. * Improve should_replace on NonceAndGasPrice (#8980) * Additional tests for NonceAndGasPrice::should_replace. * Fix should_replace in the distinct sender case. * Use natural priority ordering to simplify should_replace. * Minimal effective gas price in the queue (#8934) * Minimal effective gas price. * Fix naming, add test * Fix minimal entry score and add test. * Fix worst_transaction. * Remove effective gas price threshold. * Don't leak gas_price decisions out of Scoring. * Never drop local transactions from different senders. (#9002) * Recently rejected cache for transaction queue (#9005) * Store recently rejected transactions. * Don't cache AlreadyImported rejections. * Make the size of transaction verification queue dependent on pool size. * Add a test for recently rejected. * Fix logging for recently rejected. * Make rejection cache smaller. * obsolete test removed * obsolete test removed * Construct cache with_capacity. * Optimize pending transactions filter (#9026) * rpc: return unordered transactions in pending transactions filter * ethcore: use LruCache for nonce cache Only clear the nonce cache when a block is retracted * Revert "ethcore: use LruCache for nonce cache" This reverts commit b382c19. * Use only cached nonces when computing pending hashes. * Give filters their own locks, so that they don't block one another. * Fix pending transaction count if not sealing. * Clear cache only when block is enacted. * Fix RPC tests. * Address review comments. * A last bunch of txqueue performance optimizations (#9024) * Clear cache only when block is enacted. * Add tracing for cull. * Cull split. * Cull after creating pending block. * Add constant, remove sync::read tracing. * Reset debug. * Remove excessive tracing. * Use struct for NonceCache. * Fix build * Remove warnings. * Fix build again. * miner: add missing macro use for trace_time * ci: remove md5 merge leftovers

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Fixes for misbehavior reporting in AuthorityRound (openethereum#8998) A last bunch of txqueue performance optimizations (openethereum#9024) reduce number of constraints for triedb types (openethereum#9043) bump fs-swap to 0.2.3 so it is compatible with osx 10.11 again (openethereum#9050) Recursive test (openethereum#9042) Introduce more optional features in ethcore (openethereum#9020) Update ETSC bootnodes (openethereum#9038) Optimize pending transactions filter (openethereum#9026) eip160/eip161 spec: u64 -> BlockNumber (openethereum#9044)

andresilva added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jun 27, 2018

andresilva requested review from rphmeier and tomusdrw June 27, 2018 18:08

andresilva added the B0-patchthis label Jun 27, 2018

5chdn added this to the 1.12 milestone Jun 27, 2018

andresilva added 7 commits June 27, 2018 20:56

aura: only report after checking for repeated skipped primaries

95d6dba

aura: refactor duplicate code for getting epoch validator set

8013df6

aura: verify_external: report on validator set contract instance

ed1028e

aura: use correct validator set epoch number when reporting

afcb947

aura: use epoch set when verifying blocks

fbafc04

aura: report skipped primaries when generating seal

03210a8

aura: handle immediate transitions

dccc9e7

andresilva force-pushed the andre/fix-aura-reporting branch from df3b40b to dccc9e7 Compare June 27, 2018 19:57

aura: don't report skipped steps from genesis to first block

39ae1c2

tomusdrw reviewed Jun 29, 2018

View reviewed changes

This was referenced Jun 30, 2018

Beta: 1.11.6 backports #9015

Merged

Stable: 1.10.9 backports #9016

Merged

andresilva added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 2, 2018

aura: fix reporting test

3e2096a

5chdn added B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible. labels Jul 5, 2018

andresilva added 2 commits July 5, 2018 14:34

aura: refactor duplicate code to handle immediate_transitions

3641f17

aura: let reporting fail on verify_block_basic

86b3fdf

andresilva added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 5, 2018

tomusdrw approved these changes Jul 5, 2018

View reviewed changes

aura: add comment about possible failure of reporting

b3e9c73

tomusdrw approved these changes Jul 5, 2018

View reviewed changes

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 5, 2018

ascjones approved these changes Jul 6, 2018

View reviewed changes

ascjones merged commit e9bd41b into master Jul 6, 2018

ascjones deleted the andre/fix-aura-reporting branch July 6, 2018 09:44

Conversation

andresilva commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andresilva commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

andresilva Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

tomusdrw Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

tomusdrw Jun 29, 2018

Choose a reason for hiding this comment

Uh oh!

andresilva Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

5chdn commented Jun 29, 2018

Uh oh!

5chdn commented Jul 5, 2018

Uh oh!

andresilva commented Jul 5, 2018

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

andresilva Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

5chdn commented Jul 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andresilva commented Jun 27, 2018 •

edited

Loading

andresilva commented Jun 27, 2018 •

edited

Loading