p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified

dhruv commented at 7:42 pm on September 25, 2020: contributor

If the user runs: bitcoind -connect=X -seednode=Y, I think it is safe to ignore -seednode. A more populated addrman (via getaddr calls to peers in -seednode) is not useful in this configuration: addrman entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep addrman from getting stale. This is all done in a part of ThreadOpenConnections (below this line) which is never executed when -connect is supplied. With -connect, ThreadOpenConnections will run this loop and exit thread execution when interrupted.

Reviewers may also find it relevant that when -connect is used, we soft disable -dnsseed in init.cpp perhaps for the same reason i.e. seeding is not useful with -connect.

Running ProcessAddrFetch does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I’d also be happy to just leave a comment there mentioning ADDR_FETCH/-seednode is irrelevant when used with -connect.

If this change is accepted, the node will still make getaddr calls to peers in -connect and expand addrman. However, disabling those getaddr calls would leak information about the node’s configuration.

DrahtBot added the label P2P on Sep 25, 2020

sipa commented at 1:15 am on September 26, 2020: member

Concept ACK.

Perhaps we also want a warning that -seednode will be ignored in -connect mode?

dhruv force-pushed on Sep 26, 2020

dhruv commented at 3:52 am on September 26, 2020: contributor

Thank you, @sipa. I’ve added a log warning as well as a test for it.

dhruv force-pushed on Sep 26, 2020

DrahtBot commented at 11:28 am on September 26, 2020: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, vasild, achow101
Concept ACK	sipa, jonatack
Stale ACK	robot-dreams

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

in src/net.cpp:1788 in 8678a6f1aa outdated

