Skip to content

Commit c38f388

Browse files
vividctrlaltvividctrlalt
committed
fix(config): restore lost memoFee/allowNewReward clamps and add boundary tests
Add missing clamps in CommitteeConfig.postProcess(): - memoFee clamped to [0, 1_000_000_000] (regression from manual parsing) - allowNewReward clamped to [0, 1] (must run before cross-field check) Add boundary test coverage for every clamp in CommitteeConfig, NodeConfig, VmConfig, and Args bridge code (fetchBlockTimeout). 31 new test cases pin each clamp's below/above/in-range behavior to prevent silent regression in future refactors. Reported by reviewer kuny0707.
1 parent 651e019 commit c38f388

5 files changed

Lines changed: 318 additions & 0 deletions

File tree

‎common/src/main/java/org/tron/core/config/args/CommitteeConfig.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,15 @@ private void postProcess() {
148148
if (dynamicEnergyMaxFactor < 0) { dynamicEnergyMaxFactor = 0; }
149149
if (dynamicEnergyMaxFactor > 100_000L) { dynamicEnergyMaxFactor = 100_000L; }
150150

151+
// clamp allowNewReward to 0-1 (must run BEFORE the cross-field check below,
152+
// which depends on allowNewReward != 1)
153+
if (allowNewReward < 0) { allowNewReward = 0; }
154+
if (allowNewReward > 1) { allowNewReward = 1; }
155+
156+
// clamp memoFee to 0-1_000_000_000
157+
if (memoFee < 0) { memoFee = 0; }
158+
if (memoFee > 1_000_000_000L) { memoFee = 1_000_000_000L; }
159+
151160
// cross-field: allowOldRewardOpt requires at least one reward/vote flag
152161
if (allowOldRewardOpt == 1 && allowNewRewardAlgorithm != 1
153162
&& allowNewReward != 1 && allowTvmVote != 1) {

‎common/src/test/java/org/tron/core/config/args/CommitteeConfigTest.java‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,4 +68,166 @@ public void testAllowOldRewardOptWithPrerequisite() {
6868
withRef("committee { allowOldRewardOpt = 1, allowTvmVote = 1 }"));
6969
assertEquals(1, cc.getAllowOldRewardOpt());
7070
}
71+
72+
// ===========================================================================
73+
// Boundary tests for postProcess() clamps
74+
//
75+
// Background: PR #6615 (config bean refactor) silently dropped clamps for
76+
// memoFee and allowNewReward because no test covered the boundary cases.
77+
// These tests pin every clamp in CommitteeConfig.postProcess() so future
78+
// refactors cannot drop them undetected.
79+
// ===========================================================================
80+
81+
// ----- memoFee: clamped to [0, 1_000_000_000] -----
82+
83+
@Test
84+
public void testMemoFeeClampedBelowZero() {
85+
assertEquals(0, CommitteeConfig.fromConfig(
86+
withRef("committee { memoFee = -100 }")).getMemoFee());
87+
}
88+
89+
@Test
90+
public void testMemoFeeClampedAboveMax() {
91+
assertEquals(1_000_000_000L, CommitteeConfig.fromConfig(
92+
withRef("committee { memoFee = 5000000000 }")).getMemoFee());
93+
}
94+
95+
@Test
96+
public void testMemoFeeInRangeUnchanged() {
97+
assertEquals(500_000_000L, CommitteeConfig.fromConfig(
98+
withRef("committee { memoFee = 500000000 }")).getMemoFee());
99+
}
100+
101+
@Test
102+
public void testMemoFeeBoundaryValues() {
103+
assertEquals(0, CommitteeConfig.fromConfig(
104+
withRef("committee { memoFee = 0 }")).getMemoFee());
105+
assertEquals(1_000_000_000L, CommitteeConfig.fromConfig(
106+
withRef("committee { memoFee = 1000000000 }")).getMemoFee());
107+
}
108+
109+
// ----- allowNewReward: clamped to [0, 1] -----
110+
111+
@Test
112+
public void testAllowNewRewardClampedBelowZero() {
113+
assertEquals(0, CommitteeConfig.fromConfig(
114+
withRef("committee { allowNewReward = -5 }")).getAllowNewReward());
115+
}
116+
117+
@Test
118+
public void testAllowNewRewardClampedAboveOne() {
119+
assertEquals(1, CommitteeConfig.fromConfig(
120+
withRef("committee { allowNewReward = 99 }")).getAllowNewReward());
121+
}
122+
123+
@Test
124+
public void testAllowNewRewardBoundaryValues() {
125+
assertEquals(0, CommitteeConfig.fromConfig(
126+
withRef("committee { allowNewReward = 0 }")).getAllowNewReward());
127+
assertEquals(1, CommitteeConfig.fromConfig(
128+
withRef("committee { allowNewReward = 1 }")).getAllowNewReward());
129+
}
130+
131+
// Critical: clamp must run BEFORE the cross-field check, otherwise
132+
// `allowNewReward = 2` (intended as "enabled") would still satisfy
133+
// `allowNewReward != 1` and the cross-field check would throw.
134+
// This test pins the clamp ordering.
135+
@Test
136+
public void testAllowNewRewardClampRunsBeforeCrossFieldCheck() {
137+
CommitteeConfig cc = CommitteeConfig.fromConfig(withRef(
138+
"committee { allowOldRewardOpt = 1, allowNewReward = 2 }"));
139+
assertEquals(1, cc.getAllowNewReward());
140+
assertEquals(1, cc.getAllowOldRewardOpt());
141+
}
142+
143+
// ----- allowDelegateOptimization: clamped to [0, 1] -----
144+
145+
@Test
146+
public void testAllowDelegateOptimizationClampedBelowZero() {
147+
assertEquals(0, CommitteeConfig.fromConfig(
148+
withRef("committee { allowDelegateOptimization = -3 }"))
149+
.getAllowDelegateOptimization());
150+
}
151+
152+
@Test
153+
public void testAllowDelegateOptimizationClampedAboveOne() {
154+
assertEquals(1, CommitteeConfig.fromConfig(
155+
withRef("committee { allowDelegateOptimization = 7 }"))
156+
.getAllowDelegateOptimization());
157+
}
158+
159+
// ----- allowDynamicEnergy: clamped to [0, 1] -----
160+
161+
@Test
162+
public void testAllowDynamicEnergyClampedBelowZero() {
163+
assertEquals(0, CommitteeConfig.fromConfig(
164+
withRef("committee { allowDynamicEnergy = -1 }")).getAllowDynamicEnergy());
165+
}
166+
167+
// ----- unfreezeDelayDays: clamped to [0, 365] (boundary values) -----
168+
169+
@Test
170+
public void testUnfreezeDelayDaysBoundaryValues() {
171+
assertEquals(0, CommitteeConfig.fromConfig(
172+
withRef("committee { unfreezeDelayDays = 0 }")).getUnfreezeDelayDays());
173+
assertEquals(365, CommitteeConfig.fromConfig(
174+
withRef("committee { unfreezeDelayDays = 365 }")).getUnfreezeDelayDays());
175+
assertEquals(100, CommitteeConfig.fromConfig(
176+
withRef("committee { unfreezeDelayDays = 100 }")).getUnfreezeDelayDays());
177+
}
178+
179+
// ----- dynamicEnergyThreshold: clamped to [0, 100_000_000_000_000_000] -----
180+
181+
@Test
182+
public void testDynamicEnergyThresholdClampedBelowZero() {
183+
assertEquals(0, CommitteeConfig.fromConfig(
184+
withRef("committee { dynamicEnergyThreshold = -1 }"))
185+
.getDynamicEnergyThreshold());
186+
}
187+
188+
// ----- dynamicEnergyIncreaseFactor: clamped to [0, 10_000] -----
189+
190+
@Test
191+
public void testDynamicEnergyIncreaseFactorClamped() {
192+
assertEquals(0, CommitteeConfig.fromConfig(
193+
withRef("committee { dynamicEnergyIncreaseFactor = -1 }"))
194+
.getDynamicEnergyIncreaseFactor());
195+
assertEquals(10_000L, CommitteeConfig.fromConfig(
196+
withRef("committee { dynamicEnergyIncreaseFactor = 10001 }"))
197+
.getDynamicEnergyIncreaseFactor());
198+
assertEquals(5_000L, CommitteeConfig.fromConfig(
199+
withRef("committee { dynamicEnergyIncreaseFactor = 5000 }"))
200+
.getDynamicEnergyIncreaseFactor());
201+
}
202+
203+
// ----- dynamicEnergyMaxFactor: clamped to [0, 100_000] -----
204+
205+
@Test
206+
public void testDynamicEnergyMaxFactorClamped() {
207+
assertEquals(0, CommitteeConfig.fromConfig(
208+
withRef("committee { dynamicEnergyMaxFactor = -1 }"))
209+
.getDynamicEnergyMaxFactor());
210+
assertEquals(100_000L, CommitteeConfig.fromConfig(
211+
withRef("committee { dynamicEnergyMaxFactor = 100001 }"))
212+
.getDynamicEnergyMaxFactor());
213+
assertEquals(50_000L, CommitteeConfig.fromConfig(
214+
withRef("committee { dynamicEnergyMaxFactor = 50000 }"))
215+
.getDynamicEnergyMaxFactor());
216+
}
217+
218+
// ----- Cross-field validation for allowOldRewardOpt -----
219+
220+
@Test
221+
public void testAllowOldRewardOptWithAllowNewReward() {
222+
CommitteeConfig cc = CommitteeConfig.fromConfig(
223+
withRef("committee { allowOldRewardOpt = 1, allowNewReward = 1 }"));
224+
assertEquals(1, cc.getAllowOldRewardOpt());
225+
}
226+
227+
@Test
228+
public void testAllowOldRewardOptWithAllowNewRewardAlgorithm() {
229+
CommitteeConfig cc = CommitteeConfig.fromConfig(
230+
withRef("committee { allowOldRewardOpt = 1, allowNewRewardAlgorithm = 1 }"));
231+
assertEquals(1, cc.getAllowOldRewardOpt());
232+
}
71233
}

