-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add support for external DNS #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds an externalDNS parameter to p2p.NewCommunication; when externalIP is empty and externalDNS is provided, builds and advertises a DNS-based multiaddress. Propagates ExternalDNS through NetworkConfig (tss) and updates tests, including a new test verifying DNS advertisement. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant P2P as p2p.NewCommunication
participant Host
participant Peerstore
Caller->>P2P: NewCommunication(bootstrapPeers, port, externalIP, externalDNS, ...)
alt externalIP provided
P2P->>P2P: Build externalAddr = /ip4/<externalIP>/tcp/<port>
else externalIP empty AND externalDNS provided
P2P->>P2P: Build externalAddr = /dns4/<externalDNS>/tcp/<port>
note right of P2P #a3be8c: DNS-based fallback used
else
P2P->>P2P: externalAddr remains nil
end
P2P->>Host: Initialize host and advertise addresses (include externalAddr if set)
P2P->>Peerstore: Store/advertise peer addresses
P2P-->>Caller: return *Communication, error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tss/tss.go (1)
105-113: Breaking: p2p.NewCommunication signature now includes ExternalDNS — update callersThe NewCommunication signature was changed to accept ExternalDNS; several internal callers still use the old 5-arg form and must be updated (tests/builds will break). Outdated call sites found: keysign/ecdsa/keysign_test.go, keysign/ecdsa/keysign_old_test.go, keygen/eddsa/keygen_test.go, keysign/eddsa/keysign_test.go, keygen/ecdsa/keygen_test.go. The change in tss/tss.go is correct. Consider migrating to an options struct or functional options to avoid future signature churn.
🧹 Nitpick comments (6)
p2p/communication.go (1)
57-85: External DNS fallback is solid; prefer /dns over /dns4 and log precedence when both are set
- Using /dns4 restricts to IPv4-only. Unless you explicitly want v4-only, advertise /dns to allow either family.
- If both ExternalIP and ExternalDNS are provided, log a clear precedence warning to aid ops/debugging.
Apply this diff:
func NewCommunication( bootstrapPeers []maddr.Multiaddr, port int, externalIP string, - externalDNS string, + externalDNS string, whitelistedPeers []peer.ID, logger zerolog.Logger, ) (*Communication, error) { logger = logger.With().Str(logs.Component, "communication").Logger() @@ - if len(externalIP) != 0 { + if len(externalIP) != 0 { + if len(externalDNS) != 0 { + logger.Warn().Str("external_ip", externalIP).Str("external_dns", externalDNS). + Msg("Both ExternalIP and ExternalDNS provided; ExternalIP takes precedence") + } myself := fmt.Sprintf("/ip4/%s/tcp/%d", externalIP, port) externalAddr, err = maddr.NewMultiaddr(myself) if err != nil { return nil, errors.Wrapf(err, "external addr %q", myself) } } else if len(externalDNS) != 0 { // externalDNS is used as a fallback when externalIP is not set - myself := fmt.Sprintf("/dns4/%s/tcp/%d", externalDNS, port) + myself := fmt.Sprintf("/dns/%s/tcp/%d", externalDNS, port) externalAddr, err = maddr.NewMultiaddr(myself) if err != nil { return nil, errors.Wrapf(err, "external addr %q", myself) } }p2p/communication_test.go (3)
22-22: Call site updates look correct; add a test asserting IP precedence over DNSAll call sites updated to pass the new externalDNS argument. Please add a test that sets both ExternalIP and ExternalDNS and asserts that the advertised address is the IP-based multiaddr (to lock in precedence).
Example test to add (outside the modified ranges):
func (CommunicationTestSuite) TestEstablishP2pCommunication_IPTakesPrecedence(c *C) { log := logger("TestEstablishP2pCommunication_IPTakesPrecedence") fakeExternalIP := "11.22.33.44" fakeExternalDNS := "example.com" expected := "/ip4/11.22.33.44/tcp/3330" comm, err := NewCommunication(nil, 3330, fakeExternalIP, fakeExternalDNS, []peer.ID{}, log) c.Assert(err, IsNil) defer comm.Stop() priv := "6LABmWB4iXqkqOJ9H0YFEA2CSSx6bA7XAKGyI/TDtas=" privKey, _ := base64.StdEncoding.DecodeString(priv) c.Assert(comm.Start(privKey), IsNil) c.Assert(checkExist(comm.host.Addrs(), expected), Equals, true) }Also applies to: 66-66, 72-72, 82-82, 89-97, 110-117
124-160: Use a reserved domain (example.com) in tests to avoid external couplingSwitch test DNS to example.com (RFC 2606) to avoid referencing a real domain.
Apply this diff:
- fakeExternalDNS := "p2ptest.com" - fakeExternalDNSMultiAddr := "/dns4/p2ptest.com/tcp/2220" + fakeExternalDNS := "example.com" + fakeExternalDNSMultiAddr := "/dns4/example.com/tcp/2220"
137-139: Extract shared test-only bootstrap private key constantThe base64 private key is present in multiple tests and is flagged by secret scanners — centralize it and annotate intent.
Occurrences to update:
- p2p/communication_test.go (lines ~59, ~137)
- keygen/eddsa/keygen_test.go (line ~41)
- keygen/ecdsa/keygen_test.go (line ~45)
- keysign/eddsa/keysign_test.go (line ~43)
- keysign/ecdsa/keysign_test.go (line ~46)
Add a single test-only constant (e.g., in a small test helper package/file) and replace literals with it:
const TestBootstrapPrivKeyBase64 = "6LABmWB4iXqkqOJ9H0YFEA2CSSx6bA7XAKGyI/TDtas=" // test-only key; safe for public tests
Replace each bootstrapPrivKey := "..." with bootstrapPrivKey := testutil.TestBootstrapPrivKeyBase64 (or the chosen import path).
tss/tss_4nodes_zeta_test.go (1)
255-256: NetworkConfig updated correctly to include ExternalDNSThe empty ExternalDNS in tests keeps behavior unchanged; good alignment with the new config surface.
Consider adding a test path that sets ExternalDNS non-empty (mirroring p2p tests) in an end-to-end flow to validate propagation through Server.New.
tss/tss.go (1)
63-64: Document ExternalDNS semantics in NetworkConfigAdd a field comment clarifying that ExternalDNS is used only when ExternalIP is empty and that a DNS-based multiaddr will be advertised.
Example:
// ExternalDNS is advertised as a /dns or /dns4 multiaddr when ExternalIP is empty. ExternalDNS string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
p2p/communication.go(2 hunks)p2p/communication_test.go(6 hunks)tss/tss.go(2 hunks)tss/tss_4nodes_zeta_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
p2p/communication_test.go (1)
p2p/communication.go (1)
NewCommunication(53-101)
🪛 Gitleaks (8.28.0)
p2p/communication_test.go
[high] 137-137: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test go 1.22
- GitHub Check: lint
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
keygen/eddsa/keygen_test.go (1)
113-113: DRY up repeated constructor usage with a small helper.Multiple tests repeat the same NewCommunication(...) arguments, which increases churn when signatures evolve. Consider a helper to centralize this.
Example helper for tests (place in a test-only file, e.g., keygen/eddsa/comm_test_helpers.go):
package eddsa import ( maddr "github.com/multiformats/go-multiaddr" "github.com/libp2p/go-libp2p/core/peer" zlog "github.com/rs/zerolog/log" "github.com/zeta-chain/go-tss/p2p" ) func newTestComm(bootstrapPeers []maddr.Multiaddr, port int, whitelisted []peer.ID) (*p2p.Communication, error) { return p2p.NewCommunication(bootstrapPeers, port, "", "", whitelisted, zlog.Logger) }Then replace call sites with:
comm, err := newTestComm(nil /* or bootstrapPeers */, ports[i], whitelistedPeers)Also applies to: 119-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
keygen/ecdsa/keygen_test.go(1 hunks)keygen/eddsa/keygen_test.go(1 hunks)keysign/ecdsa/keysign_old_test.go(1 hunks)keysign/ecdsa/keysign_test.go(1 hunks)keysign/eddsa/keysign_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
keygen/ecdsa/keygen_test.go (1)
p2p/communication.go (1)
NewCommunication(53-101)
keygen/eddsa/keygen_test.go (1)
p2p/communication.go (1)
NewCommunication(53-101)
keysign/ecdsa/keysign_test.go (1)
p2p/communication.go (1)
NewCommunication(53-101)
keysign/eddsa/keysign_test.go (1)
p2p/communication.go (1)
NewCommunication(53-101)
keysign/ecdsa/keysign_old_test.go (1)
p2p/communication.go (1)
NewCommunication(53-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test go 1.22
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
keygen/ecdsa/keygen_test.go (1)
127-127: LGTM: Updated to the new p2p.NewCommunication signature.The argument order is correct and consistent across bootstrap/non-bootstrap paths.
Also applies to: 133-133
keysign/eddsa/keysign_test.go (1)
151-151: LGTM: Updated for externalDNS parameter; consistent with other suites.No further changes required here.
Also applies to: 157-157
keysign/ecdsa/keysign_test.go (1)
153-153: Approve — DNS fallback already covered by testsp2p/communication_test.go defines fakeExternalDNSMultiAddr "/dns4/p2ptest.com/tcp/2220" (around line 139) and p2p/communication.go implements the externalDNS fallback, so the empty externalDNS in the integration test is acceptable.
keysign/ecdsa/keysign_old_test.go (1)
146-146: LGTM — no 5-arg NewCommunication() calls remain; verify DNS-path testsNo 5-arg NewCommunication() calls detected. Potential externalDNS usages observed in:
- keygen/ecdsa/keygen_test.go
- keygen/eddsa/keygen_test.go
- keysign/ecdsa/keysign_old_test.go
- keysign/ecdsa/keysign_test.go
- keysign/eddsa/keysign_test.go
Ensure at least one unit test explicitly exercises the externalDNS (DNS) path or add one.
The go-tss library currently only takes an [
ExternalIP+Port] to construct p2p communication object. Ideally, it would be operationally friendly if we could add additional support for [DomainName+Port].Summary by CodeRabbit
New Features
Tests