dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Check stake before relaying high perf compact block #1110

Closed AM5800 closed 5 years ago

AM5800 commented 5 years ago

Fixes #903

High-bandwidth compact block relay can sometimes relay invalid blocks, however PoW check ensures that it is hard to create many of such blocks to cause any issues. After removing PoW we have lost this protection.

This PR introduces new AcceptStake function, which performs full stake validation as a part of AcceptBlock. At the moment this validation is only performed when new block extends active chain. Behaviour in case of fork is unchanged and subject to a separate feature/PR(which I am working on in parallel).

Signed-off-by: Aleksandr Mikhailov aleksandr@thirdhash.com

AM5800 commented 5 years ago

@scravy I deliberately decided to go without functional test because: 1) Checking that something didn't happened in functional test is somewhat hard/unreliable. In unit tests it is easier - we have AcceptBlock that either calls or does not call NewPoSValidBlock. 2) I sometimes think we write just too many "functional" tests and too few "unit-tests" (in quotes because boundaries are actually too vague). And we end up with test suite that runs forever... So if we can check this behaviour in "unit-test", why bother with "functional"?

scravy commented 5 years ago

I sometimes think we write just too many "functional" tests and too few "unit-tests" (in quotes because boundaries are actually too vague). And we end up with test suite that runs forever... So if we can check this behaviour in "unit-test", why bother with "functional"?

I agree to a certain extent and I am not saying there should not be unit tests. If there's a feature with only a functional tests added I would go raise the question "Where are all the unit tests?" and I do appreciate having unit tests on this very much.

But regarding the testability of this feature or protection, would we not be able to assert a reject message?

AM5800 commented 5 years ago

@scravy I don't think that checking reject is enough for "protection". Consider a hypothetical scenario where someone accidentally breaks AcceptStake function and it always returns true. In this case we will get our reject anyway (from ConnectBlock), but compact block will be sent

scravy commented 5 years ago

@scravy I don't think that checking reject is enough for "protection". Consider a hypothetical scenario where someone accidentally breaks AcceptStake function and it always returns true. In this case we will get our reject anyway (from ConnectBlock), but compact block will be sent

Fair enough, fine with me.

AM5800 commented 5 years ago

Putting WIP label because I would like to refactor several things

AM5800 commented 5 years ago

So, in the end I decided not to do this big refactoring. So only review fixes are here