‎common/src/test/java/org/tron/core/config/args/NodeConfigTest.java‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,4 +141,88 @@ public void testRpcUserOverrideExplicitValues() {
141141
assertEquals(8388608, rpc.getMaxMessageSize());
142142
assertEquals(16384, rpc.getMaxHeaderListSize());
143143
}
144+
145+
// ===========================================================================
146+
// Boundary tests for postProcess() clamps
147+
// Pin every clamp in NodeConfig.postProcess() so future refactors cannot
148+
// drop them undetected (regression seen in PR #6615 with CommitteeConfig).
149+
// ===========================================================================
150+
151+
// ----- blockProducedTimeOut: clamped to [30, 100] -----
152+
153+
@Test
154+
public void testBlockProducedTimeOutClampedBelowMin() {
155+
NodeConfig nc = NodeConfig.fromConfig(
156+
withRef("node { blockProducedTimeOut = 10 }"));
157+
assertEquals(30, nc.getBlockProducedTimeOut());
158+
}
159+
160+
@Test
161+
public void testBlockProducedTimeOutClampedAboveMax() {
162+
NodeConfig nc = NodeConfig.fromConfig(
163+
withRef("node { blockProducedTimeOut = 200 }"));
164+
assertEquals(100, nc.getBlockProducedTimeOut());
165+
}
166+
167+
@Test
168+
public void testBlockProducedTimeOutBoundaryValues() {
169+
assertEquals(30, NodeConfig.fromConfig(
170+
withRef("node { blockProducedTimeOut = 30 }")).getBlockProducedTimeOut());
171+
assertEquals(100, NodeConfig.fromConfig(
172+
withRef("node { blockProducedTimeOut = 100 }")).getBlockProducedTimeOut());
173+
assertEquals(75, NodeConfig.fromConfig(
174+
withRef("node { blockProducedTimeOut = 75 }")).getBlockProducedTimeOut());
175+
}
176+
177+
// ----- inactiveThreshold: minimum 1 -----
178+
179+
@Test
180+
public void testInactiveThresholdClampedBelowMin() {
181+
NodeConfig nc = NodeConfig.fromConfig(
182+
withRef("node { inactiveThreshold = 0 }"));
183+
assertEquals(1, nc.getInactiveThreshold());
184+
}
185+
186+
@Test
187+
public void testInactiveThresholdClampedNegative() {
188+
NodeConfig nc = NodeConfig.fromConfig(
189+
withRef("node { inactiveThreshold = -100 }"));
190+
assertEquals(1, nc.getInactiveThreshold());
191+
}
192+
193+
@Test
194+
public void testInactiveThresholdInRangeUnchanged() {
195+
assertEquals(1, NodeConfig.fromConfig(
196+
withRef("node { inactiveThreshold = 1 }")).getInactiveThreshold());
197+
assertEquals(600, NodeConfig.fromConfig(
198+
withRef("node { inactiveThreshold = 600 }")).getInactiveThreshold());
199+
assertEquals(1000, NodeConfig.fromConfig(
200+
withRef("node { inactiveThreshold = 1000 }")).getInactiveThreshold());
201+
}
202+
203+
// ----- maxFastForwardNum: clamped to [1, MAX_ACTIVE_WITNESS_NUM=27] -----
204+
205+
@Test
206+
public void testMaxFastForwardNumClampedBelowMin() {
207+
NodeConfig nc = NodeConfig.fromConfig(
208+
withRef("node { maxFastForwardNum = 0 }"));
209+
assertEquals(1, nc.getMaxFastForwardNum());
210+
}
211+
212+
@Test
213+
public void testMaxFastForwardNumClampedAboveMax() {
214+
NodeConfig nc = NodeConfig.fromConfig(
215+
withRef("node { maxFastForwardNum = 100 }"));
216+
assertEquals(27, nc.getMaxFastForwardNum());
217+
}
218+
219+
@Test
220+
public void testMaxFastForwardNumBoundaryValues() {
221+
assertEquals(1, NodeConfig.fromConfig(
222+
withRef("node { maxFastForwardNum = 1 }")).getMaxFastForwardNum());
223+
assertEquals(27, NodeConfig.fromConfig(
224+
withRef("node { maxFastForwardNum = 27 }")).getMaxFastForwardNum());
225+
assertEquals(4, NodeConfig.fromConfig(
226+
withRef("node { maxFastForwardNum = 4 }")).getMaxFastForwardNum());
227+
}
144228
}

