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.

[bladehan1]

[P1] readDbOption() lost friendly error handling from old code

The old Storage.setIfNeeded() wrapped every Integer.parseInt() / Long.parseLong() in try-catch and called throwIllegalArgumentException() with field name, expected type, and actual value:

// Old code (develop branch)
try {
    dbOptions.compressionType(
        CompressionType.getCompressionTypeByPersistentId(Integer.parseInt(param)));
} catch (NumberFormatException e) {
    throwIllegalArgumentException(COMPRESSION_TYPE_CONFIG_KEY, Integer.class, param);
}

New code uses bare Integer.parseInt(conf.get(...).unwrapped().toString()) (lines 232-250), losing all error context. A user writing compressionType = 1.5 or blockSize = "abc" in storage.default will see a raw NumberFormatException with no indication of which config key caused it.

Suggested fix: replace with conf.toConfig().getInt(key) / conf.toConfig().getLong(key) — Typesafe Config's typed accessors handle float truncation and provide path-aware error messages. Or restore try-catch with a descriptive message.

[vividctrlalt]

Fixed in 1292373. Restored try-catch around all 5 parseInt/parseLong calls with a throwIllegalArgumentException() helper method that produces the same error message format as the old Storage.setIfNeeded() ([storage.properties] <field> must be <type> type, actual: <value>). Thanks for catching this regression.

[bladehan1]

[P2] Bean field initializers violate the maintenance rule declared in reference.conf

reference.conf line 20 states: "Values must match the bean field initializers in the corresponding XxxConfig.java classes". Three fields break this contract:

Bean field	Java default	reference.conf value
JsonRpcConfig.maxBlockFilterNum (line 303)	0	50000 (ref line 400)
DnsConfig.maxMergeSize (line 331)	0	5 (ref line 372)
DnsConfig.changeThreshold (line 332)	0.0	0.1 (ref line 373)

At runtime this is harmless — reference.conf always overrides bean initializers via ConfigBeanFactory. But:

• If code ever constructs a bean directly (e.g. in tests), the wrong default leaks in.
• Violating the declared rule erodes trust in the maintenance contract for future contributors.

Suggested fix: Align maxBlockFilterNum = 50000. For maxMergeSize/changeThreshold, if 0 is an intentional sentinel for "not configured" (used by DNS publish validation at Args.java:1083-1087), add a comment explaining why the bean default intentionally differs from reference.conf.

[vividctrlalt]

Fixed in 1292373. Your analysis led to a deeper investigation that showed the three fields needed different treatment:

• maxBlockFilterNum: aligned Java default to 50000 to match reference.conf and develop's CommonParameter.jsonRpcMaxBlockFilterNum = 50000.
• maxMergeSize / changeThreshold: instead of changing the bean defaults, I changed reference.conf to 0 / 0.0. In develop, these keys use a hasPath guard in Args.java, and when absent the underlying PublishConfig fields stay at their JVM defaults (0). The previous reference.conf values of 5 / 0.1 would have silently activated DNS publish for users who never configured it — a behavioral change vs. develop. Now the bean default (0) and reference.conf (0) both match, and the effective runtime behavior matches develop.

Thanks for flagging this — the maintenance contract in reference.conf is now actually enforceable.

[bladehan1]

[P2] Redundant alias + manual override for PBFT fields — keep one pattern

The binding uses two mechanisms for the same fields:

1. Alias (lines 106-108): adds "PBFTExpireNum" key so ConfigBeanFactory can match setPBFTExpireNum()
2. Manual override (lines 113-114): reads from the original "pBFTExpireNum" key and directly assigns to cc.pBFTExpireNum

