refactor(config): merge config files with BeanDefaults fallback

[317787106]

What does this PR do?

Eliminates reference.conf and makes Java bean field initializers the single source of truth for configuration defaults.

Previously, configuration defaults were maintained in two places:

1. Java bean field initializers (e.g., private int maxConnections = 30)
2. common/src/main/resources/reference.conf (HOCON fallback loaded automatically by Typesafe Config)

Any new config field or default value change required keeping both in sync. reference.conf was invisible to most developers and frequently fell out of date.

Core change: Introduce BeanDefaults, a utility that serializes a bean instance's current field values into a Typesafe Config object. Each XxxConfig.fromConfig() now calls BeanDefaults.toConfig(new XxxConfig()) and uses it as a withFallback(), so ConfigBeanFactory always finds every key it needs — even when the user's config.conf only sets a subset of fields.

Specific changes:

• Add BeanDefaults.java: reflects public getter+setter pairs via BeanInfo, recurses into nested beans, serializes lists — mirrors exactly what ConfigBeanFactory binds
• Update all XxxConfig.fromConfig() methods (BlockConfig, CommitteeConfig, EventConfig, GenesisConfig, MetricsConfig, NodeConfig, RateLimiterConfig, StorageConfig, VmConfig): replace hard config.getConfig(section) calls with hasPath guard + withFallback(BeanDefaults.toConfig(...)); all classes now use an explicit userSection variable to hold the user's raw values before merging with defaults
• Remove reference.conf: no longer needed; BeanDefaults replaces its role
• Update Configuration.java: remove the withFallback(ConfigFactory.defaultReference()) call from the file-based load path
• Update config.conf: add previously undocumented parameters that were silently defaulted through reference.conf (e.g. storage.backup, storage.checkpoint, node.maxFastForwardNum, rate.limiter.p2p.*, etc.)
• Add BeanDefaultsTest.java: 16 tests covering default values, round-trip with ConfigBeanFactory, and user-value override for all major config beans

Why are these changes required?

1. Dual-source maintenance with no compile-time enforcement

The dual-source maintenance created recurring issues:

• Developers adding a new config field to a bean had to also add it to reference.conf, with no compile-time enforcement of that requirement
• reference.conf was not visible to users browsing source code or Java docs
• Divergence between Java defaults and reference.conf defaults was only caught at runtime
• Users reading reference.conf could not tell which keys were still active vs. superseded by bean logic

After this change, the only place a developer needs to touch to add a new config parameter is the Java bean class.

2. node.discovery.external.ip = null in reference.conf caused startup failure

If the existed config file contained:

node.discovery.external.ip = null

When Typesafe Config merged this with the user's config.conf, the null value propagated into the merged config. ConfigBeanFactory would then attempt to bind null to the String field, causing a startup crash even when the user had not set this key at all. This PR introduces BeanDefaults.stripNullLeaves(), which strips all HOCON null leaves from the user section before merging with bean defaults — so a user's explicit null entry never overwrites a valid Java default.

3. allowShieldedTransactionApi priority was silently broken

When both node.allowShieldedTransactionApi (modern) and node.fullNodeAllowShieldedTransaction (legacy) were present in config.conf, the modern key should take precedence. The original fallback logic checked section.hasPath("allowShieldedTransactionApi"), but section is the merged config (user values + BeanDefaults), which always contains allowShieldedTransactionApi from the defaults — making the condition always true and the entire legacy-key branch dead code. As a result, a user who set only fullNodeAllowShieldedTransaction would silently get the default value instead of their configured value.

Fixed by checking userSection.hasPath() (the raw user-supplied values only) instead of section.hasPath(). The correct semantics are now enforced and covered by tests:

• Only modern key set → modern key used
• Only legacy key set → legacy key used (with deprecation warning)
• Both keys set → modern key takes precedence
• Neither set → code default (true)

4. Legacy-key fallback pattern relied on an implicit invariant that was easy to break

Before this PR, six XxxConfig.fromConfig() methods used an inline one-step form:

Config section = config.hasPath("vm")
    ? BeanDefaults.stripNullLeaves(config.getConfig("vm")).withFallback(defaults)
    : defaults;

