Skip to content

refactor(config): extract ConfigKey and remove testnet config#6565

Merged
kuny0707 merged 14 commits intotronprotocol:developfrom
vividctrlalt:refactor/remove-testnet
Mar 3, 2026
Merged

refactor(config): extract ConfigKey and remove testnet config#6565
kuny0707 merged 14 commits intotronprotocol:developfrom
vividctrlalt:refactor/remove-testnet

Conversation

@vividctrlalt
Copy link
Contributor

Summary

  • Extract ~232 HOCON config key constants from Constant.java into a new ConfigKey.java, leaving only business constants in Constant.java
  • Remove obsolete testnet configuration and 0xa0 address format
  • Unify all config files and tests to use mainnet address format (T- Base58 / 41 hex)

Changes

  • New ConfigKey.java: All HOCON config path strings (e.g. "node.rpc.port") moved here, organized by config section
  • Cleaned Constant.java: Only ~32 business constants remain, grouped by domain (address, transaction, energy, proposal, etc.)
  • Updated Args.java: References changed from Constant.XXX to ConfigKey.XXX for config keys; address prefix hardcoded to mainnet
  • Config files: Removed testnet addressPrefix setting; net.type marked as deprecated
  • Test configs: Converted all genesis/witness addresses from 27- prefix (testnet) to T- prefix (mainnet) and a0 hex to 41 hex
  • FreezeTest: Fixed CREATE2 factory bytecode magic byte (0xa00x41) to match mainnet prefix
  • IstanbulTest: Updated expected chain ID to match new genesis block hash

Test plan

  • Full test suite: 2326 tests passed, 0 failures, 24 ignored

vividcoder and others added 13 commits February 27, 2026 15:09
ci: make build job depend on PR lint check
- Add no trailing period rule for PR title
- Add no capitalized description rule
- Add known scope validation (warning only, non-blocking)
- Add CONTRIBUTING.md link in error messages
- Merge title and description checks into a single step
- Update CONTRIBUTING.md with complete commit type, scope reference,
  and PR title/description requirements
ci: enhance PR lint with stricter title and scope checks
docs: move type and scope reference to PR guidelines
net.type had no practical effect; add net.addressPrefix to explicitly
set the address prefix for backward compatibility.

- Deprecate net.type, add net.addressPrefix to all .conf files
- Add configFilePath to CommonParameter to store resolved config path
- Simplify DynamicArgs to reuse configFilePath from startup
- Remove unused TESTNET constants and NET_TYPE/NET_CONF constants
….java

Move ~232 HOCON configuration key string constants from Constant.java to a
new package-private ConfigKey.java in org.tron.core.config.args. This separates
config-file keys (used only by Args/DynamicArgs/WitnessInitializer) from
business constants, reducing Constant.java from ~420 lines to ~65 lines.
- Remove net.addressPrefix from all 11 config files, keep net block
  with commented-out type field and deprecation note
- Remove ConfigKey.NET_ADDRESS_PREFIX constant and Args.java parsing
  logic, always use mainnet prefix 0x41
- Fix test addresses: convert 27-prefix Base58 to T-prefix, a0/A0 hex
  to 41 across actuator/servlet/relay/vm tests
- Fix hardcoded hashes: update merkle root in BlockCapsuleTest, chainId
  in AllowTvmCompatibleEvmTest and IstanbulTest
- Fix FreezeTest CREATE2: change FACTORY_CODE bytecode magic byte from
  0xa0 to 0x41 (PUSH1 60a0 -> 6041) to match new runtime prefix
- VMContractTestBase: WITNESS_SR1_ADDRESS "a0" -> "41"
- IsSRCandidateTest: nonexistentAddr hex "A0" -> "41"
- config-test-storagetest.conf: convert 16 genesis 27-prefix addresses
  to T-prefix
- config-test-index.conf: convert genesis 27-prefix address to T-prefix
- .gitignore: broaden bin/ pattern to **/bin/, add *.class
@@ -411,17 +415,11 @@ public static void setParam(final String[] args, final String confFileName) {
*/
public static void setParam(final Config config) {

Copy link

@warku123 warku123 Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vividctrlalt, thanks for this refactoring PR! The separation of ConfigKey from Constant definitely improves code maintainability.

I have a question about compatibility:

This PR completely removes the net.type configuration support and hardcodes the mainnet address prefix (0x41). While I understand the goal to simplify the codebase, I'm wondering if we should consider users who might still have net.type=testnet in their existing config files.

Would it be better to keep the config parsing but add a deprecation warning:

if (config.hasPath("net.type")) {
  logger.warn("net.type is deprecated since v4.x and will be ignored. "
      + "Mainnet address format (0x41) is now always used. "
      + "Please remove this option from your config file.");
}

What do you think?

Copy link
Contributor Author

@vividctrlalt vividctrlalt Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Regarding net.type: this config has always been misleading — it was never actually used in production. The only references were in test code. Since no real user depends on it, a deprecation warning would just add unnecessary noise. Removing it cleanly is the right approach.

Regarding the ConfigKey extraction: ~88% of the constants in Constant.java are HOCON config path strings used exclusively by Args.java. Keeping them in Constant pollutes a class that should only hold business constants. Extracting them into ConfigKey gives Args its own dedicated config key registry and keeps Constant clean

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a breaking change for anyone running a private TRON chain with net.type = testnet?

The removal of the net.type conditional means any private fork configured with net.type = testnet (using 0xa0 address prefix) would silently switch to 0x41 after upgrading, with no warning logged. While no known TRON network actually uses 0xa0, it would be safer to log a deprecation warning instead of silently ignoring it:

if (config.hasPath("net.type") 
    && "testnet".equalsIgnoreCase(config.getString("net.type"))) {
    logger.warn("net.type = testnet is deprecated and has no effect. "
        + "Address prefix defaults to mainnet (0x41).");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. However, no private chain could actually use net.type = testnet in practice — simply setting this config alone wouldn't make a functional testnet. It would require extensive changes to other configurations as well. Moreover, the codebase had already broken the semantics of this field by binding it to the meaningless 0xa0 address prefix purely for testing purposes.

So I still prefer removing it entirely. That said, adding a deprecation warning is a reasonable compromise — it's low cost and helps users understand that this config has no effect.

Reverse-engineered the Solidity source from CONTRACT_CODE and
FACTORY_CODE bytecode in FreezeTest.java. Added selector and
opcode comments above each bytecode constant.

Also removed accidental blank lines in pr-check.yml.

public static void clearParam() {
PARAMETER.shellConfFileName = "";
PARAMETER.configFilePath = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newly introduced configFilePath field seems redundant. Looking at where it's set in Args.setParam():

PARAMETER.setConfigFilePath(
    StringUtils.isNoneBlank(PARAMETER.shellConfFileName)
        ? PARAMETER.shellConfFileName : confFileName);
Config config = Configuration.getByFileName(PARAMETER.shellConfFileName, confFileName);

configFilePath is simply the resolved result of shellConfFileName ?? confFileName — no actual path construction or concatenation happens despite the name suggesting otherwise. It's then consumed only by DynamicArgs:

// init()
configFile = new File(parameter.getConfigFilePath());
// reload()
Config config = Configuration.getByFileName(parameter.getConfigFilePath(), null);

This can be achieved without adding a new field. For example, just backfill shellConfFileName with the default when it's blank:

if (StringUtils.isBlank(PARAMETER.shellConfFileName)) {
    PARAMETER.shellConfFileName = confFileName;
}

Then DynamicArgs can simply use parameter.getShellConfFileName() directly, and the new @Getter @Setter field on CommonParameter can be removed entirely.

If the intent is to keep a separate "resolved config name" field, consider at least renaming it to something more accurate like resolvedConfFileName, since the current name configFilePath implies a full filesystem path which is misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! You're right that configFilePath is essentially just the resolved result of shellConfFileName ?? confFileName. Backfilling shellConfFileName directly is cleaner. Will fix this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this variable represents the current configuration file path; this resolves the issue of whether it originates from the default value or a shell parameter.

After the node starts, reading the configuration file path only requires this variable, without needing additional checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sunny6889 Although your suggestion makes sense, but this modification would cause test failures due to shared mutable state. The root cause of this issue lies in the parameter initialization logic. Please refer to #6567 for details. The redundant variable issue you mentioned will be addressed as part of the parameter initialization logic refactoring.

Copy link
Contributor

@Sunny6889 Sunny6889 Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok we can keep this variable, but I suggest a better name such as resolvedConfFileName instead of configFilePath @vividctrlalt, as FilePath is a bit missleading.

public static void main(String[] args) {
ExitManager.initExceptionHandler();
Args.setParam(args, Constant.NET_CONF);
Args.setParam(args, "config.conf");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it should be a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer keeping it as a literal here. As the entry point of the application, explicitly specifying the config filename in main() is more readable — you can see at a glance what file the node loads on startup. This is the only place in the codebase that uses "config.conf", so there's no duplication concern and no need to extract it into a constant.

WITNESS_SR1_ADDRESS =
Constant.ADD_PRE_FIX_STRING_TESTNET + "299F3DB80A24B20A254B89CE639D59132F157F13";
// TDmHUBuko2qhcKBCGGafu928hMRj1tX2RW (test.config)
WITNESS_SR1_ADDRESS = "41" + "299F3DB80A24B20A254B89CE639D59132F157F13";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it should be a constant, mentioned as 41.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 41 hex prefix is hardcoded throughout the entire codebase — it's the only valid address prefix and will never change. Using it directly in test code is straightforward and readable.

public void get() {
Args.setParam(new String[] {"-c", TestConstants.TEST_CONF, "--keystore-factory"},
Constant.NET_CONF);
"config.conf");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it should be a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as above — there's no need to introduce a string constant for "config.conf" just for test code. Using the filename directly is clearer and more readable. The previous approach of wrapping config filenames in constants is not a common practice and adds indirection without benefit.

public void shutdownBlockTimeInitTest() {
Map<String, String> params = new HashMap<>();
params.put(Constant.NODE_SHUTDOWN_BLOCK_TIME, "0");
params.put("node.shutdown.BlockTime", "0");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it should be a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All config key constants have been moved to ConfigKey and made package-private, accessible only to Args.java. This is intentional — it prevents unrelated code from arbitrarily adding or referencing config keys. Test code cannot access these constants directly, and tests should not break the encapsulation of the main code. Given that test cases are short and focused, using inline config strings is clearer and more readable.

if (!confFile.exists()) {
logger.warn("Configuration path is required! No such file {}", confFile);
return null;
long lastModifiedTime = configFile.lastModified();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here configFile need a Null protection, since here run() is a public method, what if parameter.isDynamicConfigEnable() return false and here run() is called, it will return NullPointerException.
Another risk would be: while running the configuration file is accidentally deleted, what will happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. DynamicArgs is a Spring @component — no external code calls run() directly. This is an internal class, not a public API.

  2. Config file deleted at runtime — File.lastModified() returns 0L when the file doesn't exist and won't throw an exception. Since 0 > lastModified is false, reload() won't be triggered. No issue here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the test code is actually call this run()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimizing over-defensive checks is a good practice. The original null checks could actually hide bugs. Unnecessary null checks mislead readers into thinking null is a possible state. As for the test case, it just needs to be properly initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DynamicArgs is an internal @component where run() is only invoked after init() guarantees configFile is initialized.

logger.debug("Reloading ... ");
Config config = Configuration.getByFileName(parameter.getShellConfFileName(),
Constant.NET_CONF);
Config config = Configuration.getByFileName(parameter.getConfigFilePath(), null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please confirm is it really safe to remove the fallback Constant.NET_CONF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current code logic, this is fully guaranteed. The readability issue will be addressed in the parameter initialization refactoring.

@kuny0707 kuny0707 merged commit 67fc14e into tronprotocol:develop Mar 3, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants