Page MenuHomePhabricator

WIP - EC: basic forking test and basic block size check
AbandonedPublic

Authored by jasonbcox on Aug 8 2017, 04:21.

Details

Reviewers
deadalnix
hanchon
Group Reviewers
Restricted Project
Summary

This diff is a WIP of the Emerging Consensus. (I don't know if it's ok to push WIPs or if I should work on a github repository and then push here when the code it's ready to be fully tested and reviewed)

Right now it's very simple and it's missing a lots of features, it just does not reject blocks bigger than the max block size and it follows the chain.

A test was added to check if the nodes can reorg to a chain that have "big blocks" (using EC) and if the nodes also can go back to a "normal size blocks" chain (valid using the current consensus rules). The chain to follow is always decided using accumulated work.

For now the code is called in the CheckBlock function, but it may be useful to not modified that function and "catch" the rejected blocks to revalidate them using the new consensus rules. That way the code may be simpler to follow, but if a lots of consensus rules are modified we may ended validating every block two times (one for each set of rules).

TODOs: some are descripted in the code. We also need to think how are we calling this consensus, if we are going to keep "Emerging Consensus" or change it to something else.

Test Plan

python3 rpc-tests.py abc-ec.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
wip-ec
Lint
Lint Passed
SeverityLocationCodeMessage
Auto-Fixqa/pull-tester/rpc-tests.py:1CFMTCode style violation
Unit
No Test Coverage
Build Status
Buildable 690
Build 690: arc lint + arc unit

Event Timeline

My view is we should steer clear of the name 'Emergent Consensus', and since we are going to implement policy choice here, call it something like 'Configurable Chain Acceptance Policy' (CCAP) or similar. (since this will in future not be used only for blocksize).

Since this is a WIP, I will only comment on high level - it would be nice to see a policy interface class. May I suggest coordination with the author of D387 .

deadalnix requested changes to this revision.Aug 8 2017, 10:28
deadalnix added a subscriber: deadalnix.

You just introduced a 0(n^2) algorithm in there.

The goal of a policy is that you can swap it in and out.

You API doesn't allow to do what we want to do, namely reconsidering the block when some condition is reached.

This revision now requires changes to proceed.Aug 8 2017, 10:28
hanchon edited edge metadata.

Trying the bitcoin unlimited EC code.
There are many changes and not everything has its test. I'm going to separate this diff in 2 or 3 smaller diff (each one with their tests) so it can be reviewed.

When I have the smaller diffs ready, I'll link them so the code can be reviewed easily. The smaller diff will have the tests and the hardcoded values will get their values from global variables.

CCulianu added inline comments.
src/policy/ec/blocksizepolicyinterface.h
14 ↗(On Diff #1135)

Code style nit: put d'tor virtual decl above regular methods or at lease separate from them (at top or bottom)

Also not sure how useful a pure virtual d'tor is in this case. Why not just make default impl. of this virtual d'tor do nothing to avoid having to reimplement it each time.. , as a courtesy ? :)

deadalnix requested changes to this revision.Aug 10 2017, 10:56

This is doing way to much and at least a good chunk of it is completely unnecessary. We already have a configurable block size, nothing ot change here for instance.

qa/pull-tester/rpc-tests.py
1 ↗(On Diff #1135)

You need to install autopep8

src/chain.h
150 ↗(On Diff #1135)

Put the comment before the declaration. Also this isn't BU.

157 ↗(On Diff #1135)

Aren't these used in on disk files ? If so, you'll break everything.

src/config.h
23 ↗(On Diff #1135)

There is no need to change what's the policy to determine the block size we accept here. This is irrelevant to what is trying to be done here.

src/validation.cpp
2510 ↗(On Diff #1135)

Do not use AD. You need to reverse the logic: ask a policy if you need to accept or not.

4825 ↗(On Diff #1135)

Do not comment out code that way. If make it hard to know what you changed, and history is in source control anyway.

This revision now requires changes to proceed.Aug 10 2017, 10:56
hanchon edited edge metadata.

Removed BU code and saved not valid yet chain to be reevaluated.
TODO: fix bug with iterators, update policy values when a new chain is accepted, change variables names to something smaller.

qa/pull-tester/rpc-tests.py
1 ↗(On Diff #1177)

Second step would be to run autopep8 on these file and make another diff that just do that and then rebase that one on top of the autopep8 diff. This'll keep the automated changes separated from your changes.

src/policy/ec/blocksizepolicyinterface.h
10 ↗(On Diff #1177)

This is absolutely not necessary. The block size is already configurable.

src/policy/ec/ecblocksize.h
9 ↗(On Diff #1177)

You need to have a policy for EC, not for the block size, which is already configurable.

src/validation.cpp
2649 ↗(On Diff #1177)

braces, especially for dandling else.

4757 ↗(On Diff #1177)

remove

deadalnix requested changes to this revision.Aug 16 2017, 21:30

Back on your queue

This revision now requires changes to proceed.Aug 16 2017, 21:30
hanchon edited edge metadata.

Removed blocksize code. Moved block check to the globalconfig. Fixed index error in FindMostWorkChain.

rebase to current master to get the autopep8 changes in the rpc-tests.py file

The autopep8 changes weren't in the master branch, i'll create a new diff with them

deadalnix requested changes to this revision.Aug 22 2017, 21:05

You broke the DoS protection provided by the block size. You can't remove the checks and need to bail there or the node may end up spending a large amount of computation on a block that isn't valid. You need to mark the block as excessive there and bail. Then, when the condition fixed by the policy are met, the block needs to be reconsidered (see what reconsiderblock rpc is doing, that may help).

src/config.h
8 ↗(On Diff #1186)

Remove.

You can declare opaque classes in header. The less header you include, the less recompilation occurs when you change something in a header, which is good.

12 ↗(On Diff #1186)

See opaque declaration here.

26 ↗(On Diff #1186)

config should not do anything. Just give config information. This is a design error to put this behavior here.

src/validation.cpp
3241 ↗(On Diff #1186)

You cannot remove these checks or an attacker can force you to reconstruct blocks that are absurdly huge.

3249 ↗(On Diff #1186)

Same here. The block needs to be marked as excessive here and the code should bail.

This revision now requires changes to proceed.Aug 22 2017, 21:05
jasonbcox abandoned this revision.
jasonbcox added a reviewer: hanchon.

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