RFC: rpc: Remove dns argument for getaddednodeinfo

theuni commented at 5:59 pm on May 24, 2016: member

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.

rpc: Remove dns argument for getaddednodeinfo

It's hard to imagine a valid use-case for this, and the DNS resolve in rpc
is quite out of place.

3c6fe920d8

laanwj commented at 5:20 am on May 26, 2016: member

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. API change needs mention in the release notes

petertodd commented at 10:34 am on May 26, 2016: contributor

Concept ACK

sipa commented at 1:03 pm on May 26, 2016: member

Concept ACK

theuni commented at 3:38 pm on May 26, 2016: member

Ok, I’ll add a mention in the notes and drop the RFC in the title.

TheBlueMatt commented at 9:15 am on May 27, 2016: member

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:

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. You can view, comment on, or merge this pull request online at:

#8097

– Commit Summary –

rpc: Remove dns argument for getaddednodeinfo

– File Changes –

M src/rpc/net.cpp (86)

– Patch Links –

#8097.patch #8097.diff

You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #8097

sipa commented at 9:17 am on May 27, 2016: member

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?

TheBlueMatt commented at 6:49 pm on May 27, 2016: member

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.

sipa commented at 6:55 pm on May 27, 2016: member

Agree, the RPC and the thread should be consistent.

theuni commented at 7:24 pm on May 27, 2016: member

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?

TheBlueMatt commented at 8:56 pm on May 27, 2016: member

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.

laanwj commented at 10:52 am on May 30, 2016: member

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 doesn’t actually remove the functionality to do that.

Furthermore the getaddednodeinfo output is not stable in this case either, if the DNS entry changed from the time you connected to the time you call the RPC.

If you want this reporting it should be implemented differently: the original DNS lookup should store its result, it should not be repeated here.

sipa commented at 10:55 am on May 30, 2016: member

For an alternative (using the addrName used at connection time to detect whether an addnode is connected or not), see #8113.

laanwj commented at 1:10 pm on May 31, 2016: member

Closing in favor of #8113

laanwj closed this on May 31, 2016

MarcoFalke locked this on Sep 8, 2021

RFC: rpc: Remove dns argument for getaddednodeinfo #8097