fix(net): robustness fixes for multiple issues

[xxo1shine]

What does this PR do?
Validate inbound HELLO messages before logging or handshake processing, and tolerate optional node identity data. Handle concurrent sync queue changes, missing connection failure causes, and accurate transaction diagnostics. Correct the default QPS parameter type so fallback configuration remains usable.

Fixes #6969

Why are these changes required?

This PR has been tested by:

• Unit Tests
• Manual Testing

Follow up

Extra details

[waynercheung]

The null-cause guard and the transaction-ID fix in the error message look good.

Requesting changes until the HELLO rejection-policy change is resolved, and the missing-from and QPS initialization policies are clarified and covered by regression tests.

Other points:

• [SHOULD] Keep bounded HELLO rejection details, such as the failing field and the observed length. The generic "invalid hello message" loses the field-level diagnostics the old log had; logging the full payload or hashes is not needed.
• [SHOULD] Document and test the final logging policy for all three exception branches. #6969 gives a rationale for logging handled BAD_MESSAGE errors at WARN, but the non-P2pException branch still logs at WARN, although #6969 says unexpected exceptions will be logged at ERROR. Please fix this mismatch and test the log level, whether the stack trace is logged, and the disconnect reason.
  [SHOULD] Add a defensive callback test for EffectiveCheckService: a mocked future that is not cancelled, not successful, and whose cause() is null. Verify that the channel is closed, cur is cleared and the next attempt is triggered. This shows the defensive behavior works; it does not show that a real completed Netty failure callback can reach this state.
• [SHOULD] Update the PR description with each item's observable behavior and test evidence, as agreed in #6969. The "Why" section is empty, and the BAD_MESSAGE severity change is not described.

[halibobo1205]

+1 to @waynercheung's review. A few additions:

1. [SHOULD] #6993 (same milestone) also validates the HELLO, with the opposite choices: a 60 s vs 1 h ban for bad fields (#6993 vs this PR), and from required vs optional. The two PRs conflict in HelloMessage.java / HandshakeService.java. Please decide which PR owns HELLO validation before either merges. cc @317787106
2. [SHOULD] Iwith the CCE fixed, an explicitly configured HTTP limiter with a broken qps goes from failing startup to silently falling back. Per the #6969 acceptance criteria, fail vs fallback should be decided explicitly and tested for both HTTP and RPC. If fallback is the choice, please log at least one WARN at init (including mistyped keys such as QPS=5).
3. [SHOULD] Tests: +1 to the disconnect-reason and ban-duration tests @waynercheung asked for, and to the deterministic interleaving test committed in #6969.
4. [SHOULD] #6969 asked to handle dropping the stack trace separately from lowering the level, and BAD_MESSAGE was deliberately raised to ERROR in ab07d2f. I'd keep ERROR and only drop the stack trace, or explain the downgrade in the description.
5. Please change Fixes #6969 to Part of #6969, since the acceptance criteria for items 3, 5 and 6 aren't met yet, and re-run CI on the current base before merging (the last run is from 09-21).

[xxo1shine]

1. There is overlap between the HELLO validation in #6993 and this PR. I prefer to resolve the conflicts once the merge order is determined, rather than change this PR's implementation for that reason now.

2. The QPS change only corrects the default value's type so that the existing fallback remains usable. It does not expand the scope to unify HTTP/RPC configuration-error policies. Initialization warnings can be handled separately.

3. The current tests cover the main behavior of these changes. Additional ban-duration and concurrency-interleaving tests can be added later; I prefer not to expand this PR's test scope further.

4. Using WARN for BAD_MESSAGE follows the suggested example in #6969. The intent is both to remove the stack trace and to reduce logging noise for handled protocol errors. I prefer to retain this choice.

5. This PR makes changes addressing each of the six reported issues. However, implementation coverage and completion of all acceptance criteria are separate questions; the points above alone do not establish that retaining "Fixes" is appropriate. CI should also be rerun on the current base before merging.

[waynercheung]

I re-reviewed d910f020, which is unchanged from the previous review.

The remaining merge blocker is the HELLO contract shared with #6993: rejection policy, optional from, validation ownership and bounded diagnostics. Resolving textual conflicts alone will not reconcile these behaviors. I suggest letting #6993 own HELLO validation and admission, while retaining the connection-callback, transaction-diagnostic, sync-queue, QPS and exception-logging changes here.

Other points:

• [SHOULD] QPS: Strategy.parseStrategyParams() supports the explanation that this fixes a broken default-value fallback. Please document that rationale and add HTTP/RPC initialization tests asserting the installed adapter and effective rate for valid, empty, missing and invalid parameters. For explicitly invalid or unrecognized configuration such as QPS=5, a contextual initialization warning would also help operators detect unintended fallback.
• [SHOULD] Sync queue (non-blocking): the mock covers the two failure windows described in the reply. Please also assert fetchAble restoration and the intended sync scheduling. A small stateful test covering the common-block boundary and final block order would strengthen the regression coverage.
• [SHOULD] The non-P2pException branch still logs at WARN despite the ERROR policy stated in #6969 for unexpected exceptions. Please implement or explicitly revise that policy, document the final logging behavior, and test the level, stack-trace handling and disconnect reason.
• [SHOULD] Please describe this as Part of #6969, or explicitly list the remaining acceptance criteria and follow-up PRs. The umbrella issue should remain open until the agreed validation is complete.
• [SHOULD] Complete the PR description and rerun CI against the current target branch. The existing checks are from September 21; the target branch has since advanced, including #6992, which moved libp2p into the repository as the p2p module.

Validation: I applied this PR onto release_v4.8.3 at a901f85a in an isolated copy. It merges without textual conflicts. All 61 tests across the five affected test classes and two HTTP/RPC limiter test classes passed on JDK 17 / aarch64. Framework main/test Checkstyle also passed. This does not cover the combined result with #6993 or the full platform matrix.

I recommend keeping Request changes until the HELLO policies are reconciled and the corresponding regression tests and PR description are completed.

[xxo1shine]

Regarding HELLO, I have explained the current design choices in the corresponding threads: validation during message construction prevents invalid messages from reaching log formatting and subsequent handshake processing; validation failures result in BAD_PROTOCOL and a one-hour IP ban; and a missing from is allowed as a transitional compatibility policy, with the associated behavior change of skipping the corresponding node-ID update checks acknowledged and accepted. I would prefer to retain these choices. The behavioral differences with #6993 need to be coordinated, but they do not mean this PR must remove its HELLO changes or adopt the other PR's policies.

The QPS change fixes the mismatch between the actual type of the default value and its declared type, restoring the existing default-value fallback logic. The sync-queue test targets the two concurrency windows addressed by this fix. More comprehensive initialization, scheduling, and queue-state tests could improve coverage, but I would prefer not to expand the implementation or test scope further in this PR.

Regarding logging, the current description of #6969 calls for concise logging without stack traces for handled P2P exceptions, and this PR implements that adjustment. Non-P2pException exceptions retain the existing WARN level and full stack traces. I do not see a requirement in the issue's current description to change these to ERROR, so I do not plan to make that change here.

[waynercheung]

LGTM at d910f020. I accept validation during HELLO construction, the intentional one-hour ban, optional from, and the restored QPS fallback. I also accept WARN without stack traces for handled P2pExceptions, and WARN with stack traces for unexpected exceptions.

A few things to take care of before merging:

• The PR has merge conflicts with release_v4.8.3, which now includes #7034. When resolving, please preserve its identity protection: if from is present, compare its node ID with the established channel identity instead of updating it; if absent, keep the identity from the libp2p handshake; and keep rejecting application messages before that handshake completes. Please request another review of the resolved handshake code before merging, since this touches peer identity.
• Please rerun CI on the updated branch. The current checks are from September 21 and do not cover integration with the current target branch.
• Please complete the PR description with the rationale and the observable behavior changes: the one-hour ban and its compatibility impact, optional from, exception logging, and the HTTP QPS fallback. Also use Part of #6969 instead of Fixes, since the umbrella issue still has follow-ups.

Possible follow-ups: node-ID-only validation of from, and bounded raw endpoint fields or a smaller HELLO size cap.

[317787106]

@waynercheung @halibobo1205 To avoid conflicts with changes to HelloMessage of this PR, PR #6993 drops its newly added Endpoint validation. The remaining handshake gating, HELLO timeout, and bounded invalid-HELLO logging changes are retained.