The manual override always runs, making the alias dead code. If a future contributor removes the alias (thinking it's unused), nothing breaks. But if they remove the manual override (thinking the alias handles it), the value is bound by ConfigBeanFactory to the aliased key — which works today but couples to ConfigBeanFactory's internal property-name derivation.

Suggested fix: pick one pattern and remove the other. The manual override (lines 111-114) is simpler and more explicit — suggest keeping it and dropping the alias block (lines 106-108).

[vividctrlalt]

Fixed in 1292373, but it took more than just dropping the alias — your suggestion uncovered an additional layer worth noting:

Simply deleting the alias caused ConfigBeanFactory.create() to throw (No setting at 'PBFTExpireNum'). The reason: the class still exposed public setAllowPBFT / setPBFTExpireNum methods (hand-written after @Setter(AccessLevel.NONE) suppressed Lombok). ConfigBeanFactory scans public setters via reflection and derives AllowPBFT / PBFTExpireNum (JavaBean uppercase rule), which don't match the lowercase config keys.

So to truly keep only the manual override pattern as you suggested, I also removed the public setters — only getters are exposed now. Fields are assigned directly in fromConfig(). This means the alias is no longer needed and ConfigBeanFactory no longer touches these fields at all. Thanks for pushing on this — the original dual-mechanism code was genuinely confusing.

[bladehan1]

[Risk] Bridge layer (apply*Config methods) lacks integration test coverage

The PR adds 11 Bean-level unit test classes that verify ConfigBeanFactory binding and postProcess() clamps — good coverage at the bean layer. However, the bridge methods in Args.java (applyNodeConfig, applyCommitteeConfig, applyStorageConfig, etc.) that copy bean values into CommonParameter have no dedicated test coverage.

A missed or mis-typed field assignment in the bridge layer (e.g. wrong sign, forgotten field, narrowing cast) would not be caught by the existing bean tests. The current ArgsTest changes are minimal and do not systematically verify that every bean field reaches its CommonParameter counterpart.

Suggested improvement: Add a small integration test per apply*Config method that:

1. Constructs a Config with non-default values for a representative set of fields
2. Calls Args.setParam() (or the bridge method directly)
3. Asserts the corresponding CommonParameter fields match

This does not need to cover every field — even a sample of 5-10 fields per domain would catch most bridge regressions and would have caught the memoFee/allowNewReward clamp loss earlier.

[vividctrlalt]

Thanks for flagging the bridge layer coverage gap. I actually prototyped 7 bridge integration tests (one per apply*Config method with 5-10 representative fields) locally and they worked, but I'm deferring them to a follow-up PR for two reasons:

1. Testing revealed a pre-existing flakiness amplification risk. Each bridge test calls Args.applyConfigParams() + Args.clearParam(), which churns the PARAMETER singleton. Several network/consensus tests (PeerConnection.relayNodes static init, ShieldedReceiveTest, AdvServiceTest, etc.) depend on PARAMETER state at class-load time and are already flaky on develop (hence the existing test-retry plugin). Adding 7 more apply/clear cycles made the flaky tests trip more frequently in my runs. The right fix is test isolation improvements, which is out of scope here.

2. The bridge layer is scheduled for removal. The next major iteration of this refactor is to eliminate CommonParameter as a global singleton and have consumers read XxxConfig beans directly — which deletes the entire apply*Config bridge layer. Writing dedicated tests for code we're about to delete seemed wasteful. I'd rather invest that test-writing effort in the post-singleton world.

That said, your point stands — the memoFee/allowNewReward clamp regressions would have been caught earlier with this coverage, so the risk is real today. Happy to add the tests in this PR if you'd prefer; let me know.

[317787106]

Suggest to optimize properties like this, otherwise it is hard to understand the format:

# Example per-database overrides:  
properties = [
  #   {
  #     name = "account",
  #     path = "storage_directory_test",
  #     createIfMissing = true,
  #     paranoidChecks = true,
  #     verifyChecksums = true,
  #     compressionType = 1,        // compressed with snappy
  #     blockSize = 4096,           // 4  KB =         4 * 1024 B
  #     writeBufferSize = 10485760, // 10 MB = 10 * 1024 * 1024 B
  #     cacheSize = 10485760,       // 10 MB = 10 * 1024 * 1024 B
  #     maxOpenFiles = 100
  #   }
]

[vividctrlalt]

Good suggestion — the inline field names with unit annotations read much better than the current form.

I'd like to scope this out of #6615 to keep this PR focused on the bean-binding refactor. Plan is to open a dedicated follow-up PR on reference.conf that addresses:

• The properties array example you outlined above (all fields inline, with unit comments)
• Documentation comments for every key in reference.conf — parameter meaning, accepted value range, units, and the effect on node behavior
• Consistent comment style across sections (storage, node, vm, event.subscribe, etc.)

The goal is that anyone reading reference.conf standalone can understand what each key does without cross-referencing Args.java or other source. Will link the new PR here once it's opened.

[317787106]

maxActiveNodesWithSameIp has been replaced by maxConnectionsWithSameIp, should be deleted.

[vividctrlalt]

Thanks — fixed in 8a76db8. One nuance I want to flag because it affects back-compat:

The legacy key maxActiveNodesWithSameIp is still read on develop (Args.java:392-399) and takes priority over maxConnectionsWithSameIp when both are set. Users who haven't migrated their config.conf would break if we dropped this path entirely. So the fix is targeted:

• reference.conf — removed the maxActiveNodesWithSameIp = 2 line. Shipping it meant HOCON merges always made section.hasPath(...) true, causing the alias-fallback branch to silently clobber any user-supplied maxConnectionsWithSameIp.
• NodeConfig.java — removed the unused bean field maxActiveNodesWithSameIp (it was only declared to satisfy ConfigBeanFactory, never actually read), but kept the alias read at fromConfig() that peeks at section.hasPath("maxActiveNodesWithSameIp") directly. Users who still set the legacy key in their config.conf get it routed to maxConnectionsWithSameIp, matching develop.

Added regression tests in NodeConfigTest:

• testMaxConnectionsWithSameIpNotOverriddenByReferenceConfAlias — verifies the fix
• testMaxActiveNodesWithSameIpLegacyAliasStillWorks — verifies back-compat
• testLegacyAliasTakesPriorityOverModernKey — verifies develop parity

[317787106]

In version 4.8.1, we init compactThreads like this:

    int compactThreads = config.hasPath(prefix + "compactThreads")
        ? config.getInt(prefix + "compactThreads")
        : max(Runtime.getRuntime().availableProcessors(), 1, true);

But it is hardcoded as 32 here. Suggest use 0.

[vividctrlalt]

Thanks — adopted the 0 = auto convention in 8a76db8.

Changes:

• StorageConfig.DbSettingsConfig.compactThreads default: 32 → 0
• reference.conf: compactThreads = 0
• Added DbSettingsConfig.postProcess() that expands 0 → Math.max(Runtime.getRuntime().availableProcessors(), 1), matching develop Args.java:1609-1611

Regression tests in StorageConfigTest:

• testCompactThreadsAutoExpand — asserts 0 expands to availableProcessors
• testCompactThreadsExplicitPreserved — asserts non-zero values pass through

Also took this as a cue to sweep the rest of dbSettings against develop's fallbacks — blocksize, level0FileNumCompactionTrigger, and targetFileSizeBase had drifted too; all are now aligned in the same commit. testDbSettingsDefaults was updated to assert the new aligned values.

[317787106]

validContractProto.threads is hardcoded as 2, but in 4.8.1, we use Runtime.getRuntime().availableProcessors(). Suggest use 0 instead.

[vividctrlalt]

Thanks — adopted in 8a76db8.

Changes:

• reference.conf: validContractProto.threads = 0
• NodeConfig.ValidContractProtoConfig.threads default: 2 → 0
• NodeConfig.postProcess() expands 0 → Runtime.getRuntime().availableProcessors(), matching develop Args.java:743-746 (also keeps this symmetric with the existing solidity.threads and validateSignThreadNum handling)

Regression tests in NodeConfigTest:

• testValidContractProtoThreadsDefaultAutoExpands — asserts the default expands to availableProcessors
• testValidContractProtoThreadsExplicitPreserved — asserts explicit non-zero values pass through

[317787106]

It’s a bit hard to follow these "||" conditions. Maybe consider splitting them?
Also, if topics is not empty—even when all enable = false—an empty (but non-null) EventPluginConfig will still be created, which triggers the event plugin. In this case, triggering the plugin is actually the unexpected behavior.

[vividctrlalt]

Thanks for the review.

1. On readability: agreed, the || chain is hard to follow. Will refactor (extract a helper + simplify the condition) in a follow-up PR.

2. On "triggers the event plugin": traced the call chain — the plugin is not triggered in this case. EventPluginLoader.start() is only invoked from Manager.startEventSubscribing(), and that call is guarded on isEventSubscribe() (= ec.isEnable()):

// Manager.java:564
if (Args.getInstance().isEventSubscribe()) {
  startEventSubscribing();
}

So when enable = false, the plugin never starts — regardless of whether eventPluginConfig is null or populated. The only consumer of eventPluginConfig (besides Args) is Manager.java:2151, which is behind this guard.

This also matches develop's behavior: template-based deployments on develop already populate eventPluginConfig with enable=false (since config.conf ships the section), and the plugin still doesn't start.

Planned follow-up (separate PR): tighten applyEventConfig to only build eventPluginConfig when ec.isEnable() is true — makes the "enable=false ⇒ null" semantic strict and cleans up the condition at the same time.

[lxcmyf]

Several fallback values here no longer match 's runtime defaults when the keys are omitted. In particular, , , , and used different fallback values before, so this becomes a silent RocksDB tuning change for minimal configs.

[vividctrlalt]

Replying on the corrected thread — looks like this one's template placeholders didn't resolve. TL;DR: all four drifted dbSettings keys are aligned with develop in 8a76db8.

[lxcmyf]

This changes the runtime fallback for node.trustNode. On develop, trustNodeAddr stayed null unless the key was explicitly present. With this default in reference.conf, any trimmed external config that omits node.trustNode will now behave as if a localhost trust node was configured.

[vividctrlalt]

Fixed in 8a76db8. reference.conf no longer ships a literal trust-node address; trustNode = "" so the existing Args bridge logic (Args.java:645-648) converts the empty string to null, preserving develop's behavior:

if (StringUtils.isEmpty(PARAMETER.trustNodeAddr)) {
  String trustNode = nc.getTrustNode();
  PARAMETER.trustNodeAddr = StringUtils.isEmpty(trustNode) ? null : trustNode;
}

(Kept an empty-string default rather than removing the key entirely because NodeConfig.trustNode is a declared bean field — ConfigBeanFactory requires the key to be bindable. Empty-string → null happens at the bridge layer.)

Regression test: NodeConfigTest.testTrustNodeNotDefaultedByReferenceConf.

[lxcmyf]

This changes the fallback for node.validContractProto.threads. On develop, an absent key meant availableProcessors(), but this now defaults to 2, which can silently shrink the validation pool on multi-core nodes.

[vividctrlalt]

Fixed in 8a76db8 — same fix @317787106 flagged on this line. reference.conf now uses validContractProto.threads = 0; NodeConfig.postProcess() expands 0 → Runtime.getRuntime().availableProcessors(), matching develop Args.java:743-746. Symmetric with the existing solidity.threads and validateSignThreadNum handling in the same postProcess().

Regression tests:

• testValidContractProtoThreadsDefaultAutoExpands — default expands to availableProcessors()
• testValidContractProtoThreadsExplicitPreserved — explicit non-zero passes through

[lxcmyf]

Several storage.dbSettings.* fallback values here no longer match develop's runtime defaults when the keys are omitted. In particular, compactThreads, blocksize, level0FileNumCompactionTrigger, and targetFileSizeBase used different fallback values before, so this becomes a silent RocksDB tuning change for minimal configs.

[vividctrlalt]

Thanks for catching this — fixed in 8a76db8. Verified each value against develop's Args.initRocksDbSettings (Args.java:1605-1626):

Key	develop fallback	PR (before)	PR (fixed)
compactThreads	max(availableProcessors, 1)	32	0 (auto-expanded in postProcess)
blocksize	16	64	16
level0FileNumCompactionTrigger	2	4	2
targetFileSizeBase	64	256	64

The other dbSettings keys (levelNumber, maxBytesForLevelBase, maxBytesForLevelMultiplier, targetFileSizeMultiplier, maxOpenFiles) already matched — no change there.

StorageConfigTest.testDbSettingsDefaults now asserts the aligned values, plus new cases testCompactThreadsAutoExpand / testCompactThreadsExplicitPreserved to catch future drift.

[lvs0075]

[SHOULD] node.fullNodeAllowShieldedTransaction legacy key silently ignored

reference.conf provides allowShieldedTransactionApi = true, so after withFallback, section.hasPath("allowShieldedTransactionApi") is always true — the legacy compatibility branch in
NodeConfig.fromConfig() is dead code. A user with only node.fullNodeAllowShieldedTransaction = false in their config.conf will silently get true.

Fix: remove allowShieldedTransactionApi from reference.conf and rely on the bean field initializer (= true) as the default.