Skip to content

feat(p2p): Add DHT discovery (Issue: #55)#99

Closed
Joyabrata001 wants to merge 1 commit intoS4tvara:mainfrom
Joyabrata001:feature/dht-discovery
Closed

feat(p2p): Add DHT discovery (Issue: #55)#99
Joyabrata001 wants to merge 1 commit intoS4tvara:mainfrom
Joyabrata001:feature/dht-discovery

Conversation

@Joyabrata001
Copy link
Copy Markdown

@Joyabrata001 Joyabrata001 commented Oct 5, 2025

Implements DHT-based peer discovery using libp2p’s Kademlia DHT. (#55 )

Changes

  • Updated internal/config/vault.go:

    • Added DiscoveryConfig and DHTConfig structs.
    • Added default discovery configuration under BuildVaultConfigWithDeduplication. (requires improvement)
  • Updated internal/p2p/factory.go:

    • CreateDHT() now returns a new DHTDiscovery instance.
  • Updated internal/p2p/dht.go:

    • Implements config.Discovery using libp2p’s Kademlia DHT.
    • Handles peer discovery, bootstrapping, and lifecycle management.
  • Added internal/p2p/dht_test.go:

    • Test for DHT discovery lifecycle and bootstrap connectivity.

    • Includes relay and NAT diagnostic checks.

    • go test ./internal/p2p -run TestDHTDiscovery -v

Scope of improvement:

  • internal/config/vault.go

    • Replace the hardcoded bootstrap peer with a configurable list or fallback to dht.GetDefaultBootstrapPeerAddrInfos() only when DHT is explicitly enabled.
    • The configuration structure below is hardcoded and needs to be made flexible:
      // Set default discovery settings
      config.Discovery = DiscoveryConfig{
          MDNS: true,
          DHT: DHTConfig{
              Enabled: true,
              BootstrapNodes: []string{
                  "/ip4/104.131.131.82/tcp/4001/p2p/QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ",
              },
          },
      }
  • internal/p2p/dht.go

    • Discovery interval and channel buffer are not configurable as of now.
    • Code can be logically refactored better
  • internal/p2p/dht_test.go

    • Replace time.Sleep with context or event-based waits
    • Safely consume peerChan to avoid goroutine panics on close

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 5, 2025

Codecov Report

❌ Patch coverage is 62.29508% with 46 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/p2p/dht.go 68.46% 22 Missing and 13 partials ⚠️
internal/config/vault.go 0.00% 10 Missing ⚠️
internal/p2p/discovery.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Joyabrata001
Copy link
Copy Markdown
Author

Joyabrata001 commented Oct 5, 2025

@SubstantialCattle5 Unfortunately I am new testing and require a bit of help.

@Joyabrata001 Joyabrata001 changed the title feat(p2p): Add DHT discovery feat(p2p): Add DHT discovery (Issue: #55) Oct 5, 2025
@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 5, 2025

Hi @Joyabrata001,
Sure where exactly are you stuck??

@Joyabrata001 Joyabrata001 force-pushed the feature/dht-discovery branch 2 times, most recently from 13a4d0d to 1fc8407 Compare October 5, 2025 11:07
@Joyabrata001
Copy link
Copy Markdown
Author

Joyabrata001 commented Oct 5, 2025

@SubstantialCattle5

  1. Failing the dependecy check
    govulncheck flagged a vulnerability due to usage of github.com/libp2p/go-libp2p-kad-dht.

  2. I have implemented dht_test.go where:
    This test sets up a temporary libp2p host with relay and NAT support, then defines some static relay addresses to make sure it can connect. It creates a DHTDiscovery instance using the given bootstrap nodes, starts the discovery process, and periodically looks for peers on the DHT. As peers are found, it logs them along with connection and disconnection events. Finally, it stops the discovery service after sometime.

Now, I am new to testing and cant seem to be able to interpret the results of the Codecov report and unable to proceed.

@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 5, 2025

Took a dig through the vulnerability report

  1. Failing the dependecy check
    govulncheck flagged a vulnerability due to usage of github.com/libp2p/go-libp2p-kad-dht.

versions before 0.20 had Access Control Bypass vulnerability, but in this case since we're using 0.35 we're pretty safe.
I would adivse you to add .govulncheckignore file in the root of the project and add a special rule for this library.
(something like this, but please check the rules first)

# GO-2024-3218 is a false positive for v0.35.1.
# The vulnerability only impacts versions <= 0.20.0.
github.com/libp2p/go-libp2p-kad-dht GO-2024-3218

@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 5, 2025

Hi @Joyabrata001,

This test sets up a temporary libp2p host with relay and NAT support, then defines some static relay addresses to make sure it can connect. It creates a DHTDiscovery instance using the given bootstrap nodes, starts the discovery process, and periodically looks for peers on the DHT. As peers are found, it logs them along with connection and disconnection events. Finally, it stops the discovery service after sometime.

You’re well within the required target for code coverage for this project.
image
The CI for code coverage has passed successfully. The comment highlights the files with missing test coverage, which you may want to improve.
image
Hope this helps

Cheers,
nilay

@Joyabrata001 Joyabrata001 force-pushed the feature/dht-discovery branch from 1fc8407 to 421e1a5 Compare October 5, 2025 14:12
@Joyabrata001
Copy link
Copy Markdown
Author

Joyabrata001 commented Oct 5, 2025

@SubstantialCattle5

govulncheck findings

Key Findings for CI:

  1. No Ignore Feature: govulncheck does not support an ignore file to suppress specific CVEs.
  2. Non-Blocking Exit Code: When run with a structured output format (e.g., sarif, json), the command exits with a success code (0), even if vulnerabilities are found.

Conclusion:

We can safely integrate the check using a structured format without blocking the build pipeline. However, identified vulnerabilities will need manual review.

For that this change has to be done to ci.yml

- name: Check for vulnerabilities
      uses: golang/govulncheck-action@v1
      with:
        go-version-input: '1.24'
        go-package: './...'
        format: 'sarif'  # <-- added

@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 5, 2025

golang/go#59507

surprisingly this issue is still not resolved. Gimme some time i'll update the ci

@Joyabrata001
Copy link
Copy Markdown
Author

Sure, thanks!

@S4tvara
Copy link
Copy Markdown
Owner

S4tvara commented Oct 5, 2025

@Joyabrata001 I've updated the ci runner can you make another pr, such that it can run with the new ci runner

@S4tvara S4tvara closed this Oct 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants