Robustness fixes for multiple issues

Summary

This issue consolidates six robustness problems:

The connection-failure callback in EffectiveCheckService dereferences future.cause() without a null check, potentially interrupting subsequent cleanup.
HELLO messages are fully rendered and logged before valid() validation.
The default parameter in QpsStrategy has a runtime type inconsistent with its declared Double.class type, potentially causing a ClassCastException and configuration fallback.
The unrequested-transaction error in TransactionsMsgHandler records the hash of the entire transaction message instead of the current transaction ID.
The compound isEmpty() / peekLast() operation on syncBlockToFetch has a concurrency window that may cause a null dereference.
P2P exception logging prints a full stack trace for BAD_MESSAGE, while other handled P2P exceptions log only the exception message, resulting in inconsistent and unnecessarily verbose logging.

Root Cause

1. Missing null check for the failure cause in EffectiveCheckService

} else if (!future.isSuccess()) {
  logger.warn("Connect to chosen peer {} fail, cause:{}",
      cur, future.cause().getMessage());
  future.channel().close();
  cur = null;
  triggerNext();
}

If future.cause() is null in this branch, evaluating future.cause().getMessage() throws an NPE. The subsequent operations that close the channel, reset cur, and advance peer selection are skipped.

Actual reachability requires checking listener invocation timing, cancellation semantics, and other cleanup paths. The fact that the cause() API permits null does not by itself establish that this state occurs in the failure callback.

2. HELLO messages are logged before validation

P2pEventHandlerImpl.processMessage() logs messages before dispatching them:

if (PeerConnection.needToLog(msg)) {
  logger.info("Receive message from peer: {}, {}",
      peer.getInetSocketAddress(), msg);
}

HELLO validation through valid() occurs later, inside handshakeService.processHelloMessage(). Consequently, peer-controlled fields that fail validation may already have been rendered by toString() and written to logs, causing unnecessary formatting and logging overhead.

3. QpsStrategy default parameter type mismatch

The default QPS value is stored as an int in a ParamItem declared as Double.class. This creates a mismatch between the declared type and the actual boxed type, Integer.

A subsequent direct cast to Double may throw ClassCastException. If the surrounding code catches the exception and falls back to a default strategy, custom endpoint rate-limit settings may not take effect as intended.

4. The unrequested-transaction error uses the batch message ID

if (!peer.getAdvInvRequest().containsKey(item)) {
  throw new P2pException(TypeEnum.BAD_MESSAGE,
      "trx: " + msg.getMessageId() + " without request.");
}

Here, msg is a TransactionsMessage. Its getMessageId() returns the hash of the entire message payload rather than the ID of the current transaction in the loop. The current transaction ID is already available in the local variable id.

5. A concurrency window exists between checking and reading syncBlockToFetch

Calling isEmpty() and then dereferencing the result of peekLast() does not guarantee that the shared queue remains nonempty between those operations.

If another thread clears the queue or removes its final element during that interval, peekLast() may return null, causing an NPE.

This is another instance of the access pattern addressed by fix 8b4a5dddf3, but this location appears not to have been covered. The specific remaining location should be checked against that fix during implementation.

6. Inconsistent stack-trace logging for P2P exceptions

if (type.equals(P2pException.TypeEnum.BAD_MESSAGE)) {
  logger.error("Message from {} process failed, {} \n type: ({})",
      peer.getInetSocketAddress(), msg, type, ex);
} else {
  logger.warn("Message from {} process failed, {} \n type: ({}), detail: {}",
      peer.getInetSocketAddress(), msg, type, ex.getMessage());
}

The BAD_MESSAGE branch passes ex as the trailing throwable argument, causing the logger to print its full stack trace. Other P2P exception types log only ex.getMessage(). Repeated malformed messages can therefore generate unnecessary stack-trace output.

Impact

Connection management: Interrupted cleanup may affect effective-peer selection and connection resource release. Whether the interruption persists depends on other recovery paths.
HELLO logging: Invalid input still incurs rendering and logging costs.
Endpoint rate limiting: Configuration may not take effect as intended, and fallback behavior may conceal configuration errors.
Transaction diagnostics: Reporting the wrong hash makes log investigation and transaction tracing harder without changing transaction validation.
Sync processing: A concurrent NPE may interrupt the current sync-processing flow. Its scope depends on exception handling and retry behavior.
P2P logging: Full stack traces for handled malformed-message errors increase log volume and operational noise.

The current descriptions do not establish consensus divergence or financial impact.

Suggested Fix

Null-check future.cause() and ensure that diagnostic logging failures cannot prevent necessary cleanup. Also check exception isolation between cleanup operations and whether repeated cleanup is safe.
Validate HELLO fields before fully rendering the message, or log only a bounded summary.
Store the default QPS value as a Double, keeping the declared, stored, and read types consistent. Log configuration parsing failures explicitly.
Use the current transaction ID in the error message:

"trx: " + id + " without request."

Read the queue's final element once and check the returned value instead of relying on an earlier isEmpty() result:

BlockId last = syncBlockToFetch.peekLast();
if (last != null) {
  // Use the value read into last.
}

If subsequent operations require the queue state to remain unchanged, use a consistent synchronization mechanism as well.

Use concise logging without a throwable argument for handled P2P exceptions, including BAD_MESSAGE:

logger.warn("Message from {} process failed, {} \n type: ({}), detail: {}",
    peer.getInetSocketAddress(), msg, type, ex.getMessage());

Thanks for consolidating these findings. I confirmed that the referenced code patterns exist, but the scope and verification plan should be tightened before implementation.

For EffectiveCheckService, please provide a reproducible path where a non-cancelled, unsuccessful ChannelFuture has a null cause. A listener is normally invoked after completion, so this may be defensive hardening rather than a reachable defect. Cleanup should be protected with finally regardless.
HELLO validation should happen before full message rendering and, ideally, before updateNodeId or any other peer-state mutation. Please add tests for invalid and boundary-sized fields.
The QpsStrategy type mismatch is valid, but its impact should be described precisely. A valid QPS parameter is converted to Double; the mismatch appears when empty or malformed input leaves the integer default in place. HTTP and RPC initialization also appear to handle this failure differently, so both paths need tests.
The transaction diagnostic fix is straightforward. Please test that the exception contains the current transaction ID rather than the batch message ID.
Reading peekLast() once and null-checking it prevents the immediate NPE, but it does not provide a stable queue state. Please confirm whether snapshot semantics are sufficient or whether synchronization is required, and add a concurrency regression test.
Removing the throwable argument and changing the severity from ERROR to WARN are separate behavioral changes. The stack trace can be suppressed while retaining ERROR; any severity downgrade should be justified independently.

These items affect unrelated subsystems, so this issue can remain the umbrella tracker, but the fixes should be submitted as separate PRs—or at minimum clearly separated commits—with focused tests and an explicit observable-behavior statement for each item. Once items 1 and 3 are clarified, the implementation can proceed item by item.

@lxcmyf

@lxcmyf, these distinctions are valid. I will first tighten the issue description: item 1 does not yet establish a reachable case where an unsuccessful, non-cancelled ChannelFuture has a null cause(), so I will describe the null handling as defensive hardening and ensure diagnostic logging cannot interrupt cleanup. For item 3, I will clarify that a valid QPS parameter is converted to Double; the type mismatch occurs when empty or malformed input leaves the integer default in place. I will test the HTTP and RPC initialization paths separately.

For implementation, item 2 will verify that HELLO validation precedes updateNodeId and other peer-state changes, with invalid and boundary-sized fields covered by tests. Item 4 will test that the exception contains the current transaction ID. For item 5, I will determine whether subsequent processing only needs the value read once or requires a stable queue state, then add a concurrency regression test and synchronize where needed. For item 6, BAD_MESSAGE is a recognized, handled P2P exception, so I will log it at WARN consistently with other handled P2P exceptions and omit the full stack trace; unexpected or unhandled exceptions will remain at ERROR. I will split the six items into focused PRs by subsystem, each stating its observable behavior and test results.

Thanks, this plan addresses my concerns. Please update the issue description accordingly and link each focused PR back to this issue when it is opened, including its observable-behavior statement and test results.

@lxcmyf

@lxcmyf, I will update the issue description to reflect our discussion: clarify the reachability limitation for item 1, the conditions that trigger the type mismatch in item 3, and the rationale for logging handled P2P exceptions at WARN in item 6. Each focused PR will link back to this issue and state its observable behavior and test results.

These acceptance criteria look reasonable. I would make a few implementation details explicit:

Item 3: Empty input, missing qps, and invalid values such as qps=abc or qps=0 all reach the boxed-integer default; the invalid cases do so through the reset in parseStrategyParams(). HTTP currently fails initialization with TronError(RATE_LIMITER_INIT), following the fatal initialization policy introduced in c9b7fbc47e, while RPC retains the DefaultBaseQqsAdapter installed in its first initialization loop.

Correcting the default type therefore changes these HTTP cases from startup failure to successful fallback. That behavior should be an explicit decision. The invalid-value cases already emit a generic parsing warning; empty or missing parameters may not. Please test the installed adapter and effective rate for valid, empty, missing, and invalid inputs, and distinguish unknown-adapter errors from construction failures in the RPC warning.

Item 2: Agreed on covering rejection-path logging. Currently, when valid() returns false, processHelloMessage() disconnects and returns; it does not normally route that rejection through processException(). The existing logging costs are the pre-validation INFO rendering and the full hash encoding in the validation-failure log.

"No full rendering of an invalid HELLO on any rejection path" would capture the intended behavior, including future error-routing changes. Validation should also precede updateNodeId, with tests verifying that invalid messages do not reach that state update.

Item 5: The remaining window is the tail-trimming loop in ChainInventoryMsgHandler.processMessage(), before the blockLock section. Concurrent removal in processSyncBlock(), or clearing in startSync() / PeerConnection.onDisconnect(), can invalidate the earlier nonempty observation. Commit 8b4a5ddd addressed a different peek()/pop() sequence.

Please use a deterministic interleaving test and assert correct trimming and block ordering around the common-block boundary. If locking is chosen, the relevant removal and clearing paths must participate in the same synchronization.

Item 6: The current behavior is BAD_MESSAGE: ERROR with a throwable; other P2pException: WARN without a throwable; non-P2pException: WARN with a throwable. Commit ab07d2fe2b explicitly changed BAD_MESSAGE from WARN to ERROR, so lowering it should have a stated rationale. Also, keeping unexpected exceptions at ERROR would actually require changing the current non-P2pException branch.

Please specify the intended severity, throwable handling, and rendering bounds for all three branches, while preserving the existing disconnect behavior.

Agreed that item 1 remains defensive hardening and the transaction-ID correction can proceed independently.

@xxo1shine

Thanks for tracing the existing behavior and the relevant history. I agree that these details should become explicit acceptance criteria.

For item 3, correcting the boxed default type must not silently change the HTTP fatal-initialization policy as an unintended side effect. Whether invalid, empty, or missing QPS parameters should fail startup or fall back must be decided and tested explicitly for HTTP and RPC.

For item 6, suppressing the stack trace should be separated from changing the severity. Given that BAD_MESSAGE was deliberately changed to ERROR by ab07d2fe2b, my preference is to retain ERROR unless a separate operational rationale supports lowering it. The current non-P2pException branch should also be documented accurately and considered separately.

@xxo1shine, please incorporate these clarifications—together with the deterministic interleaving requirement for item 5—into the issue description and the focused PR acceptance criteria before implementation.

@waynercheung

@waynercheung, thank you for your comments. For item 2, invalid HELLO messages will not be fully rendered on any rejection path, and validation will occur before updateNodeId. For item 3, the tests will cover valid, empty, missing, and invalid QPS parameters, and verify the adapter installed by HTTP/RPC and its effective rate so that correcting the type does not unintentionally change initialization behavior. For item 5, a deterministic interleaving test will cover the common-block boundary, and the relevant removal and clearing paths will use a consistent synchronization mechanism. For item 6, the severity, throwable handling, and rendering bounds will be defined separately for all three exception branches. BAD_MESSAGE is an expected remote protocol error and will be logged at WARN without a full stack trace; unexpected exceptions will be logged at ERROR. The existing disconnect behavior will remain unchanged.

@lxcmyf

@lxcmyf, thank you for the additional clarification. For item 3, the tests will cover valid, empty, missing, and invalid QPS parameters and verify the HTTP/RPC initialization result, installed adapter, and effective rate so that correcting the type does not unintentionally change the existing policy. For item 5, a deterministic interleaving test will cover queue trimming and block ordering around the common-block boundary. For item 6, the three exception branches will be handled separately. BAD_MESSAGE is an expected remote protocol error for which the peer is already disconnected; logging it at ERROR with a full stack trace creates unnecessary operational noise, so it will be logged at WARN without the stack trace. Unexpected exceptions will be logged at ERROR, while the existing disconnect behavior will remain unchanged.

Navigation Menu

Repository navigation

Robustness fixes for multiple issues #6969

Summary

Root Cause

Impact

Suggested Fix

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Robustness fixes for multiple issues #6969

Description

Summary

Root Cause

Impact

Suggested Fix

Activity

lxcmyf commented on Sep 14, 2026

xxo1shine commented on Sep 14, 2026

lxcmyf commented on Sep 14, 2026

xxo1shine commented on Sep 14, 2026

waynercheung commented on Sep 14, 2026

lxcmyf commented on Sep 15, 2026

xxo1shine commented on Sep 16, 2026

xxo1shine commented on Sep 16, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions