Repository navigation
Conversation
Add HTTP and Unix-domain-socket transports for the shared Admin JSON-RPC API, with an attach console for trusted node operators. Include transport configuration, socket permissions, request handling, bounded client lifecycle, and startup isolation regression tests.
…ken count restrict
Add peer management to Admin HTTP and IPC, including active-peer inspection, persistent IP blocking, and optional IPC text output. Implement the management APIs and TCP connection policy in the built-in p2p module. Retain the standalone blocked-ips option and consolidate its tests in StartAppArgsTest. Base the change on feature/admin_rpc with java-tron 4.8.3.
# Conflicts: # common/src/main/resources/reference.conf # framework/build.gradle # framework/src/main/java/org/tron/core/config/args/Args.java # framework/src/main/java/org/tron/core/config/args/CommandLineArguments.java # framework/src/main/java/org/tron/core/services/admin/AdminJsonRpc.java # framework/src/main/java/org/tron/core/services/admin/AdminJsonRpcImpl.java # framework/src/main/java/org/tron/core/services/admin/AdminJsonRpcRequestHandler.java # framework/src/main/java/org/tron/core/services/admin/http/AdminRpcHttpService.java # framework/src/main/java/org/tron/core/services/admin/http/AdminRpcServlet.java # framework/src/main/java/org/tron/core/services/admin/http/VirtualHostValidator.java # framework/src/main/java/org/tron/core/services/admin/ipc/client/IpcClient.java # framework/src/main/java/org/tron/core/services/admin/ipc/client/IpcConsoleCommands.java # framework/src/main/java/org/tron/core/services/admin/ipc/client/IpcResponse.java # framework/src/main/java/org/tron/core/services/admin/ipc/server/IpcRequestHandler.java # framework/src/main/java/org/tron/core/services/admin/ipc/server/IpcSocketFiles.java # framework/src/main/java/org/tron/core/services/jsonrpc/JsonRpcServlet.java # framework/src/main/java/org/tron/program/FullNode.java # framework/src/test/java/org/tron/core/services/admin/http/AdminRpcServletTest.java # framework/src/test/java/org/tron/core/services/admin/ipc/client/IpcClientTest.java # framework/src/test/java/org/tron/core/services/admin/ipc/client/IpcConsoleCommandsTest.java # framework/src/test/java/org/tron/core/services/admin/ipc/client/IpcResponseTest.java # framework/src/test/java/org/tron/core/services/admin/ipc/server/AdminJsonRpcRequestTest.java # framework/src/test/java/org/tron/core/services/admin/ipc/server/IpcRequestHandlerTest.java # framework/src/test/java/org/tron/core/services/admin/ipc/server/IpcServiceTest.java # framework/src/test/java/org/tron/core/services/admin/ipc/server/IpcSocketFilesTest.java # framework/src/test/java/org/tron/program/FullNodeTest.java # gradle/verification-metadata.xml
…ating request for two jsonrpc services
# Conflicts: # p2p/src/main/java/org/tron/p2p/P2pService.java # p2p/src/main/java/org/tron/p2p/connection/business/pool/ConnPoolService.java # p2p/src/main/java/org/tron/p2p/utils/NetUtil.java # p2p/src/test/java/org/tron/p2p/connection/ChannelManagerTest.java # p2p/src/test/java/org/tron/p2p/connection/ConnPoolServiceTest.java
lxcmyf
left a comment
There was a problem hiding this comment.
Three blocking correctness and security issues are noted inline.
| InetAddress address = parseIp(ip); | ||
| synchronized (managementLock) { | ||
| P2pService p2pService = requireP2pService(); | ||
| if (blockedIps.containsKey(address)) { |
There was a problem hiding this comment.
[MUST] This no-op assumes the framework snapshot matches the live P2P policy, but a persistence failure intentionally leaves them divergent. If admin_unblockIp applies the live unblock and the store write fails, a later admin_blockIp returns success here without restoring the block, so the query reports this IP as blocked while TCP connections are accepted. Reconcile the live policy on both no-op paths, or roll it back when persistence fails.
There was a problem hiding this comment.
Thanks for raising this. The scenario assumes the node continues normal operation after CommonStore.put fails. For the underlying disk failure considered here, the node can no longer persist blocks and requires operator intervention; subsequent Admin mutations are not a supported recovery path. We intentionally apply the P2P policy first so a block takes effect immediately, and we surface and log any persistence failure. Given this failure model, we’ll keep the current ordering.
There was a problem hiding this comment.
[MUST] This failure model is not enforced: a CommonStore.put failure is returned as a normal JSON-RPC error, and the node continues accepting Admin calls. After a failed unblockIp(), the framework snapshot still contains the IP while the live P2P policy does not, so the next blockIp() reports a successful no-op without restoring the block. Please reconcile the live snapshot on both no-op paths, or make peer-management mutations unavailable after persistence fails, and cover both cross-operation sequences with regression tests.
There was a problem hiding this comment.
Thanks for pointing this out. We now validate the IP and persist the blocked-IP snapshot before updating the P2P policy in the latest commit 32d7bdf . If the CommonStore write fails, the runtime policy and framework snapshot remain unchanged, avoiding the no-op sequence described here. The database write and in-memory update cannot be atomic, so an exception during the subsequent P2P update can still leave them inconsistent. This change narrows the failure case but does not claim full atomicity.
| @Override | ||
| public PeerOperationResult addPeer(String endpoint) | ||
| throws JsonRpcInvalidParamsException, JsonRpcInternalException { | ||
| logger.info("Admin write attempt method=admin_addPeer target={}", safeTarget(endpoint)); |
There was a problem hiding this comment.
[MUST] Attempt-only method logging omits the transport, available caller context, and final outcome, so binding errors, rejected requests, and business failures (success: false) are not audited. Add auditing in the shared HTTP/IPC invocation path and record transport, method, caller context, and outcome for every mutating request, including notifications, without logging raw parameters.
There was a problem hiding this comment.
Thanks for the review. I agree the current logs are operational logs, not a complete audit trail. Mutating calls that reach AdminJsonRpcImpl log an attempt, including notifications; successful state changes are also logged by PeerManagementService. Binding failures do not change state. These logs provide a useful initial record for incident analysis, though they do not capture transport, caller context, or every failure outcome.
Full auditing across HTTP and IPC would require changes to the shared dispatch path, where we do not yet have an audit facility. I propose documenting the current limits and tracking that work separately. This would defer the audit requirement in #6989, so I’d like maintainer agreement to make it a follow-up rather than a blocker for this PR.
| } | ||
| logger.info("Connect to peer {}", address); | ||
| peerClientCache.put(address.getAddress(), System.currentTimeMillis()); | ||
| if (peerClient.connectAsync(n, false) == null) { |
There was a problem hiding this comment.
[MUST] Removing an active node does not cancel a dial or handshake already started here. Because the channel is registered only after the handshake, removePeer can remove the address and find no connected channel, then this pending connection completes and remains active. Track and cancel pending active-node dials, or tag and revalidate them at handshake admission.
There was a problem hiding this comment.
Thanks for pointing this out. The race is real: a dial already in progress may complete after admin_removePeer returns. The intended behavior of removePeer is to remove the endpoint from the active-node set and disconnect currently registered peer connections; it does not guarantee that the endpoint cannot connect again. The scheduler already rechecks active-node membership before starting a dial, so removal prevents subsequent active-node attempts.
Preventing an endpoint from connecting through in-flight dials, discovery, or inbound connections would require a broader exclusion policy. For an IP that must not reconnect, operators can use admin_blockIp. I’ll clarify the pending-dial limitation in the PR description, but I don’t think cancelling in-flight dials is required for the specified removePeer behavior.
There was a problem hiding this comment.
[DISCUSS] The implementation matches the newly documented narrow contract, but an already-started TCP connection can still finish its handshake after admin_removePeer succeeds and remain connected. Because admin_blockIp is IP-wide rather than an exact-endpoint substitute, please get maintainer agreement on this behavior and document it in the operator-facing Admin RPC reference, or close/revalidate in-flight outbound channels.
There was a problem hiding this comment.
Thanks for following up. I agree that admin_blockIp is IP-wide and is not an exact-endpoint substitute. Reliably cancelling an in-flight dial would require tracking its channel through the handshake and coordinating that lifecycle with removal; it would still not prevent connections through discovery or inbound paths.
The intended admin_removePeer contract is to stop active-node scheduling and disconnect connections registered at the time of the call. I’ll document that a pending dial may complete afterward, and that disconnectedCount covers only connections disconnected during the call, in the operator-facing Admin RPC reference. I’d appreciate maintainer confirmation that this contract is appropriate for this PR.
| try { | ||
| return deserializeBlockedIps(storedValue); | ||
| } catch (IOException e) { | ||
| logger.warn("Invalid blocked IP data in CommonStore key blocked-ips; deleting it", e); |
There was a problem hiding this comment.
[SHOULD] Keep the manual blocklist closed on stored-data errors
If the stored blocked-ips value has one malformed entry or an unsupported format, this path deletes the only persisted copy and returns an empty set. configure() then installs that empty set before P2P starts, so every previously blocked IP can connect again after a restart. The existing test explicitly confirms this behavior.
Suggestion: Preserve the stored value and stop or isolate network startup until an operator explicitly repairs or resets the blocklist.
There was a problem hiding this comment.
Thanks for flagging this. In bfe0dbc, we changed the deserialization-error path to log a warning and retain the original blocked-ips value while starting with an empty runtime blocklist. This gives operators a chance to recover the stored record. A subsequent successful admin_blockIp call writes a new snapshot and replaces it.
We chose to keep network startup available when this operational record is malformed. We recognize that the previous manual blocks are not enforced until the record is restored and the node restarted, or those IPs are blocked again. The updated tests cover both retention and replacement.
|
[SHOULD] Please update the corrupt-blocklist recovery description to match |
Write the validated snapshot before updating the runtime policy so a database failure cannot change P2P state. Adjust the peer management tests to cover the new ordering and failure behavior.
@lxcmyf Thanks, I have updated the descritption about corrupt-blocklist recovery. |
What does this PR do?
Adds runtime peer management to Admin JSON-RPC over HTTP and IPC, so operators can adjust active nodes, inspect connections, and block malicious IPs without restarting FullNode.
The API adds
admin_addPeer,admin_removePeer,admin_disconnectPeer,admin_listActivePeers,admin_blockIp,admin_unblockIp, andadmin_listBlockedIps. Mutations return a sharedPeerOperationResult; active-peer queries reuse the connection, latency, and synchronization statistics used byPeerConnection.log(). The IPC client also supports optional text-table output foradmin_listActivePeers.The five mutating Admin methods emit INFO-level write-attempt logs. Each line records the method and endpoint/IP target; unsafe or overlong targets appear as
omitted. The log indicates an attempted call, not a successful mutation.Why are these changes required?
Operators need to disconnect or block identified malicious peers and add trusted endpoints promptly when investigating degraded connectivity or network isolation. Persistent manual IP blocking keeps those decisions effective across node restarts.
Implementation
This PR builds on the Admin HTTP/IPC framework introduced in upstream PR #6979 and targets
release_v4.8.3. It adds peer-management APIs to that framework, together with the supporting behavior and tests inframeworkand:p2p.frameworkowns Admin HTTP/IPC dispatch, operation coordination, peer statistics, and persistence of normalized IPs andblockedAtMillistimestamps inCommonStore["blocked-ips"].p2pprovidesaddActiveNode,removeActiveNode,disconnect, andreplaceBlockedIps, plus TCP admission, dialing, candidate filtering, and disconnection enforcement.ConnectionPolicypublishes an immutable IP set through a volatile reference and normalizes IPv4-mapped IPv6 addresses for matching.P2pConfig.blockedIpssupplies the initial policy; runtime changes update the policy without writing back to startup configuration.p2p-standalone.jarretains--blocked-ips/-b. Its argument coverage is consolidated inStartAppArgsTest, and the module README documents startup and runtime usage.Behavior and compatibility
1.Active peers
admin_addPeeradds an endpoint to the active-node set without adding it totrustNodesor waiting for a successful connection. It rejects endpoints whose IP is manually blocked.admin_removePeerremoves an active endpoint and disconnects any currently registered connection to it. It does not cancel a dial or handshake already in progress, so that connection may complete after the call.admin_disconnectPeerdisconnects the exact endpoint without removing it from the active-node set, so it may reconnect later.admin_listActivePeersreports current connection, latency, and synchronization statistics. Mutations reportsuccess,changed, anddisconnectedCount, distinguishing a successful no-op from a business failure.node.dynamicConfig.enableis enabled, add/remove operations return a structured failure directing operators to updatenode.active. The other peer-management operations remain available.2.Manual IP blocklist
admin_blockIpandadmin_unblockIpaccept IPv4 or IPv6 literals. Repeated blocking is a successful no-op that preserves the originalblockedAtMillistimestamp; unblocking an absent IP is also a successful no-op.admin_listBlockedIpsreturns normalized IPs and their timestamps.3.Network scope and compatibility
Connect.protoaddsDisconnectReason.REQUESTED = 0x0Ffor explicit disconnects. The existing disconnect message structure, consensus rules, and chain-data formats remain unchanged.4.JSON-RPC behavior
JsonRpcRequestValidator. Admin follows the existing public endpoint: explicitid: nulland empty method names are invalid; numeric IDs are not limited to signed 64-bit values;params: nullpasses envelope validation and reaches method binding.Testing
Validation on macOS ARM64 / JDK 17 includes:
./gradlew clean build -x testand./gradlew test(full multi-module suite, completed during development)./gradlew :framework:checkstyleMain :framework:checkstyleTest :plugins:checkstyleMain(completed during development)check_reference_conf.py,check_reference_comments.py, andcheck_math_usage.sh(completed during development)./gradlew :framework:test --tests org.tron.core.services.admin.AdminJsonRpcImplTest./gradlew :framework:test --tests org.tron.core.services.admin.ipc.server.AdminJsonRpcRequestTest --tests org.tron.core.services.jsonrpc.JsonRpcServletTest./gradlew :framework:checkstyleMain :framework:checkstyleTest(after the logging and validation changes)Coverage includes startup policy installation, persistence and corrupt-data recovery, update ordering and retries, idempotency, concurrent mutations, dynamic-config restrictions, IP normalization, and TCP inbound/outbound rejection. HTTP/IPC dispatch, error codes, peer statistics, IPC text output, network lifecycle, and effective-connection compatibility have regression tests. The standalone p2p JAR build and
--blocked-ipshelp invocation were also checked during development.Deferred audit logging
Issue #6989 specifies audit logging in the shared Admin invocation path. This PR includes INFO-level write-attempt logs for mutating methods on HTTP and IPC, including notifications that reach the method, and service logs for completed changes. These operational logs do not capture transport, caller context, binding failures, or every final outcome. Full audit logging across the shared HTTP/IPC dispatch path is deferred to follow-up work because it requires broader changes and is not an immediate operational need for this feature.