Page MenuHomePhabricator

Replaced boolean options in CheckBlock functions with a dedicated class
ClosedPublic

Authored by jasonbcox on Mar 6 2018, 19:30.

Details

Summary

The use of booleans to pass options down multiple levels of functions is bad practice because context gets lost.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1181
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2189
Build 2522: Bitcoin ABC Buildbot (legacy)
Build 2521: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Mar 7 2018, 06:11
deadalnix requested changes to this revision.Mar 7 2018, 14:07
deadalnix added inline comments.
src/validation.cpp
3315 ↗(On Diff #3115)

There is a name convention for these things. We don't care that this only check PoW, we care about this doing all the checks that are not dependent on context.

This revision now requires changes to proceed.Mar 7 2018, 14:07
src/validation.cpp
3336 ↗(On Diff #3115)

And this is why the flag is passed down. The whole point of creating functions is to create abstractions. We don't care what they do exactly, we care that we can trust they do it properly.

A valid block must have a valid header. This doesn't need to know what a valid header is. By changing this, you just broke the abstraction.

schancel requested changes to this revision.Mar 7 2018, 16:39

Okay, makes sense.

Added BlockValidationOptions according to feedback and offline discussions.

deadalnix requested changes to this revision.Mar 9 2018, 22:44

Please fix the diff description. Passing a set of boolean to a constructor is a bit scary. It's very easy to mess up the ordering. One option is to use a similar structure as what was done for sighash, having a COW structure that you can get modified version of.

However, generally, I like where this is going. More type safety is always good. I'm a masochist, I like the compiler to tell me how bad I'm behaving.

src/validation.h
251 ↗(On Diff #3137)

You should make member private and have functiosn such as shouldValidatePoW() or shouldValidateMerkleRoot() . This will leave you with more option to tweak the internals later on.

251 ↗(On Diff #3137)

You can also pack this struct tight by doing

bool checkPoW : 1;
bool checkMerkleRoot : 1;
This revision now requires changes to proceed.Mar 9 2018, 22:44
jasonbcox retitled this revision from Cleaned up unnecessary conditional in CheckBlockHeader and renamed to CheckBlockHeaderPoW to Replaced boolean options in CheckBlock functions with a dedicated struct.Mar 10 2018, 17:32
jasonbcox edited the summary of this revision. (Show Details)

Hm. Linter fix wasn't actually applied apparently...

Updated according to feedback and changed struct -> class

jasonbcox marked an inline comment as done.
jasonbcox retitled this revision from Replaced boolean options in CheckBlock functions with a dedicated struct to Replaced boolean options in CheckBlock functions with a dedicated class.Mar 13 2018, 05:16
src/validation.h
251 ↗(On Diff #3147)

I have no idea what true alignment specification does. Can you point me to some docs?

257 ↗(On Diff #3147)

I'd prefer this constructor didn't exist and we had construction methods that are named for these flags. Kind of like what you did with sighashtype

Fixed bit field weirdness that I introduced before, and replaced optional constructor with setter functions instead.

jasonbcox added inline comments.
src/validation.h
251 ↗(On Diff #3147)

For posterity, we chatted about this on slack and I was confused with another programming language. In C++, the colon on a member variable definition is used for defining bit fields: http://en.cppreference.com/w/cpp/language/bit_field

deadalnix requested changes to this revision.Mar 15 2018, 15:02
deadalnix added inline comments.
src/validation.cpp
1925 ↗(On Diff #3171)

andreicow

3317 ↗(On Diff #3171)

This type of comment tends to become obsolete real quick. People can change the implementation and not change the comment.

It's usually best to describe the interface that is provided to a potential caller. If the caller needs to know what is inside anyway, then the abstraction is not built right. For instance

Return true if the provided block header is valid.
Only verify PoW if blockValidationOptions is configured to do so. This allows validation of headers on which the PoW hasn't been done, for instance to validate template handed to mining software.
This do not run any check that depend on the context. For this, you may want to have a look at ContextualCheckBlockHeader.

My english is not the best here, but I think you get the spirit. Describe the contract you are giving to the caller, bot the actual implementation.

3342 ↗(On Diff #3171)

Remove.

src/validation.h
265 ↗(On Diff #3171)

You don't need setters (also, in case you need to create a modified version of this, COW is preferable, but that shouldn't be required).

268 ↗(On Diff #3171)

const x2

This revision now requires changes to proceed.Mar 15 2018, 15:02
jasonbcox marked 2 inline comments as done.

Fixed according to feedback.

Although I would have liked to style this class like SigHashType, it made it unnecessarily complicated. We can switch to COW if/when the complexity increases.

COW makes the struct stateless, which is a huge plus.

This revision is now accepted and ready to land.Mar 31 2018, 08:25
This revision was automatically updated to reflect the committed changes.