Repository navigation
fix: time out idle HTTP connections and harden start.sh - #7003
SeriousCoding789 wants to merge 12 commits into
Conversation
The BlockId overload had no callers. Every caller uses getBranch(Sha256Hash, Sha256Hash), which rejects a branch whose parent is missing.
download() now prints a notice and returns non-zero; release files must be fetched and verified manually. It returns instead of exiting so that restart() still completes when rebuildManifest reaches it.
Connections that send nothing could hold every maxHttpConnectNumber slot until the connector's 30-second idle timeout and lock other clients out. When the limit is reached, the open connections now time out after 10 seconds of inactivity. Connections still being accepted at that moment keep the connector's default idle timeout.
Downloads: - TLS certificates are verified and a failed request leaves no file. - Release jars must carry a valid GPG signature from the release key listed under "Integrity Check" in README.md. A jar that fails is not used: --upgrade keeps the old jar and --download the existing one. - The mainnet config comes from java-tron and the Nile config from nile-testnet. Bugs: - --stop looped forever, the node was restarted two or three times per run, and -c was passed again on every start. - A jar argument such as my.jar was ignored, and -j with a path lost the path before starting. - --net ignored an existing config file; --net and --release kept going after a failed download. - Linux heap sizing needed bc, and the macOS memory check failed with an expr error. - Manifest rebuilding used a wrong URL and never ran the plugin. It now runs and, by default, rewrites manifests of 128 MB or more. - The latest release was picked in name order, clone failures were ignored, $darwin was never set, and failures exited with status 0. Platforms: - start.sh reads the Java version and architecture of its JVM. JDK 8 keeps the CMS options and JDK 17 uses the ZGC options from start.sh.simple, which is removed. ARM64 downloads the aarch64 jars and skips manifest rebuilding. - -s stops the node and the open file limit is raised to 65535, as in start.sh.simple. The default JVM_MX, used on macOS, is 12g. README and shell.md describe the single script.
SeriousCoding789
changed the title
Downloads: - TLS certificates are verified and a failed request leaves no file. - Release jars must carry a valid GPG signature from the release key listed under "Integrity Check" in README.md. A jar that fails is not used: --upgrade keeps the old jar and --download the existing one. - The mainnet config comes from java-tron and the Nile config from nile-testnet. Bugs: - --stop looped forever, the node was restarted two or three times per run, and -c was passed again on every start. - A jar argument such as my.jar was ignored, and -j with a path lost the path before starting. - --net ignored an existing config file; --net and --release kept going after a failed download. - Linux heap sizing needed bc, and the macOS memory check failed with an expr error. - On macOS without JAVA_HOME the script derived JAVA_HOME=/usr from the /usr/bin/javac stub, and /usr/bin/java then ran itself forever; the JDK now comes from /usr/libexec/java_home. - Manifest rebuilding used a wrong URL and never ran the plugin. It now runs and, by default, rewrites manifests of 128 MB or more. - The latest release was picked in name order, clone failures were ignored, $darwin was never set, and failures exited with status 0. Platforms: - start.sh reads the Java version and architecture of its JVM. JDK 8 keeps the CMS options and JDK 17 uses the ZGC options from start.sh.simple, which is removed. ARM64 downloads the aarch64 jars and skips manifest rebuilding. - -s stops the node and the open file limit is raised to 65535, as in start.sh.simple. The default JVM_MX, used on macOS, is 12g. README and shell.md describe the single script.
Invalid node.dns.changeThreshold and node.dns.maxMergeSize now fail startup with TronError(PARAMETER_INIT); add tests for both.
Adds comments to the Java detection, the release download and signature check, the memory sizing, the manifest rebuild and the option handling of start.sh, describing what each branch does. JVM_MX drops from 12g to 9g; Linux derives the heap from the total memory, so the default applies to macOS only.
start.sh had several problems that predate this branch: - --stop matched every process whose command line contained the jar name and dropped any line containing "start", so it could kill other nodes and miss its own. The node id is now kept in <jar name>.pid; a directory with a start.log but no pid file was used by the old script and its node is still found by name. --stop also finds a node set up by --release or -cb from the parent directory, and says so when no node was started from the current one. - sh start.sh failed under dash; the script now reruns itself under bash. - an option without its value looped forever; needValue exits instead. - --disable-rewrite-manifest was only accepted as --disable-rewrite-manifes. - the first unknown option swallowed all following arguments; each unknown token is now passed on and parsing continues, with -- as an explicit end. - paths with spaces broke unquoted expansions; FullNode options are an array now. - --release downloaded straight onto FullNode.jar; it now uses a temporary file like --upgrade. - gc.log was archived before the node was stopped, and by --download. - a JDK that exists but cannot run went unnoticed, and a JVM that died right after the start was reported as running. - the manifest rebuild looked for the database next to the caller instead of the node directory, --net stored a relative config path, the gc log rotation deleted unrelated files, ulimit lowered a higher limit, and the rebuild message named logs/archive.log instead of logs/toolkit.log. shell.md documents the pid file, -- and the corrected option spelling.
| publishConfig.setChangeThreshold(dns.getChangeThreshold()); | ||
| } else if (Double.compare(dns.getChangeThreshold(), 0.0) != 0) { | ||
| logger.error("Check node.dns.changeThreshold, should be bigger than 0, default 0.1"); | ||
| throw new TronError("Check node.dns.changeThreshold, should be bigger than 0, default 0.1", |
There was a problem hiding this comment.
[NIT] Fail-fast on invalid dns parameters also fires when node.dns.publish=false
loadDnsPublishParameters now throws TronError(PARAMETER_INIT) on an invalid node.dns.changeThreshold / node.dns.maxMergeSize, and this validation runs unconditionally on the startup path. An existing node that carries an invalid value in its config but has node.dns.publish=false (i.e. never uses DNS publishing) previously only logged an error and will now fail to start at all after upgrading.
The error message is clear and the fix is a one-line config edit, so this is not blocking — but the behavior-change surface is wider than "validation for the DNS publish feature" suggests.
Suggestion: either (a) skip the validation when dns.isPublish() is false, or (b) keep the current behavior and explicitly disclose in the PR description / release notes that invalid dns parameters now fail startup even with publish disabled.
|
|
||
| // Once maxHttpConnectNumber is reached, open connections time out after this much | ||
| // inactivity, so connections that send nothing cannot hold every slot. | ||
| private static final long CONNECTION_LIMIT_IDLE_TIMEOUT_MS = 10_000; |
There was a problem hiding this comment.
[NIT] While saturated, the 10s idle timeout applies to all established connections, not just inactive ones
When maxHttpConnectNumber is reached, Jetty's ConnectionLimit.limit() applies this idle timeout to every connected endpoint, not only to connections that have sent nothing (verified against jetty-server 9.4.58 sources). During saturation, a keep-alive client with more than 10s between two requests on the same connection will be disconnected and must reconnect. Request processing time does not count as idle, so normal API calls are unaffected.
This is inherent to the upstream API and a reasonable price for freeing slots with a minimal change, but the PR description's "idle connections" phrasing reads narrower than the actual behavior.
Suggestion: add one sentence to the PR description or the http/shell documentation describing the saturated-state effect on keep-alive clients; introduce a config key for the timeout only if field evidence shows real clients being hurt.
|
|
||
| // Below the connector's default 30-second idle timeout, above the limit's 10-second one | ||
| // applied twice (Jetty half-closes an idle connection before closing it). | ||
| private static final int TIMEOUT_MS = 25_000; |
There was a problem hiding this comment.
[NIT] ConnectionLimitTest takes ~20s and runs with a tight timing margin
The test must really wait for the hardcoded CONNECTION_LIMIT_IDLE_TIMEOUT_MS = 10_000 in HttpService to fire, and Jetty closes an idled connection in two idle periods (half-close first, verified in jetty-io 9.4.58), so slot release takes ~20s. The test method therefore runs ~20.3s, well above the project's per-test performance baseline, and the client-side timeout of 25s (TIMEOUT_MS here) leaves only ~5s of margin against the ~20s expected close point — an occasional failure on a loaded CI machine is plausible (the @Test(timeout = 60_000) backstop turns it into a rerun, not a false diagnosis).
Suggestion: make the idle timeout injectable (e.g. a @VisibleForTesting setter or constructor parameter on the service/limit wiring) so the test can use a 1-2s timeout, finish within ~5s, and keep a comfortable wait margin. Directional improvement; need not block this PR.
|
|
||
| The script runs on x86_64 with JDK 8 and on ARM64 with JDK 17. It picks the JVM options for the Java version it finds, and downloads the release jars built for that architecture (`FullNode-aarch64.jar` on ARM64). | ||
|
|
||
| Downloaded release jars are verified against the GPG signature published with each release, made by the key listed under "Integrity Check" in the [README](./README.md), so `gpg` must be installed; a jar that fails verification is not used. The mainnet config is downloaded from java-tron and the Nile testnet config from [nile-testnet](https://github.com/tron-nile-testnet/nile-testnet). |
There was a problem hiding this comment.
[NIT] Documentation & metadata backlog (2 items rolled up)
Grouped as one comment since both are doc-or-metadata asks. This is a cross-cutting concern — the anchored line itself is fine.
-
Keyserver network dependency and trust-anchor boundary not fully documented (shell.md:13, README "Integrity Check"). The verification documented here fetches the release key fresh from keys.openpgp.org / keyserver.ubuntu.com on every download, so installing a release jar now requires outbound access to those keyservers, and fails closed when both are unreachable. Also worth one sentence on the trust-anchor boundary: the hardcoded fingerprint protects existing deployments against release-asset tampering, but cannot protect against a compromise of the GitHub repo that hosts both the script and the jars. Impact: operators troubleshooting "why won't it install" lack a documented path. Suggestion: add a sentence on the keyserver network requirement to shell.md, and optionally note the trust-anchor boundary in the README "Integrity Check" section.
-
Branch naming convention (cross-cutting, no code anchor). The head branch
fix_audit_issueshas nofeature//hotfix/style prefix and no/separator, departing from the documented convention in CONTRIBUTING.md. No action needed for this PR; please use a prefixed name for future branches.
Suggestion: one sentence in shell.md about the keyserver requirement (plus an optional README note), and a prefixed branch name next time.
4 participants
What does this PR do?
maxHttpConnectNumberis reached:HttpServicesetsConnectionLimit.setIdleTimeout(10_000).start.shdownloads: TLS certificates are checked, and release jars must carry a valid GPG signature from the release key. The mainnet config is fetched from java-tron and the Nile config from nile-testnet.start.shon both x86_64 (JDK 8) and ARM64 (JDK 17): it picks the JVM options for the Java version it finds and the release jars for the JVM's architecture.start.sh.simpleis removed.start.shbugs:--stoplooping forever, the node being restarted several times per run,-cbeing passed repeatedly, a hang on macOS whenJAVA_HOMEis not set (the script derivedJAVA_HOME=/usr, so the/usr/bin/javastub ran itself forever; it now uses/usr/libexec/java_home),--stopkilling any process whose command line contains the jar name while missing a node whose arguments contain "start",sh start.shfailing under dash, an option without its value looping forever,--disable-rewrite-manifestonly being accepted misspelled, the first unknown option swallowing all following arguments, paths with spaces,--releasedownloading straight ontoFullNode.jar,gc.logbeing archived before the node was stopped, a JDK that exists but cannot run going unnoticed, and a JVM that dies right after the start being reported as running.start.shand updateshell.md.node.dns.changeThresholdornode.dns.maxMergeSizeinstead of only logging an error.KhaosDatabase.getBranch(BlockId, BlockId), which has no callers, and an unused import inManager.Why are these changes required?
start.shdownloaded withwget --no-check-certificateand checked the jar against asha256sum.txtthat releases do not publish, so a downloaded jar was never verified.start.shonly worked on x86_64 / JDK 8, since the JDK 17 JVM rejects its CMS options; ARM64 users needed a separate template.start.sh --stopfound the node withps -ef | grep -v start | grep FullNode.jar, so it could stop another node with the same jar name, or none at all. The node id is now kept in<jar name>.pid; a directory with astart.logbut no pid file was used by the previous script and its node is still found by name.This PR has been tested by:
ConnectionLimitTestpasses (20.3 s) and fails without theHttpServicechange; newArgsTestcases for an invalidnode.dns.changeThreshold/node.dns.maxMergeSizefail without theArgschange;KhaosDatabaseTest,ManagerMockTest, HTTP / JSON-RPC service tests and checkstyle pass.start.shtest suite on macOS (bash 3.2), Ubuntu 22.04 (arm64 and amd64), Debian 11 and Rocky Linux 8 covers downloads with wget and curl, verification of the real release signatures,--release/--upgrade/--download/--net, failure paths, and the JVM command for every common option; the JDK 8 command is unchanged. With real JDKs,start.shstartsFullNode.jar --helpon macOS / JDK 8 and Linux amd64 / JDK 8, andFullNode-aarch64.jar --helpon Linux arm64 / JDK 17, both withJAVACMDpreset and with the script locating the JDK itself. The final version passes 196 checks on macOS, 112 with the real network (a downloadedFullNode-aarch64.jarmatches the GitHub digest; tampered, unsigned and unverifiable downloads are removed) and 39 in an Ubuntu 26.04 / JDK 17 container, includingsh start.shunder dash, two nodes with the same jar name in different directories, a node started by the previous script, and--stopfrom the parent of a--releasesetup.Follow up
None.
Extra details
start.shnow needsgpg; the release key is fetched by fingerprint from keys.openpgp.org or keyserver.ubuntu.com.start.sh --stopis run in the directory the node was started from, or in the parent of a--release/-cbsetup; elsewhere it reports that no node was started there and exits 1. A start takes 3 seconds longer, the time used to confirm that the JVM is still running.--ends the script options; everything after it goes to FullNode unchanged.JVM_MXinstart.shis 9g. It applies on macOS; on Linux the heap is still sized from the machine's memory or-mem.start.shnow works as documented: when a LevelDB database exists andArchiveManifest.jaris missing, the verified plugin is downloaded and run before start. By default it only rewrites manifests of 128 MB or more (-m); it is skipped on ARM64 and can be turned off with-dr.