Queue malice reports by DemiMarie · Pull Request #107 · poanetwork/parity-ethereum

DemiMarie · 2019-03-06T14:02:31Z

This is #98, reopened.

afck

Still looks good to me, once we've found a way to properly test it.
And ideally we should test both ways to submit a report:

Including it in a block we're authoring.
Sending it to a peer as a transaction.

DemiMarie · 2019-03-07T12:59:31Z

@afck How should we test it? What about using #101?

afck · 2019-03-07T13:08:06Z

Yes, but possibly #101 will have to be extended to test both cases.

DemiMarie · 2019-03-07T13:46:23Z

We also need to test the case where a node submits a block, even though they are not a validator.

varasev · 2019-03-11T09:12:34Z

I'm going to test this after I make some high priority tasks (describing the contracts in README, Whitepaper, and commenting contracts' code to help auditors understand the contracts easily - I think it may take 1-2 weeks).

This is a squash of 18 commits: Queue failed reports for later insertion into blocks If we fail to send a transaction that reports a validator as malicious, store it on a stack. When we next seal a block, included the failed transactions. Insert our reports every time we prepare a block Implement queued reporting Prioritize the `emitInitiateChange` call Fix warnings These warnings cluttered build output, and made it hard to see actual warnings in new code. Implement (partial) cache purging. Once an entry that cannot be purged is found, further entries are skipped. Finish report queuing Use the correct reporting address Drop reports beyond MAX_QUEUED_REPORTS This helps prevent denial-of-service attacks. Drop queued report limits We now only keep 500 reports that have not been confirmed, or 100 reports that have been. Lower limits on queued reports Add a script to generate ABI files and use it to fix compilation error due to incomplete ABI json. Remove old validator reports Reports over 100 blocks old are useless anyway. Fix an unused variable warning in the test client Show a better error when the validator set contract is bad Fix backwards comparison between bad and current block numbers Drop reports for block numbers ahead of the current one Fix review comments from Andreas Remove a commented-out constant, and use the standard library `VecDeque::retain()` method instead of (inefficiently) reimplementing it ourselves.

Having created a transaction doesn't guarantee that it will be included in a block. We should queue the transaction anyway. Also remove some unnecessary closures and match statements.

afck · 2019-04-25T09:00:04Z

I squashed, rebased and added another commit.

vkomenda · 2019-04-25T11:39:17Z

+				let result = client.call_contract(BlockId::Latest, self.contract_address, data)
+					.expect("this is a bug in the genesis block; either the validator set contract is missing, or it is invalid; this is a fatal error in the configuration of Parity, so we cannot recover; qed");
+				decoder.decode(&result[..])
+					.expect("this is a bug in the genesis block; either the validator set contract is missing, or it is invalid; this is a fatal error in the configuration of Parity, so we cannot recover; qed")


This error message should differ from the one just above.

I agree; I'm not even convinced we should panic here: The network could still continue without a malice report queue, and just print a warning. Or is that too dangerous?

A common solution is returning an Err. This is the only occurrence of a panic in such a case as far as I can see.

vkomenda · 2019-04-25T11:41:10Z

