Page MenuHomePhabricator

[move only] move default global vars to defaults.h
AbandonedPublic

Authored by schancel on Nov 6 2018, 20:32.

Details

Reviewers
jasonbcox
Group Reviewers
Restricted Project
Summary

As per title.

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
defaults
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3793
Build 5660: Bitcoin ABC Buildbot (legacy)
Build 5659: arc lint + arc unit

Event Timeline

schancel marked an inline comment as done.

Add missing header references.

@deadalnix
Should

/** Default setting for maximum allowed size for a block, in bytes */
static const uint64_t DEFAULT_MAX_BLOCK_SIZE = 32 * ONE_MEGABYTE;

from consensus.h be moved here? I think it should be.

src/defaults.h
146 ↗(On Diff #5678)

I'll address this in a bit

There's also a few things in policy.h

Fabien added inline comments.
src/validation.cpp
22

Duplicate

@deadalnix
Should

/** Default setting for maximum allowed size for a block, in bytes */
static const uint64_t DEFAULT_MAX_BLOCK_SIZE = 32 * ONE_MEGABYTE;

from consensus.h be moved here? I think it should be.

I'm torn. I think there's an argument for keeping it in consensus. The argument for moving it to defaults is because it can be overridden by miners, but tweaking this value can lead to chain instability if every miner has a different max set (allowing others to exploit this boundary, etc).

Otherwise, the diff looks fine to me. Getting many of these defaults out of validation.h to somewhere better described makes sense.

jasonbcox requested changes to this revision.Nov 13 2018, 04:18

Thought about it some more. I don't think this fixes the root cause of the issue: validation.h/cpp is full of junk that shouldn't be there. Moving these defaults out looks like on the surface, but would be a non-issue if code was properly segmented into appropriate headers. That way, defaults would live where they are used.

This revision now requires changes to proceed.Nov 13 2018, 04:18