refactor(config): merge config files with BeanDefaults fallback#76
Open
317787106 wants to merge 10 commits into
Open
refactor(config): merge config files with BeanDefaults fallback#76317787106 wants to merge 10 commits into
317787106 wants to merge 10 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request replaces bundled Typesafe Config defaults ( ChangesBean Defaults Infrastructure
Per-Configuration Class Migrations
Default Source Deletion
Framework Configuration Updates
Test Updates
Overlay Removal
Import Cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
317787106
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="common/src/main/java/org/tron/core/config/Configuration.java">
<violation number="1" location="common/src/main/java/org/tron/core/config/Configuration.java:51">
P1: `parseFile(confFile)` drops the default `reference.conf` fallback, so file-based config loading can lose required defaults and diverge from classpath loading behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (confFile.exists()) { | ||
| config = ConfigFactory.parseFile(confFile) | ||
| .withFallback(ConfigFactory.defaultReference()); | ||
| config = ConfigFactory.parseFile(confFile); |
There was a problem hiding this comment.
P1: parseFile(confFile) drops the default reference.conf fallback, so file-based config loading can lose required defaults and diverge from classpath loading behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/src/main/java/org/tron/core/config/Configuration.java, line 51:
<comment>`parseFile(confFile)` drops the default `reference.conf` fallback, so file-based config loading can lose required defaults and diverge from classpath loading behavior.</comment>
<file context>
@@ -48,10 +48,11 @@ public static com.typesafe.config.Config getByFileName(
if (confFile.exists()) {
- config = ConfigFactory.parseFile(confFile)
- .withFallback(ConfigFactory.defaultReference());
+ config = ConfigFactory.parseFile(confFile);
} else if (Thread.currentThread().getContextClassLoader().getResourceAsStream(fileName)
!= null) {
</file context>
Suggested change
| config = ConfigFactory.parseFile(confFile); | |
| config = ConfigFactory.parseFile(confFile) | |
| .withFallback(ConfigFactory.defaultReference()); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
common/src/main/java/org/tron/core/config/args/StorageConfig.java (1)
115-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
DbSettingsConfigandTxCacheConfigbean defaults diverge fromconfig.confproduction valuesFollowing the same pattern as
NodeConfig, severalStorageConfigfield defaults don't match the values thatconfig.confestablishes. Operators using custom partial configs will silently receive suboptimal LevelDB tuning and incorrect cache initialization behavior:
Location Field Bean default config.confvalueLine 117 DbSettingsConfig.blocksize1664Line 120 DbSettingsConfig.level0FileNumCompactionTrigger24Line 121 DbSettingsConfig.targetFileSizeBase64256Line 182 TxCacheConfig.initOptimizationfalsetrueThe LevelDB tuning mismatches (
blocksize,level0FileNumCompactionTrigger,targetFileSizeBase) can significantly degrade storage performance.initOptimization = falsemakes transaction cache initialization slower on startup.🛠️ Proposed fix
- private int blocksize = 16; + private int blocksize = 64; - private int level0FileNumCompactionTrigger = 2; + private int level0FileNumCompactionTrigger = 4; - private long targetFileSizeBase = 64; + private long targetFileSizeBase = 256;- private boolean initOptimization = false; + private boolean initOptimization = true;Also applies to: 180-183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/org/tron/core/config/args/StorageConfig.java` around lines 115 - 123, StorageConfig's LevelDB tuning defaults and TxCacheConfig's initOptimization need to match production config.conf values: update StorageConfig fields blocksize to 64, level0FileNumCompactionTrigger to 4, and targetFileSizeBase to 256, and set TxCacheConfig.initOptimization to true so partial/bean-based configs behave like the shipped config; locate and change the default field initializers in the StorageConfig (blocksize, level0FileNumCompactionTrigger, targetFileSizeBase) and the TxCacheConfig class (initOptimization) accordingly.common/src/main/java/org/tron/core/config/args/NodeConfig.java (1)
35-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBean defaults diverge from
config.confvalues — behavioral regressions for custom configsWith
reference.confremoved, the bean field initializers now define the effective defaults for any deployment that doesn't explicitly set a key. Several of these defaults contradict the values thatconfig.conf(and implicitly the oldreference.conf) established:
Location Field Bean default config.confvalueLine 35 minParticipationRate015Line 69 blockProducedTimeOut50documented as 75(line 322 comment)Line 135 DiscoveryConfig.enablefalsetrueLine 136 DiscoveryConfig.persistfalsetrueLine 261 RpcConfig.maxConnectionIdleInMillisLong.MAX_VALUE60000The most impactful regressions for operators running custom configs:
minParticipationRate = 0disables the participation-rate guard on block production entirely.discovery.enable = falseanddiscovery.persist = falsesilently disable peer discovery.maxConnectionIdleInMillis = Long.MAX_VALUEmeans idle gRPC connections are never terminated.Align the bean field initializers with the intended production defaults:
🛠️ Proposed fix
- private int minParticipationRate = 0; + private int minParticipationRate = 15; - private int blockProducedTimeOut = 50; + private int blockProducedTimeOut = 75;public static class DiscoveryConfig { - private boolean enable = false; - private boolean persist = false; + private boolean enable = true; + private boolean persist = true;- private long maxConnectionIdleInMillis = Long.MAX_VALUE; + private long maxConnectionIdleInMillis = 60000;Also applies to: 69-69, 135-136, 261-261
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@common/src/main/java/org/tron/core/config/args/NodeConfig.java` at line 35, The bean field initializers in NodeConfig and nested configs no longer match the intended production defaults in config.conf; update the default values so minParticipationRate = 15 (not 0), blockProducedTimeOut = 75 (not 50), DiscoveryConfig.enable = true and DiscoveryConfig.persist = true (not false), and RpcConfig.maxConnectionIdleInMillis = 60000 (not Long.MAX_VALUE) by changing the field initializers for minParticipationRate, blockProducedTimeOut, DiscoveryConfig.enable, DiscoveryConfig.persist, and RpcConfig.maxConnectionIdleInMillis to those values so runtime defaults match the documented config.conf behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/src/main/java/org/tron/core/config/BeanDefaults.java`:
- Around line 48-72: The current outer try-catch around
Introspector.getBeanInfo(...) and the PropertyDescriptor loop swallows any
Exception (in BeanDefaults.java) so a single getter.invoke(bean) failure aborts
the rest of the loop; move the per-property error handling inside the
for(PropertyDescriptor pd : info.getPropertyDescriptors()) loop by wrapping only
the getter.invoke(bean)/map.put(key, toValue(value)) lines in a try-catch that
handles and ignores/logs per-property invocation exceptions, and reduce the
outer catch to only handle IntrospectionException from Introspector.getBeanInfo;
keep references to Introspector.getBeanInfo, PropertyDescriptor,
pd.getReadMethod()/getWriteMethod(), getter.invoke(bean), map.put(key,
toValue(value)) when making the change.
---
Outside diff comments:
In `@common/src/main/java/org/tron/core/config/args/NodeConfig.java`:
- Line 35: The bean field initializers in NodeConfig and nested configs no
longer match the intended production defaults in config.conf; update the default
values so minParticipationRate = 15 (not 0), blockProducedTimeOut = 75 (not 50),
DiscoveryConfig.enable = true and DiscoveryConfig.persist = true (not false),
and RpcConfig.maxConnectionIdleInMillis = 60000 (not Long.MAX_VALUE) by changing
the field initializers for minParticipationRate, blockProducedTimeOut,
DiscoveryConfig.enable, DiscoveryConfig.persist, and
RpcConfig.maxConnectionIdleInMillis to those values so runtime defaults match
the documented config.conf behavior.
In `@common/src/main/java/org/tron/core/config/args/StorageConfig.java`:
- Around line 115-123: StorageConfig's LevelDB tuning defaults and
TxCacheConfig's initOptimization need to match production config.conf values:
update StorageConfig fields blocksize to 64, level0FileNumCompactionTrigger to
4, and targetFileSizeBase to 256, and set TxCacheConfig.initOptimization to true
so partial/bean-based configs behave like the shipped config; locate and change
the default field initializers in the StorageConfig (blocksize,
level0FileNumCompactionTrigger, targetFileSizeBase) and the TxCacheConfig class
(initOptimization) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8081123-e5d7-4bd1-b04d-84076b781203
📒 Files selected for processing (15)
common/src/main/java/org/tron/core/config/BeanDefaults.javacommon/src/main/java/org/tron/core/config/Configuration.javacommon/src/main/java/org/tron/core/config/args/BlockConfig.javacommon/src/main/java/org/tron/core/config/args/CommitteeConfig.javacommon/src/main/java/org/tron/core/config/args/EventConfig.javacommon/src/main/java/org/tron/core/config/args/GenesisConfig.javacommon/src/main/java/org/tron/core/config/args/MetricsConfig.javacommon/src/main/java/org/tron/core/config/args/NodeConfig.javacommon/src/main/java/org/tron/core/config/args/RateLimiterConfig.javacommon/src/main/java/org/tron/core/config/args/StorageConfig.javacommon/src/main/java/org/tron/core/config/args/VmConfig.javacommon/src/main/resources/reference.confcommon/src/test/java/org/tron/core/config/BeanDefaultsTest.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/core/config/args/ArgsTest.java
💤 Files with no reviewable changes (1)
- common/src/main/resources/reference.conf
317787106
changed the title
Resolve conflict by keeping deletion of common/src/main/resources/reference.conf, which is intentional — BeanDefaults replaces its role in this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rible; fix the bug of allowShieldedTransactionApi
There was a problem hiding this comment.
1 issue found across 20 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="common/src/main/java/org/tron/core/config/BeanDefaults.java">
<violation number="1" location="common/src/main/java/org/tron/core/config/BeanDefaults.java:72">
P2: remapKey should not overwrite an already-present canonical key; legacy key remapping currently changes user precedence when both keys are set.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| if (!config.hasPath(fromKey)) { | ||
| return config; | ||
| } | ||
| return config.withValue(toKey, config.getValue(fromKey)).withoutPath(fromKey); |
There was a problem hiding this comment.
P2: remapKey should not overwrite an already-present canonical key; legacy key remapping currently changes user precedence when both keys are set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/src/main/java/org/tron/core/config/BeanDefaults.java, line 72:
<comment>remapKey should not overwrite an already-present canonical key; legacy key remapping currently changes user precedence when both keys are set.</comment>
<file context>
@@ -43,6 +46,45 @@ public static Config toConfig(Object bean) {
+ if (!config.hasPath(fromKey)) {
+ return config;
+ }
+ return config.withValue(toKey, config.getValue(fromKey)).withoutPath(fromKey);
+ }
+
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/src/main/java/org/tron/core/config/args/MetricsConfig.java`:
- Around line 29-36: MetricsConfig currently doesn’t expose the nested
InfluxDbConfig so defaults and bindings are ignored; add a field of type
InfluxDbConfig with getter/setter (and initialize it to new InfluxDbConfig())
inside the MetricsConfig class so BeanDefaults.toConfig(new MetricsConfig()) and
ConfigBeanFactory.create(...) can pick up defaults and user values; refer to the
nested class name InfluxDbConfig and the outer class MetricsConfig when adding
the private InfluxDbConfig influxDb = new InfluxDbConfig() plus corresponding
getInfluxDb()/setInfluxDb() methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01466168-62f3-4d53-9b90-e424317c6d22
📒 Files selected for processing (12)
common/src/main/java/org/tron/common/parameter/CommonParameter.javacommon/src/main/java/org/tron/core/config/BeanDefaults.javacommon/src/main/java/org/tron/core/config/args/CommitteeConfig.javacommon/src/main/java/org/tron/core/config/args/MetricsConfig.javacommon/src/main/java/org/tron/core/config/args/NodeConfig.javacommon/src/main/java/org/tron/core/config/args/StorageConfig.javacommon/src/main/java/org/tron/core/config/args/VmConfig.javacommon/src/test/java/org/tron/core/config/args/NodeConfigTest.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/common/ParameterTest.javaframework/src/test/java/org/tron/core/config/args/ArgsTest.java
💤 Files with no reviewable changes (3)
- common/src/main/java/org/tron/common/parameter/CommonParameter.java
- framework/src/main/java/org/tron/core/config/args/Args.java
- framework/src/test/java/org/tron/common/ParameterTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- common/src/main/java/org/tron/core/config/args/CommitteeConfig.java
- common/src/main/java/org/tron/core/config/args/VmConfig.java
- common/src/test/java/org/tron/core/config/args/NodeConfigTest.java
- common/src/main/java/org/tron/core/config/BeanDefaults.java
- framework/src/test/java/org/tron/core/config/args/ArgsTest.java
- common/src/main/java/org/tron/core/config/args/NodeConfig.java
Comment on lines
+29
to
+36
| @Getter | ||
| @Setter | ||
| public static class InfluxDbConfig { | ||
| private String ip = ""; | ||
| private int port = 8086; | ||
| private String database = "metrics"; | ||
| private int metricsReportInterval = 10; | ||
| } |
There was a problem hiding this comment.
Wire InfluxDbConfig into MetricsConfig so it can actually bind.
InfluxDbConfig is declared, but MetricsConfig has no corresponding field/getter. As a result, Line 39 (BeanDefaults.toConfig(new MetricsConfig())) and Line 44 (ConfigBeanFactory.create(...)) will ignore these defaults and user values.
Suggested fix
public class MetricsConfig {
private PrometheusConfig prometheus = new PrometheusConfig();
+ private InfluxDbConfig influxDb = new InfluxDbConfig();
`@Getter`
`@Setter`
public static class PrometheusConfig {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@common/src/main/java/org/tron/core/config/args/MetricsConfig.java` around
lines 29 - 36, MetricsConfig currently doesn’t expose the nested InfluxDbConfig
so defaults and bindings are ignored; add a field of type InfluxDbConfig with
getter/setter (and initialize it to new InfluxDbConfig()) inside the
MetricsConfig class so BeanDefaults.toConfig(new MetricsConfig()) and
ConfigBeanFactory.create(...) can pick up defaults and user values; refer to the
nested class name InfluxDbConfig and the outer class MetricsConfig when adding
the private InfluxDbConfig influxDb = new InfluxDbConfig() plus corresponding
getInfluxDb()/setInfluxDb() methods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Eliminates
reference.confand makes Java bean field initializers the single source of truth for configuration defaults.Previously, configuration defaults were maintained in two places:
private int maxConnections = 30)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.confwas 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 TypesafeConfigobject. EachXxxConfig.fromConfig()now callsBeanDefaults.toConfig(new XxxConfig())and uses it as awithFallback(), soConfigBeanFactoryalways finds every key it needs — even when the user'sconfig.confonly sets a subset of fields.Specific changes:
BeanDefaults.java: reflects public getter+setter pairs viaBeanInfo, recurses into nested beans, serializes lists — mirrors exactly whatConfigBeanFactorybindsXxxConfig.fromConfig()methods (BlockConfig,CommitteeConfig,EventConfig,GenesisConfig,MetricsConfig,NodeConfig,RateLimiterConfig,StorageConfig,VmConfig): replace hardconfig.getConfig(section)calls withhasPathguard +withFallback(BeanDefaults.toConfig(...)); all classes now use an explicituserSectionvariable to hold the user's raw values before merging with defaultsreference.conf: no longer needed;BeanDefaultsreplaces its roleConfiguration.java: remove thewithFallback(ConfigFactory.defaultReference())call from the file-based load pathconfig.conf: add previously undocumented parameters that were silently defaulted throughreference.conf(e.g.storage.backup,storage.checkpoint,node.maxFastForwardNum,rate.limiter.p2p.*, etc.)BeanDefaultsTest.java: 16 tests covering default values, round-trip withConfigBeanFactory, and user-value override for all major config beansWhy are these changes required?
1. Dual-source maintenance with no compile-time enforcement
The dual-source maintenance created recurring issues:
reference.conf, with no compile-time enforcement of that requirementreference.confwas not visible to users browsing source code or Java docsreference.confdefaults was only caught at runtimereference.confcould not tell which keys were still active vs. superseded by bean logicAfter 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 = nullin reference.conf caused startup failureThe old
reference.confcontained:When Typesafe Config merged this with the user's
config.conf, thenullvalue propagated into the merged config.ConfigBeanFactorywould then attempt to bindnullto theStringfield, causing a startup crash even when the user had not set this key at all. This PR introducesBeanDefaults.stripNullLeaves(), which strips all HOCONnullleaves from the user section before merging with bean defaults — so a user's explicitnullentry never overwrites a valid Java default.3.
allowShieldedTransactionApipriority was silently brokenWhen both
node.allowShieldedTransactionApi(modern) andnode.fullNodeAllowShieldedTransaction(legacy) were present inconfig.conf, the modern key should take precedence. The original fallback logic checkedsection.hasPath("allowShieldedTransactionApi"), butsectionis the merged config (user values +BeanDefaults), which always containsallowShieldedTransactionApifrom the defaults — making the condition always true and the entire legacy-key branch dead code. As a result, a user who set onlyfullNodeAllowShieldedTransactionwould silently get the default value instead of their configured value.Fixed by checking
userSection.hasPath()(the raw user-supplied values only) instead ofsection.hasPath(). The correct semantics are now enforced and covered by tests: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:This form does not expose the raw user values as a named variable. Any developer adding a legacy-key fallback (like the
allowShieldedTransactionApicase above) would be tempted to writesection.hasPath()— which checks the merged config and silently makes the fallback dead code. All six classes now use the two-step form with an explicituserSectionvariable:The naming makes the invariant obvious:
userSectionis what the user wrote;sectionis the merged result. Future legacy-key checks must useuserSection.hasPath().This PR has been tested by:
BeanDefaultsTest,NodeConfigTest,BlockConfigTest,VmConfigTest,StorageConfigTest,CommitteeConfigTest,EventConfigTest,GenesisConfigTest,MetricsConfigTest,RateLimiterConfigTest— all pass)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Removals