Page MenuHomePhabricator

Excessive blocks flag and mode
AbandonedPublic

Authored by jasonbcox on Aug 25 2017, 04:34.

Details

Reviewers
deadalnix
hanchon
Group Reviewers
Restricted Project
Maniphest Tasks
T94: (EC) Mark the block as excessive
Summary

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.

Test Plan

python3 qa/pull-tester/rpc-tests.py
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
T94-marks
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 792
Build 792: arc lint + arc unit

Event Timeline

hanchon created this revision.Aug 25 2017, 04:34
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 25 2017, 04:34
deadalnix requested changes to this revision.Aug 26 2017, 12:48
deadalnix added a subscriber: deadalnix.

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

This revision now requires changes to proceed.Aug 26 2017, 12:48
hanchon updated this revision to Diff 1261.Aug 28 2017, 22:21
hanchon edited edge metadata.

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)

hanchon edited the test plan for this revision. (Show Details)Aug 28 2017, 22:22
hanchon marked 5 inline comments as done.Aug 28 2017, 22:28
hanchon added inline comments.
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.

deadalnix added inline comments.Aug 29 2017, 11:19
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

likestamp

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.

deadalnix added inline comments.Aug 29 2017, 11:21
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.

sickpig added a subscriber: sickpig.Sep 4 2017, 20:43
sickpig added inline comments.
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?

deadalnix requested changes to this revision.Dec 5 2017, 14:27

Back on your queue.

This revision now requires changes to proceed.Dec 5 2017, 14:27
jasonbcox commandeered this revision.Sep 20 2018, 21:20
jasonbcox abandoned this revision.
jasonbcox added a reviewer: hanchon.

Cleaning up old diffs. Abandoning this to clear it off the review board.