Conversation
| if (!valid()) { | ||
| return "P2P_HELLO: invalid hello message"; | ||
| } |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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().
| if (peerHeadBlockId.getNum() <= headBlockNum | ||
| && peerHeadBlockId.getNum() >= chainBaseManager.getLowestBlockNum() | ||
| && !chainBaseManager.containBlockInMainChain(peerHeadBlockId)) { | ||
| logger.info("Peer {} head block is not in my main chain, peer->{}", |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
| if (!msg.valid()) { | ||
| logger.warn("Peer {} invalid hello message parameters, GenesisBlockId: {}, SolidBlockId: {}, " | ||
| + "HeadBlockId: {}, address: {}, sig: {}, codeVersion: {}", | ||
| logger.warn("Peer {} invalid hello message parameters, genesisHashLength: {}, " |
There was a problem hiding this comment.
[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 |
There was a problem hiding this comment.
[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.
|
Hello
در تاریخ شنبه ۳ اکتبر ۲۰۲۶، ۱۶:۳۳ 3for ***@***.***> نوشت:
… ***@***.**** commented on this pull request.
------------------------------
In
framework/src/main/java/org/tron/core/net/message/handshake/HelloMessage.java
<#6993 (comment)>
:
> return false;
}
return true;
}
+ public boolean validEndPoint() {
+ Endpoint from = this.helloMessage.getFrom();
+ ByteString ipv4 = from.getAddress();
+ ByteString ipv6 = from.getAddressIpv6();
+ if (from.getPort() <= 0 || from.getPort() > 0xFFFF
[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.
—
Reply to this email directly, view it on GitHub
<#6993?email_source=notifications&email_token=AQS3YGXPD7OGHXHUMGMNW235SD2LTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNBQGA4TCMJVG4ZKM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#pullrequestreview-5400911572>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQS3YGTTBDSTKGB6LB6PU4D5SD2LTAVCNFSNUABFKJSXA33TNF2G64TZHMYTCNJUGEYTQMRWHNEXG43VMU5TKNJTG4ZTSNJRGI3KC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AQS3YGTHDDKX3BEPKDRYSG35SD2LTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNBQGA4TCMJVG4ZKM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ>
and Android
<https://github.com/notifications/mobile/android/AQS3YGVTVF3IXQBIH4MEHDL5SD2LTA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKNBQGA4TCMJVG4ZKM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
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.
BAD_PROTOCOLbefore parsing or PBFT dispatch, and reject null or empty frames safely.TIME_OUTdisconnect, while retaining synchronization and request timeout checks. MakehelloMessageReceivevolatile for visibility to the status-check thread.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 beforetoString()constructs aNode; preserve the log format for valid HELLO messages.endpointValidflag instead of full hashes. Read diagnostic lengths withByteString.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:
HandShakeServiceTest; HELLO timeout scenarios consolidated intoPeerStatusCheckMockTest. The latest timeout consolidation passed 24 related tests.HelloMessageTest.testValidAddressLogFormatverified on an actual Java 8 runtime after fixing its JDK-dependent IPv6 formatting expectation../gradlew checkstyleMain checkstyleTestandgit diff --checkpassed 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