+				if current_block_number > 100 && current_block_number - 100 > block {
+					return false // Report is too old and cannot be used
+				}
+				let (data, decoder) = validator_set::functions::malice_reported_for_block::call(


We should include this method in the test validator contract.

Working on this now.

Ah, sorry, me too! I would have already pushed it, if it hadn't caused the tests to fail in new, original and unexpected ways!

test engines::validator_set::contract::tests::reports_validators ... test engines::validator_set::contract::tests::reports_validators has been running for over 60 seconds

I suspect this is actually a problem with the queue implementation; why else would the test get stuck?
I'll add my contract change to the other PR first, then get back to this issue…

Same for me. It gets stuck :(

Added to #129 now.

And that shows that we really need to get #129 merged before we can continue with this one.

And not only reports_validators:

test engines::validator_set::contract::tests::reports_validators ... test engines::validator_set::contract::tests::reports_validators has been running for over 60 seconds test ethereum::ethash::tests::can_do_proof_of_work_unordered_verification_fail ... test ethereum::ethash::tests::can_do_proof_of_work_unordered_verification_fail has been running for over 60 seconds test ethereum::ethash::tests::can_do_seal256_verification_fail ... test ethereum::ethash::tests::can_do_seal256_verification_fail has been running for over 60 seconds

varasev · 2019-05-14T11:37:10Z

Can I start to test this with posdao-test-setup or there are some fixes waiting to be done?

afck · 2019-05-14T11:47:16Z

Part of this is already in #129.
We still need to figure out how to re-enable and fix the part that's commented out in there; but probably this PR should be closed in favor of #129.

varasev · 2019-05-14T12:11:51Z

We still need to figure out how to re-enable and fix the part that's commented out in there

Do you mean this?

parity-ethereum/ethcore/src/engines/validator_set/safe_contract.rs

Lines 344 to 362 in 74fea45

    
           // TODO: Enable re-sending reports. Figure out why this makes the `reports_validators` test hang. 
        
           // 
        
           // let mut nonce = client.latest_nonce(address); 
        
           // debug!(target: "engine", "Checking for cached reports"); 
        
           // for (address, block, data) in self.queued_reports.lock().iter() { 
        
           // 	debug!(target: "engine", "Trying to report"); 
        
           // 	while match self.transact(data.clone(), nonce) { 
        
           // 		Ok(()) => false, 
        
           // 		Err(Error(ErrorKind::Transaction(transaction::Error::Old), _)) => true, 
        
           // 		Err(err) => { 
        
           // 			warn!(target: "engine", "Cannot report validator {} for misbehavior on block {}: {}", 
        
           // 				  address, block, err); 
        
           // 			false 
        
           // 		} 
        
           // 	} { 
        
           // 		nonce += U256::from(1); 
        
           // 	} 
        
           // 	nonce += U256::from(1); 
        
           // }

afck · 2019-05-14T12:19:36Z

Yes, exactly. But I think I already have a solution. Will push it to #129.

varasev · 2019-05-30T11:07:05Z

Done in #129.

afck reviewed Mar 7, 2019

View reviewed changes

varasev mentioned this pull request Mar 8, 2019

Service transactions consume account nonces! #97

Closed

4 tasks

varasev mentioned this pull request Apr 1, 2019

Failover script for a backed up node gnosischain/posdao-test-setup#41

Open

vkomenda force-pushed the aura-pos branch from 414ab08 to 9e77f74 Compare April 18, 2019 11:54

DemiMarie and others added 2 commits April 25, 2019 10:50

Queue reports even if transaction creation was successful.

e4cfd7d

Having created a transaction doesn't guarantee that it will be included in a block. We should queue the transaction anyway. Also remove some unnecessary closures and match statements.

afck force-pushed the no-consume-nonces branch from 85b58a9 to e4cfd7d Compare April 25, 2019 08:59

vkomenda reviewed Apr 25, 2019

View reviewed changes

This was referenced Apr 30, 2019

Fix the validator set unit tests. #93

Closed

Queue malice reports and fix the unit tests. #129

Merged

varasev mentioned this pull request May 17, 2019

POSDAO reportMalicious implementation #136

Closed

phahulin closed this in #129 May 30, 2019

Conversation

DemiMarie commented Mar 6, 2019

Uh oh!

afck left a comment

Choose a reason for hiding this comment

Uh oh!

DemiMarie commented Mar 7, 2019

Uh oh!

afck commented Mar 7, 2019

Uh oh!

DemiMarie commented Mar 7, 2019

Uh oh!

varasev commented Mar 11, 2019

Uh oh!

afck commented Apr 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkomenda Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 May 14, 2019

Uh oh!

afck commented May 14, 2019

Uh oh!

varasev commented May 14, 2019

Uh oh!

afck commented May 14, 2019

Uh oh!

varasev commented May 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vkomenda Apr 25, 2019 •

edited

Loading