Skip to content
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

"ipfs stats dht" doesn't work with Routing.Type=custom #9482

Open
3 tasks done
MichaelMure opened this issue Dec 8, 2022 · 7 comments
Open
3 tasks done

"ipfs stats dht" doesn't work with Routing.Type=custom #9482

MichaelMure opened this issue Dec 8, 2022 · 7 comments
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up topic/commands Topic commands topic/dht Topic dht

Comments

@MichaelMure
Copy link
Contributor

MichaelMure commented Dec 8, 2022

Checklist

Installation method

built from source

Version

ipfs version 0.17.0

Config

...

  "Routing": {
    "Methods": {
      "find-peers": {
        "RouterName": "ParallelHelper"
      },
      "find-providers": {
        "RouterName": "ParallelHelper"
      },
      "get-ipns": {
        "RouterName": "ParallelHelper"
      },
      "provide": {
        "RouterName": "WanDHT"
      },
      "put-ipns": {
        "RouterName": "ParallelHelper"
      }
    },
    "Routers": {
      "CidContact": {
        "Parameters": {
          "Endpoint": "https://cid.contact/reframe"
        },
        "Type": "reframe"
      },
      "ParallelHelper": {
        "Parameters": {
          "Routers": [
            {
              "IgnoreErrors": true,
              "RouterName": "CidContact",
              "Timeout": "3s"
            },
            {
              "ExecuteAfter": "2s",
              "IgnoreErrors": false,
              "RouterName": "WanDHT",
              "Timeout": "5m"
            }
          ]
        },
        "Type": "parallel"
      },
      "WanDHT": {
        "Parameters": {
          "AcceleratedDHTClient": true,
          "Mode": "dhtserver",
          "PublicIPNetwork": true
        },
        "Type": "dht"
      }
    },
    "Type": "custom"
  },

...

Description

When using custom routing, for example using the config detailed in the v0.16 release notes, the ipfs stats dht command return Error: routing service is not a DHT, even though there is a DHT in there.

I understand the relative complexity of searching for a DHT in a custom routing pipeline and the possible edge cases, but it would be nice if that command would work in the (very common) case where there is exactly one DHT. Not only DHT stats are obviously useful, but my understanding is that it's also the only way to figure out if the accelerated DHT is ready to publish.

@MichaelMure MichaelMure added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Dec 8, 2022
@lidel
Copy link
Member

lidel commented Dec 13, 2022

This is a problem because Kubo 0.18 will use Routing.Type=auto by default (#9475) and ipfs stats dht is broken there as well.

Not a blocker for RC1, but we need to fix this before final release.

@lidel lidel changed the title "ipfs stats dht" doesn't work with custom routing "ipfs stats dht" doesn't work with Routing.Type=auto|custom Dec 13, 2022
@lidel lidel added topic/dht Topic dht topic/commands Topic commands exp/expert Having worked on the specific codebase is important P1 High: Likely tackled by core team if no one steps up and removed need/triage Needs initial labeling and prioritization labels Dec 13, 2022
@BigLep BigLep mentioned this issue Jan 3, 2023
@ajnavarro
Copy link
Member

I'm trying to find a solution to this problem.

Before, If we had activated DHT we were using dualDHT by default, so there are a lot of places where we are supposing that we have this DHT implementation, and querying WAN or LAN depending on the case.

With the new routing modular system, we can have from 0 to N DHTs, and we cannot know if it is WAN or LAN.

These are the commands on the need of having a DHT instance instead of a Router:

Proposed solution

  • When composing the final Router from configuration, also return map[string]routing.Router with all the routers in use.
  • On DHT stats, instead of using lan, wanserver or wan as parameters to filter DHT stats, use the specified router name from the configuration. If that routing system is a valid DHT (IpfsDHT, dualDHT, or FullRTDHT) then return the stats.
  • On DHT query command, go through all routers and query the ones implementing GetClosestPeers method. Join all the results in one.

@lidel WDYT?

@lidel
Copy link
Member

lidel commented Jan 11, 2023

@ajnavarro sgtm.

If this makes things easier, for Kubo 0.18 we only need to fix stats for the default behavior, which is Routing.Type=auto (runs the same DHT setup as old dht, WAN+LAN). We can address custom routers later.

@ajnavarro
Copy link
Member

@lidel Sounds good. I'll focus on fixing the problem with the default config first, and tackle the general problem after.

@ajnavarro
Copy link
Member

@lidel this is needed to be able to get DHT routers from the parallel router: libp2p/go-libp2p-routing-helpers#68

@BigLep
Copy link
Contributor

BigLep commented Jan 12, 2023

2023-01-12 update as I understand it: #9538 addresses the default case so we can unblock #9417 . We won't close the issue until we have fixes for non-default configuration.

lidel pushed a commit that referenced this issue Jan 12, 2023
Fixes default auto mode, but Routing.Type=custom needs more work.
Continued in #9482
@lidel lidel changed the title "ipfs stats dht" doesn't work with Routing.Type=auto|custom "ipfs stats dht" doesn't work with Routing.Type=custom Jan 12, 2023
galargh pushed a commit that referenced this issue Jan 23, 2023
Fixes default auto mode, but Routing.Type=custom needs more work.
Continued in #9482
@ajnavarro ajnavarro removed their assignment Feb 5, 2023
@guseggert
Copy link
Contributor

This is a larger problem than just ipfs stats dht, it also affects ipfs dht query and any other code that requires a Kademlia DHT specifically (and not just a content router). The same root cause is behind #9682.

We should refactor and simplify here, part of the reason this happened is because the wiring is very complex due to fx and interface abuse.

Jorropo added a commit to Jorropo/go-ipfs that referenced this issue May 19, 2023
hannahhoward pushed a commit to filecoin-project/kubo-api-client that referenced this issue Jun 19, 2023
Fixes default auto mode, but Routing.Type=custom needs more work.
Continued in ipfs/kubo#9482
hannahhoward pushed a commit to filecoin-project/kubo-api-client that referenced this issue Jun 19, 2023
Fixes default auto mode, but Routing.Type=custom needs more work.
Continued in ipfs/kubo#9482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/expert Having worked on the specific codebase is important kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up topic/commands Topic commands topic/dht Topic dht
Projects
No open projects
Status: 🥞 Todo
Development

No branches or pull requests

5 participants