‎common/src/test/java/org/tron/core/config/args/VmConfigTest.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,4 +70,22 @@ public void testPartialConfig() {
7070
assertFalse(vm.isSupportConstant()); // default
7171
assertEquals(500, vm.getLruCacheSize()); // default
7272
}
73+
74+
// ===========================================================================
75+
// Boundary tests for postProcess() clamps
76+
// Pin every clamp in VmConfig.postProcess() so future refactors cannot
77+
// drop them undetected (regression seen in PR #6615 with CommitteeConfig).
78+
// ===========================================================================
79+
80+
// ----- estimateEnergyMaxRetry: clamped to [0, 10] -----
81+
82+
@Test
83+
public void testEstimateEnergyMaxRetryBoundaryValues() {
84+
assertEquals(0, VmConfig.fromConfig(
85+
withRef("vm { estimateEnergyMaxRetry = 0 }")).getEstimateEnergyMaxRetry());
86+
assertEquals(10, VmConfig.fromConfig(
87+
withRef("vm { estimateEnergyMaxRetry = 10 }")).getEstimateEnergyMaxRetry());
88+
assertEquals(3, VmConfig.fromConfig(
89+
withRef("vm { estimateEnergyMaxRetry = 3 }")).getEstimateEnergyMaxRetry());
90+
}
7391
}

