chore(p2p): internalize libp2p v2.2.9 as a local p2p module - #14
Open
barbatos2011 wants to merge 7 commits into
Open
chore(p2p): internalize libp2p v2.2.9 as a local p2p module#14barbatos2011 wants to merge 7 commits into
p2p module#14barbatos2011 wants to merge 7 commits into
Conversation
added 4 commits
August 13, 2026 11:29
Vendors tronprotocol/libp2p tag v2.2.9 (c564f263d310d7a964035d3b597634aba6bda86d) into a local `p2p` Gradle module, ahead of switching `common` off the external io.github.tronprotocol:libp2p Maven artifact. The source in this commit is byte-identical to `git archive v2.2.9 src/main`, so a reviewer can diff it directly against the upstream tag and confirm nothing was altered on the way in. Everything we change about it lands in the next commit, separately and for exactly that reason. Two arrangements differ from the upstream layout: - The example code moves out of src/main into its own `example` sourceSet. It still compiles, so an API change in main surfaces here too, but it is not packaged into p2p.jar and is not run as tests. - Generated protobuf sources are gitignored and rebuilt by :p2p:generateProto, so they stay out of the diff. p2p tracks rootProject.grpcVersion rather than pinning libp2p's own gRPC version, so the module cannot drift from the Netty the rest of the build resolves.
Four mechanical rewrites plus formatting, applied on top of the pristine v2.2.9 source added in the previous commit. Kept separate so commit 1 stays diffable against the upstream tag. - log. -> logger. (158 call sites). The root lombok.config sets lombok.log.fieldName=logger, so @slf4j generates `logger`, not `log`. - Math. -> StrictMath. (6 sites). CI enforces a check-math rule that rejects java.lang.Math anywhere in the tree, to keep arithmetic deterministic across JVMs and architectures. - BasicThreadFactory.builder() -> new BasicThreadFactory.Builder() (13 sites). builder() needs commons-lang3 3.12+; the project pins 3.4 globally. Rewriting to the 3.0 API keeps that pin rather than forcing a silent global upgrade. - toLowerCase()/toUpperCase() -> Locale.ROOT (4 sites). The root build enables errorprone StringCaseLocaleUsage as ERROR on every subproject except protocol and errorprone, so this is compile-forced. It is the one change here that is not purely cosmetic: behaviour is identical for ASCII but differs under a Turkish locale. Matches the project's own idiom in Args.java:1273. Formatting: google-java-format over the 9 vendored org/web3j/** files, which came in AOSP 4-space style, plus import reordering and hand fixes for the remainder. This takes :p2p:checkstyleMain from 605 violations to 0. No functional change beyond the Locale.ROOT note above.
Replaces io.github.tronprotocol:libp2p:2.2.9 with `api project(":p2p")`,
collapsing 17 lines of dependency plus excludes into one.
The dom4j exclusion tail (jaxen, stax-api, msv, xsdlib, relaxngDatatype,
pull-parser, xpp3) that used to sit on the libp2p dependency here does not
disappear: it arrives via the Aliyun and Route53 SDKs, which are now p2p's own
dependencies. The exclusions move with them, into a configurations.configureEach
block in p2p/build.gradle. Dropping them would silently re-admit artifacts the
project has excluded for years.
Removing a dependency also removes it as a version requester, so every version
the libp2p POM declared was checked against what :p2p now declares. All twelve
match, except two deliberate differences:
- commons-lang3: libp2p declared 3.18.0 at runtime scope, which won conflict
resolution against the root's 3.4 and put 3.18.0 on :framework:runtimeClasspath.
:p2p now declares 3.18.0 for the same reason. Pinning the root's 3.4 here --
which is what the module needs to *compile*, since the source uses the
3.0-compatible `new BasicThreadFactory.Builder()` -- would have shipped a 2015
release and reintroduced CVE-2025-48924.
- grpc-netty: libp2p pinned 1.81.0; :p2p tracks rootProject.grpcVersion (1.83.0)
so it cannot drift from the Netty the rest of the build resolves.
Adding a project to the dependency graph also needs three task-dependency edges
that an external jar did not, each of which Gradle reported as an
implicit_dependency and answered by disabling execution optimizations:
- framework's buildFullNodeJar and plugins' binaryRelease both zip up
runtimeClasspath and maintain a hand-written dependsOn list of project jars.
:common now exposes p2p via `api`, so p2p-1.0.0.jar is on both classpaths;
without the edge a parallel build could assemble the shipped fat jar before
:p2p:jar exists.
- p2p's own processExampleResources reads src/example/resources, which the
protobuf plugin claims as an output of generateExampleProto because
generatedFilesBaseDir points at $projectDir/src.
verification-metadata.xml gains three components that resolve once p2p compiles
in-tree: bcutil-jdk18on:1.84, gson:2.9.0 and gson-parent:2.9.0. Checksums were
taken from Maven Central and cross-checked against the published .sha1.
gson 2.9.0 is older than the 2.14.0 used elsewhere, and that is fine: it only
appears on :p2p's isolated compile classpath. :framework's runtimeClasspath
still resolves gson:2.9.0 -> 2.14.0.
Verified with :framework:dependencies and :framework:dependencyInsight on
runtimeClasspath: `project :p2p` present, no external libp2p artifact, gson at
2.14.0 and commons-lang3 at 3.18.0 -- the same versions the node shipped before.
A full build reports zero implicit_dependency warnings.
Ports all 23 of v2.2.9's test files to framework/src/test/java, following the project-wide convention already used by actuator, chainbase, consensus and common. Package names are preserved. None of the four rewrites from the previous commit applied: the test sources use none of those patterns. Build wiring: - The AWS Route53 and Aliyun SDKs plus dnsjava are added as testImplementation. They are implementation-scope in :p2p and so are not transitively visible here, but the DNS tests need them. - Those SDKs drag in the same dom4j tail that :p2p excludes module-wide. Without mirroring the exclusions, dependency verification fails on 9 artifacts. They are mirrored scoped to test configurations only, leaving the main runtime classpath untouched. - :framework:jacocoTestReport now includes p2p's class and source dirs. :p2p has no test sourceSet, so :p2p:jacocoTestReport emits nothing, and this report covered framework's classes only, leaving p2p at zero packages despite being exercised by these tests. Generated protobuf code is excluded, matching the checkstyle exclusion. Moving these into framework's test JVM changes their isolation requirements: the task uses forkEvery = 100, so up to 100 classes share a process, whereas in libp2p's own module each ran alone (and its CI never ran tests at all). Three places leaked process-wide state and now clean up after themselves: - ConnPoolServiceTest and SocketTest call ChannelManager.close(), which latches a static isShutdown that init() never clears. Left set, every later test that starts p2p gets a PeerClient whose connect() returns null and a ConnPoolService that skips reconnection -- an order-dependent failure that is painful to diagnose. Both teardowns reset it. - HandshakeServiceTest saves and restores Parameter.handlerList instead of clearing it, since that is the registry P2pService.register() appends to. - DnsManagerTest restores DnsManager's statics rather than leaving them pointing at mocks from a finished class. Four upstream tests were unreliable by construction rather than merely flaky: - NetUtilTest.testGetIP called three public IP-echo services and asserted all three returned the same string. That needs the network and assumes a single egress address. It now runs against a loopback HttpServer, which exercises the same fetch/parse/validate path deterministically and reaches the rejection branches too. - NetUtilTest.testGetLanIP compared getLanIP() against the source address the kernel picks for a socket to www.baidu.com. Those are different definitions -- interface enumeration versus the routing table -- and disagree on any multi-homed host. It now asserts the contract getLanIP() actually has, and needs no network. - NetUtilTest.testExternalIp dereferenced a result that is null when every IP-echo service fails. It now assumes a result before asserting the address is routable. - ConnPoolServiceTest.getNodes_orderByUpdateTimeDesc asserted the returned list was ordered by updateTime. getNodes() sorts, truncates to max(limit * 10, 50) candidates, then calls Collections.shuffle() -- with two nodes that assertion is a coin flip. It now asserts membership, and a new test covers the descending sort where it is observable: above the candidate bound. NodeHandlerTest also loses an unused org.checkerframework import that does not resolve on this classpath. Eight test classes are added for code the upstream suite did not reach: ByteArray, PublishService config validation and static-node publishing, the varint32 frame decoder that fronts every channel pipeline, AwsClient's change computation, AliClient's request/retry/pagination logic, HandshakeService's accept and reject branches, DnsManager's node filtering, Channel's value semantics, and the DisconnectCode to DisconnectReason mapping. Where a collaborator is genuinely external -- the Aliyun SDK, process-wide ChannelManager state -- it is mocked, so the logic under test is real and only the transport is faked. This takes p2p from 35% line coverage on the upstream tests alone to 60.90% (2142/3517), clearing the >60% changed-line gate. What is still uncovered needs a live connection: ConnPoolService.onConnect/onDisconnect/onMessage, NodeDetectService, PeerClient, Channel.init/send. Upstream's SocketTest for exactly that is entirely commented out, so it would take integration tests with real channels rather than more unit tests. Two pre-existing libp2p defects surfaced while writing these and are reported in the PR description rather than fixed here, since this PR claims no functional change: RootEntry.java:67 and Algorithm.java:121 both throw unchecked exceptions that escape a catch(DnsException) written to tolerate unparseable input, so one malformed TXT record aborts the whole publish or collection.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
38 issues found across 136 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="p2p/src/main/java/org/tron/p2p/discover/NodeManager.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/NodeManager.java:14">
P2: When `init()` is called again, this assignment abandons the running `KadService` while its executors and server remain active. Stop the existing discovery manager before replacing it; otherwise the new server cannot bind the UDP port and the new service cannot send discovery messages.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/connection/socket/PeerServer.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/connection/socket/PeerServer.java:29">
P1: When shutdown runs while the asynchronous `start(port)` is still binding, this condition skips the close. The start thread then binds and waits on `closeFuture` indefinitely, leaving the listener and Netty threads active after `ChannelManager.close()` returns. Track shutdown state with proper synchronization, and have startup abort or close the channel when shutdown has already begun.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/sync/RandomIterator.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/sync/RandomIterator.java:66">
P2: After `hasNext()` returns true, a normal `iterator.next()` performs a second DNS sync and discards the node prefetched into `cur`; it can even return `null` immediately after `hasNext()` returned true. Cache the look-ahead result and have `next()` consume it, or remove the `Iterator` implementation.</violation>
<violation number="2" location="p2p/src/main/java/org/tron/p2p/dns/sync/RandomIterator.java:90">
P2: When the iterator has no valid tree URLs, `pickTree()` calls `random.nextInt(0)` and every `next()` fails with `IllegalArgumentException`. Return `null` before selecting a random index when `clientTrees` is empty.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/discover/socket/DiscoverServer.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/socket/DiscoverServer.java:72">
P1: When `close()` runs before this asynchronous bind completes, it sees `channel == null` and returns after setting `shutdown`; this bind then still succeeds and waits indefinitely. Coordinate channel publication with shutdown, and close a channel bound after shutdown is requested or wait for the startup thread.</violation>
</file>
<file name="p2p/src/example/java/org/tron/p2p/example/DnsExample2.java">
<violation number="1" location="p2p/src/example/java/org/tron/p2p/example/DnsExample2.java:52">
P2: `TestMessage` is neither serializable nor encoded with the handler's wire format, so this call returns null and closes the channel. Encode the type byte and payload directly before sending.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/sync/SubtreeSync.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/sync/SubtreeSync.java:57">
P1: When a subtree TXT lookup temporarily returns no record, `resolveAll` discards that hash and marks the subtree complete. `ClientTree` then skips it while the root sequence is unchanged; throw on null before polling so the next sync retries it.</violation>
</file>
<file name="p2p/src/example/java/org/tron/p2p/example/StartApp.java">
<violation number="1" location="p2p/src/example/java/org/tron/p2p/example/StartApp.java:57">
P2: When `--trust-ips` contains more than one address, this code resolves the entire comma-separated value as one hostname, so none of the listed peers becomes trusted. Split the option and resolve each IP independently before setting `trustNodes`.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/update/AliClient.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/update/AliClient.java:199">
P1: When a stale-record deletion fails after retries, `submitChanges` still reports publication success and leaves the old DNS record active. Check the return value and propagate a deletion failure before logging success.</violation>
</file>
<file name="p2p/src/example/java/org/tron/p2p/example/DnsExample1.java">
<violation number="1" location="p2p/src/example/java/org/tron/p2p/example/DnsExample1.java:84">
P2: This example publishes DNS trees with a committed private signing key, so every deployment using the sample shares an identity that anyone can use to forge signed trees. Read a unique key from deployment configuration instead of embedding it in source.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/tree/Algorithm.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/tree/Algorithm.java:31">
P1: When the public key’s X coordinate starts with a zero nibble, this slice drops that nibble and includes part of Y, so DNS tree publication produces an unusable public key. Left-pad the full public-key value to 128 hex characters before extracting X.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/utils/NetUtil.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/utils/NetUtil.java:80">
P1: An advertised endpoint with a port outside 1–65535 passes `validNode`, then can throw `IllegalArgumentException` when discovery handles its socket address. Reject invalid ports in `validNode` before accepting the message.</violation>
<violation number="2" location="p2p/src/main/java/org/tron/p2p/utils/NetUtil.java:227">
P2: When a seed hostname is configured, `parseInetSocketAddress` performs a blocking JVM DNS lookup before `InetUtil` can submit its parallel, bounded lookups. Parse hostnames without resolving them here, or move hostname resolution entirely into `InetUtil`.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/DnsNode.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/DnsNode.java:29">
P2: When a `DnsNode` is constructed with an ID, this call discards it, so `compress` omits `nodeId` and decompressed DNS entries cannot preserve peer identity. Pass the constructor's `id` to `Node`.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/NodeHandler.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/NodeHandler.java:65">
P1: When an incumbent answers its eviction challenge, the replacement handler remains `ALIVE` without table membership and can never become active. Explicitly reject `replaceCandidate` when the incumbent survives.</violation>
<violation number="2" location="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/NodeHandler.java:106">
P2: When an active peer fails the compatibility check, this transition marks its handler dead but leaves the peer in the routing table. Remove active entries when transitioning to `DEAD`, or keep the handler active until the table entry is removed.</violation>
<violation number="3" location="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/NodeHandler.java:120">
P2: After a handler exhausts its initial ping retries, a later successful pong does not restore its retry budget. Reset `pingTrials` when a pong is accepted so recovered nodes receive the normal challenge budget.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/tree/BranchEntry.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/tree/BranchEntry.java:24">
P2: When a DNS branch TXT value contains an empty or non-hash child, `parseEntry` accepts it and `SubtreeSync` queues it for resolution, so malformed publisher data can abort synchronization or silently omit leaves. Validate every child with the fixed-length base32 hash rules and reject the branch before returning it.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/utils/CollectionUtils.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/utils/CollectionUtils.java:13">
P2: When `limit` is zero and `items` is non-empty, this loop returns every item instead of truncating to zero. Return an empty list before iterating when `limit == 0`.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/connection/socket/P2pChannelInitializer.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/connection/socket/P2pChannelInitializer.java:58">
P2: When channel initialization throws, this catch logs and returns while leaving the Netty channel open with a partial pipeline. Close `ch` or rethrow the failure so Netty cannot retain an unusable peer connection.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/discover/socket/MessageHandler.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/socket/MessageHandler.java:65">
P1: When a remote UDP packet causes `eventHandler.handleEvent` to throw, this closes the shared UDP listener and takes discovery offline until `DiscoverServer` restarts it. Repeated packets can keep causing this outage; handle per-datagram failures without closing the listener.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/table/NodeBucket.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/table/NodeBucket.java:34">
P1: When a bucket is full, `getLastSeen()` selects the newest entry instead of the least-recently-seen entry. Challenge the oldest entry (`sorted.get(sorted.size() - 1)`) so active peers are retained and stale peers are eligible for replacement.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/connection/ChannelManager.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/connection/ChannelManager.java:66">
P2: After `close()` sets `isShutdown`, a later `init()` leaves it true, so `PeerClient.connectAsync()` returns null and the connection pool skips reconnection. Reset `isShutdown` during initialization.</violation>
<violation number="2" location="p2p/src/main/java/org/tron/p2p/connection/ChannelManager.java:94">
P1: When an old connection closes after a replacement to the same remote address is admitted, this removal deletes the replacement from `channels`. Remove the entry only when its mapped value is this `channel`.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/sync/ClientTree.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/sync/ClientTree.java:113">
P1: When a referenced TXT record is temporarily unavailable, this line permanently drops its hash from `linkSync.missing` and treats the signed tree as complete. Keep unresolved hashes queued or fail the sync, and apply the same handling to the ENR removal in `syncNextRandomNode` so transient DNS propagation does not produce an incomplete node set.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/table/NodeEntry.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/table/NodeEntry.java:14">
P2: When an existing handler learns a different node ID, this cached distance remains based on the old ID, so `NodeTable` keeps the peer in the wrong Kademlia bucket and may evict or fail-find the wrong peers. Recompute the distance from the current node ID, or update/rebucket the entry whenever the node ID changes.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/update/PublishService.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/update/PublishService.java:138">
P1: When DNS publishing is enabled without `dnsPrivate`, `checkConfig` still accepts the configuration and `Tree.makeTree` skips signing, so the publisher emits a `tree://null@...`/unsigned tree. Reject missing private keys before starting the publisher.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/tree/RootEntry.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/tree/RootEntry.java:67">
P1: When a root TXT value is shorter than `rootPrefix`, this line throws `StringIndexOutOfBoundsException`; prefixless values are also parsed as roots. `AwsClient.computeChanges` catches only `DnsException`, so malformed existing root data aborts publish; validate the prefix and length first.</violation>
<violation number="2" location="p2p/src/main/java/org/tron/p2p/dns/tree/RootEntry.java:70">
P2: When the outer root payload or embedded signature contains malformed Base64, `Algorithm.decode64` throws unchecked `IllegalArgumentException` instead of the declared `DnsException`. Catch decoder failures and convert them to the appropriate root or signature parse error so corrupt DNS data cannot escape root resolution and publishing paths.</violation>
</file>
<file name="p2p/src/example/java/org/tron/p2p/example/ImportUsing.java">
<violation number="1" location="p2p/src/example/java/org/tron/p2p/example/ImportUsing.java:51">
P2: When this example sends its `TestMessage`, `ByteArray.fromObject` returns `null` because `TestMessage` is not serializable, so `Channel.send` closes the channel. Build a protocol payload with the `TEST` byte followed by the message data instead of Java serialization.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/connection/business/keepalive/KeepAliveService.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/connection/business/keepalive/KeepAliveService.java:40">
P1: When a peer answers before the following assignments run, `processMessage` clears `waitForPong`, then this code sets it back to true. Record `pingSent` and mark `waitForPong` before sending the ping, otherwise a healthy peer can be disconnected after 20 seconds.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/table/NodeTable.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/protocol/kad/table/NodeTable.java:115">
P2: When more than 16 peers share a distance bucket, `DistanceComparator` returns zero for all of them, so this sort preserves arbitrary `HashMap` order. The following truncation can omit XOR-closer peers from `getClosestNodes`; sort by the full XOR distance before limiting the response.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/discover/Node.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/discover/Node.java:155">
P1: When a peer keeps the same ID but advertises a different endpoint, equal `Node` objects produce different hashes, so `HashSet` lookups and deduplication fail. Hash the same byte-array identity used by `equals`.</violation>
<violation number="2" location="p2p/src/main/java/org/tron/p2p/discover/Node.java:169">
P1: When two peer IDs contain different invalid UTF-8 byte sequences, `getIdString()` can make them equal and the node table can treat distinct peers as the same node. Compare the ID byte arrays directly with `Arrays.equals` instead of decoding them to `String`.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/dns/update/AwsClient.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/dns/update/AwsClient.java:99">
P1: When the zone ID is omitted and nested Route53 zones exist, `findZoneID` can select the parent zone because it returns the first suffix match. Keep the longest matching hosted-zone name, or require an explicit zone ID.</violation>
</file>
<file name="p2p/src/main/java/org/web3j/utils/Numeric.java">
<violation number="1" location="p2p/src/main/java/org/web3j/utils/Numeric.java:43">
P2: When `decodeQuantity` receives a negative or unprefixed value, it returns it instead of rejecting it. Validate the quantity digits and require the `0x` form before parsing.</violation>
<violation number="2" location="p2p/src/main/java/org/web3j/utils/Numeric.java:213">
P2: When `hexStringToByteArray` receives a non-hex character, it silently transforms the input into bytes, so `Hash.sha3(String)` hashes the wrong data. Reject either nibble before constructing each byte.</violation>
</file>
<file name="p2p/src/main/java/org/tron/p2p/connection/socket/PeerClient.java">
<violation number="1" location="p2p/src/main/java/org/tron/p2p/connection/socket/PeerClient.java:38">
P2: `connect` blocks until the peer disconnects, so callers cannot continue after the connection is established. Wait only for the connect future and return.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| logger.error("The configuration items related to the AwsRoute53 dns server cannot be empty"); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
P1: When DNS publishing is enabled without dnsPrivate, checkConfig still accepts the configuration and Tree.makeTree skips signing, so the publisher emits a tree://null@.../unsigned tree. Reject missing private keys before starting the publisher.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/tron/p2p/dns/update/PublishService.java, line 138:
<comment>When DNS publishing is enabled without `dnsPrivate`, `checkConfig` still accepts the configuration and `Tree.makeTree` skips signing, so the publisher emits a `tree://null@...`/unsigned tree. Reject missing private keys before starting the publisher.</comment>
<file context>
@@ -0,0 +1,146 @@
+ logger.error("The configuration items related to the AwsRoute53 dns server cannot be empty");
+ return false;
+ }
+ return true;
+ }
+
</file context>
| } | ||
|
|
||
| public static RootEntry parseEntry(String e) throws DnsException { | ||
| String value = e.substring(rootPrefix.length()); |
There was a problem hiding this comment.
P1: When a root TXT value is shorter than rootPrefix, this line throws StringIndexOutOfBoundsException; prefixless values are also parsed as roots. AwsClient.computeChanges catches only DnsException, so malformed existing root data aborts publish; validate the prefix and length first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/tron/p2p/dns/tree/RootEntry.java, line 67:
<comment>When a root TXT value is shorter than `rootPrefix`, this line throws `StringIndexOutOfBoundsException`; prefixless values are also parsed as roots. `AwsClient.computeChanges` catches only `DnsException`, so malformed existing root data aborts publish; validate the prefix and length first.</comment>
<file context>
@@ -0,0 +1,113 @@
+ }
+
+ public static RootEntry parseEntry(String e) throws DnsException {
+ String value = e.substring(rootPrefix.length());
+ DnsRoot dnsRoot1;
+ try {
</file context>
|
|
||
| for (String key : existing.keySet()) { | ||
| if (!records.containsKey(key)) { | ||
| deleteRecord(existing.get(key).getRecordId()); |
There was a problem hiding this comment.
P1: When a stale-record deletion fails after retries, submitChanges still reports publication success and leaves the old DNS record active. Check the return value and propagate a deletion failure before logging success.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/tron/p2p/dns/update/AliClient.java, line 199:
<comment>When a stale-record deletion fails after retries, `submitChanges` still reports publication success and leaves the old DNS record active. Check the return value and propagate a deletion failure before logging success.</comment>
<file context>
@@ -0,0 +1,341 @@
+
+ for (String key : existing.keySet()) {
+ if (!records.containsKey(key)) {
+ deleteRecord(existing.get(key).getRecordId());
+ deleteCount++;
+ }
</file context>
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return this.format().hashCode(); |
There was a problem hiding this comment.
P1: When a peer keeps the same ID but advertises a different endpoint, equal Node objects produce different hashes, so HashSet lookups and deduplication fail. Hash the same byte-array identity used by equals.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/tron/p2p/discover/Node.java, line 155:
<comment>When a peer keeps the same ID but advertises a different endpoint, equal `Node` objects produce different hashes, so `HashSet` lookups and deduplication fail. Hash the same byte-array identity used by `equals`.</comment>
<file context>
@@ -0,0 +1,197 @@
+
+ @Override
+ public int hashCode() {
+ return this.format().hashCode();
+ }
+
</file context>
| public static String compressPubKey(BigInteger pubKey) { | ||
| String pubKeyYPrefix = pubKey.testBit(0) ? "03" : "02"; | ||
| String pubKeyHex = pubKey.toString(16); | ||
| String pubKeyX = pubKeyHex.substring(0, 64); |
There was a problem hiding this comment.
P1: When the public key’s X coordinate starts with a zero nibble, this slice drops that nibble and includes part of Y, so DNS tree publication produces an unusable public key. Left-pad the full public-key value to 128 hex characters before extracting X.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/tron/p2p/dns/tree/Algorithm.java, line 31:
<comment>When the public key’s X coordinate starts with a zero nibble, this slice drops that nibble and includes part of Y, so DNS tree publication produces an unusable public key. Left-pad the full public-key value to 128 hex characters before extracting X.</comment>
<file context>
@@ -0,0 +1,150 @@
+ public static String compressPubKey(BigInteger pubKey) {
+ String pubKeyYPrefix = pubKey.testBit(0) ? "03" : "02";
+ String pubKeyHex = pubKey.toString(16);
+ String pubKeyX = pubKeyHex.substring(0, 64);
+ String hexPub = pubKeyYPrefix + pubKeyX;
+ return hexPub;
</file context>
Comment on lines
+57
to
+60
| InetSocketAddress address = new InetSocketAddress(cli.getOptionValue("t"), 0); | ||
| List<InetAddress> trustNodes = new ArrayList<>(); | ||
| trustNodes.add(address.getAddress()); | ||
| Parameter.p2pConfig.setTrustNodes(trustNodes); |
There was a problem hiding this comment.
P2: When --trust-ips contains more than one address, this code resolves the entire comma-separated value as one hostname, so none of the listed peers becomes trusted. Split the option and resolve each IP independently before setting trustNodes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/example/java/org/tron/p2p/example/StartApp.java, line 57:
<comment>When `--trust-ips` contains more than one address, this code resolves the entire comma-separated value as one hostname, so none of the listed peers becomes trusted. Split the option and resolve each IP independently before setting `trustNodes`.</comment>
<file context>
@@ -0,0 +1,385 @@
+ }
+
+ if (cli.hasOption("t")) {
+ InetSocketAddress address = new InetSocketAddress(cli.getOptionValue("t"), 0);
+ List<InetAddress> trustNodes = new ArrayList<>();
+ trustNodes.add(address.getAddress());
</file context>
Suggested change
| InetSocketAddress address = new InetSocketAddress(cli.getOptionValue("t"), 0); | |
| List<InetAddress> trustNodes = new ArrayList<>(); | |
| trustNodes.add(address.getAddress()); | |
| Parameter.p2pConfig.setTrustNodes(trustNodes); | |
| List<InetAddress> trustNodes = new ArrayList<>(); | |
| for (String trustIp : cli.getOptionValue("t").split(",")) { | |
| InetAddress trustAddress = new InetSocketAddress(trustIp.trim(), 0).getAddress(); | |
| if (trustAddress != null) { | |
| trustNodes.add(trustAddress); | |
| } | |
| } | |
| Parameter.p2pConfig.setTrustNodes(trustNodes); |
| } | ||
|
|
||
| public static BigInteger decodeQuantity(String value) { | ||
| if (isLongValue(value)) { |
There was a problem hiding this comment.
P2: When decodeQuantity receives a negative or unprefixed value, it returns it instead of rejecting it. Validate the quantity digits and require the 0x form before parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/web3j/utils/Numeric.java, line 43:
<comment>When `decodeQuantity` receives a negative or unprefixed value, it returns it instead of rejecting it. Validate the quantity digits and require the `0x` form before parsing.</comment>
<file context>
@@ -0,0 +1,252 @@
+ }
+
+ public static BigInteger decodeQuantity(String value) {
+ if (isLongValue(value)) {
+ return BigInteger.valueOf(Long.parseLong(value));
+ }
</file context>
| int startIdx; | ||
| if (len % 2 != 0) { | ||
| data = new byte[(len / 2) + 1]; | ||
| data[0] = (byte) Character.digit(cleanInput.charAt(0), 16); |
There was a problem hiding this comment.
P2: When hexStringToByteArray receives a non-hex character, it silently transforms the input into bytes, so Hash.sha3(String) hashes the wrong data. Reject either nibble before constructing each byte.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/web3j/utils/Numeric.java, line 213:
<comment>When `hexStringToByteArray` receives a non-hex character, it silently transforms the input into bytes, so `Hash.sha3(String)` hashes the wrong data. Reject either nibble before constructing each byte.</comment>
<file context>
@@ -0,0 +1,252 @@
+ int startIdx;
+ if (len % 2 != 0) {
+ data = new byte[(len / 2) + 1];
+ data[0] = (byte) Character.digit(cleanInput.charAt(0), 16);
+ startIdx = 1;
+ } else {
</file context>
| }); | ||
|
|
||
| } catch (Exception e) { | ||
| logger.error("Unexpected initChannel error", e); |
There was a problem hiding this comment.
P2: When channel initialization throws, this catch logs and returns while leaving the Netty channel open with a partial pipeline. Close ch or rethrow the failure so Netty cannot retain an unusable peer connection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/tron/p2p/connection/socket/P2pChannelInitializer.java, line 58:
<comment>When channel initialization throws, this catch logs and returns while leaving the Netty channel open with a partial pipeline. Close `ch` or rethrow the failure so Netty cannot retain an unusable peer connection.</comment>
<file context>
@@ -0,0 +1,62 @@
+ });
+
+ } catch (Exception e) {
+ logger.error("Unexpected initChannel error", e);
+ }
+ }
</file context>
| public NodeEntry(byte[] ownerId, Node n) { | ||
| this.node = n; | ||
| entryId = n.getHostKey(); | ||
| distance = distance(ownerId, n.getId()); |
There was a problem hiding this comment.
P2: When an existing handler learns a different node ID, this cached distance remains based on the old ID, so NodeTable keeps the peer in the wrong Kademlia bucket and may evict or fail-find the wrong peers. Recompute the distance from the current node ID, or update/rebucket the entry whenever the node ID changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At p2p/src/main/java/org/tron/p2p/discover/protocol/kad/table/NodeEntry.java, line 14:
<comment>When an existing handler learns a different node ID, this cached distance remains based on the old ID, so `NodeTable` keeps the peer in the wrong Kademlia bucket and may evict or fail-find the wrong peers. Recompute the distance from the current node ID, or update/rebucket the entry whenever the node ID changes.</comment>
<file context>
@@ -0,0 +1,88 @@
+ public NodeEntry(byte[] ownerId, Node n) {
+ this.node = n;
+ entryId = n.getHostKey();
+ distance = distance(ownerId, n.getId());
+ touch();
+ }
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Netty 4.2 split io.netty.handler.codec.protobuf out of netty-codec into its own artifact, so both :framework and :p2p have to declare it explicitly -- each puts the varint32 framing codecs on its channel pipelines. Both carried the literal 4.2.15.Final. Netty itself is not declared anywhere; it arrives transitively through grpc-netty, which :p2p tracks as rootProject.grpcVersion. So a grpc bump moves Netty while these two literals stay put -- exactly the mismatch that broke p2p's pipeline when develop moved to Netty 4.2 in the first place. Extract nettyVersion next to grpcVersion so the coupling is visible in one place. Resolution is unchanged: netty-codec-protobuf still resolves to 4.2.15.Final on both :framework:compileClasspath and :p2p:compileClasspath.
barbatos2011
force-pushed
the
chore/internalize-libp2p-v229
branch
from
August 22, 2026 03:51
c4b005b to
0dd786b
Compare
:framework uses org.tron.p2p in 17 files under src/main/java, but declared no
dependency on it. The types arrive three hops away, through
:common -> :crypto -> :chainbase, because common exposes p2p with
`api project(":p2p")`.
That export is not a mistake and is not removable here: CommonParameter
publishes `P2pConfig p2pConfig` and `PublishConfig dnsPublishConfig` as public
@Getter fields, so p2p types are part of :common's own API surface. Narrowing
it to `implementation` would break every caller of getP2pConfig(). Actually
de-coupling the graph means moving those fields out of CommonParameter, which
is a functional refactor and out of scope for this PR.
What is fixable now is the undeclared direct use. Declare it, so framework does
not depend on an unrelated module's export choice for code it uses itself.
api rather than implementation, because framework re-exports p2p types itself:
P2pEventHandlerImpl extends org.tron.p2p.P2pEventHandler, HelloMessage.getFrom()
returns org.tron.p2p.discover.Node, PeerManager.add/remove take
org.tron.p2p.connection.Channel, and Args.loadDnsPublishConfig returns
PublishConfig. implementation would compile today only because the transitive
api chain still supplies those types to consumers -- the moment that chain is
narrowed, it breaks.
No resolution change -- p2p was already on framework's compile and runtime
classpaths via the transitive api.
barbatos2011
force-pushed
the
chore/internalize-libp2p-v229
branch
from
August 22, 2026 03:54
0dd786b to
5071aec
Compare
getClosestNodes_nodesMoreThanBucketCapacity built both nodes from the same
byte[]:
byte[] bytes = new byte[64];
bytes[0] = 15; Node nearNode = new Node(bytes, ...);
bytes[0] = 70; Node farNode = new Node(bytes, ...);
Node keeps the reference it is handed (this.id = id), so the second mutation
rewrote nearNode's id too and both nodes ended up identical. The test still
passed, but only because Node.equals compares getIdString(): the surviving
farNode satisfies closest.contains(nearNode). Nothing the method claims to
check was actually checked -- the comment "nearnode's distance is 252, far's
is 255, others' are 253" was never exercised.
Give each node its own array. Those three distances now hold: with the home id
all zeros, distance is 256 minus the leading zero bits of the id, so 0x0F ->
252, 0x11 -> 253, 0x46 -> 255.
Also assert what the trailing comment already promised but never verified --
that the farthest node is excluded, and that the result is capped at
BUCKET_SIZE. Confirmed both bite: restoring the shared array makes the test
fail on the new assertion.
Unrelated and pre-existing: this class cannot run on its own, because it reads
Parameter.p2pConfig without setting it and depends on another test class having
initialised it. Verified against the unmodified branch -- running the class
alone fails there too. Not addressed here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
chore(p2p): internalize libp2p v2.2.9 as a local
p2pmoduleReplaces the external
io.github.tronprotocol:libp2p:2.2.9Maven dependency with a localp2p/Gradle module built from the same source.Supersedes tronprotocol#6673, which was approved on the merits but closed because it touched too many files while 4.8.2 was already full. This is a fresh branch — no history is reused.
Reproducing the diff
tronprotocol/java-trondevelop@4a21592f95e37908b21bc3f611c6e7a1a67f09f3tronprotocol/libp2ptagv2.2.9@c564f263d310d7a964035d3b597634aba6bda86dCommit 1 is byte-comparable to the upstream tag. It contains only
git archive v2.2.9 src/mainplus the new build files, so a reviewer can diff it directly againstc564f263and see that no vendored line was altered. Everything we changed about that source lands in commit 2, separately, for exactly this reason.File count
p2p/src/main/java(vendored)p2p/src/main/protosp2p/src/example/javaframework/src/test/java/org/tron/p2pThe example code moved to its own
examplesourceSet: it compiles (so API changes surface here too) but is not packaged intop2p.jarand not run as tests. Protobuf-generated sources undersrc/main/java/org/tron/p2p/protos/are gitignored and regenerated at build time, so they are not in the diff.Tests live in
framework/src/test/java/rather thanp2p/src/test/, following the project-wide convention already used byactuator,chainbase,consensusandcommon.Problems encountered
Three of these did not exist when tronprotocol#6673 was written; all were confirmed by compiling.
H1 — Netty 4.2 split out
netty-codec-protobufdevelopnow resolves Netty 4.2.15.Final (via the gRPC 1.83 bump in tronprotocol#6891).io.netty.handler.codec.protobufno longer arrives transitively, soProtobufVarint32FrameDecoder/ProtobufVarint32LengthFieldPrepender— on every p2p channel pipeline — failed to resolve. Declared explicitly inp2p/build.gradle, mirroring whatframework/build.gradle:43already does for the same reason, excludes included.p2ptracksrootProject.grpcVersionrather than pinning libp2p's own gRPC version, so it cannot drift from the Netty the rest of the build resolves.H2 — errorprone
StringCaseLocaleUsage(the one behavioural deviation)The root build enables exactly two errorprone rules as ERROR on every subproject except
protocol/errorprone:StringCaseLocaleUsageandStringCaseLocaleUsageMethodRef. libp2p has 4 baretoLowerCase()/toUpperCase()calls, which are nowtoLowerCase(Locale.ROOT)/toUpperCase(Locale.ROOT), matching the project's own idiom (Args.java:1273).This is the only change in this PR that is not purely mechanical. It is compile-forced and behaviour-identical for ASCII input, but it is a real semantic change under a Turkish locale, so it is called out here rather than buried in a style commit.
H3 —
BasicThreadFactory.builder()needs commons-lang3 3.12+The project pins commons-lang3 3.4 globally; v2.2.9 uses
builder()in 13 places. Rewritten tonew BasicThreadFactory.Builder()(the 3.0 API) rather than bumping commons-lang3, which would have been a silent global upgrade.dom4j exclusion tail
The
jaxen/stax-api/msv/xsdlib/relaxngDatatype/pull-parser/xpp3exclusions used to sit on thelibp2pdependency incommon/build.gradle. That transitive tail comes from the Aliyun / Route53 SDKs, so internalizing moves it intop2p/build.gradleas aconfigurations.configureEachblock.The same tail also reaches
:framework's test classpath, because the DNS tests need those SDKs astestImplementation(they areimplementation-scope in:p2pand so not transitively visible). Without mirroring the exclusions there, dependency verification fails on 9 artifacts. They are mirrored scoped to test configurations only, so the main runtime classpath is untouched.Task-dependency edges an external jar did not need
Adding a project to the dependency graph needs three explicit task edges that a Maven artifact did not. Gradle reported each as an
implicit_dependencyand responded by disabling execution optimizations "to ensure correctness":framework'sbuildFullNodeJarandplugins'binaryReleaseboth zip upruntimeClasspathand maintain a hand-writtendependsOnlist of project jars — the plugins one carries a comment explaining it exists so "partial / parallel builds cannot run binaryRelease before the dependency jars exist".:commonnow exposes p2p viaapi, sop2p-1.0.0.jaris on both classpaths, and neither list had been updated. Without the edge a parallel build could assemble the shipped fat jar before:p2p:jaris written.:p2p:processExampleResourcesreadssrc/example/resources, which the protobuf plugin claims as an output ofgenerateExampleProtobecausegeneratedFilesBaseDirpoints at$projectDir/src.A full build now reports zero
implicit_dependencywarnings.Coverage attribution for a module with no local tests
:p2phas no test sourceSet, so:p2p:jacocoTestReportproduces no report at all, and:framework:jacocoTestReportreported on framework's classes only — p2p was contributing 0 packages to coverage despite being exercised by 121 tests.:framework's report now adds p2p's class and source dirs, with**/protos/**excluded to match the checkstyle exclusion.Dependency verification
Three components needed adding to
gradle/verification-metadata.xml:org.bouncycastle:bcutil-jdk18on:1.84,com.google.code.gson:gson:2.9.0andcom.google.code.gson:gson-parent:2.9.0. Checksums were taken from Maven Central and cross-checked against the published.sha1.gson 2.9.0 is older than the 2.14.0 used elsewhere. This does not change the assembled node:
:p2p's isolated compile classpath sees 2.9.0, while:framework'sruntimeClasspathstill resolvesgson:2.9.0 -> 2.14.0.Version preservation across the switch
Removing a dependency also removes it as a version requester, so every version the libp2p POM declared was checked against what
:p2pnow declares. All twelve match except two deliberate differences::p2pdeclarescommons-lang3grpc-nettyrootProject.grpcVersion(1.83.0)commons-lang3is the one that nearly went wrong. libp2p declared 3.18.0 at runtime scope, which won conflict resolution against the root build's 3.4 and put 3.18.0 on:framework:runtimeClasspath.:p2pinitially pinned 3.4 — the version the module needs to compile, since the source uses the 3.0-compatiblenew BasicThreadFactory.Builder()— which left no requester for anything newer and would have shipped a 2015 release, reintroducing CVE-2025-48924 (ClassUtils.getAbbreviatedNameuncontrolled recursion, fixed in 3.18.0).:p2pnow declares 3.18.0.Verified with
./gradlew :framework:dependencyInsight --configuration runtimeClasspath:gsonat 2.14.0 andcommons-lang3at 3.18.0 — the same versions the node shipped before.Tests
All 23 of v2.2.9's own test files are ported, plus 4 new ones. Two upstream tests were unreliable by construction and were fixed rather than carried over as-is:
NetUtilTest.testGetIPcalled three public IP-echo services and asserted all three returned the same string — a network dependency, and a coin flip on any host with more than one egress address. It now runs against a loopbackHttpServer, which exercises the same fetch/parse/validate path deterministically and covers the rejection branches too. (libp2p's own CI never ran its tests, so this had not shown up.)ConnPoolServiceTest.getNodes_orderByUpdateTimeDescasserted thatgetNodes()returns nodes ordered byupdateTimedescending.getNodes()sorts, truncates tomax(limit * 10, 50)candidates, and then callsCollections.shuffle()— so with two nodes that assertion passes about half the time. It now asserts membership, and a new test (getNodes_prefersNewestAboveCandidateSize) covers the descending sort where it is actually observable: above the candidate bound.One import was dropped:
NodeHandlerTesthad an unusedorg.checkerframework.checker.units.qual.Nimport — an IDE auto-import artifact that does not resolve on this classpath.Coverage: p2p is at 61.05% line coverage (2147/3517), clearing the CI gate of >60% on changed lines. v2.2.9's own tests reach about 35%; the rest comes from eight added test classes covering
ByteArray,PublishService(config validation and static-node publishing), the varint32 frame decoder that fronts every channel pipeline,AwsClient's change computation,AliClient's request/retry/pagination logic,HandshakeService's accept/reject branches,DnsManager's node filtering,Channel's value semantics, and theDisconnectCode -> DisconnectReasonmapping.Where a collaborator is genuinely external — the Aliyun SDK, process-wide
ChannelManagerstate — it is mocked, so the logic under test is real and only the transport is faked. What remains uncovered is code that needs a live connection:ConnPoolService.onConnect/onDisconnect/onMessage,NodeDetectService,PeerClient,Channel.init/send. Upstream's ownSocketTestfor exactly that is entirely commented out, so it would need integration tests with real channels rather than more unit tests.Defects found while testing (not fixed here)
Two instances of the same shape: a DNS parse helper throws an unchecked exception past a
catch (DnsException)that was clearly written to tolerate unparseable input, so one malformed TXT record aborts the whole operation instead of being skipped.1. Short root entry.
RootEntry.parseEntrydoese.substring(rootPrefix.length())with no length guard:p2p/src/main/java/org/tron/p2p/dns/tree/RootEntry.java:67Any value shorter than the 13-character
tree-root-v1:prefix throwsStringIndexOutOfBoundsException. The caller atp2p/src/main/java/org/tron/p2p/dns/update/AwsClient.java:334catches onlyDnsException, so it escapes and abortscomputeChanges, failing the entire publish.2. Malformed base64 in a nodes entry.
Algorithm.decode64callsBase64.getUrlDecoder().decode()directly:p2p/src/main/java/org/tron/p2p/dns/tree/Algorithm.java:121which throws
IllegalArgumentExceptionon malformed input.NodesEntry.parseEntryconverts onlyInvalidProtocolBufferExceptionandUnknownHostExceptionintoDnsException, so theIllegalArgumentExceptionescapes both it and theDnsException-only catch atp2p/src/main/java/org/tron/p2p/dns/update/AliClient.java:138, aborting the wholecollectRecords— and with it thedeploy()that called it.A third, latent instance:
BranchEntry.parseEntry(p2p/src/main/java/org/tron/p2p/dns/tree/BranchEntry.java:19) does the same unguardedsubstring(branchPrefix.length()), and unlike its siblings does not even declarethrows DnsException. Its only caller checkstxt.startsWith(branchPrefix)first, so it is not currently reachable with a short string — a hazard for the next caller, not a live bug.LinkEntry.parseEntryshows the correct shape: prefix check, length check, andcatch (RuntimeException)around the base32 decode.Reachability — these are operator-side, not peer-reachable
Traced every path that reaches these parsers:
Message.parsecatch (Exception)P2pPacketDecodercatch (Exception)RandomIterator.next()catch (Exception)aroundsyncRandom()(ClientTreehas no catch of its own)catch (DnsException)So no remote peer can trigger these. Both live instances are on the operator's own publish path: they abort a DNS publish for whoever runs it, they do not give an attacker anything. That is worth stating plainly, because "unchecked exception on a parse path" in a networking module reads much worse than it is here.
All three are pre-existing in libp2p and out of scope for a no-functional-changes PR, so they are reported rather than fixed. The tests that surfaced them use inputs that reach the intended
DnsExceptionpath and document the unchecked one in a comment.Known security issues — pre-existing, deferred
These exist in libp2p today and are unchanged by this PR. Internalizing the source is what makes them fixable in-tree; each gets its own follow-up:
compressPubKeydrops leading zerosAwsClientswallowsInterruptedExceptionwithout restoring the interrupt flag (dns/update/AwsClient.java, now commented rather than silently empty)Mixing any of them in would break the "no functional changes" claim, which is the only thing that makes a diff this size reviewable.
Build wiring cleanups
Two follow-on commits, both build-only, no code touched.
nettyVersionextracted to the rootext.:frameworkand:p2peach declarenetty-codec-protobufexplicitly (see H1) and each carried the literal4.2.15.Final. Netty itself is declared nowhere — it arrives transitively viagrpc-netty, which:p2ptracks asrootProject.grpcVersion. So a grpc bump moves Netty while those two literals stay put, which is exactly the mismatch H1 describes. The version now sits next togrpcVersionso the coupling is visible in one place. Resolution is unchanged:4.2.15.Finalon both compile classpaths.:frameworknow declares its direct:p2pdependency. framework usesorg.tron.p2pin 17 files undersrc/main/javabut declared nothing, relying on a three-hop transitive through:common -> :crypto -> :chainbase.That export is not removable here, and it is worth being precise about why:
CommonParameterpublishesP2pConfig p2pConfigandPublishConfig dnsPublishConfigas public@Getterfields, so p2p types are part of:common's own API surface andapi project(":p2p")is forced. Narrowing it toimplementationwould break every caller ofgetP2pConfig(). Genuinely decoupling the graph means moving those two fields out ofCommonParameter, which is a functional refactor and deliberately out of scope. Until then,:p2pstays on the compile classpath of every module in the graph — as the externallibp2partifact did before this PR, for the same reason.What is fixable now is the undeclared direct use, so it is declared:
implementation, notapi, since framework does not re-export p2p types. No resolution change.Scope — what this PR does not change
.protofiles move verbatim)org.tron.p2p.**), so imports in dependent code are untouchedp2premains usable standalone — the example sourceSet still compiles against itVerification
:p2p:compileJava+:p2p:compileExampleJava(strict verification):framework:compileJava --rerun-tasksproject :p2pon framework runtimeClasspathlibp2partifactsgson:2.9.0 -> 2.14.0:p2p:checkstyleMain:framework:checkstyleTestcommons-lang3unchanged at 3.18.0implicit_dependencywarnings./gradlew buildAll 9 build failures are pre-existing and unrelated to p2p; none is a p2p test:
ValidateMultiSignContractTest.testTip854RejectsMalformedCalldata,AllowTvmLondonTest.testBaseFee,AllowTvmLondonTest.testStartWithEF) — these also failed on the pre-change tree and pass on retry.BindExceptioninMetrics.init. Four framework test classes (SRMetricsTest,PrometheusApiServiceTest,JsonrpcServiceTest,RpcApiServicesTest) each bind the same hard-coded Prometheus port 9527, while the test task runs up to 4 parallel forks. Verified pre-existing by running those four classes together with no p2p tests in the run: it reproduces, and hits a different class each time. The ~45 test classes this PR adds change how classes distribute across forks, which is what made two of them land concurrently — the race is java-tron's, not p2p's, but this PR makes it more likely to surface.| Changed-line coverage > 60% | PASS — 60.90% (2142/3517) |
Known remaining CI-reliability risk
Two ported upstream tests still need live DNS against a third-party zone and are left as-is rather than disabled, since they exercise real discovery behaviour:
RandomTest.testRandomIteratorandSyncTestboth synctree://…@nile.trondisco.netthrough hard-coded public resolvers.RandomTestalready failed once in a local batch run and only went green via thetest-retryplugin (maxRetries = 5). On a runner with restricted egress, or if that DNS tree is re-published or retired, they fail permanently.LookUpTxtTestin the same package already@Ignores its network tests, so that is the precedent if reviewers would rather these be skipped than retried. Disabling them costs roughly 1.5 points of coverage, which still clears the gate.ConnPoolServiceTestandSocketTestalso bind fixed ports (10000 / 10001) while the test task runs up to 4 parallel forks.PeerServer.startonly logs on bind failure, so a collision would let the test pass without exercising anything. java-tron's own tests usePublicMethod.chooseRandomPort()for this reason.