The use of booleans to pass options down multiple levels of functions is bad practice because context gets lost.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Commits
- rSTAGING778f27f7e2b0: Replaced boolean options in CheckBlock functions with a dedicated class
rABC778f27f7e2b0: Replaced boolean options in CheckBlock functions with a dedicated class
make check
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- checkblockheader
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 2043 Build 2233: Bitcoin ABC Buildbot (legacy) Build 2232: arc lint + arc unit
Event Timeline
| 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. |
| 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. |
Comment Actions
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; |
Comment Actions
Fixed bit field weirdness that I introduced before, and replaced optional constructor with setter functions instead.
| src/validation.h | ||
|---|---|---|
| 251 | 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 | |
| src/validation.cpp | ||
|---|---|---|
| 1925 ↗ | (On Diff #3171) |
|
| 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 |
Comment Actions
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.