Queue malice reports and fix the unit tests. #129
Conversation
|
This still doesn't fix a single test (#93), since there are several remaining problems:
|
|
Could you please clarify where we could see the unit test code for this test contract? |
|
Sure, this is about |
|
Could you also tell how the bytecode in |
|
I generated the bytcode using Remix from |
|
I tried too, but got a different one 😆 |
How do you determine the fact that the validator set is still the same? Seems there are no such checks in |
|
Weird; I used the default: 0.5.1+commit.c8a2cb62 without optimization.
I added print statements right after where |
|
@afck, is |
|
It is being called, multiple times, in blocks 2 and 3. |
|
Can you check whether this line is called? This is where the new set of validators should be coming from. |
|
Looks like that never gets called, no. |
|
I recall that it can only be called on the start of an epoch. |
I still get a bit different bytecode the last 34 bytes are different. Here are my remix settings: |
|
For the test contract https://github.com/poanetwork/parity-ethereum/blob/d9c611f11beb7fc777f2da56cbc8fab60fcd381b/ethcore/res/validator_contract.sol it doesn't matter whether the |
|
@varasev, |
…or that the call failed. Which is plausible because it's tried with zero gas. But even with default gas it failed for me, and that's what I'm having trouble debugging.
I'm not sure about that. Parity calls |
If I try again I'm getting yet another bytecode: No idea what's going on. Is Remix putting some kind of session identifier into the bytecode?? |
Sure, I'm just saying that the |
But it does. |
I just added some empty lines, comments, removed some existing comments, recompiled and those last 34 bytes changed. Probably it adds some hash of the source code into the bytecode 🤷♂️ I'll create an issue to their repo to ask about it (here it is) |
|
If the |
|
I added a print after the call to |
Could you show you test code for that? |
|
Getting new validators from the active validators in the contract would not make an atomic validator set switch. The pending validators are enclosed in |
Yeah, but it is weird that the |
|
OK, now The only thing we should add there is to call the So, first we should call the parity-ethereum/ethcore/src/engines/validator_set/safe_contract.rs Lines 364 to 367 in 539cf15 If After this small improvement, I'll retest it and then we will merge the |
|
I squashed and pushed the changes. |
|
Ok, there is one unit test failure at the moment: |
|
Right, sorry! We need to update the test contracts again: they don't have |
|
Seems to be so. I tried to launch unit tests for the Do you mean this test contract? |
|
Yes… I added the method and that fixed the test. But it's used in two places, and it somehow broke the other. I'm looking into that now… |
|
I think we should add there mapping(address => bool) banned;
function isValidatorBanned(address _miningAddress) public view returns(bool) {
return banned[_miningAddress];
}and change the function reportMalicious(address validator, uint256 blockNum, bytes calldata) external {
maliceReported[keccak256(abi.encode(validator, blockNum))].push(msg.sender);
if (validators[indices[validator]] == validator) {
banned[validator] = true;
validators[indices[validator]] = validators[validators.length-1];
delete indices[validator];
delete validators[validators.length-1];
validators.length--;
}
} |
|
I pushed a more low-tech solution, but yours would more faithfully reproduce the production logic, of course. 👍 |
|
Ok, I agree with your version and going to launch the unit tests for |
|
Seems that's OK now?
What other tests did you mean? |
|
The ones in |
This adds the malice report queue: Reports are included whenever we create a block ourselves, and they are also being resent as regular transactions until they have been successfully included in a block. reportMalicious now matches the ABI used by the validator set code. It also checks whether the removed validator exists. Squashed commits: ``` * Update the test validator set contract. `reportMalicious` now matches the ABI used by the validator set code. It also checks whether the removed validator exists. * Add maliceReportedForBlock to test contract. * Add back benign report check. * Fix reportBenign ABI. * Explicitly check malice report in the test. * Queue malice reports 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. * Queue reports even if transaction creation was successful. 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. * Fix malice report nonce addr; disable transactions. The nonce for malice reports needs to be the engine signer's, not the contract's. Creating malice report transactions makes the `reports_validators` test hang, so it is disabled for now. * Make the unit tests work again. * Retry sending queued malice reports. * Fix malice report block numbers. * added a constant to limit resending of returned reports * skip at least one block when reporting malicious validators * restored instantaneous malicious reports * Filter malice reports more aggressively. * Update test contracts: add isValidatorBanned. ```
This adds the malice report queue: Reports are included whenever we create a block ourselves, and they are also being resent as regular transactions until they have been successfully included in a block. reportMalicious now matches the ABI used by the validator set code. It also checks whether the removed validator exists. Squashed commits: ``` * Update the test validator set contract. `reportMalicious` now matches the ABI used by the validator set code. It also checks whether the removed validator exists. * Add maliceReportedForBlock to test contract. * Add back benign report check. * Fix reportBenign ABI. * Explicitly check malice report in the test. * Queue malice reports 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. * Queue reports even if transaction creation was successful. 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. * Fix malice report nonce addr; disable transactions. The nonce for malice reports needs to be the engine signer's, not the contract's. Creating malice report transactions makes the `reports_validators` test hang, so it is disabled for now. * Make the unit tests work again. * Retry sending queued malice reports. * Fix malice report block numbers. * added a constant to limit resending of returned reports * skip at least one block when reporting malicious validators * restored instantaneous malicious reports * Filter malice reports more aggressively. * Update test contracts: add isValidatorBanned. ```
This adds the malice report queue: Reports are included whenever we create a block ourselves, and they are also being resent as regular transactions until they have been successfully included in a block.
reportMaliciousnow matches the ABI used by the validator set code. It also checks whether the removed validator exists.Closes #107.
Closes #93.