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

dhruv · 2020-09-25T19:42:24Z

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.

sipa · 2020-09-26T01:15:24Z

Concept ACK.

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

dhruv · 2020-09-26T03:52:06Z

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

DrahtBot · 2020-09-26T11:28:43Z

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.

robot-dreams

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

src/net.cpp

dhruv · 2020-09-29T20:33:59Z

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

robot-dreams · 2020-10-02T05:43:43Z

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 · 2020-10-03T05:55:02Z

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.

robot-dreams

utACK c9c6388b7747032615cb550036d330fb12beedc7

Thanks for looking into the interactions so thoroughly!

src/net.h

robot-dreams · 2020-10-03T22:38:08Z

utACK a16825b42190a2780baeb7a262c9b6cda57a2755

jonatack

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?

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

jonatack · 2020-10-04T11:59:38Z

src/init.cpp

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

jonatack · 2020-10-04T12:00:45Z

src/init.cpp

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

test/functional/feature_config_args.py

naumenkogs · 2020-10-06T09:47:49Z

src/init.cpp

This could have gone inside the condition right above.

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.

I guess @naumenkogs meant this:

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

naumenkogs · 2020-10-06T09:53:04Z

src/init.cpp

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)?

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.

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 · 2020-10-06T10:14:21Z

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

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 · 2020-10-08T16:38:36Z

src/init.cpp

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.

dhruv · 2020-10-08T16:41:41Z

src/init.cpp

dhruv · 2020-10-08T17:17:31Z

src/init.cpp

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.

dhruv · 2020-10-08T17:18:43Z

src/init.cpp

test/functional/feature_config_args.py

dhruv · 2020-10-08T17:55:40Z

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 · 2020-10-08T17:58:49Z

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 · 2021-03-12T17:05:08Z

Rebased. Ready for further review.

achow101 · 2022-10-12T18:19:13Z

Are you still working on this?

dhruv · 2022-10-12T23:23:43Z

@achow101 yes

maflcko · 2022-10-13T07:44:40Z

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

dhruv · 2022-10-14T21:26:48Z

@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.

mzumsande

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.

mzumsande · 2022-10-18T14:25:25Z

test/functional/feature_config_args.py

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).

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.

dhruv · 2022-10-18T16:57:15Z

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 · 2022-10-19T20:20:24Z

Code Review ACK 2555a39

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.

vasild

ACK 2555a39

fanquake · 2022-10-21T10:07:40Z

cc @ryanofsky from a settings / init pov.

achow101 · 2023-02-17T19:15:05Z

ACK 2555a39

…if -connect is specified 2555a39 p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta) Pull request description: 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](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted. Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-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. ACKs for top commit: mzumsande: Code Review ACK 2555a39 achow101: ACK 2555a39 vasild: ACK 2555a39 Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3

DrahtBot added the P2P label Sep 25, 2020

dhruv force-pushed the seednode-not-needed-with-connect branch from 42c6b67 to 8208494 Compare September 26, 2020 03:50

dhruv force-pushed the seednode-not-needed-with-connect branch from 8208494 to 8678a6f Compare September 26, 2020 05:02

DrahtBot mentioned this pull request Sep 26, 2020

p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty #19884

Merged

robot-dreams reviewed Sep 26, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

dhruv force-pushed the seednode-not-needed-with-connect branch 2 times, most recently from 0bbc945 to 50abb30 Compare September 26, 2020 23:32

dhruv force-pushed the seednode-not-needed-with-connect branch from 50abb30 to a1467aa Compare October 3, 2020 05:48

dhruv force-pushed the seednode-not-needed-with-connect branch from a1467aa to c9c6388 Compare October 3, 2020 05:58

robot-dreams reviewed Oct 3, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

dhruv force-pushed the seednode-not-needed-with-connect branch from c9c6388 to a16825b Compare October 3, 2020 22:08

DrahtBot mentioned this pull request Oct 4, 2020

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

Draft

jonatack reviewed Oct 4, 2020

View reviewed changes

naumenkogs reviewed Oct 6, 2020

View reviewed changes

dhruv force-pushed the seednode-not-needed-with-connect branch from a16825b to 0b59d7b Compare October 8, 2020 17:52

dhruv commented Oct 8, 2020

View reviewed changes

dhruv force-pushed the seednode-not-needed-with-connect branch 2 times, most recently from 4609c75 to b485325 Compare October 8, 2020 18:46

DrahtBot added the Needs rebase label Feb 12, 2021

DrahtBot mentioned this pull request Feb 21, 2021

test: Avoid connecting to real network when running tests #21254

Merged

DrahtBot added the Needs rebase label Feb 25, 2021

dhruv force-pushed the seednode-not-needed-with-connect branch from 1ba1285 to 03c2bff Compare March 12, 2021 16:50

DrahtBot removed the Needs rebase label Mar 12, 2021

DrahtBot mentioned this pull request Mar 23, 2021

p2p: Ignore ports on I2P addresses #21514

Closed

glozow requested a review from mzumsande October 12, 2022 18:19

mzumsande reviewed Oct 18, 2022

View reviewed changes

dhruv force-pushed the seednode-not-needed-with-connect branch from 03c2bff to bcb9541 Compare October 18, 2022 16:38

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

2555a39

dhruv force-pushed the seednode-not-needed-with-connect branch from bcb9541 to 2555a39 Compare October 18, 2022 16:51

DrahtBot mentioned this pull request Oct 19, 2022

net: Make AddrFetch connections to fixed seeds #26114

Closed

mzumsande mentioned this pull request Oct 19, 2022

p2p: Don't require services from ADDR_FETCH peers #26343

Closed

fanquake requested review from naumenkogs and vasild October 20, 2022 00:30

vasild approved these changes Oct 21, 2022

View reviewed changes

DrahtBot mentioned this pull request Nov 28, 2022

net, refactor: Kill proxy globals #26590

Closed

DrahtBot requested a review from robot-dreams February 17, 2023 19:15

achow101 merged commit f722a9b into bitcoin:master Feb 17, 2023

darosior mentioned this pull request Mar 22, 2023

wallet: Migrate non-HD keys with single combo containing a list of keys #26627

Closed

bitcoin locked and limited conversation to collaborators Feb 27, 2024

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

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

Uh oh!

Conversation

dhruv commented Sep 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Sep 26, 2020

Uh oh!

dhruv commented Sep 26, 2020

Uh oh!

DrahtBot commented Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

robot-dreams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhruv commented Sep 29, 2020

Uh oh!

robot-dreams commented Oct 2, 2020

Uh oh!

dhruv commented Oct 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robot-dreams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robot-dreams commented Oct 3, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruv Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naumenkogs Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naumenkogs Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naumenkogs commented Oct 6, 2020

Uh oh!

dhruv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruv Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruv commented Sep 25, 2020 •

edited

Loading

DrahtBot commented Sep 26, 2020 •

edited

Loading

dhruv commented Oct 3, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

dhruv Oct 8, 2020 •

edited

Loading

naumenkogs Oct 6, 2020 •

edited

Loading

naumenkogs Oct 6, 2020 •

edited

Loading

dhruv left a comment •

edited

Loading

dhruv Oct 8, 2020 •

edited

Loading

dhruv commented Oct 8, 2020 •

edited

Loading

mzumsande Oct 18, 2022 •

edited

Loading

dhruv commented Oct 18, 2022 •

edited

Loading