Skip to content

fix(net): gate peer traffic on hello validation and enforce timeout - #6993

Open
317787106 wants to merge 8 commits into
tronprotocol:release_v4.8.3from
317787106:fix/hello_check
Open

317787106 wants to merge 8 commits into
tronprotocol:release_v4.8.3from
317787106:fix/hello_check

Conversation

@317787106

@317787106 317787106 commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Require a validated application-layer HELLO before processing peer traffic, enforce a handshake deadline, and validate untrusted HELLO fields before formatting logs.

  • Before HELLO validation succeeds, allow only HELLO and DISCONNECT. Reject other message types with BAD_PROTOCOL before parsing or PBFT dispatch, and reject null or empty frames safely.
  • Add a 10-second HELLO timeout based on the local channel start time, independent of synchronization flags or recent peer activity. Use the existing status-check cycle and TIME_OUT disconnect, while retaining synchronization and request timeout checks. Make helloMessageReceive volatile for visibility to the status-check thread.
  • Extract validEndPoint() to validate the raw protobuf endpoint: port 1–65535, at least one IP address, a 200-byte limit per address field, and valid IPv4/IPv6 literals for every nonempty field. Run validation before toString() constructs a Node; preserve the log format for valid HELLO messages.
  • For invalid HELLO messages, log the three block-hash lengths and an endpointValid flag instead of full hashes. Read diagnostic lengths with ByteString.size() to avoid copying untrusted fields for logging.

Why are these changes required?

Peers could reach business handlers, including PBFT, before completing the application handshake, while handshake timeout depended indirectly on synchronization state. Malformed endpoint fields could also reach address formatting before validation, and oversized hashes could produce excessive warning output.

Handshake admission retains the existing version, genesis, and solid-block compatibility checks without adding a main-chain membership requirement for the peer's unfinalized head. This avoids rejecting otherwise compatible peers during a temporary fork, including reconnection after a disconnect or restart. Subsequent fork handling remains with the existing synchronization and broadcast logic.

This PR has been tested by:

  • Focused unit tests and regressions on ARM64 / JDK 17 covering message gating, endpoint validation, bounded logging, handshake compatibility, peer connections, and timeouts.
  • Logging scenarios consolidated into HandShakeServiceTest; HELLO timeout scenarios consolidated into PeerStatusCheckMockTest. The latest timeout consolidation passed 24 related tests.
  • HelloMessageTest.testValidAddressLogFormat verified on an actual Java 8 runtime after fixing its JDK-dependent IPv6 formatting expectation.
  • ./gradlew checkstyleMain checkstyleTest and git diff --check passed locally.

The full repository test suite and manual multi-node network testing were not run locally.

Follow up

The planned libp2p v2.3.0 fixes will be integrated into java-tron separately. This PR covers java-tron's application-layer HELLO handling and does not update the libp2p dependency.

Extra details

  • Ten seconds is the timeout threshold; disconnection occurs on the existing periodic status-check cycle.
  • No changes to the wire format, configuration, or fork-choice logic are introduced. The implementation remains Java 8 compatible.

@github-actions
github-actions Bot requested a review from xxo1shine September 22, 2026 08:57
Comment on lines +124 to +126
if (!valid()) {
return "P2P_HELLO: invalid hello message";
}

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.

[SHOULD] HELLO logging can still trigger synchronous DNS resolution, so the valid() guard is incomplete. valid() does not validate from.address, meaning a HELLO message that passes the length checks can still reach getFrom().getPreferInetSocketAddress() at line 131. When the local p2pConfig.ip is non-empty and net INFO logging is enabled, a hostname supplied by the remote peer can trigger synchronous resolution through InetSocketAddress.

This logging occurs before processHelloMessage validation. The default AsyncAppender also formats the message before enqueueing it, so DNS resolution latency can block the current Netty EventLoop and other connections running on it.

Please make toString() output only bounded fields that do not require resolution, and add literal validation for a non-empty IP field as well as port validation.

