Remove nMaxBlockSize global and move it to config
AbandonedPublic
Actions

Authored by jasonbcox on Jun 28 2018, 01:32.

Details

Reviewers

deadalnix
schancel

Group Reviewers

Restricted Project

Summary

Cleaning up a global variable. Enough said.

Test Plan

make check && test_runner && test_runner --extended

Repository

rABC Bitcoin ABC

Branch

globals

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2770
Build 3650: Bitcoin ABC Buildbot (legacy)
Build 3649: arc lint + arc unit

src/test/miner_tests.cpp
709	Story time: This is why globals are dangerous. This function fails under specific circumstances because it used to rely on the global max block size value. The code smell was the GlobalConfig being init'd in this function, and yet it accesses the same max block size that the entirely other GlobalConfig does which is init'd in the tests (such as BlockAssembler_construction below). This led to a weird bug where the test fails after trying to migrate away from the global. The simple solution was to use the same config (passed as a param) instead of init'ing a strange secondary instance.

deadalnix requested changes to this revision.Jun 30 2018, 14:58

deadalnix added inline comments.

src/config.cpp
10	These should be part of `Config`.
12	Do not throw strings. This should probably also be in the initializer list.
src/config.h
57	It seems like `GlobalConfig` was bastardized over time. As its name indicate, it's a wrapper that is supposed to access globals as if they were in the config. These should move to `Config`.
src/test/miner_tests.cpp
709	Create the config you want in there and do not rely on global at all. The fact that the `Config` was of type `GlobalConfig` should have been a tell that is relies on globals.
src/validation.cpp
3545	It doesn't seems to be a good idea to query the same infos again and again from an object. Whythe change ?

This revision now requires changes to proceed.Jun 30 2018, 14:58

jasonbcox planned changes to this revision.Jul 2 2018, 16:11

jasonbcox added inline comments.

src/config.cpp
12	It should not, because SetMaxBlockSize() can fail. If it fails during construction, we should throw. Looks like the throw type should be std::runtime_error.
src/config.h
57	Wow I did not know that GlobalConfig was intended to access globals. I thought "Global" in the class name was indicating an attempt to have it replace the use of global variables rather than utilize them. Can you elaborate on the intent? Will GlobalConfig go away in favor of a generic Config once we've migrated off globals entirely?
src/validation.cpp
3545	Perhaps not. I thought this just looked cleaner. I'll revert.