refactor(config): replace manual parsing with ConfigBeanFactory binding

[vividctrlalt]

What

Replace ~850 lines of manual if (config.hasPath(KEY)) / getXxx blocks in
Args.applyConfigParams() with ConfigBeanFactory.create() automatic binding.
Each config.conf domain now maps to a typed Java bean class. Delete ConfigKey.java
(~100 string constants) — config.conf becomes the sole source of truth for key names.

Why

Adding a new config parameter previously required editing 3 files (config.conf,
ConfigKey.java, Args.java). The manual parsing was error-prone, hard to review,
and duplicated every key name as a Java string constant. With bean binding, adding
a parameter only requires adding a field to the bean class and a line in reference.conf.

Introducing reference.conf

Previously, default values were scattered across three places: bean field initializers,
ternary expressions in Args.java, and comments in ConfigKey.java. Defaults could
be inconsistent, and there was no single place to see all config parameters and their
default values at a glance.

reference.conf is the official recommended practice of the Typesafe Config library
(see official docs):
libraries and applications declare all config parameters and their defaults in
src/main/resources/reference.conf, and users only need to override the values they
want to change — Typesafe Config merges them automatically.
Akka, Play Framework, and Spark all follow this convention.

Benefits:

• Single source of defaults: all default values centralized in one file, eliminating
  scatter and inconsistency
• Self-documenting: opening reference.conf shows the complete list of config
  parameters, defaults, and comments — no need to dig through Java code
• ConfigBeanFactory safety net: bean binding requires every field to have a
  corresponding config key. reference.conf ensures binding never fails due to
  missing keys, even when users don't configure a value
• Community convention: projects using Typesafe Config naturally expect
  reference.conf to exist, lowering the learning curve for new contributors

Changes

Commit 1: Core refactor — ConfigBeanFactory binding

• Create typed bean classes for all config domains: VmConfig, BlockConfig,
  CommitteeConfig, MetricsConfig, NodeConfig, EventConfig, StorageConfig,
  GenesisConfig, RateLimiterConfig, MiscConfig, LocalWitnessConfig
• Simplify Args.applyConfigParams() from ~850 lines to ~50 lines of domain binding calls
• Delete ConfigKey.java (~100 string constants)
• Migrate Storage.java static getters to read from StorageConfig bean
• Add unit tests for all config bean classes

Commit 2: reference.conf as single source of defaults

• Add common/src/main/resources/reference.conf with defaults for all config domains
• Move scattered default values from bean field initializers into reference.conf
• Expose config beans as static singletons (NodeConfig.getInstance() etc.)
• Auto-bind discovery, PBFT, and list fields in NodeConfig

Commit 3: Storage cleanup

• Move default/defaultM/defaultL LevelDB option reading from Storage into
  StorageConfig, so Storage no longer touches Config directly
• Add DbOptionOverride with nullable boxed types for partial override semantics
• Fix cacheSize type from int to long to match LevelDB Options API

Scope

• Only changes the reading side: how config.conf values are parsed into Java objects
• Does NOT move fields out of CommonParameter (future Phase 2)
• Does NOT change config.conf format or CLI parameter handling
• Does NOT modify the 847 CommonParameter.getInstance() call sites

Future Plan

This refactor is Phase 1 — it only replaces the config reading layer. After bean
binding, values are still copied one by one to CommonParameter's flat fields, because
847 call sites across the codebase depend on CommonParameter.getInstance().

The goal of Phase 2 is to remove the CommonParameter intermediary:

1. Untangle CLI parameter interference: some config parameters are currently written
   by both config.conf and CLI arguments (JCommander), converging on CommonParameter.
   The CLI override logic needs to be unified into Typesafe Config's override mechanism
   (ConfigFactory.systemProperties() or custom overrides), eliminating CLI's direct
   write dependency on CommonParameter
2. Gradually migrate call sites: replace 847 CommonParameter.getInstance().getXxx()
   calls with direct domain bean access — NodeConfig.getInstance().getXxx(),
   VmConfig.getInstance().getXxx(), etc.
3. Delete CommonParameter: once all call sites are migrated and CLI arguments no
   longer write directly, remove CommonParameter and the bridge-copy code in Args

End state: config.conf → reference.conf fallback → ConfigBeanFactory → domain
bean singletons. Fully type-safe, no intermediary layer, no string constants.

Test

• All existing tests pass (framework full test suite)
• Added 12 new test classes covering all config bean domains
• Checkstyle passes

[317787106]

DynamicArgs: NodeConfig constructed twice per reload — wrong abstraction boundary

private void updateActiveNodes(Config config) {
    NodeConfig nodeConfig = NodeConfig.fromConfig(config);  // parse #1
    ...
}

private void updateTrustNodes(Config config) {
    NodeConfig nodeConfig = NodeConfig.fromConfig(config);  // parse #2 — same Config object
    ...
}

Beyond the redundant ConfigBeanFactory reflection cost, there is a deeper engineering issue:

Broken single source of truth. Both methods are steps of one atomic reload, yet they produce two separate NodeConfig instances from the same input. The code gives no guarantee they reflect the same state — any future side effect or environment-sensitive substitution in fromConfig() could cause them to silently diverge. The root cause: Config is the wrong parameter type here. Both methods''' actual contract is NodeConfig.

[vividctrlalt]

Re: DynamicArgs NodeConfig parsed twice — Fixed. NodeConfig is now parsed once in reload() and passed to both methods.

[317787106]

Unused files these related to configuration common/src/main/java/org/tron/core/config/README.md and common/src/main/java/org/tron/core/config/args/Overlay.java can be deleted.

[317787106]

Including getDbEngineFromConfig, a total of 12 methods are no longer used and can be removed.

[vividctrlalt]

Re: Storage 12 unused methods — Fixed. All removed, confirmed zero callers.

[317787106]

Suggest to replace class-level @Getter/@Setter with per-field annotations in CommitteeConfig.

The previous approach used class-level annotations but silently suppressed them on allowPBFT and pBFTExpireNum via AccessLevel.NONE. This is misleading — a reader assumes all fields are covered until they scan the entire file to find the exceptions. The code has poor readability.

Per-field annotations make each field's contract explicit. The two non-standard fields carry no Lombok annotations at all, so their manual accessors immediately signal that special handling is required.

There is no need to modify the configuration parameter name solely due to case differences.

Lombok best practice: use class-level annotations only when the policy is uniform across all fields; switch to per-field when exceptions exist.

Suggest to optimize like this concisely:

//no Getter Setter
@Slf4j
public class CommitteeConfig {

  @Getter @Setter private long allowCreationOfContracts = 0;
  ...
  @Getter @Setter private long changedDelegation = 0;

  // These two fields which violates JavaBean naming convention are excluded from auto-binding and
  // handled manually in fromConfig().
  @Getter private long allowPBFT = 0;
  @Getter private long pBFTExpireNum = 20;

  @Getter @Setter private long allowTvmFreeze = 0;
   ...
  @Getter @Setter private long dynamicEnergyMaxFactor = 0;

  // proposalExpireTime is NOT a committee field — it's in block.* and handled by BlockConfig

  // Defaults come from reference.conf (loaded globally via Configuration.java)

  private static final String PBFT_EXPIRE_NUM_KEY = "pBFTExpireNum";
  private static final String ALLOW_PBFT_KEY = "allowPBFT";

  public static CommitteeConfig fromConfig(Config config) {
    Config section = config.getConfig("committee");

    CommitteeConfig cc = ConfigBeanFactory.create(section, CommitteeConfig.class);
    // Ensure the manually-named fields get the right values from the original keys
    cc.allowPBFT = section.hasPath(ALLOW_PBFT_KEY) ? section.getLong(ALLOW_PBFT_KEY) : 0;
    cc.pBFTExpireNum = section.hasPath(PBFT_EXPIRE_NUM_KEY)
        ? section.getLong(PBFT_EXPIRE_NUM_KEY) : 20;

    cc.postProcess();
    return cc;
  }

  private void postProcess() {
  ...

[vividctrlalt]

Re: CommitteeConfig per-field @Getter/https://github.com/Setter — Good suggestion for readability. However, the root cause is that allowPBFT and pBFTExpireNum violate JavaBean naming convention (consecutive uppercase "PBFT"). Switching annotation style is a cosmetic fix.

Since PBFT is not yet active, I'd suggest addressing this in a future version by renaming the config keys to standard camelCase (allowPbft, pbftExpireNum). That eliminates the need for manual binding entirely, and the annotation style issue goes away with it. The same approach should apply to other beans (NodeConfig, StorageConfig, EventConfig) that have similar AccessLevel.NONE workarounds.

[317787106]

It should throws TronError instead of IllegalArgumentException here, as the IllegalArgumentException here appears to be an oversight and may be silently swallowed by an upstream catch block, causing the node to start with an invalid committee configuration instead of exiting immediately.

[vividctrlalt]

Agree that TronError is the better choice semantically. However, this PR is a pure refactor — the original code in Args.java used IllegalArgumentException here, so I kept it unchanged to avoid behavioral changes.

Also, in the current call stack (FullNode.main → Args.setParam → applyConfigParams → CommitteeConfig.fromConfig → postProcess), there is no catch(Exception) that would swallow it. The IllegalArgumentException propagates to the UncaughtExceptionHandler and exits the node correctly.

That said, unifying all config validation errors to TronError would be a good follow-up.

[317787106]

It missing default value is "metrics" here, it should be same as that in code,

[vividctrlalt]

Fixed. Changed to database = "metrics" to match MetricsConfig.InfluxDbConfig default.

[vividctrlalt]

Good catch. Both files are indeed unused:

• Overlay.java — created in 2018, never assigned in Args.java. CommonParameter.overlay is always null. However, deleting it requires also removing the field from CommonParameter, OverlayTest.java, and the assertNull in ParameterTest.java.
• README.md — an outdated (2018) config doc placed inside the Java source tree. Content is stale and the location is wrong (gets packaged into the jar).

Both are out of scope for this PR (config binding refactor). I'd suggest a separate cleanup PR to remove them along with any other dead config artifacts.

[warku123]

I noticed the test configuration files changed external.ip from null to "":

-  external.ip = null
+  external.ip = ""

Could this affect the external IP detection logic? In the original code, null might trigger auto-detection fallback, while empty string "" could be treated as a valid (but empty) value. Want to confirm this is intentional and won't break IP auto-discovery in tests.

[vividctrlalt]

Good question, but verified safe. Both values trigger the same fallback path:

// Args.java:1157
private static void externalIp(NodeConfig nodeConfig) {
    String externalIp = nodeConfig.getDiscoveryExternalIp();
    if (StringUtils.isEmpty(externalIp)) {   // null AND "" are both empty
      if (PARAMETER.nodeExternalIp == null) {
        PARAMETER.nodeExternalIp = PARAMETER.p2pConfig.getIp();  // auto-detect
        ...

StringUtils.isEmpty() treats both null and "" as empty, so the auto-detection fallback triggers in both cases. No behavioral difference.

The reason for changing null to "" is that ConfigBeanFactory cannot bind a null value to a String field — it throws ConfigException$Null. Empty string is the only way to express "unset" for a String field under bean binding.

[kuny0707]

[Behavioral regression] memoFee clamping is lost.

The original Args.java clamped memoFee to [0, 1_000_000_000]:

// Args.java (develop branch)
if (config.hasPath(ConfigKey.MEMO_FEE)) {
  PARAMETER.memoFee = config.getLong(ConfigKey.MEMO_FEE);
  if (PARAMETER.memoFee > 1_000_000_000) {
    PARAMETER.memoFee = 1_000_000_000;
  }
  if (PARAMETER.memoFee < 0) {
    PARAMETER.memoFee = 0;
  }
}

After this PR, postProcess() does not clamp memoFee, so out-of-range values are passed through unchanged. For example:

• committee.memoFee = 5000000000 → develop clamps to 1_000_000_000, this PR keeps 5_000_000_000
• committee.memoFee = -100 → develop clamps to 0, this PR keeps -100

Suggested fix in postProcess():

if (memoFee < 0) { memoFee = 0; }
if (memoFee > 1_000_000_000L) { memoFee = 1_000_000_000L; }

Verified by a regression test against this PR branch — 2 of 5 memoFee boundary tests fail without this clamp.

[vividctrlalt]

Great catch — fixed in c38f388. Both clamps restored in CommitteeConfig.postProcess():

if (allowNewReward < 0) { allowNewReward = 0; }
if (allowNewReward > 1) { allowNewReward = 1; }

if (memoFee < 0) { memoFee = 0; }
if (memoFee > 1_000_000_000L) { memoFee = 1_000_000_000L; }

Also added 31 boundary test cases across CommitteeConfigTest, NodeConfigTest, VmConfigTest, and ArgsTest to pin every clamp (below/above/in-range) so this kind of regression cannot happen silently again. Specifically pinned the allowNewReward = 2 + allowOldRewardOpt = 1 case you flagged — verifies the clamp runs before the cross-field check.

Thanks for the rigorous review — the original Args.java had no test coverage for these clamps, so writing the regression test was the only way this could have been caught.

[kuny0707]

[Behavioral regression] allowNewReward clamping is lost — and this changes the cross-field check semantics below.

The original Args.java clamped allowNewReward to [0, 1]:

// Args.java (develop branch)
if (config.hasPath(ConfigKey.ALLOW_NEW_REWARD)) {
  PARAMETER.allowNewReward = config.getLong(ConfigKey.ALLOW_NEW_REWARD);
  if (PARAMETER.allowNewReward > 1) { PARAMETER.allowNewReward = 1; }
  if (PARAMETER.allowNewReward < 0) { PARAMETER.allowNewReward = 0; }
}

This is especially critical because the cross-field validation in postProcess() (line 152) uses allowNewReward != 1:

if (allowOldRewardOpt == 1 && allowNewRewardAlgorithm != 1
    && allowNewReward != 1 && allowTvmVote != 1) {
  throw new IllegalArgumentException(...);
}

Without clamping, the semantics diverge from develop:

Config	develop behavior	this PR behavior
allowNewReward = 2 + allowOldRewardOpt = 1	Clamped to 1, check passes	Stays 2, check fails, throws exception
allowNewReward = 99	Clamped to 1	Stays 99
allowNewReward = -5	Clamped to 0	Stays -5

A user who configured allowNewReward = 2 (intending to enable it) would have their node start fine on develop, but fail to start after this PR.

Suggested fix in postProcess(), before the cross-field check at line 152:

if (allowNewReward < 0) { allowNewReward = 0; }
if (allowNewReward > 1) { allowNewReward = 1; }

Verified by a regression test against this PR branch — the cross-field semantic test fails without this clamp.

[vividctrlalt]

Same fix in c38f388 — see the reply on the allowNewReward thread above. Thanks again.

[halibobo1205]

Suggestion: Replace ConfigBeanFactory with custom annotations to eliminate manual fallback code

The current approach uses ConfigBeanFactory.create() for auto-binding, but due to JavaBean naming convention limitations, many fields require manual fallback (@Getter(AccessLevel.NONE) + hand-written getters/setters + per-field assignment in fromConfig()). This affects at least:

• CommitteeConfig: allowPBFT, pBFTExpireNum (consecutive uppercase "PBFT")
• NodeConfig: isOpenFullTcpDisconnect (boolean "is" prefix), shutdown.BlockTime/BlockHeight/BlockCount (PascalCase keys)
• NodeConfig.HttpConfig/RpcConfig/JsonRpcConfig: all PBFT-related fields (6 total)
• EventConfig: nativeQueue (Java reserved word "native"), topics (optional fields per item)
• StorageConfig: merkleRoot, default/defaultM/defaultL (partial overrides)

These scattered manual fallbacks are easy to miss during maintenance — the memoFee/allowNewReward clamp regression is a case in point.

Approach: Custom @ConfigValue annotation + unified binder

Core idea: annotate fields with the explicit config key, so binding no longer depends on setter name inference.

1) Define annotations

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.FIELD)
public @interface ConfigValue {
    String value();                       // config key path (relative to section)
    boolean required() default false;
}

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.FIELD)
public @interface Clamp {
    long min() default Long.MIN_VALUE;
    long max() default Long.MAX_VALUE;
}

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface ConfigSection {
    String value();                       // section path prefix
}

2) Bean class comparison

Current CommitteeConfig (ConfigBeanFactory + manual fallback):

@Getter @Setter
public class CommitteeConfig {
    private long allowCreationOfContracts = 0;

    // PBFT naming breaks JavaBean convention → ConfigBeanFactory can't bind → manual handling
    @Getter(AccessLevel.NONE) @Setter(AccessLevel.NONE)
    private long allowPBFT = 0;
    @Getter(AccessLevel.NONE) @Setter(AccessLevel.NONE)
    private long pBFTExpireNum = 20;

    public long getAllowPBFT() { return allowPBFT; }
    // ... hand-written getters/setters ...

    public static CommitteeConfig fromConfig(Config config) {
        Config section = config.getConfig("committee");
        Config aliased = section;
        if (section.hasPath("pBFTExpireNum") && !section.hasPath("PBFTExpireNum")) {
            aliased = aliased.withValue("PBFTExpireNum", section.getValue("pBFTExpireNum"));
        }
        CommitteeConfig cc = ConfigBeanFactory.create(aliased, CommitteeConfig.class);
        cc.allowPBFT = section.hasPath("allowPBFT") ? section.getLong("allowPBFT") : 0;
        cc.pBFTExpireNum = section.hasPath("pBFTExpireNum") ? section.getLong("pBFTExpireNum") : 20;
        cc.postProcess(); // lots of clamp code
        return cc;
    }
}

With annotations:

@Getter
@ConfigSection("committee")
public class CommitteeConfig {
    @ConfigValue("allowCreationOfContracts")
    private long allowCreationOfContracts = 0;

    @ConfigValue("allowPBFT")        // explicit key — no setter inference needed
    private long allowPBFT = 0;

    @ConfigValue("pBFTExpireNum")
    private long pBFTExpireNum = 20;

    @ConfigValue("memoFee")
    @Clamp(min = 0, max = 1_000_000_000)  // declarative clamp replaces postProcess()
    private long memoFee = 0;

    @ConfigValue("allowNewReward")
    @Clamp(min = 0, max = 1)
    private long allowNewReward = 0;

    @ConfigValue("unfreezeDelayDays")
    @Clamp(min = 0, max = 365)
    private long unfreezeDelayDays = 0;
}

Same for NodeConfig — all manual fallbacks eliminated:

@ConfigSection("node")
public class NodeConfig {
    @ConfigValue("isOpenFullTcpDisconnect")   // "is" prefix no longer a problem
    private boolean isOpenFullTcpDisconnect = false;

    @ConfigValue("shutdown.BlockTime")        // PascalCase key no longer a problem
    private String shutdownBlockTime = "";

    @ConfigValue("shutdown.BlockHeight")
    private long shutdownBlockHeight = -1;
}

3) Unified binder (~100-150 lines)

public class ConfigBinder {
    public static <T> T bind(Config rootConfig, Class<T> beanClass) {
        ConfigSection section = beanClass.getAnnotation(ConfigSection.class);
        Config cfg = (section != null) ? rootConfig.getConfig(section.value()) : rootConfig;
        T bean = newInstance(beanClass);
        for (Field field : getAllFields(beanClass)) {
            ConfigValue cv = field.getAnnotation(ConfigValue.class);
            if (cv == null) continue;
            String key = cv.value();
            if (!cfg.hasPath(key)) {
                if (cv.required()) throw new TronError("Missing: " + key, PARAMETER_INIT);
                continue; // use field initializer default
            }
            field.setAccessible(true);
            Object value = resolveValue(cfg, key, field);
            Clamp clamp = field.getAnnotation(Clamp.class);
            if (clamp != null && value instanceof Number) {
                long v = ((Number) value).longValue();
                v = Math.max(v, clamp.min());
                v = Math.min(v, clamp.max());
                value = v;
            }
            field.set(bean, value);
        }
        return bean;
    }
}

4) Usage

// Before: each bean needs its own fromConfig() factory method
CommitteeConfig cc = CommitteeConfig.fromConfig(config);
NodeConfig nc = NodeConfig.fromConfig(config);

// After: unified call
CommitteeConfig cc = ConfigBinder.bind(config, CommitteeConfig.class);
NodeConfig nc = ConfigBinder.bind(config, NodeConfig.class);

Summary

Dimension	ConfigBeanFactory (current)	Annotation approach
PBFT naming issues	Manual aliasing + manual assignment	@ConfigValue("pBFTExpireNum") — solved
"is" prefix / PascalCase	Manual reads	Annotation specifies key directly
Java reserved words (native)	withoutPath + manual binding	Annotation specifies key directly
fromConfig() boilerplate	One factory method per bean	Unified ConfigBinder.bind()
Clamp validation	postProcess() per bean	Declarative @Clamp
Cost of adding new config	Add field + possibly modify fromConfig	Add one annotated field
Implementation cost	Zero (built-in Typesafe Config API)	~100-150 lines of binder code
External dependencies	None	None

The key advantage is centralizing key-to-field mappings from scattered fromConfig() code into field-level declarations. Adding a new config parameter becomes a single annotated field — no parsing logic, no risk of missing a clamp. Worth considering as a follow-up iteration.

[bladehan1]

Review: default value behavioral changes in reference.conf

[bladehan1]

Default value behavioral change — blockProducedTimeOut: 50 → 75

The old Args.java hardcoded fallback was BLOCK_PRODUCE_TIMEOUT_PERCENT = 50 (used when the key was absent from config). In the old shipped config.conf this key was commented out, so the effective default for all nodes not explicitly setting it was 50.

reference.conf now sets it to 75, which changes block production timing for any node that omits this key in their config.conf.

Suggest changing to 50 to preserve behavioral equivalence, or documenting this as an intentional change in the PR description.

[vividctrlalt]

Thanks — fixing both in the same commit. The reference.conf should reflect the code-level default (the value the runtime uses when the key is absent), not whatever was in the shipped sample config.conf. I'll change blockProducedTimeOut back to 50 and txCache.initOptimization back to false.

[bladehan1]

Default value behavioral change — txCache.initOptimization: false → true

The old Storage.java field defaulted to false, and the hasPath && getBoolean pattern also yielded false when the key was absent. The old shipped config.conf had this set to true, so mainnet nodes are unaffected.

However, custom/test configurations that omit this key will now get true instead of false, changing tx cache initialization behavior.

Suggest either:

• Changing to false to match the old code-level default, or
• Documenting this as an intentional improvement (since true is the recommended production setting)

[vividctrlalt]

See reply on the blockProducedTimeOut thread — fixing both in the same commit. Thanks for catching these.

[vividctrlalt]

Two thoughts:

1. The annotation approach is the right direction and worth doing as a follow-up. This PR is already large (27 commits, ~3000 lines), and the main goal — making reference.conf the single source of defaults — is in place. Layering another refactor on top would make review harder. Better to ship this and tackle the binder rewrite as a focused next-version PR.

2. One concern about the trade-off, though: the current ConfigBeanFactory approach has a useful side effect — it implicitly enforces JavaBean-compliant naming for new config keys. If a contributor adds a key like myNewPBFTField, binding fails and they're forced to either rename it to camelCase or write manual fallback (which raises the cost of bad naming). This natural friction has been a soft guardrail.

   The annotation approach is more flexible (accepts any key name via @ConfigValue("...")), which is great for supporting legacy keys, but it removes that guardrail. New contributors could introduce inconsistent naming without anything pushing back. Worth thinking about whether to add a separate convention check (e.g. a checkstyle rule or test) to keep new keys standardized when migrating to the annotation model.