‎framework/src/test/java/org/tron/core/config/args/ArgsTest.java‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,5 +370,50 @@ public void testConfigStorageDefaults() {
370370

371371
Args.clearParam();
372372
}
373+
374+
// ===========================================================================
375+
// Boundary tests for clamps applied in Args.java bridge code (not in
376+
// bean postProcess()).
377+
//
378+
// fetchBlockTimeout is read from NodeConfig but clamped in Args.applyNodeConfig
379+
// to range [100, 1000]. Pin this clamp here so any future refactor that moves
380+
// it (e.g. into NodeConfig.postProcess()) preserves the behavior.
381+
// ===========================================================================
382+
383+
@Test
384+
public void testFetchBlockTimeoutClampedBelowMin() {
385+
Map<String, String> override = new HashMap<>();
386+
override.put("storage.db.directory", "database");
387+
override.put("node.fetchBlock.timeout", "50");
388+
Config config = ConfigFactory.parseMap(override)
389+
.withFallback(ConfigFactory.defaultReference());
390+
Args.applyConfigParams(config);
391+
Assert.assertEquals(100, Args.getInstance().getFetchBlockTimeout());
392+
Args.clearParam();
393+
}
394+
395+
@Test
396+
public void testFetchBlockTimeoutClampedAboveMax() {
397+
Map<String, String> override = new HashMap<>();
398+
override.put("storage.db.directory", "database");
399+
override.put("node.fetchBlock.timeout", "2000");
400+
Config config = ConfigFactory.parseMap(override)
401+
.withFallback(ConfigFactory.defaultReference());
402+
Args.applyConfigParams(config);
403+
Assert.assertEquals(1000, Args.getInstance().getFetchBlockTimeout());
404+
Args.clearParam();
405+
}
406+
407+
@Test
408+
public void testFetchBlockTimeoutInRangeUnchanged() {
409+
Map<String, String> override = new HashMap<>();
410+
override.put("storage.db.directory", "database");
411+
override.put("node.fetchBlock.timeout", "500");
412+
Config config = ConfigFactory.parseMap(override)
413+
.withFallback(ConfigFactory.defaultReference());
414+
Args.applyConfigParams(config);
415+
Assert.assertEquals(500, Args.getInstance().getFetchBlockTimeout());
416+
Args.clearParam();
417+
}
373418
}
374419

0 commit comments

Comments
 (0)