[WIP] Extensible Blockchain Protocol first approach
AbandonedPublic
Actions

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 2114
Build 2373: Bitcoin ABC Buildbot (legacy)
Build 2372: arc lint + arc unit

Event Timeline

hanchon created this revision.Mar 2 2018, 21:01

Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 2 2018, 21:01

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

hanchon edited the summary of this revision. (Show Details)Mar 2 2018, 21:08

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B1968: Diff 3030!Mar 2 2018, 21:20

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

src/config.h
68 ↗	(On Diff #3030)	Do not introduce more global state. There is plenty enough already.
src/validation.cpp
158 ↗	(On Diff #3030)	The codebase seems to be already able to track multiple header chain. Why is a new datastructure necessary ?
2363 ↗	(On Diff #3030)	Please use clang-format 4.0 .
3364 ↗	(On Diff #3030)	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 a subscriber: schancel.Mar 4 2018, 20:01

schancel added inline comments.

src/validation.cpp
158 ↗	(On Diff #3030)	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.

schancel added inline comments.Mar 4 2018, 20:15

src/validation.cpp
3364 ↗	(On Diff #3030)	This should probably only accept blocks that are within a certain threshold of a valid maximum?

hanchon added inline comments.Mar 5 2018, 18:13

src/config.h
68 ↗	(On Diff #3030)	Yes, this global state should be replaced with the correct reference in the AcceptBlock's parameters
src/validation.cpp
158 ↗	(On Diff #3030)	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 ↗	(On Diff #3030)	I'll update my clang
3364 ↗	(On Diff #3030)	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)

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B2114: Diff 3234!Mar 16 2018, 14: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	Then pass the config down, do not add more global state.
src/validation.cpp
3389		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		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		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		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 ↗	(On Diff #3030)	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.