search

poanetwork / posdao-contracts

Smart contracts for POSDAO (Proof of Stake Decentralized Autonomous Organization consensus), a DPOS consensus implemented in Solidity and running within EVM with swappable BFT consensus
Other
105 stars 49 forks source link

Add a getter for reportedly benign validators #34

Closed vkomenda closed 5 years ago

vkomenda commented 5 years ago

Parity unit tests check whether a validator has been reported as benign. For compatibility with those tests we need a method that checks that a given validator has been reported as such. Since reportBenign is currently a no-op and benign reports are not stored, we also need to start storing them for the purpose of such checks.

varasev commented 5 years ago

Let's just remove the unit tests related to benign reports because:

1) these reports make no sense for us (I even blocked them through TxPermission contract).

2) reportBenign function would allow a validator to send fake reports to fill block gas limit since each its call consumes gas. We have the same problem with reportMalicious at the moment, but I'm going to solve that with the TxPermission.allowedTxTypes.

3) it's better not to add excess functions into posdao-contracts (like reportBenign in this case) because excess code (especially which is not used in real work) increases the gas needed to deploy the contract. I didn't yet measure the gas needed to deploy each contract in posdao-contracts repo, but the contracts are already large enough to think of saving the gas. That way, we can get a problem in the future: when we want to upgrade some contract, we will need to deploy its new implementation. If the deployment gas is more than block gas limit, we won't be able to deploy new contracts version due to this limitation. So, it's better to decrease the size of contracts' code instead of increasing it.

4) Generally, we don't need the unit tests in Parity for those functions which we won't ever use.

vkomenda commented 5 years ago

3. it's better not to add excess functions into posdao-contracts (like reportBenign in this case)

@varasev, are you saying we should remove the reportBenign method placeholder? That is better than only removing the affected Parity unit tests, in particular because the deployment cost will be reduced.

varasev commented 5 years ago

Yes, I have already removed it from ValidatorSetAuRa in today's commit: https://github.com/poanetwork/posdao-contracts/commit/4825aba66020d1e3b5e9566d98a0e932e84f74f5#diff-e4a492be7f662d6148e33349be7eacffL77 - it's not yet merged into master at the moment since I'm testing the changes with posdao-test-setup.