The blocks are set as excessive when they do not respect block size consensus rules.
The code acts the same way that when the block are invalid, but now it set the new flag.The code that stores the block is not included in this diff to keep it simple, it'll be uploaded in another one on top of this.
Details
Details
- Reviewers
deadalnix hanchon - Group Reviewers
Restricted Project - Maniphest Tasks
- T94: (EC) Mark the block as excessive
python3 qa/pull-tester/rpc-tests.py
make check
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- T94-marks
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 792 Build 792: arc lint + arc unit
Event Timeline
Comment Actions
I really do think this deserve unit tests rather than integration tests, so we can check that the right flags are set at the right places.
| src/consensus/validation.h | ||
|---|---|---|
| 27 ↗ | (On Diff #1211) | Is there a reason to put it in 3rd position ? From a logical perspective, it should be between valid and invalid =, unless there is some reason not to ? |
| 71 ↗ | (On Diff #1211) | You should forward to a call to DoS . There are also many parameters that do not seem useful. |
| 89 ↗ | (On Diff #1211) | There should probably a a IsNotValid (matching both invalid and excessive). |
| src/net_processing.cpp | ||
| 1019 ↗ | (On Diff #1211) | Use IsNotValid or alike. |
| src/validation.cpp | ||
| 2509 ↗ | (On Diff #1211) | I think it is worth adding a new mask for that one. |
| 3148 ↗ | (On Diff #1211) | If we are going to increase ban score for excessive blocks, we need to make sure we have some automatic way to decrement ban score. Also 100 seems excessive (!). |
| 3155 ↗ | (On Diff #1211) | dito |
Comment Actions
IsNotValid method created to group the excessive and invalid modes.
Added tests for the IsNotValid and IsExcessive functions.
Created new flag for blocks not valid, using the previous not valid block flag + the is excessive flag.
Mode Excessive moved to second position in the ValidationState to represent that the invalid mode is more restrictive.
Ban score when validating an excessive block modified to 10. (It may need to be lower)
| src/consensus/validation.h | ||
|---|---|---|
| 71 ↗ | (On Diff #1211) | DoS is now called, I'm checking if all the parameters are going to be needed or If I can just remove them. |
| src/validation.cpp | ||
| 3148 ↗ | (On Diff #1211) | I reduced the score to 10, but it may need to be lower. It should allow a node to send enough big blocks to activate EC but without attacking the node. When the select best chain algorithm is ready, this score should be easier to determinate. |
| src/consensus/validation.h | ||
|---|---|---|
| 57 | Dos has all these parameters because down the road this is the one that get called. Excessive do not need to conform the the API. For instance, it doesn't make sense to tag a block excessive because of corruption. | |
| src/test/blockcheck_tests.cpp | ||
| 25 |
| |
| 57 | Should there be a different code for excessive and invalid ? | |
| src/validation.cpp | ||
| 2513 | This comment is not accurate anymore. | |
| 3556 | This should be refactored to use IsNotValid. The only thing that differs between the 2 is the flag added to nStatus so that can be a different check. | |
| src/validation.cpp | ||
|---|---|---|
| 3148 ↗ | (On Diff #1211) | I'd say the best strategy here is probably to have a way to decrease the ban score of a node if the node doesn't misbehave for a while. But that is somewhat different from what is done here, so just doing 10 is fine, IMO. |
| qa/rpc-tests/abc-ec.py | ||
|---|---|---|
| 2 | Why adding Core copyright here? Isn't this file a brand new one created in the context of this diff? | |