This form does not expose the raw user values as a named variable. Any developer adding a legacy-key fallback (like the allowShieldedTransactionApi case above) would be tempted to write section.hasPath() — which checks the merged config and silently makes the fallback dead code. All six classes now use the two-step form with an explicit userSection variable:

Config userSection = config.hasPath("vm")
    ? BeanDefaults.stripNullLeaves(config.getConfig("vm"))
    : ConfigFactory.empty();
Config section = userSection.withFallback(defaults);

The naming makes the invariant obvious: userSection is what the user wrote; section is the merged result. Future legacy-key checks must use userSection.hasPath().

This PR has been tested by:

• Unit Tests (BeanDefaultsTest, NodeConfigTest, BlockConfigTest, VmConfigTest, StorageConfigTest, CommitteeConfigTest, EventConfigTest, GenesisConfigTest, MetricsConfigTest, RateLimiterConfigTest — all pass)
• Manual Testing

[bladehan1]

[SHOULD] Silent swallow of getter exceptions hides root cause

The catch (Exception ignored) here drops any getter failure (typically InvocationTargetException from a buggy bean getter) without any log record. When this happens, the affected property is silently absent from the defaults map and ConfigBeanFactory later throws Missing path xxx pointing at user config — the real cause (a broken bean getter) becomes invisible to operators and to anyone debugging a startup failure.

Keep the best-effort behavior, but add a debug-level log so the root cause is recoverable when needed (e.g. via -Dlogger.org.tron.core.config.BeanDefaults=DEBUG).

Suggestion: instantiate a private static Logger and call logger.debug("Skipping property {}.{} due to introspection failure", bean.getClass().getName(), pd.getName(), e) inside the catch block.

[bladehan1]

[SHOULD] Priority semantics flip for allowShieldedTransactionApi is a behavior change, not a bug fix

The PR description frames this as fixing a dead-code bug, but in develop today the legacy branch is not dead — it actually overrides the modern key whenever a user sets fullNodeAllowShieldedTransaction. The old test testShieldedApiLegacyKeyTakesPriorityOverModern explicitly asserts legacy-wins. This PR flips that to modern-wins, and replaces the test accordingly.

That is a deliberate semantic change — not a refactor — and users who relied on legacy-wins (e.g. ops scripts that only set the legacy key while a stale allowShieldedTransactionApi lingers in their config) will see different behavior after upgrade.

Suggestion: add a "Behavior changes" section to the PR description making the priority flip explicit, and define a deprecation timeline for the legacy key (e.g. emit warn for two releases, remove in the third).

[317787106]

@bladehan1 It doesn't belongs to behavior change, it remains same as v.4.8.1 that introduced in PR 6371. It fixs the bug that exists in PR 6615.

[bladehan1]

[NIT] toValue(null) → "" silently masks type mismatches

toValue(null) returns an empty string regardless of the declared field type. Today every nested bean field is eagerly initialized (private DiscoveryConfig discovery = new DiscoveryConfig();), so this never triggers — but a future change to lazy-init style (private DiscoveryConfig discovery;) would silently emit discovery = "" into the defaults map, and ConfigBeanFactory would then fail to bind a String value into a nested bean type with an obscure error.

Suggestion: either restrict the null → "" coercion to declared String types only, or throw IllegalStateException when a nested-bean / numeric / List getter returns null, so the broken bean is identified up-front rather than failing later in ConfigBeanFactory.

[bladehan1]

[NIT] Import order: project imports placed between third-party imports

import org.tron.core.config.BeanDefaults; is inserted between com.typesafe.config.* and lombok.* imports, which breaks the project's Google Java Style import grouping (java/javax → com → org → lombok, with org.tron colocated with other org.*). Several other refactored files have the same issue (StorageConfig, etc.) — checkstyle CI will flag them.

Suggestion: run ./gradlew checkstyleMain locally and let it auto-format imports; the same fix should be applied to the other refactored config files.

[bladehan1]

[NIT] Class Javadoc lacks usage-scope constraint