1781@@ -1782,9 +1782,14 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1782     // Connect to specific addresses
1783     if (!connect.empty())
1784     {
1785+        {
1786+            LOCK(m_addr_fetches_mutex);
1787+            if (!m_addr_fetches.empty()) {
1788+                LogPrintf("-seednode is ignored in -connect mode\n");

robot-dreams commented at 7:23 pm on September 26, 2020:

Could this warning get printed if someone manually sets -dnsseed, since it looks like both -seednode and -dnsseed could result in nonempty m_addr_fetches?

If so, would it make sense to do one or more of the following:

Add a warning about -dnsseed as well
Move the warnings to where the args are parsed

dhruv commented at 8:19 pm on September 26, 2020:

Thanks for the review and great catch, @robot-dreams! I’ve moved the warning to init.cpp. It’s much cleaner.

robot-dreams commented at 7:45 pm on September 26, 2020: contributor

Concept ACK, thanks for noticing this case and cleaning it up!

dhruv force-pushed on Sep 26, 2020

dhruv commented at 8:33 pm on September 29, 2020: contributor

Comments above have been addressed. This PR is ready for review.

robot-dreams commented at 5:43 am on October 2, 2020: contributor

Thanks for addressing!

One more question: if someone manually sets -dnsseed, so that the SoftSetBoolArg doesn’t do anything, is it also worth printing a warning in that case?

I’m asking since it looks like ThreadDNSAddressSeed might also append entries to m_addr_fetches, which is accessed in the (now removed) ProcessAddrFetch call.

dhruv force-pushed on Oct 3, 2020

dhruv commented at 5:55 am on October 3, 2020: contributor

Great question, @robot-dreams!

With -connect=X -dnsseed=1, ThreadDNSAddressSeed uses some domains owned by contributors (see CMainParams) to expand the peers in addrman. If -proxy is set, or if a seed domain does not support x9.domain (which tests for NODE_NETWORK | NODE_WITNESS), getaddr calls are made to a peer dns seed resolves to. i.e. ADDR_FETCH connections to an address that a dees like dnsseed.emzy.de resolves to. Otherwise (and this is mostly the case), DNS resolved addresses are used as peer candidates in addrman. So, sometimes, DNS seeds act as seednodes (eg. in -proxy mode) requiring a further getaddr call (this case is affected by removing the ProcessAddrFetch() call), but mostly, they directly dns resolve to peers which support NODE_NETWORK | NODE_WITNESS (this case is unaffected).

if someone manually sets -dnsseed, so that the SoftSetBoolArg doesn’t do anything, is it also worth printing a warning in that case?

Yes, I think it’s worth adding a warning for the case: -connect=X -dnsseed=1 -proxy=Y. I’ve added it.

Rebased with master.

dhruv force-pushed on Oct 3, 2020

in src/net.h:85 in c9c6388b77 outdated

86@@ -87,6 +87,7 @@ static const bool DEFAULT_BLOCKSONLY = false;
87 static const int64_t DEFAULT_PEER_CONNECT_TIMEOUT = 60;
88 
89 static const bool DEFAULT_FORCEDNSSEED = false;
90+static const bool DEFAULT_DNSSEED = true;

robot-dreams commented at 6:40 am on October 3, 2020:

Mandatory nit involving constants (feel free to ignore): If you’re going to add this, is it worth updating init.cpp where argsman.AddArg is called? Or is it worth not adding this constant in the first place (I’m not sure how much clarity it adds)?

dhruv commented at 10:09 pm on October 3, 2020:

Good call on updating argsman.AddArg - I’ve pushed that up.

The benefit to adding this constant is that it increases the likelihood that all invocations use a consistent default.

robot-dreams commented at 6:46 am on October 3, 2020: contributor

utACK c9c6388b7747032615cb550036d330fb12beedc7

Thanks for looking into the interactions so thoroughly!

dhruv force-pushed on Oct 3, 2020

robot-dreams commented at 10:38 pm on October 3, 2020: contributor

utACK a16825b42190a2780baeb7a262c9b6cda57a2755

in src/init.cpp:2004 in a16825b421 outdated

1999@@ -2000,6 +2000,13 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
2000         if (connect.size() != 1 || connect[0] != "0") {
2001             connOptions.m_specified_outgoing = connect;
2002         }
2003+        if (!connOptions.m_specified_outgoing.empty() && !connOptions.vSeedNodes.empty()) {
2004+            LogPrintf("-seednode is ignored in -connect mode\n");

jonatack commented at 11:59 am on October 4, 2020:

maybe s/in -connect mode/when -connect is used/

dhruv commented at 4:41 pm on October 8, 2020:

Done

in src/init.cpp:2008 in a16825b421 outdated

1999@@ -2000,6 +2000,13 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
2000         if (connect.size() != 1 || connect[0] != "0") {
2001             connOptions.m_specified_outgoing = connect;
2002         }
2003+        if (!connOptions.m_specified_outgoing.empty() && !connOptions.vSeedNodes.empty()) {
2004+            LogPrintf("-seednode is ignored in -connect mode\n");
2005+        }
2006+
2007+        if (args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED) && args.IsArgSet("-proxy")) {
2008+            LogPrintf("-dnsseed is ignored in -connect mode when -proxy is specified\n");

jonatack commented at 12:00 pm on October 4, 2020:

maybe s/in -connect mode when/when -connect is used and/

dhruv commented at 5:18 pm on October 8, 2020:

Done

in test/functional/feature_config_args.py:227 in a16825b421 outdated

144@@ -145,12 +145,40 @@ def test_networkactive(self):
145             self.start_node(0, extra_args=['-nonetworkactive=1'])
146         self.stop_node(0)
147 
148+    def test_connect_with_seednode(self):

jonatack commented at 12:03 pm on October 4, 2020:

 0    def test_connect_with_seednode(self):
 1        self.log.info('Test -connect with -seednode')
 2        seednode_ignored = ['-seednode is ignored in -connect mode\n']
 3
 4        # When -connect is supplied, expanding addrman via getaddr calls to ADDR_FETCH(-seednode)
 5        # nodes is irrelevant and -seednode is ignored.
 6        with self.nodes[0].assert_debug_log(expected_msgs=seednode_ignored):
 7            self.start_node(0, extra_args=['-connect=fakeaddress1', '-seednode=fakeaddress2'])
 8
 9        # With -proxy, dns seeds are added to be used as ADDR_FETCH connections instead of resolving
10        # to peers. ADDR_FETCH connections are not used in -connect mode.
11        with self.nodes[0].assert_debug_log(expected_msgs=['-dnsseed is ignored in -connect mode when -proxy is specified\n']):
12            self.restart_node(0, extra_args=['-connect=fakeaddress1', '-dnsseed=1', '-proxy=1.2.3.4'])
13
14        for arg in ['-connect=0', '-noconnect']:
15            with self.nodes[0].assert_debug_log(expected_msgs=[], unexpected_msgs=seednode_ignored):
16                self.restart_node(0, extra_args=[arg, '-seednode=fakeaddress2'])
17        self.stop_node(0)

add logging
use restart_node
use empty array for unneeded expected messages
make more clear to the reader the differences between each test
use the same “-seednode is ignored” message (not with/without ‘\n’) for each test
place the comments above the section rather than inside it for readability
a bit shorter

(some of these are subjective)

dhruv commented at 5:39 pm on October 8, 2020:

@jonatack Thank you for being so thorough! I learnt a lot here.

The only thing I didn’t do was

use empty array for unneeded expected messages

because looking at this code, I think that assert_debug_log would return too soon and not actually verify that the unexpected msgs are not logged unless I also give it an expected message that would log after the unexpected message. The underlying issue is how can the assert know that something did not happen for sure? It either has to wait until the timeout, or until something that we are sure will happen after the unexpected thing might.

Am I missing something?

jonatack commented at 12:33 pm on October 4, 2020: contributor

Tested Concept ACK

Could/should the warnings be placed in the -connect section of InitParameterInteraction() and logged at the top along with the other parameter interactions?

0$ ./src/bitcoind -regtest -connect=fakeaddress1 -seednode=fakeaddress2
12020-10-04T12:22:30Z Bitcoin Core version v0.20.99.0-a16825b421-dirty (debug build)
22020-10-04T12:22:30Z InitParameterInteraction: parameter interaction: -connect set -> setting -dnsseed=0
32020-10-04T12:22:30Z InitParameterInteraction: parameter interaction: -connect set -> setting -listen=0

in src/init.cpp:1802 in a16825b421 outdated

1999@@ -2000,6 +2000,13 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
2000         if (connect.size() != 1 || connect[0] != "0") {
2001             connOptions.m_specified_outgoing = connect;
2002         }
2003+        if (!connOptions.m_specified_outgoing.empty() && !connOptions.vSeedNodes.empty()) {

naumenkogs commented at 9:47 am on October 6, 2020:

This could have gone inside the condition right above.

dhruv commented at 4:38 pm on October 8, 2020:

The condition above is true when connect.size() == 0 (happens in the -noconnect -seednode=fakeaddress test case) and the new condition is not.

I could move the whole condition inside as-is, but wouldn’t gain in clarity or brevity.

vasild commented at 9:07 am on October 21, 2022:

I guess @naumenkogs meant this:

0if (connect.size() != 1 || connect[0] != "0") {
1    connOptions.m_specified_outgoing = connect;
2
3    if (!connOptions.vSeedNodes.empty()) {
4        LogPrintf("-seednode is ignored when -connect is used\n");
5    }
6}

in src/init.cpp:2007 in a16825b421 outdated

1999@@ -2000,6 +2000,13 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
2000         if (connect.size() != 1 || connect[0] != "0") {
2001             connOptions.m_specified_outgoing = connect;
2002         }
2003+        if (!connOptions.m_specified_outgoing.empty() && !connOptions.vSeedNodes.empty()) {
2004+            LogPrintf("-seednode is ignored in -connect mode\n");
2005+        }
2006+
2007+        if (args.GetBoolArg("-dnsseed", DEFAULT_DNSSEED) && args.IsArgSet("-proxy")) {

naumenkogs commented at 9:53 am on October 6, 2020:

But this is true even if no -connect is configured? Then the warning is confusing… Perhaps move this into the block above (same suggestion there)?

naumenkogs commented at 9:54 am on October 6, 2020:

Warning says -dnsseed is ignored. But it will be shown even if a user haven’t touched -dnsseed at all, just when they configured a -proxy. Feels like bad UX.

dhruv commented at 5:17 pm on October 8, 2020:

Reg. the first comment: If the configuration is -dnsseed=1 -proxy=X, ThreadDNSAddressSeed will queue ADDR_FETCH connections and they will get processed because ThreadOpenConnections will invoke ProcessAddrFetch with some frequency. (This will no longer happen when using -connect=X -dnsseed=1 -proxy=X after this PR is merged)

Reg. the second comment: You’re right. If the configuration is -connect=X -proxy=Y, the log would print and the user might be confused. I have updated the code to check explicitly that the user set the arg. I have also added a test case to confirm the same.

naumenkogs commented at 10:14 am on October 6, 2020: member

I find it hard to navigate this PR and make sense of everything. Could you split it into commits with separate concerns and add meaningful commit messages? Similar to your PR original post, but with less uncertainty :)

dhruv force-pushed on Oct 8, 2020

dhruv commented at 5:53 pm on October 8, 2020: contributor

Thank you for thorough and educational reviews, @jonatack, @naumenkogs ! I have pushed up changes and rebased master since since I had to force push anyway.

dhruv commented at 5:55 pm on October 8, 2020: contributor

Could/should the warnings be placed in the -connect section of InitParameterInteraction() and logged at the top along with the other parameter interactions? @jonatack I’d actually have liked that too, but the place it is right now also checks for -connect=0. I was trying to avoid replicating that check as it is an unintuitive one.

dhruv commented at 5:58 pm on October 8, 2020: contributor

Could you split it into commits with separate concerns and add meaningful commit messages? Similar to your PR original post, but with less uncertainty :) @naumenkogs I can definitely do that if it makes sense for commit atomicity. Since the actual change is only removing the ProcessAddrFetch line in ThreadOpenConnections and every thing else (the log warnings, tests, etc.) support that change, I thought we would need all that in one commit for the history to make sense. Thoughts?

dhruv force-pushed on Oct 8, 2020

DrahtBot added the label Needs rebase on Feb 12, 2021

dhruv force-pushed on Feb 14, 2021

DrahtBot removed the label Needs rebase on Feb 14, 2021

dhruv commented at 6:13 am on February 14, 2021: contributor

Rebased. Ready for further review.

DrahtBot added the label Needs rebase on Feb 25, 2021

dhruv force-pushed on Mar 12, 2021

dhruv commented at 5:05 pm on March 12, 2021: contributor

Rebased. Ready for further review.

DrahtBot removed the label Needs rebase on Mar 12, 2021

achow101 commented at 6:19 pm on October 12, 2022: member

Are you still working on this?

glozow requested review from mzumsande on Oct 12, 2022

dhruv commented at 11:23 pm on October 12, 2022: contributor

@achow101 yes

maflcko commented at 7:44 am on October 13, 2022: member

Mind doing a rebase, otherwise it will be harder to review, given all the other related changes in the meantime

dhruv commented at 9:26 pm on October 14, 2022: contributor

@MarcoFalke working through bip324 PRs to bring them up to date with the released draft and I’ll get to the rebases right after that.

in test/functional/feature_config_args.py:239 in 03c2bfffd6 outdated

223+        # When -connect is supplied, expanding addrman via getaddr calls to ADDR_FETCH(-seednode)
224+        # nodes is irrelevant and -seednode is ignored.
225+        with self.nodes[0].assert_debug_log(expected_msgs=seednode_ignored):
226+            self.start_node(0, extra_args=['-connect=fakeaddress1', '-seednode=fakeaddress2'])
227+
228+        # With -proxy, dns seeds are added to be used as ADDR_FETCH connections instead of resolving

mzumsande commented at 2:25 pm on October 18, 2022:

If -proxy is set, or if a seed domain does not support x9.domain (which tests for NODE_NETWORK | NODE_WITNESS), getaddr calls are made to the dns seed itself as a peer. i.e. ADDR_FETCH connections to an address like dnsseed.emzy.de.

I used to think the same, but @sipa explained to me that this is not what’s happening: We don’t make a ADDR_FETCH connection to a node run by the DNS seed, in fact the DNS seed operator does not even need to run a reachable bitcoind node. Instead, we connect to the peer that the DNS seed currently resolves to - which is basically just a random node on the network not under the control of the DNS seed operator, and it changes every once in a while (~30minutes?) for a given DNS seed. Since this is a common misconception, I think it would be good to describe it not just here in the test code, but also in net (If you’d prefer not to do it here, I could open a PR).

dhruv commented at 4:54 pm on October 18, 2022:

Oh yeah, he mentioned this to me as well. Embarrassingly, I forgot to update this PR. Updated this comment, added a comment in net and updated a PR comment from me to @robot-dreams as well to reflect this.

mzumsande commented at 3:25 pm on October 18, 2022: contributor

Concept ACK

I agree that the call to ProcessAddrFetch() is unnecessary when -connect is used - I don’t see a use case for harvesting addresses that you’ll never use.

Automatic rebase on master seems to work fine without any silent conflicts.

In principle, we could disable various other unnecessary things related to addr relay in -connect mode (e.g. sending out GETADDR messages) in follow-ups, but this would mean making the existing code more complicated and I’m not convinced that’s worth it.

dhruv force-pushed on Oct 18, 2022

p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified 2555a3950f

dhruv force-pushed on Oct 18, 2022

dhruv commented at 4:57 pm on October 18, 2022: contributor

Automatic rebase on master seems to work fine without any silent conflicts.

Thanks, @mzumsande! Rebased.

In principle, we could disable various other unnecessary things related to addr relay in -connect mode (e.g. sending out GETADDR messages)

See my OP in the PR. I thought about this, but then realized that this would mean the node we are -connected to will be able to fingerprint that’s our configuration and make it clear that we are vulnerable to eclipse attacks. Thoughts?

I agree though, if we want to do that at all, we do it in a followup.

mzumsande commented at 8:20 pm on October 19, 2022: contributor

Code Review ACK 2555a3950f0304b7af7609c1e6c696993c50ac72

See my OP in the PR. I thought about this, but then realized that this would mean the node we are -connected to will be able to fingerprint that’s our configuration and make it clear that we are vulnerable to eclipse attacks. Thoughts?

Oops, missed this. I agree, fingerprinting is another good reason. I think in practice we’d often trust the node we -connect to more than a random peer, and if this is our only connection to the network, the peer could guess this anyway by the lack of transactions / blocks /addresses we advertise to them. In general, it is probably not worth optimizing -connect mode too much by adding conditionals at various spots in net_processing just to save a few bytes of bandwidth or memory here and there, because it’s a rather special-purpose way of running a node used only in rare occasions.

fanquake requested review from naumenkogs on Oct 20, 2022

fanquake requested review from vasild on Oct 20, 2022

vasild approved

vasild commented at 9:28 am on October 21, 2022: contributor

ACK 2555a3950f0304b7af7609c1e6c696993c50ac72

fanquake commented at 10:07 am on October 21, 2022: member

cc @ryanofsky from a settings / init pov.

achow101 commented at 7:15 pm on February 17, 2023: member

ACK 2555a3950f0304b7af7609c1e6c696993c50ac72

DrahtBot requested review from robot-dreams on Feb 17, 2023

achow101 merged this on Feb 17, 2023

achow101 closed this on Feb 17, 2023

sidhujag referenced this in commit eddb84f785 on Feb 17, 2023

bitcoin locked this on Feb 27, 2024

p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified #20018

Reviews

Conflicts