Skip to content

docs(config): add configuration guides#6790

Open
317787106 wants to merge 14 commits into
tronprotocol:release_v4.8.2from
317787106:feature/config_comment
Open

docs(config): add configuration guides#6790
317787106 wants to merge 14 commits into
tronprotocol:release_v4.8.2from
317787106:feature/config_comment

Conversation

@317787106
Copy link
Copy Markdown
Collaborator

@317787106 317787106 commented May 20, 2026

What does this PR do?

  1. Rewrites reference.conf comments — every parameter now has an inline comment describing its purpose, valid range, and unit. The file is the authoritative default-value reference for all node operators and contributors.

  2. Slims down config.conf — removes the large duplicated comment blocks that belonged in reference.conf, leaving only the parameters whose shipped defaults intentionally differ from reference.conf and the minimal comments users need to act on.

  3. Fixes default values for three under-specified parameters:

    • node.rpc.maxConcurrentCallsPerConnection: Integer.MAX_VALUE0 (0 = no limit)
    • node.rpc.maxConnectionIdleInMillis / maxConnectionAgeInMillis: changed to 0 (0 = no limit)
    • node.dns.maxMergeSize: 15; changeThreshold: 0.80.1 (original value in release_v4.8.1. No effective runtime change.)
  4. Removes node.receiveTcpMinDataLength — the parameter was never read by any business logic after being assigned. Removing it eliminates dead configuration surface.

  5. Adds two documentation files under docs/:

    • configuration.md — user-facing guide: explains the reference.conf / config.conf two-layer system, how to start a node with a config file, and the most common configuration sections with examples.
    • configuration-conventions.md — developer-facing guide: covers ConfigBeanFactory auto-binding rules, the camelCase naming requirement, nesting depth limits, the four-step checklist for adding a new parameter, special type handling (List<String>, human-readable sizes), and backward-compatibility patterns.
  6. Removes human-readable size support from maxMessageSize parametersnode.http.maxMessageSize, node.rpc.maxMessageSize, and node.jsonrpc.maxMessageSize now accept plain integers (bytes) only.

Why are these changes required?

config.conf previously duplicated most of reference.conf's comment content, making it hard to maintain and causing users to question which file to trust. Meanwhile reference.conf lacked inline documentation for many parameters. There was also no written guide explaining the two-layer config system or the conventions developers must follow when extending it, leading to inconsistencies (non-camelCase keys, missing reference.conf entries, etc.).

The three maxMessageSize parameters do not benefit from unit-suffix parsing — plain byte values are unambiguous and consistent with every other numeric parameter in the file. Removing the special-case parsing simplifies NodeConfig and removes a hidden dependency on Config.getMemorySize.

This PR has been tested by

  • Unit Tests
  • Manual Testing

@github-actions github-actions Bot requested a review from halibobo1205 May 20, 2026 09:45
@317787106 317787106 changed the title doc(config): add documents of how to change config file as necessary docs(config): add documents of how to change config file as necessary May 20, 2026
@317787106 317787106 changed the title docs(config): add documents of how to change config file as necessary docs(config): add configuration guides May 20, 2026
@barbatos2011
Copy link
Copy Markdown
Contributor

Minor PR description correction:

In point 3, the dns "before" values are listed as maxMergeSize: 1 → 5 and changeThreshold: 0.8 → 0.1, but the actual diff shows:

- private int maxMergeSize = 0;
- private double changeThreshold = 0.0;
+ private int maxMergeSize = 5;
+ private double changeThreshold = 0.1;

I also want to clarify the "why" — the framing "align with actual recommended values" reads as if 5/0.1 are new recommendations, but they're actually the library defaults already in use today. Tracing Args.loadDnsPublishParameters:

int maxMergeSize = dns.getMaxMergeSize();
if (maxMergeSize >= 1 && maxMergeSize <= 5) {
    publishConfig.setMaxMergeSize(maxMergeSize);
} else if (maxMergeSize != 0) {     // ← 0 is a sentinel, skipped silently
    logger.error("Check node.dns.maxMergeSize, should be [1~5], default 5");
}

A 0 value (the old default) takes the else if (... != 0) skip path — no setter call, no error — leaving publishConfig to use its own field default. Confirmed against libp2p 2.2.7 source (org.tron.p2p.dns.update.PublishConfig):

private double changeThreshold = 0.1;
private int maxMergeSize = 5;

So the effective runtime value for an operator enabling dns.publish = true without overriding these two keys is already 5 / 0.1 today. This PR makes reference.conf show that explicitly rather than via the sentinel-zero indirection. Net runtime change: zero.

Suggested rewording for point 3, last bullet:

node.dns.maxMergeSize: 05; changeThreshold: 0.00.1 (self-documentation — these are the values libp2p's PublishConfig was already using internally; the old 0 was a sentinel that fell through to the library default. No effective runtime change.)

This makes the change less alarming for reviewers — it's not a default-value behavior shift, just clearer documentation in reference.conf.

@barbatos2011
Copy link
Copy Markdown
Contributor

Doc accuracy review — checked both new docs against current code and against real startup logs. The dev-facing configuration-conventions.md is uniformly accurate. The user-facing configuration.md has two non-trivial inaccuracies worth fixing:

1. config.conf is not "External"

The location table says:

| config.conf | External, supplied by the operator via -c | ... |

But config.conf is actually bundled in the jar — confirmed both by inspection and by the configuration-conventions.md Step 4 which correctly states this:

$ jar tf FullNode.jar | grep -E '^(config|reference)\.conf$'
reference.conf
config.conf

config.conf serves dual purpose: bundled template and optionally edited & passed via -c. Suggest updating the row to reflect both.

2. "If -c is omitted, the node uses only the defaults from reference.conf" is incorrect

FullNode.main passes "config.conf" as the default config name when -c is absent:

// FullNode.java:27
Args.setParam(args, "config.conf");

Configuration.resolveConfigFile then falls back to the classpath when no file is found on disk, loading the bundled framework/src/main/resources/config.conf. That file diverges from reference.conf on several values, e.g.:

key bundled config.conf reference.conf
node.discovery.enable true false
node.discovery.persist true false
node.backup.priority 8 0

Empirically verified by running the built jar with no -c flag:

$ java -jar FullNode.jar
$ grep "Discover enable" logs/tron.log
INFO  [main] [app](Args.java:1158) Discover enable: true

If the doc were correct, this would be false. It's true because bundled config.conf wins. Suggested rewording:

If -c is omitted, the node loads the config.conf bundled inside the jar (the same file shipped with the distribution) merged with reference.conf as fallback. The bundled file already enables discovery/persist for mainnet operation. For production, copy it out, edit, and pass the edited copy via -c to make your configuration visible to operators.

3. The openPrintLog claim is wrong on two counts

At startup, the node logs the resolved configuration (merged result of
config.conf + reference.conf) when node.openPrintLog = true (the default).

Gate is fictitious. openPrintLog is checked in 5 production sites only — all runtime verbosity for tx/inventory/peer logs (PendingManager, P2pEventHandlerImpl, InventoryMsgHandler). It does not gate startup config logging.

Scope is misrepresented. The actual startup dump is Args.logConfig() (called unconditionally from applyConfigParams at line 781). It prints ~30 specific fields under five section headers — never the full merged config. Verified empirically against tron.log:

INFO  [main] [app] ************************ System info ************************
INFO  [main] [app] ************************ Net config ************************      (~24 fields)
INFO  [main] [app] ************************ Backup config ************************   (~4 fields)
INFO  [main] [app] ************************ Code version *************************   (~2 fields)
INFO  [main] [app] ************************ DB config *************************      (~2 fields)
INFO  [main] [app] ************************ shutDown config *************************  (~3 fields)

Suggested rewording:

At startup, the node unconditionally logs a summary of key parameters under Net config, Backup config, Code version, DB config, and shutDown config headers (see Args.logConfig() for the exact fields). For parameters not in this summary, you must inspect runtime behavior or consult reference.conf directly — the full merged configuration is never dumped.

Note: node.openPrintLog is a separate flag that controls runtime verbosity of P2P/inventory/pending-tx logs, not startup config logging.

Otherwise

configuration-conventions.md (the developer-facing doc) is uniformly accurateConfigBeanFactory binding rules, @Setter(NONE) pattern, is-prefix and PBFT exceptions, normalizeMaxMessageSizes, List<String> manual read, the 4-step recipe, deprecation pattern — all check out against current source. The "Configuration Parameter vs. Constant" decision table is particularly strong and should help prevent future dead-parameter accumulation.

@317787106
Copy link
Copy Markdown
Collaborator Author

317787106 commented May 21, 2026

@barbatos2011 The default of maxMergeSize is 5 and changeThreshold is 0.1 in release_v4.8.1. So I use the original value.

Good catch for the document issuse:

  1. Update the purpose of config.conf
  2. Update the description when -c is omitted
  3. Fix the meaning of openPrintLog in configuration.md.

Comment thread framework/src/main/java/org/tron/core/config/args/Args.java
@barbatos2011
Copy link
Copy Markdown
Contributor

Process question, not blocking: this PR targets release_v4.8.2 rather than develop. The PR contents are docs (docs/configuration.md, docs/configuration-conventions.md) plus default-value alignments and a dead-parameter removal — all of which look like develop material rather than release-branch hotfix scope.

Same observation applies to #6788. Is there a 4.8.2-specific reason to land config tooling/docs work directly on the release branch, or would these be better as develop PRs that get cherry-picked into release_v4.8.2 if/when needed? Just want to make sure release-branch divergence stays minimal.

Comment thread framework/src/main/resources/config.conf Outdated
Comment thread common/src/main/java/org/tron/core/config/args/NodeConfig.java
@@ -515,79 +514,14 @@ public void testAllowShieldedTransactionApiDefault() {
}

@Test
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[NIT] maxMessageSize negative value is silently accepted

After removing normalizeMaxMessageSizes(), ConfigBeanFactory binds user-supplied values directly to the int/long fields. Negative values like node.rpc.maxMessageSize = -1 are accepted without validation and passed to gRPC's maxInboundMessageSize(-1), which will throw an IllegalArgumentException at server-build time.

The old normalizeMaxMessageSizes() path explicitly rejected negative values. That guard is now gone.

Suggestion: Add a test case (or validation in postProcess()) for negative maxMessageSize values to document the expected failure mode.

@@ -515,79 +514,14 @@ public void testAllowShieldedTransactionApiDefault() {
}

@Test
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[NIT] maxMessageSize values exceeding Integer.MAX_VALUE are untested

The old test suite had testRpcMaxMessageSizeExceedsIntMax which verified that values larger than Integer.MAX_VALUE were rejected. That test was removed along with normalizeMaxMessageSizes().

With the new plain-integer path, ConfigBeanFactory will attempt to bind a value like 4294967296 to an int field. The behavior (startup failure vs silent truncation) is currently untested and undefined.

Suggestion: Add a characterization test for oversized maxMessageSize values to document the expected behavior.

@317787106 317787106 requested a review from bladehan1 May 21, 2026 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants