Differential D2014

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

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 (branched from master)

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3792
Build 5658: Bitcoin ABC Buildbot (legacy)
Build 5657: arc lint + arc unit

Event Timeline

schancel created this revision.Nov 6 2018, 20:32

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 6 2018, 20:32

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B3792: Diff 5678!Nov 6 2018, 20:37

Add missing header references.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

schancel added a child revision: D1936: Move handling for `-maxtxfee` to the config object.Nov 6 2018, 23:13

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3793: Diff 5679.Nov 6 2018, 23:16

@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	I'll address this in a bit

There's also a few things in policy.h

Fabien added a subscriber: Fabien.Nov 8 2018, 09:46

Fabien added inline comments.

src/validation.cpp
22 ↗	(On Diff #5679)	Duplicate

In D2014#43395, @schancel wrote:
@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.

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

schancel removed a child revision: D1936: Move handling for `-maxtxfee` to the config object.Nov 21 2018, 17:59

schancel added a child revision: D1936: Move handling for `-maxtxfee` to the config object.Nov 21 2018, 18:00

schancel removed a child revision: D1936: Move handling for `-maxtxfee` to the config object.Nov 25 2018, 09:44

schancel abandoned this revision.Dec 1 2018, 16:30

schancel added a child revision: D1936: Move handling for `-maxtxfee` to the config object.Mar 17 2019, 06:28