BeanDefaults exposes three public static entry points (toConfig / stripNullLeaves / remapKey). The Javadoc explains the design intent (ConfigBeanFactory fallback) but does not say "use here only." Because this class lives in the common/ leaf module, business modules (actuator, chainbase, etc.) could plausibly start using toConfig as a generic bean-to-Config serializer, which would couple a leaf utility to runtime concerns it was not designed for.

Suggestion: extend the class-level Javadoc with a one-line constraint such as: "Intended exclusively for ConfigBeanFactory fallback inside org.tron.core.config.args.*. For general bean serialization, use Jackson/Gson directly."

[bladehan1]

[NIT] Document that reference.conf is no longer expected on the load() path

ConfigFactory.load(fileName) historically picked up the in-jar reference.conf automatically. After this PR, reference.conf is deleted, but a stale copy left over in a fat-jar by a dirty build cache would still be loaded by this branch — silently masking the new bean-based defaults.

Suggestion: add a comment here noting that reference.conf was removed in this PR and any residual copy in the resource path is a build artifact to be cleaned; consider an optional packaging-time check that asserts no reference.conf ends up in the published jar.

[bladehan1]

[NIT] stripNullLeaves coverage is limited to String-typed null leaves

The current tests cover null leaves at scalar String fields and inside a nested object. They do not cover three other shapes that real users will produce by mistake:

1. Numeric field set to null (e.g. vm.maxEnergyLimitForConstant = null)
2. Whole nested object set to null (e.g. storage.dbSettings = null)
3. List field set to null (e.g. node.fastForward = null)

For each, the expected behavior is: stripNullLeaves removes the null and BeanDefaults provides the field default, so the node still starts.

Suggestion: add three tests, one per shape above. If the current implementation does not handle one of them correctly, tighten either stripNullObject or document the limit in the Javadoc of stripNullLeaves.

[bladehan1]

[QUESTION] Add automated guard for fields whose names start with two consecutive uppercase letters

The pBFTEnable / pBFTPort / pBFTExpireNum cases are handled by manual BeanDefaults.remapKey calls, but the rule "any field whose JavaBean property name starts with two consecutive uppercase letters needs a remap" lives only as commentary. A future contributor adding (for example) xPBFTSomething will likely forget to remap, and ConfigBeanFactory will silently fall back to the bean default while ignoring the user's value.

Question: would you consider adding a unit test that uses reflection to enumerate all *Config fields, detects names matching \b[A-Z]{2,} after the first letter, and asserts each one is either (a) absent from config.conf, or (b) covered by a remapKey call? It's a small one-time investment that turns a silent failure mode into a CI failure.

[bladehan1]

[QUESTION] Provide a default-value parity check vs the deleted reference.conf

In the old model reference.conf was a runtime requirement — without it ConfigBeanFactory would throw Missing — so its values were the actual runtime defaults, and the bean field initializers (private int maxConnections = 30) were effectively dead code (always overwritten via the fallback chain). After this PR the bean field initializers become live for the first time. Any historical drift between a field initializer and the value that used to live in reference.conf is now a silent user-observable behavior change for anyone running with their own config.conf.

Manual spot-checking of the operationally critical keys (seed.node.ip.list, event.subscribe.topics, node.fastForward, genesis.block.*) confirms the shipped framework/src/main/resources/config.conf carries equivalent values, so users on the shipped template are unaffected. But every scalar key needs the same per-key check, and there is no automated guard against future drift.

Question: could the team commit the pre-deletion reference.conf as common/src/test/resources/reference.conf.legacy-fixture and add a parity test (BeanDefaults.toConfig(...) chained over all 9 beans, compared key-by-key against the fixture, with intentional differences explicitly whitelisted)? It would lock the contract permanently and catch future drift in CI.

If a parity test is too heavy for this PR, the minimum ask is to include the per-scalar-key audit result in the PR description as a checklist signed off by both author and reviewer.

[bladehan1]

[SHOULD] Consider splitting: stripNullLeaves bug fix and reference.conf removal are independent changes

This PR combines two technically independent changes whose risk profiles differ by an order of magnitude:

1. stripNullLeaves bug fix (~30 lines) — pure bug fix for the startup crash when a user config contains an explicit null leaf. Self-contained, low risk, does not move the default-value source. Could be cherry-picked into a current release.

2. Remove reference.conf + introduce BeanDefaults reflection-based fallback (~1900 lines) — pure architectural refactor to eliminate dual-source maintenance. High risk: in the old model reference.conf was a runtime requirement, so bean field initializers were effectively dead code (always overwritten via the fallback chain). Removing reference.conf makes those initializers live for the first time, and any historical drift becomes a silent user-observable behavior change. Needs default-value parity verification, the pBFT* remap idiom, and confirmation that the shipped config.conf carries the operationally critical keys that used to live in reference.conf.

The two changes are loosely coupled: fixing #1 does not require touching reference.conf at all (just call stripNullLeaves on the user-supplied config before merging). Likewise, #2 does not require stripNullLeaves — it's an independent architectural choice.

Merging them together makes both halves harder to review (the small bug fix gets buried in the large refactor's diff), and creates a coupling problem for future rollback decisions: if a user-observable drift surfaces in the architectural part and #2 needs to be reverted, the urgent bug fix in #1 would be reverted with it.

Suggestion: split into PR-A (just stripNullLeaves + call site, keep reference.conf unchanged) and PR-B (BeanDefaults + delete reference.conf + nine fromConfig changes), in that order. Alternatively, if you prefer to keep them together, list the independent logical changes in the PR description with explicit rollback boundaries so a future revert decision can target one without the other.

[bladehan1]

[SHOULD] Two legacy-key fallbacks in same method use opposite priority semantics

This method now contains two legacy-key fallback handlers with opposite precedence semantics, which is a cognitive trap for future maintainers:

• maxActiveNodesWithSameIp (legacy) → maxConnectionsWithSameIp (modern): legacy unconditionally overrides modern (legacy-wins). Asserted by testLegacyAliasTakesPriorityOverModernKey.
• fullNodeAllowShieldedTransaction (legacy) → allowShieldedTransactionApi (modern): modern wins; legacy only used when modern is absent. Asserted by testShieldedApiModernKeyTakesPriorityOverLegacy.

A developer adding a third legacy-key fallback will have no signal which pattern to follow — and the inconsistency is invisible without reading both tests.

Suggestion: add a header comment to fromConfig documenting the asymmetry and its history (bug-for-bug compatibility for maxActiveNodesWithSameIp, intentional fix for allowShieldedTransactionApi); ideally, plan a follow-up to unify on modern-wins (with deprecation warning on legacy keys) so a single consistent rule applies.

[bladehan1]

common/src/main/java/org/tron/core/config/args/NodeConfig.java:475-489 (note: this code is pre-existing in the PR base and not part of the diff hunks, so this comment is posted at issue-level)

[NIT] Three private static helpers are unused

getInt, getLong, and getString are defined here but never called — only getBool is used (line 359). SpotBugs / unused-warning checks will likely flag these, and a future reader will be unsure whether they were left in deliberately or by accident.

Suggestion: delete the three unused helpers; if they were intentionally reserved for future legacy-key handling, keep them with a brief explanatory comment and @SuppressWarnings("unused").

[bladehan1]

[SHOULD] PR description's "dual-source maintenance" premise is misframed; the real problem is DX, not engineering debt

The PR description's "What does this PR do?" and "Why are these changes required? §1 Dual-source maintenance with no compile-time enforcement" frame the core motivation as eliminating a dual-source maintenance problem. This diagnosis does not match the runtime reality of the old model.

There is no dual-source maintenance problem:

1. In the pre-PR model, reference.conf is a runtime-required fallback: ConfigBeanFactory.create() throws ConfigException.Missing for any absent key, so the node fails to start and every config-bean unit test fails. That makes reference.conf the canonical default source at runtime.

2. Bean field initializers (e.g. private int maxConnections = 30) are always overwritten during the binding flow: ConfigBeanFactory.create() writes the merged-config value back into the bean field. The initializer never takes effect at runtime — it is dead-code placeholder, not a source of truth.

3. Since the bean field initializer is not a source of truth, there is no "two places to keep in sync." There is only one source of truth: reference.conf. The "developers had to keep both in sync" framing is therefore inaccurate.

4. The "no compile-time enforcement" framing is also inaccurate: there is a strong runtime/test-time enforcement loop. Forget to update reference.conf for a new field, and ConfigBeanFactory throws Missing at startup or in any binding-related unit test. That is a fast feedback loop — feedback point is later than javac, but it is not absent.

The actual problem the PR is responding to is DX (developer experience), not engineering debt:

• reference.conf is invisible to developers reading source / Javadoc. They must know to look at it.
• Jumping to a bean field in the IDE shows the initializer (e.g. = 30), but that value is dead code. New contributors easily mistake it for the real default and become confused when production behavior differs.
• When adding a new field, the runtime feedback loop catches missing-reference.conf cases, but it surfaces during onboarding ("why does my unit test fail?") rather than at the point of edit.
• Default-value changes require touching two files, increasing review-time cross-file diff burden.

These are real, but they are DX problems. Diagnosing them as "dual-source maintenance / engineering debt" leads to over-treatment: a ~1900-line architectural refactor (delete reference.conf + introduce BeanDefaults + nine fromConfig rewrites + Args.java bridges) for a problem that, as stated, does not exist.

Lighter alternatives that target the actual DX issue:

• Option E (alternative design) — Keep reference.conf + add stripNullLeaves to fix the null-leaf startup crash + add a CONTRIBUTING.md / modules/config.md paragraph saying "reference.conf is the canonical default source; bean field initializers are placeholders, not runtime defaults." Roughly 50 lines, zero behavior change, fully reversible.
• DX option — IDE plugin / Javadoc @see annotations / build-time generator that surfaces reference.conf defaults in bean-field comments. DX is improved without removing the runtime fallback.
• Architectural refactor (this PR's current direction) — Acceptable, but should be motivated as a DX upgrade (not as fixing a non-existent dual-source problem) and ideally proposed as an independent RFC, separate from the urgent null-bug fix.

Suggestions:

1. Revise the "Why are these changes required?" section. State the actual motivation (DX: invisibility of reference.conf, IDE jumps to dead-code initializers) rather than the dual-source framing. Acknowledge that reference.conf was, and is, a single canonical source at runtime.

2. Decide separately whether this PR's path or a lighter intervention is the right response to the DX issue. The decision deserves an explicit RFC-style discussion rather than being implicit in a PR titled refactor(config).

3. Consider splitting: a small PR for stripNullLeaves (low risk, ships fast) + a separate RFC / PR for any DX-driven architectural change.

This is the most important review concern in this PR — every other finding (default-value parity, pBFT* remap idiom, userSection vs section invariant, Args.java scope) descends from accepting the original "dual-source" framing. If we agree the diagnosis was off, several of those concerns disappear because the architectural change itself becomes optional.

[waynercheung]

[MUST] Removing defaultReference() here regresses external partial config files. seed.node.ip.list is still read manually in MiscConfig.fromConfig() and no longer has any fallback on the file-load path, so a minimal -c file now yields an empty seed set instead of the legacy bootstrap defaults. Please keep defaultReference() until the remaining top-level/manual-read domains are migrated, or add an equivalent fallback for them before deleting reference.conf.

[waynercheung]

[MUST] This is not parity-preserving with the old runtime defaults. reference.conf previously supplied a populated genesis.block, while BeanDefaults.toConfig(new GenesisConfig()) only supplies empty strings/lists. Combined with Args.applyGenesisConfig(), an external partial config that omits genesis.block now falls back to GenesisBlock.getDefault() instead of the shipped genesis config. Please preserve the legacy genesis fallback here, or exclude genesis.block from the reference.conf removal in this PR.

[waynercheung]

[MUST] This changes the default event.subscribe.topics contract. reference.conf previously provided 7 default topic entries; after this change topics stays empty unless the user writes it explicitly. That breaks parity for external partial config files. Please preserve the legacy default topic list here, or keep reference.conf on the file-load path until event.subscribe is migrated completely.