RFC: rpc: Remove dns argument for getaddednodeinfo #8097
Conversation
It's hard to imagine a valid use-case for this, and the DNS resolve in rpc is quite out of place.
|
Concept/ut ACK. This function was crazy, it shouldn't be doing DNS lookups in the RPC function. I also can't imagine this being useful to anyone. |
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
Ok, I'll add a mention in the notes and drop the RFC in the title. |
|
Knowing whether or not your addnodes are connected is a useful feature, and not trivial from getpeerinfo. I certainly use this regularly. On May 24, 2016 11:24:09 AM PDT, Cory Fields notifications@github.com wrote:
|
|
Can't we just check addrName to see whether we're connected to a particular addnode? That changes the question from "is there a connection whose IP corresponds to what name N currently refers to?" to "is there a connection to what N referred to at the time of connection?". I don't think that matters, or is it intended to migrate addnode'd connections even when the IP their name refers to changes? |
|
We could, but thats a bit of an awkward question given the behavior of the auto-connect thread (which I do not think should change). If you change the question to that, then the background auto-connect thread may still spin trying to connect to/connecting to the host that was added, as its hostname has changed, even though the RPC will tell you you're connected. |
|
Agree, the RPC and the thread should be consistent. |
|
They're not consistent, though. If I call "getaddednodeinfo true", and there's a disconnected dns entry, it will resolve at that moment and return with a list of ip's. However, the connection thread re-resolves and attempts to connect up to 2 min later. So when using a seednode here (the only plausible use-case I can imagine), the results won't match what the thread actually tries to connect to because the attempt could be minutes later. Odds are I'll find that it connected to an IP that wasn't in the previous list. So I'm not sure how the result is any more useful than dropping to a terminal and running dig. Thinking more constructively: maybe the fix (which I think we might've discussed at one point) would be to have the pending connection attempts stored so that the rpc could grab them, rather than working in parallel. I'm still not convinced that this is broadly useful, though I suppose the burden's on me to prove otherwise. @TheBlueMatt Maybe it would be more helpful if you described your specific use-case? |
|
I dont think this is useful for addnode'ing a seednode, indeed, but the use-case is allowing one to set a hostname to addnode, and then change the hostname freely, having the addnode follow the hostname as it changes. This is useful eg if you have a bunch of nodes which all addnode something, and you want to migrate the addnode'ed node. |
This doesn't actually remove the functionality to do that. Furthermore the If you want this reporting it should be implemented differently: the original DNS lookup should store its result, it should not be repeated here. |
|
For an alternative (using the addrName used at connection time to detect
whether an addnode is connected or not), see #8113.
|
|
Closing in favor of #8113 |
As discussed with @laanwj and @sdaftuar in Zurich. The output format is kept the same as before, though the input params change.
As far as I can tell, this has no test coverage, and I'm willing to bet it gets very little usage. The behavior is subtle and non-obvious: it does a real-time resolve in order to check whether an addnode entry is connected at the time of that resolve, and makes no mention of whether or not a connection once matched a resolved addnode.
This complicates the net encapsulation work, so I propose that we simply drop it unless there's a good reason not to. If preferred, it could swallow a dummy bool parameter, in order to remain api compatible.