feat(net): support config domain besides IP#68
Open
Conversation
📝 WalkthroughWalkthroughThe PR updates the libp2p dependency and introduces DNS resolution capabilities for backup member configuration. A new InetUtil utility class handles network endpoint parsing and DNS lookups. BackupManager implements periodic DNS re-resolution with scheduled refresh, while Args.java adds validation for configured backup members. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration Loader
participant Args as Args Class
participant InetUtil as InetUtil (DNS)
participant LookUp as LookUpTxt
participant BackupMgr as BackupManager
Note over Config,BackupMgr: Initial Backup Member Resolution
Config->>Args: Load backup members list
Args->>Args: checkBackupMembers()
Args->>InetUtil: resolveInetAddress(each member)
InetUtil->>LookUp: lookUpIp(hostname)
LookUp-->>InetUtil: Resolved InetAddress
InetUtil-->>Args: Return address or null
Args-->>Config: Validation complete
Note over Config,BackupMgr: Runtime Periodic DNS Refresh (every 60s)
BackupMgr->>BackupMgr: refreshMemberIps() (scheduled)
BackupMgr->>InetUtil: resolveInetAddress(cached domains)
InetUtil->>LookUp: lookUpIp(hostname)
LookUp-->>InetUtil: New InetAddress
InetUtil-->>BackupMgr: Updated address
BackupMgr->>BackupMgr: Update members list & domainIpCache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
common/build.gradle (1)
25-25: Remove the commented previous dependency line.Line [25] leaves stale configuration in source; please drop it to avoid ambiguity in future dependency updates.
Suggested cleanup
- //api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/build.gradle` at line 25, Remove the stale commented dependency line "//api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7'," from the Gradle file; locate the commented dependency string and delete that entire comment so only the active dependency declarations remain, avoiding ambiguity during future dependency updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/src/main/java/org/tron/common/backup/BackupManager.java`:
- Around line 195-212: The refreshMemberIps method currently calls
members.remove(oldIp) when a domain's IP changes, which can drop a shared IP
still referenced by other domains; modify refreshMemberIps to avoid removing an
IP blindly by either (A) maintaining a reference count map keyed by IP
(increment when any domain or literal member maps to an IP, decrement and only
remove from members when count reaches zero) or (B) after resolving all domains
in domainIpCache, recompute the members set from scratch using the updated
domainIpCache plus the static/literal backup members and excluding localIp;
update domainIpCache and members atomically so refreshMemberIps no longer calls
members.remove(oldIp) for a single-domain change.
In `@framework/src/main/java/org/tron/core/config/args/Args.java`:
- Around line 1663-1673: The current checkBackupMembers() eagerly resolves every
entry in PARAMETER.backupMembers using resolveInetAddress and throws a TronError
on any unresolved host, causing startup to fail on transient DNS issues; change
it to mirror BackupManager behavior by skipping unresolved entries instead of
throwing: call resolveInetAddress(member), if it returns null log a warning (or
info) mentioning the member and continue, otherwise keep the resolved address
(or no-op), and remove the throw of TronError so startup proceeds and
BackupManager can retry DNS later.
In `@framework/src/main/java/org/tron/core/config/args/InetUtil.java`:
- Around line 21-30: The code calls NetUtil.parseInetSocketAddress(...) inside
getInetSocketAddressList without guarding for IllegalArgumentException so a
single malformed "host:port" aborts the whole list; wrap each call to
NetUtil.parseInetSocketAddress (both where host is read into host variable and
the later call around line 65) in a try/catch for IllegalArgumentException, skip
the offending entry (do not add to ret or domainEntries), and emit a warning log
or comment; ensure you continue processing remaining items so
getInetSocketAddressList returns only the valid InetSocketAddress entries.
---
Nitpick comments:
In `@common/build.gradle`:
- Line 25: Remove the stale commented dependency line "//api group:
'io.github.tronprotocol', name: 'libp2p', version: '2.2.7'," from the Gradle
file; locate the commented dependency string and delete that entire comment so
only the active dependency declarations remain, avoiding ambiguity during future
dependency updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79f8d959-fa29-4ae7-a380-88f3b4ca309f
📒 Files selected for processing (5)
common/build.gradleframework/src/main/java/org/tron/common/backup/BackupManager.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/config/args/InetUtil.javagradle/verification-metadata.xml
Comment on lines
+195
to
+212
| private void refreshMemberIps() { | ||
| for (Map.Entry<String, String> entry : domainIpCache.entrySet()) { | ||
| String domain = entry.getKey(); | ||
| String oldIp = entry.getValue(); | ||
| InetAddress inetAddress = resolveInetAddress(domain); | ||
| if (inetAddress == null) { | ||
| logger.warn("DNS refresh: failed to re-resolve backup member domain {}, keep it", domain); | ||
| continue; | ||
| } | ||
| String newIp = inetAddress.getHostAddress(); | ||
| if (!newIp.equals(oldIp)) { | ||
| logger.info("DNS refresh: backup member {} IP changed {} -> {}", domain, oldIp, newIp); | ||
| members.remove(oldIp); | ||
| if (!localIp.equals(newIp)) { | ||
| members.add(newIp); | ||
| } | ||
| domainIpCache.put(domain, newIp); | ||
| } |
There was a problem hiding this comment.
Don't remove a shared IP blindly during DNS refresh.
Line 207 drops oldIp from members as soon as one domain changes. That breaks when two domains, or a domain plus a literal backup member, currently resolve to the same IP: refreshing one entry removes the other still-valid peer from the set.
members needs either reference counting per IP or a full recompute from all current sources after each refresh, rather than members.remove(oldIp) on a single domain change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/main/java/org/tron/common/backup/BackupManager.java` around
lines 195 - 212, The refreshMemberIps method currently calls
members.remove(oldIp) when a domain's IP changes, which can drop a shared IP
still referenced by other domains; modify refreshMemberIps to avoid removing an
IP blindly by either (A) maintaining a reference count map keyed by IP
(increment when any domain or literal member maps to an IP, decrement and only
remove from members when count reaches zero) or (B) after resolving all domains
in domainIpCache, recompute the members set from scratch using the updated
domainIpCache plus the static/literal backup members and excluding localIp;
update domainIpCache and members atomically so refreshMemberIps no longer calls
members.remove(oldIp) for a single-domain change.
Comment on lines
+1663
to
+1673
| checkBackupMembers(); | ||
| } | ||
|
|
||
| private static void checkBackupMembers() { | ||
| for (String member : PARAMETER.backupMembers) { | ||
| InetAddress inetAddress = resolveInetAddress(member); | ||
| if (inetAddress == null) { | ||
| throw new TronError("Failed to resolve backup member: " + member, | ||
| TronError.ErrCode.PARAMETER_INIT); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't make node startup fail on transient DNS for backup peers.
checkBackupMembers() now resolves every backup member during config load and throws on any miss. That is much stricter than framework/src/main/java/org/tron/common/backup/BackupManager.java:86-90, which already skips unresolved members, and BackupManager.java:130-138,195-214, which retries DNS later. A temporary DNS issue now prevents the node from booting at all.
Possible fix
private static void checkBackupMembers() {
for (String member : PARAMETER.backupMembers) {
InetAddress inetAddress = resolveInetAddress(member);
if (inetAddress == null) {
- throw new TronError("Failed to resolve backup member: " + member,
- TronError.ErrCode.PARAMETER_INIT);
+ logger.warn("Failed to resolve backup member during startup, will retry later: {}",
+ member);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| checkBackupMembers(); | |
| } | |
| private static void checkBackupMembers() { | |
| for (String member : PARAMETER.backupMembers) { | |
| InetAddress inetAddress = resolveInetAddress(member); | |
| if (inetAddress == null) { | |
| throw new TronError("Failed to resolve backup member: " + member, | |
| TronError.ErrCode.PARAMETER_INIT); | |
| } | |
| } | |
| checkBackupMembers(); | |
| } | |
| private static void checkBackupMembers() { | |
| for (String member : PARAMETER.backupMembers) { | |
| InetAddress inetAddress = resolveInetAddress(member); | |
| if (inetAddress == null) { | |
| logger.warn("Failed to resolve backup member during startup, will retry later: {}", | |
| member); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/main/java/org/tron/core/config/args/Args.java` around lines
1663 - 1673, The current checkBackupMembers() eagerly resolves every entry in
PARAMETER.backupMembers using resolveInetAddress and throws a TronError on any
unresolved host, causing startup to fail on transient DNS issues; change it to
mirror BackupManager behavior by skipping unresolved entries instead of
throwing: call resolveInetAddress(member), if it returns null log a warning (or
info) mentioning the member and continue, otherwise keep the resolved address
(or no-op), and remove the throw of TronError so startup proceeds and
BackupManager can retry DNS later.
Comment on lines
+21
to
+30
| public static List<InetSocketAddress> getInetSocketAddressList(List<String> items) { | ||
| List<InetSocketAddress> ret = new ArrayList<>(); | ||
| if (items.isEmpty()) { | ||
| return ret; | ||
| } | ||
| // Collect entries whose host part is a domain name (not an IP literal). | ||
| List<String> domainEntries = new ArrayList<>(); | ||
| for (String item : items) { | ||
| String host = NetUtil.parseInetSocketAddress(item).getHostString(); | ||
| if (!NetUtil.validIpV4(host) && !NetUtil.validIpV6(host)) { |
There was a problem hiding this comment.
Guard parseInetSocketAddress() failures instead of aborting the whole list.
Line 29 and Line 65 call NetUtil.parseInetSocketAddress(...) without handling IllegalArgumentException. framework/src/test/java/org/tron/core/net/NodeTest.java:28-51 shows that parser throws on malformed values, so one bad host:port now fails the entire config load instead of skipping just that entry like the rest of this method does for unresolved DNS results.
Possible fix
+ private static InetSocketAddress tryParseInetSocketAddress(String configString) {
+ try {
+ return NetUtil.parseInetSocketAddress(configString);
+ } catch (IllegalArgumentException e) {
+ logger.warn("Invalid address format, skip: {}", configString);
+ return null;
+ }
+ }
+
public static List<InetSocketAddress> getInetSocketAddressList(List<String> items) {
List<InetSocketAddress> ret = new ArrayList<>();
if (items.isEmpty()) {
return ret;
}
@@
List<String> domainEntries = new ArrayList<>();
for (String item : items) {
- String host = NetUtil.parseInetSocketAddress(item).getHostString();
+ InetSocketAddress parsed = tryParseInetSocketAddress(item);
+ if (parsed == null) {
+ continue;
+ }
+ String host = parsed.getHostString();
if (!NetUtil.validIpV4(host) && !NetUtil.validIpV6(host)) {
domainEntries.add(item);
}
}
@@
for (String configString : items) {
InetSocketAddress inetSocketAddress;
- InetSocketAddress parsed = NetUtil.parseInetSocketAddress(configString);
+ InetSocketAddress parsed = tryParseInetSocketAddress(configString);
+ if (parsed == null) {
+ continue;
+ }
if (NetUtil.validIpV4(parsed.getHostString()) || NetUtil.validIpV6(parsed.getHostString())) {
inetSocketAddress = parsed;
} else {
inetSocketAddress = domainResolved.get(configString);
}Also applies to: 63-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@framework/src/main/java/org/tron/core/config/args/InetUtil.java` around lines
21 - 30, The code calls NetUtil.parseInetSocketAddress(...) inside
getInetSocketAddressList without guarding for IllegalArgumentException so a
single malformed "host:port" aborts the whole list; wrap each call to
NetUtil.parseInetSocketAddress (both where host is read into host variable and
the later call around line 65) in a try/catch for IllegalArgumentException, skip
the offending entry (do not add to ret or domainEntries), and emit a warning log
or comment; ensure you continue processing remaining items so
getInetSocketAddressList returns only the valid InetSocketAddress entries.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 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="common/build.gradle">
<violation number="1" location="common/build.gradle:24">
P0: **Supply-chain risk**: This replaces the official `io.github.tronprotocol` libp2p artifact with a personal GitHub fork (`com.github.317787106`) resolved via JitPack. For a blockchain node, pulling a core networking dependency from an individual's fork introduces serious supply-chain risk — the fork is outside the project's normal review/release process and could contain unaudited changes. If this is intentional for local development, it should not be merged to a shared branch; if the upstream library needs patches, they should be contributed back and released under the official group ID.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| api 'org.aspectj:aspectjweaver:1.9.8' | ||
| api 'org.aspectj:aspectjtools:1.9.8' | ||
| api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{ | ||
| api group: 'com.github.317787106', name: 'libp2p', version: 'v0.0.9',{ |
There was a problem hiding this comment.
P0: Supply-chain risk: This replaces the official io.github.tronprotocol libp2p artifact with a personal GitHub fork (com.github.317787106) resolved via JitPack. For a blockchain node, pulling a core networking dependency from an individual's fork introduces serious supply-chain risk — the fork is outside the project's normal review/release process and could contain unaudited changes. If this is intentional for local development, it should not be merged to a shared branch; if the upstream library needs patches, they should be contributed back and released under the official group ID.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At common/build.gradle, line 24:
<comment>**Supply-chain risk**: This replaces the official `io.github.tronprotocol` libp2p artifact with a personal GitHub fork (`com.github.317787106`) resolved via JitPack. For a blockchain node, pulling a core networking dependency from an individual's fork introduces serious supply-chain risk — the fork is outside the project's normal review/release process and could contain unaudited changes. If this is intentional for local development, it should not be merged to a shared branch; if the upstream library needs patches, they should be contributed back and released under the official group ID.</comment>
<file context>
@@ -21,7 +21,8 @@ dependencies {
api 'org.aspectj:aspectjweaver:1.9.8'
api 'org.aspectj:aspectjtools:1.9.8'
- api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{
+ api group: 'com.github.317787106', name: 'libp2p', version: 'v0.0.9',{
+ //api group: 'io.github.tronprotocol', name: 'libp2p', version: '2.2.7',{
exclude group: 'io.grpc', module: 'grpc-context'
</file context>
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.
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details
Summary by cubic
Adds support for using domain names (not just IPs) in network config for seed nodes and backup members. DNS is resolved at startup and backup member IPs auto-refresh on changes.
New Features
Dependencies
libp2ptocom.github.317787106:libp2p:v0.0.9; updatedverification-metadata.xml.Written for commit aadffe7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores