Page MenuHomePhabricator

[WIP] Extensible Blockchain Protocol first approach
AbandonedPublic

Authored by jasonbcox on Mar 2 2018, 21:01.

Details

Reviewers
deadalnix
hanchon
Group Reviewers
Restricted Project
Summary

This is a basic implementation of the extensible blockchain protocol that allows the node to dinamically adapt its max block size when a new consensus is determined by the network.
The code was tested on a private testnet that simulated an extensible blockchain protocol. The firsts blocks were all 1Mb, then ~20 blocks of 2MB, followed by ~20 blocks of 4Mb and ~20 blocks of 8Mb. The node followed the chain.
There are some TODOs to improve the current code that are being worked on.
Also some functional test are being developed so the code can be tested using the current abc tools.

Some tests may break because we no longer return an error when a oversize blocks arrives.

Test Plan

None yet

Diff Detail

Repository
rABC Bitcoin ABC
Branch
EBP
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1968
Build 2085: Bitcoin ABC Buildbot (legacy)
Build 2084: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Mar 3 2018, 12:21
deadalnix added a subscriber: deadalnix.

This has no test and worse, is breaking existing tests.

src/config.h
68

Do not introduce more global state. There is plenty enough already.

src/validation.cpp
158

The codebase seems to be already able to track multiple header chain. Why is a new datastructure necessary ?

2363

Please use clang-format 4.0 .

3364

You are introducing a DoS vector right there (this vector also existed in bitprim codebase when I reviewed it).

This revision now requires changes to proceed.Mar 3 2018, 12:21
schancel retitled this revision from Extensible Blockchain Protocol first approach to [WIP] Extensible Blockchain Protocol first approach.Mar 4 2018, 19:49
schancel added inline comments.
src/validation.cpp
158

Are you talking about mapBlockIndex ? That's used for many other things right now. We'd need to verify that putting excessive blocks there doesn't cause some strange side effect.

src/validation.cpp
3364

This should probably only accept blocks that are within a certain threshold of a valid maximum?

src/config.h
68

Yes, this global state should be replaced with the correct reference in the AcceptBlock's parameters

src/validation.cpp
158

We wanted to keep the EBP blocks in a different array so they are treated as a different set. The mapBlockIndex is called in a lot of functions and we didn't want to break anything with this change

2363

I'll update my clang

3364

We can mark the blocks that are bigger than a max_value as invalid and reject it.
But what maximum value will be ok?
We can set for example a 128Mb limit, to avoid the validation of bigger blocks, but if the network grows to a point that a 128Mb block is valid, then this node will not be compatible.
I have no problem with this scenario but I thought that EBP would allow any changes on the block size.

If the DoS vector is because the rest of the code below the blocksize, we are planing to delay this validations until the EBP chain is long enough to be the valid one. (Comment on the Line 3857)
We are planing on only validate the PoW and then the Size to determinate if the block is EBP or not.
If the block is EBP we will check the rest of the consensus rules on that block at the moment that we know that the chain is ready (long enough) to be the active chain.
If the block is not EBP, we'll just check all the consensus rules the same way that they are being checked right now.

Pushing local changes before traveling to SV
The blocks are fully rejected if they are bigger than a value. (TODO: discuss what max block size whould be ok for this check)
The EBP block's transactions are validated only when their chain have enough PoW to become the active one. (The CheckBlock function was split in two functions)
A basic RPC test was created to simulate an EBP activation using 2 nodes.

(TODO: Update my clang format and autopep)

deadalnix requested changes to this revision.Apr 14 2018, 01:49

There are numerous problem with the design of that code, in addition, the DoS vector is still present (and last I checked, also present in bitprim's codebase).

src/config.h
68 ↗(On Diff #3234)

Then pass the config down, do not add more global state.

src/validation.cpp
3389 ↗(On Diff #3234)

It seems that you did various refactoring in addition to adding a ton of new code. This is impossible to review in that state and have any degree of confidence the code is correct.

3725 ↗(On Diff #3234)

Checking these in their own function is not appropriate. This forces all the checks that may trigger the block to be put asside while not invalid to be clobbed together. This is undesirable. The code that accept the block right now can accept the block as valid, or reject it for various reasons. one of these reasons is because the block is oversized. In such a case, the block may be reconsidered depending on what is mined in the future.

3774 ↗(On Diff #3234)

This seems to be a refactoring. Please submit a diff for refatorings. Mixing them with code changes is a recipe for disaster. Plus pay attention to details. The error messages are all wrong now.

3967 ↗(On Diff #3234)

This should have been the point where you realize that this whole piece of code doesn't belong here. AcceptBlock is a function that verify and accept a block, as per the config it's been passed down. It is not the function that decides which chain is valid, nor change the config or anything. You pass it a block, it accepts it if the block is valid, or mark it in some fashion if it isn't.

158

As a result we have a new storage that surely come with its own class of bugs, the consensus code for blocks is all mixed up with the logic to select block and manage configs. When you fear to change something the right path forward is to add test for that thing and/or to refactor the interface in order to enforce correct use. Create a second thing that has no more test and no better interface is not exactly how you keep technical debt under control.

This revision now requires changes to proceed.Apr 14 2018, 01:49
jasonbcox abandoned this revision.
jasonbcox added a reviewer: hanchon.