Report malice on sibling blocks from the same validator by vkomenda · Pull Request #165 · poanetwork/parity-ethereum

vkomenda · 2019-07-04T17:10:20Z

Fixes #164

Genysys · 2019-07-04T18:15:20Z

 	/// modifications.
 	posdao_transition: Option<BlockNumber>,
+	/// History of block hashes recently received from peers.
+	received_block_hashes: RwLock<BTreeMap<(u64, Address), H256>>,


Trying to understand the code base better so I can contribute. Why this added to the ~AuthorityRound only and not AuthorityRoundParams?

Because this is a run time map that is built from blocks received from the peers.

afck · 2019-07-04T19:53:52Z

+			if &new_hash != old_hash {
+				trace!(target: "engine", "Validator {} produced sibling blocks", header.author());
+				self.validators.report_malicious(header.author(), set_number, header.number(), Default::default());
+				Err(EngineError::SiblingBlocks(*header.author()))?;


I actually think we shouldn't return an error.
Because if the sender produced two sibling blocks, it could happen that the second one ends up getting finalized, so we need to make sure we import it. And the block itself isn't invalid, after all.
So I'd just log and report, and then continue.

vkomenda · 2019-07-04T22:12:14Z

cargo test in ethcore shows a deadlock in f93eaa4.

afck · 2019-07-05T06:25:21Z

+			.read()
+			.iter()
+			.map(|(k, _)| k)
+			.filter(|(n, _)| *n < oldest_block_number)


Since a BTreeMap's keys are already ordered (and tuples are ordered lexicographically), it shouldn't be necessary to iterate through all of them. You could replace filter with take_while, or use range or split_off. (Not sure what's best.)

afck · 2019-07-05T06:25:41Z

+			.received_block_hashes
+			.read()
+			.iter()
+			.map(|(k, _)| k)


iter().map(|(k, _)| k) is equivalent to keys().

vkomenda · 2019-07-05T13:16:32Z

There is a problem with upgrading the Weak pointer in 90dfaaf. I'm not removing the WIP status yet.

afck · 2019-07-05T15:47:03Z

+			let received_block_hashes = self.received_block_hashes.read();
+			received_block_hashes.get(&received_block_key).map(|h| h.clone())
+		};
+		if let Some(old_hash) = old_hash {


If we don't use old_hash elsewhere, we could just do this in one line:

if self.received_block_hashes.read().get(&received_block_key).any(|h| *h != new_hash) {

varasev · 2019-07-11T12:39:41Z

Can we simulate somehow the producing of sibling blocks on the same step? (for testing purposes like we did for testing reportMalicious feature in the afck-sim-malice branch).

afck

Looks good! 👍
(Just a non-blocking nit-pick.)

afck · 2019-07-22T11:54:01Z

+			.ok_or(EngineError::FailedSystemCall("Failed to upgrade to BlockchainClient.".to_string()))?;
+
+		// Remove older block hash records.
+		let oldest_block_number = full_client.best_block_header().number().saturating_sub(100);


Let's turn 100 into a constant. (SIBLING_DETECTION_PERIOD? MALICE_REPORTING_HORIZON? DUPLICATE_BLOCK_SEARCH_LIMIT? A_BETTER_NAME_I_CANT_THINK_OF_RIGHT_NOW!)

afck · 2019-07-22T11:56:25Z

But let's hold off merging until we've tested it, of course.
Yes, maybe we should update afck-sim-malice to produce sibling blocks.

afck

(Just two more nit-picks, but nothing blocking.)

afck · 2019-08-13T09:30:20Z

 			Err(EngineError::DoubleVote(*header.author()))?;
 		}
+		// Report malice if the validator produced other sibling blocks in the same step.
+		let received_block_key = (header.number(), header.author().clone());


I think Address is Copy:

Suggested change

let received_block_key = (header.number(), header.author().clone());

let received_block_key = (header.number(), *header.author());

Thanks! I rely on clippy too much in such cases, and it's not usable here at all.

afck · 2019-08-13T09:33:43Z

+			.collect();
+		for k in keys_to_remove {
+			self.received_block_hashes.write().remove(&k);
+		}


Could this be simplified using BTreeMap::split_off?

let rbh = self.received_block_hashes.write(); let new_rbh = rbh.split_off((oldest_block_number, Address::zero())); *rbh = new_rbh;

Sure, thanks!

varasev · 2019-08-13T10:43:11Z

Let's test this before merging

vkomenda · 2019-08-13T10:48:40Z

Let's test this before merging

I'm working on this.

varasev · 2019-09-12T10:56:47Z

Please don't merge this yet. It should be tested manually with posdao-test-setup first

afck · 2019-10-07T10:18:55Z

I had to rebase this, since aura-pos was rebased on v2.5.9.

varasev · 2019-10-18T08:34:05Z

@afck I wanted to test this PR with posdao-test-setup but seems it's a bit different from the upstream's merged version https://github.com/paritytech/parity-ethereum/pull/11160/files. Could you please check the changes in #165 and update them according to upstream's? After that, I will test this PR with posdao-test-setup.

afck · 2019-10-18T09:16:26Z

Closing in favor of #196.

varasev · 2019-10-18T09:38:36Z

Thanks, I will test #196

Genysys reviewed Jul 4, 2019

View reviewed changes

afck reviewed Jul 4, 2019

View reviewed changes

afck reviewed Jul 5, 2019

View reviewed changes

vkomenda marked this pull request as ready for review July 9, 2019 13:44

vkomenda assigned afck and varasev Jul 9, 2019

afck approved these changes Jul 22, 2019

View reviewed changes

afck force-pushed the aura-pos branch from 31e72e2 to 5355d05 Compare July 24, 2019 12:32

vkomenda force-pushed the vk-sibling-blocks branch 3 times, most recently from a4c5f66 to 75c2be2 Compare August 6, 2019 10:49

This was referenced Aug 6, 2019

Test reporting sibling blocks produced by the same validator #184

Closed

Test reporting sibling blocks from a validator gnosischain/posdao-test-setup#57

Closed

afck approved these changes Aug 13, 2019

View reviewed changes

vkomenda mentioned this pull request Aug 14, 2019

Test of sibling block malice gnosischain/posdao-test-setup#60

Closed

vkomenda force-pushed the vk-sibling-blocks branch from eb340eb to 70f4ec6 Compare September 5, 2019 17:17

afck requested changes Sep 6, 2019

View reviewed changes

Comment thread ethcore/src/engines/authority_round/mod.rs Outdated

vkomenda force-pushed the vk-sibling-blocks branch from 70f4ec6 to d00d1ac Compare September 10, 2019 14:15

afck approved these changes Sep 12, 2019

View reviewed changes

afck force-pushed the aura-pos branch from 52cb3b3 to 1e6a30b Compare October 7, 2019 10:03

added the map of received block header hashes

af706da

vkomenda added 6 commits October 7, 2019 12:08

do not return an error and remove older received block records

4d18359

optimised older record removal

2abdbb5

block hash comparison optimisation and the weak client ref fix

4ec216f

SIBLING_MALICE_DETECTION_PERIOD constant

4fa3d41

review comments

48856fd

using step numbers instead of block numbers

89c5848

afck force-pushed the vk-sibling-blocks branch from d00d1ac to 89c5848 Compare October 7, 2019 10:18

afck mentioned this pull request Oct 10, 2019

Aura: Report malice on sibling blocks from the same validator openethereum/parity-ethereum#11160

Merged

afck closed this Oct 18, 2019

afck deleted the vk-sibling-blocks branch October 18, 2019 09:16

	let received_block_key = (header.number(), header.author().clone());
	let received_block_key = (header.number(), *header.author());

Conversation

vkomenda commented Jul 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkomenda commented Jul 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkomenda commented Jul 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varasev commented Jul 11, 2019

Uh oh!

afck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afck commented Jul 22, 2019

Uh oh!

afck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varasev commented Aug 13, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkomenda commented Aug 13, 2019

Uh oh!

Uh oh!

varasev commented Sep 12, 2019

Uh oh!

afck commented Oct 7, 2019

Uh oh!

varasev commented Oct 18, 2019

Uh oh!

afck commented Oct 18, 2019

Uh oh!

varasev commented Oct 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

varasev commented Aug 13, 2019 via email •

edited

Loading