This is an incomplete fix for a pre-existing path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 31861e5. valid() now validates the raw endpoint’s address lengths, IP literals, and port range before toString() calls getFrom(). This prevents peer-supplied hostnames from triggering DNS lookups during logging while preserving the existing log format.

Added regression tests for invalid endpoints, boundary values, and the AsyncAppender path, including verification that rejected endpoints never reach getFrom().

Comment on lines +115 to +118
if (peerHeadBlockId.getNum() <= headBlockNum
&& peerHeadBlockId.getNum() >= chainBaseManager.getLowestBlockNum()
&& !chainBaseManager.containBlockInMainChain(peerHeadBlockId)) {
logger.info("Peer {} head block is not in my main chain, peer->{}",

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.

[SHOULD] A handshake should not be rejected simply because an unfinalized head is not on the local main chain. Two healthy nodes may share the same solid block while having different unfinalized heads, such as A100 and B100. Manager keeps competing branches at the same height in KhaosDB, while containBlockInMainChain() only checks BlockStore. As a result, even if B100 is already cached locally, this new check still returns FORKED and triggers the default 60-second ban path, preventing the peers from exchanging subsequent blocks.

This is a newly introduced regression. Please restrict chain-incompatibility checks to the finalized range and add a handshake test covering a normal short-lived fork. Simply adding a KhaosDB lookup would still not cover a legitimate fork head that has not yet been received locally.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in efcb9b3. Removed the newly added head-membership check, so peers are no longer rejected solely because their unfinalized head is absent from the local main chain. This preserves the existing sync and broadcast paths for resolving transient forks.

For nodes restarting after a transient fork, the advertised head comes from the restored database/checkpoint state and may differ from the pre-restart in-memory head. Removing this check avoids rejecting an otherwise compatible reconnecting peer solely because its recovered head is absent from the local main chain. Subsequent recovery still follows the existing synchronization, block-validation, and rollback rules; this change does not guarantee immediate convergence.

Updated regression tests to cover differing heads with compatible solid blocks, including cached and unknown fork heads. Existing genesis, version, and solid-block compatibility checks remain in place.

Comment on lines 55 to 57

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.

[SHOULD] Log amplification from invalid hello messages. When valid() fails, all three hashes are written to the WARN log using toHexString() without any length limit. With a maximum frame size of 5 MB, a single invalid hello can generate roughly 10 MB of log output.

Please consider truncating these values or logging only their lengths.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 639130f. The invalid-HELLO warning now logs only the three hash lengths using ByteString.size(), avoiding unbounded hex conversion and byte-array copies for logging. An endpointValid field also identifies endpoint-validation failures.

Added regression coverage in HandShakeServiceTest for oversized hashes and invalid endpoints, verifying bounded warning output and the existing INCOMPATIBLE_PROTOCOL disconnect behavior.

Comment on lines 49 to +50
if (!msg.valid()) {
logger.warn("Peer {} invalid hello message parameters, GenesisBlockId: {}, SolidBlockId: {}, "
+ "HeadBlockId: {}, address: {}, sig: {}, codeVersion: {}",
logger.warn("Peer {} invalid hello message parameters, genesisHashLength: {}, "

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.

[SHOULD] Move the entire msg.valid() check before getFrom() / updateNodeId(). updateNodeId() can close the later-established connection with the same nodeId, so an invalid HELLO may affect other peers before being rejected.

Add a regression test to ensure an invalid HELLO does not call getFrom() / updateNodeId() or close other connections.

Endpoint from = this.helloMessage.getFrom();
ByteString ipv4 = from.getAddress();
ByteString ipv6 = from.getAddressIpv6();
if (from.getPort() <= 0 || from.getPort() > 0xFFFF

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.

[SHOULD] Add a rejection condition in validEndPoint() for from.getNodeId().size() != Constant.NODE_ID_LEN. Currently, empty and non-standard-length nodeIds can pass endpoint validation and then participate in connection deduplication.

Use the protocol constant and cover the 0, 63, 64, and 65-byte boundaries. Existing valid HELLO test data should also use a 64-byte nodeId.

@nak23fr7

nak23fr7 commented Oct 3, 2026 via email

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants